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.
Darin, if you think it is a good idea, I can do the patch.
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.
(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 ...
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.
(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.
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
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.
Ping review. The upcoming patch in bug 39854 will likely count on this one landed.
(In reply to comment #8) > Ping review. > > The upcoming patch in bug 39854 will likely count on this one landed. Re-ping review.
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>