Bug 166735 – Give RenderObject a Page& getter.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

Now that we're getting better at felling render trees, it seems like we can make some promises about there always being a Page while a tree is standing.

Having a Page& RenderObject::page() will get rid of a ton of sketchy branches.

[Andreas Kling]

Created attachment 298118 [details]
Patch for EWS

[Andreas Kling]

Created attachment 298123 [details]
Patch II for EWS

[Andreas Kling]

Created attachment 298127 [details]
Patch

[Andreas Kling]

Created attachment 298128 [details]
Patch

[Darin Adler]

Comment on attachment 298128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298128&action=review

Almost enough to make me think I don’t need to keep moving things from Page to MainFrame.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:4145
> +Page& RenderLayerCompositor::page() const
> +{
> +    return m_renderView.page();
> +}

Can this be inlined, or would that require adding more includes?

> Source/WebCore/rendering/RenderObject.cpp:146
>  RenderTheme& RenderObject::theme() const
>  {
> -    ASSERT(document().page());
> -    return document().page()->theme();
> +    return page().theme();
>  }

Can this be inlined, or would that require adding more includes?

> Source/WebCore/rendering/RenderObject.h:1013
> +    ASSERT(frame().page());

I would love it if we added a brief comment here explaining why this is guaranteed to be non-null.

> Source/WebCore/rendering/RenderTheme.cpp:789
> +    return o.page().focusController().isActive();

How about the word object instead of "o"?

[Andreas Kling]

(In reply to comment #5)
> Comment on attachment 298128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298128&action=review
> 
> Almost enough to make me think I don’t need to keep moving things from Page
> to MainFrame.

Almost!

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:4145
> > +Page& RenderLayerCompositor::page() const
> > +{
> > +    return m_renderView.page();
> > +}
> 
> Can this be inlined, or would that require adding more includes?

Yeah, include cycle problem. We can solve it but I don't think it's important.

> > Source/WebCore/rendering/RenderObject.cpp:146
> >  RenderTheme& RenderObject::theme() const
> >  {
> > -    ASSERT(document().page());
> > -    return document().page()->theme();
> > +    return page().theme();
> >  }
> 
> Can this be inlined, or would that require adding more includes?

Same here.

> > Source/WebCore/rendering/RenderObject.h:1013
> > +    ASSERT(frame().page());
> 
> I would love it if we added a brief comment here explaining why this is
> guaranteed to be non-null.

Ok, I will add a comment here, and a longer comment in Frame::willDetachPage().

[Andreas Kling]

Created attachment 298197 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 298197 [details]
Patch for landing

Clearing flags on attachment: 298197

Committed r210436: <http://trac.webkit.org/changeset/210436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.