WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
60724
Add InlineWalker class to hold state for repeated calls to bidiNext
https://bugs.webkit.org/show_bug.cgi?id=60724
Summary
Add InlineWalker class to hold state for repeated calls to bidiNext
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
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
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.
Top of Page
Format For Printing
XML
Clone This Bug