Bug 60724 - Add InlineWalker class to hold state for repeated calls to bidiNext
Summary: Add InlineWalker class to hold state for repeated calls to bidiNext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 50912
  Show dependency treegraph
 
Reported: 2011-05-12 13:56 PDT by Eric Seidel (no email)
Modified: 2011-06-04 12:34 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.85 KB, patch)
2011-05-12 13:58 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2011-06-04 11:50 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-05-12 13:56:39 PDT
Add InlineWalker class to hold state for repeated calls to bidiNext
Comment 1 Eric Seidel (no email) 2011-05-12 13:58:46 PDT
Created attachment 93332 [details]
Patch
Comment 2 Levi Weintraub 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?
Comment 3 Eric Seidel (no email) 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.
Comment 4 Eric Seidel (no email) 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 2011-05-25 22:40:08 PDT
Eric, are you going to upload a new patch?
Comment 7 Eric Seidel (no email) 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.
Comment 8 Brent Fulgham 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.
Comment 9 Eric Seidel (no email) 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.
Comment 10 Eric Seidel (no email) 2011-06-04 11:50:18 PDT
Created attachment 96028 [details]
Patch
Comment 11 Adam Barth 2011-06-04 11:53:17 PDT
Comment on attachment 96028 [details]
Patch

ok
Comment 12 WebKit Review Bot 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>
Comment 13 WebKit Review Bot 2011-06-04 12:34:46 PDT
All reviewed patches have been landed.  Closing bug.