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-
(committed with r60753, reviewed by Darin Adler) patch v2 (32.06 KB, patch)
2010-05-30 22:23 PDT, Antonio Gomes
no flags
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.