Bug 39928 – Add a convenient helper getter for Frame* to RenderObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antonio Gomes]

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.

[Antonio Gomes]

Darin, if you think it is a good idea, I can do the patch.

[Darin Adler]

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]

(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]

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]

(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 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]

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]

Ping review.

The upcoming patch in bug 39854 will likely count on this one landed.

[Antonio Gomes]

(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 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>