Add InlineWalker class to hold state for repeated calls to bidiNext
Created attachment 93332 [details] Patch
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?
(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.
@rniwa: ping. This is not the end of this refactoring, but I do think it's better than what we have today.
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.
Eric, are you going to upload a new patch?
Best to r- it if you want me to do that. :) I'd forgotten about this bug. Distracted by other things at the moment.
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.
(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.
Created attachment 96028 [details] Patch
Comment on attachment 96028 [details] Patch ok
Comment on attachment 96028 [details] Patch Clearing flags on attachment: 96028 Committed r88122: <http://trac.webkit.org/changeset/88122>
All reviewed patches have been landed. Closing bug.