WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
39928
Add a convenient helper getter for Frame* to RenderObject
https://bugs.webkit.org/show_bug.cgi?id=39928
Summary
Add a convenient helper getter for Frame* to RenderObject
Antonio Gomes
Reported
2010-05-29 22:21:01 PDT
I think RenderObject and derivated classes are calling document()->frame() enough so it worth to add a shortcut: frame(). See RenderObject-based classes doing document()->frame() here
http://pastebin.com/nzKxaSPM
ps: I only grep'ed for files that match *Render* and inside WebCore/rendering/ , so there are possible more call sites.
Attachments
patch v1
(26.90 KB, patch)
2010-05-30 20:31 PDT
,
Antonio Gomes
darin
: review-
Details
Formatted Diff
Diff
(committed with r60753, reviewed by Darin Adler) patch v2
(32.06 KB, patch)
2010-05-30 22:23 PDT
,
Antonio Gomes
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Antonio Gomes
Comment 1
2010-05-29 22:25:45 PDT
Darin, if you think it is a good idea, I can do the patch.
Darin Adler
Comment 2
2010-05-30 11:17:59 PDT
I can’t think of a good argument against this, except that we don’t want to have getters for everything everywhere. We can make it an inline function.
Antonio Gomes
Comment 3
2010-05-30 20:29:22 PDT
(In reply to
comment #2
)
> I can’t think of a good argument against this, except that we don’t want to have getters for everything everywhere. We can make it an inline function.
Right. It'd be just another convenience method like Frame::settings instead of the full real path, Frame::Page::settings(). Patch coming ...
Antonio Gomes
Comment 4
2010-05-30 20:31:09 PDT
Created
attachment 57429
[details]
patch v1 Added "Frame*" getter to RenderObject, and changed call sites previously going through document()->frame() to just use the newly introduced frame() method.
Darin Adler
Comment 5
2010-05-30 20:55:33 PDT
(In reply to
comment #3
)
> Right. It'd be just another convenience method like Frame::settings instead of the full real path, Frame::Page::settings().
Generally speaking I'm not sure how many of these convenience functions we want, which is what I meant by "we don't want to have getters for everything everywhere". Frame::settings is not an inline function, so it's actually slower than calling Frame::page and then Page::settings. But further, if we add too man of these then we create too many alternate ways to do the same thing. Neither of these are arguments against RenderObject::frame, though.
Darin Adler
Comment 6
2010-05-30 20:57:57 PDT
Comment on
attachment 57429
[details]
patch v1
> - Frame* frame = document()->frame(); > + Frame* frame = frame();
I don't think this will work correctly, and I'm surprised it compiles successfully. It needs to be: Frame* frame = this->frame(); review- because of this, and as I said, I am surprised this compiles! Otherwise, looks great and would be r=me
Antonio Gomes
Comment 7
2010-05-30 22:23:56 PDT
Created
attachment 57434
[details]
(committed with
r60753
, reviewed by Darin Adler) patch v2 (In reply to
comment #6
)
> (From update of
attachment 57429
[details]
) > > - Frame* frame = document()->frame(); > > + Frame* frame = frame(); > > I don't think this will work correctly, and I'm surprised it compiles successfully. It needs to be: > Frame* frame = this->frame();
Good catch, I've missed that one. Fixed.
Antonio Gomes
Comment 8
2010-05-31 11:35:50 PDT
Ping review. The upcoming patch in
bug 39854
will likely count on this one landed.
Antonio Gomes
Comment 9
2010-06-04 14:45:23 PDT
(In reply to
comment #8
)
> Ping review. > > The upcoming patch in
bug 39854
will likely count on this one landed.
Re-ping review.
Antonio Gomes
Comment 10
2010-06-05 08:38:37 PDT
Comment on
attachment 57434
[details]
(committed with
r60753
, reviewed by Darin Adler) patch v2 Clearing flags on attachment: 57434 Committed
r60753
: <
http://trac.webkit.org/changeset/60753
>
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