Bug 60724 – Add InlineWalker class to hold state for repeated calls to bidiNext

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add InlineWalker class to hold state for repeated calls to bidiNext

[Eric Seidel (no email)]

Created attachment 93332 [details]
Patch

[Levi Weintraub]

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)]

(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)]

@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 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]

Eric, are you going to upload a new patch?

[Eric Seidel (no email)]

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 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)]

(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)]

Created attachment 96028 [details]
Patch

[Adam Barth]

Comment on attachment 96028 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 96028 [details]
Patch

Clearing flags on attachment: 96028

Committed r88122: <http://trac.webkit.org/changeset/88122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.