Bug 60724

Summary: Add InlineWalker class to hold state for repeated calls to bidiNext
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, hyatt, leviw, leviw, mitz, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 50912    
Attachments:
Description Flags
Patch
none
Patch none

Eric Seidel (no email)
Reported 2011-05-12 13:56:39 PDT
Add InlineWalker class to hold state for repeated calls to bidiNext
Attachments
Patch (5.85 KB, patch)
2011-05-12 13:58 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch (5.82 KB, patch)
2011-06-04 11:50 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2011-05-12 13:58:46 PDT
Created attachment 93332 [details] Patch
Levi Weintraub
Comment 2 2011-05-12 14:20:40 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=93332&action=review This is a great step in the right direction! > Source/WebCore/ChangeLog:8 > + This is one more little step towards removing (naked) bidiNext usage. Awesome! > Source/WebCore/rendering/InlineIterator.h:251 > + InlineWalker(RenderObject* root, bool skipInlines = true, InlineBidiResolver* observer = 0) An enum for skipInlines would be more informative, no?
Eric Seidel (no email)
Comment 3 2011-05-12 14:22:20 PDT
(In reply to comment #2) > > Source/WebCore/rendering/InlineIterator.h:251 > > + InlineWalker(RenderObject* root, bool skipInlines = true, InlineBidiResolver* observer = 0) > > An enum for skipInlines would be more informative, no? So true! Will fix in a follow-up patch.
Eric Seidel (no email)
Comment 4 2011-05-13 11:37:55 PDT
@rniwa: ping. This is not the end of this refactoring, but I do think it's better than what we have today.
Ryosuke Niwa
Comment 5 2011-05-13 11:50:27 PDT
Comment on attachment 93332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93332&action=review > Source/WebCore/rendering/InlineIterator.h:251 > + InlineWalker(RenderObject* root, bool skipInlines = true, InlineBidiResolver* observer = 0) Should the first argument be RenderBlock since we always pass "this"? Or do you have a plan to use it elsewhere? And I concur Levi's point that we should use enum for skipInlines but skipInlines is always false in this version so I would have got rid of the argument altogether and waited until the next patch that actually make use of this argument. The same goes for observer. Since we're not using this argument at the moment, I don't think we should be adding it.
Ryosuke Niwa
Comment 6 2011-05-25 22:40:08 PDT
Eric, are you going to upload a new patch?
Eric Seidel (no email)
Comment 7 2011-05-26 06:22:38 PDT
Best to r- it if you want me to do that. :) I'd forgotten about this bug. Distracted by other things at the moment.
Brent Fulgham
Comment 8 2011-05-31 20:37:46 PDT
Comment on attachment 93332 [details] Patch r- to encourage eseidel to upload his new patch, and to resolve the question of enum-versus-next-release for the skipInlines argument.
Eric Seidel (no email)
Comment 9 2011-06-04 11:29:59 PDT
(In reply to comment #5) > (From update of attachment 93332 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93332&action=review > > > Source/WebCore/rendering/InlineIterator.h:251 > > + InlineWalker(RenderObject* root, bool skipInlines = true, InlineBidiResolver* observer = 0) > > Should the first argument be RenderBlock since we always pass "this"? No. There is no reason to limit the walk under a RenderBlock. It's any RenderObject subtree. > Or do you have a plan to use it elsewhere? bidi-isolate fixes use this elsewhere, yes. > And I concur Levi's point that we should use enum for skipInlines but skipInlines is always false in this version so I would have got rid of the argument altogether and waited until the next patch that actually make use of this argument. The same goes for observer. Since we're not using this argument at the moment, I don't think we should be adding it. Sure, will remove.
Eric Seidel (no email)
Comment 10 2011-06-04 11:50:18 PDT
Created attachment 96028 [details] Patch
Adam Barth
Comment 11 2011-06-04 11:53:17 PDT
Comment on attachment 96028 [details] Patch ok
WebKit Review Bot
Comment 12 2011-06-04 12:34:41 PDT
Comment on attachment 96028 [details] Patch Clearing flags on attachment: 96028 Committed r88122: <http://trac.webkit.org/changeset/88122>
WebKit Review Bot
Comment 13 2011-06-04 12:34:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.