FrameView should have an isMainFrameView()

[Tim Horton]

There's a lot of "m_frame && m_frame->page() && m_frame->page()->mainFrame() == m_frame".

Also I have already (maybe twice) made the mistake of not doing the requisite null-checks.

[Tim Horton]

Created attachment 207992 [details]
patch

Maybe there's some compelling reason that we haven't done this before? I don't know, but I'd love to!

[zalan]

Yay. I wanted to do this for such a long time :) (I was a bit hesitant though, whether WebCore::Frame should have an isMainFrame() function instead (copy what WK2 already has) and use that in FrameView (and in other classes))

Anyway there are a few more places where you check for isMainFrameView()

FrameView::setFrameRect()
FrameView::performPostLayoutTasks()
FrameView::pagination()
FrameView::paintOverhangAreas()

[Tim Horton]

(In reply to comment #2)
> Yay. I wanted to do this for such a long time :) (I was a bit hesitant though, whether WebCore::Frame should have an isMainFrame() function instead (copy what WK2 already has) and use that in FrameView (and in other classes))
> 
> Anyway there are a few more places where you check for isMainFrameView()
> 
> FrameView::setFrameRect()
> FrameView::performPostLayoutTasks()
> FrameView::pagination()
> FrameView::paintOverhangAreas()

There are even more than that! I will post a new patch in a moment.

[Tim Horton]

Created attachment 207997 [details]
more places

[Simon Fraser (smfr)]

Comment on attachment 207997 [details]
more places

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

> Source/WebCore/page/FrameView.cpp:427
> +bool FrameView::isMainFrameView() const
> +{
> +    return m_frame && m_frame->page() && m_frame->page()->mainFrame() == m_frame;
> +}
> +

Does this do the right thing for a page in the page cache? What is the right thing?

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 207997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207997&action=review
> 
> > Source/WebCore/page/FrameView.cpp:427
> > +bool FrameView::isMainFrameView() const
> > +{
> > +    return m_frame && m_frame->page() && m_frame->page()->mainFrame() == m_frame;
> > +}
> > +
> 
> Does this do the right thing for a page in the page cache? What is the right thing?

A "web page" in the page cache is a much different concept than a WebCore::Page

WebCore::Page represents the browser tab.  And that WebCore::Page has a single WebCore::Frame that is ALWAYS the main frame of that tab, even across navigations.

I think this check is fine.

[Darin Adler]

(In reply to comment #1)
> Maybe there's some compelling reason that we haven't done this before? I don't know, but I'd love to!

Generally it’s hard to know how much to replicate across the core classes. Should Document have isMainFrameDocument in it? Should Frame have isMainFrame or only FrameTree, etc.?

[Tim Horton]

http://trac.webkit.org/changeset/154116

[Ryosuke Niwa]

Qt build fixed in http://trac.webkit.org/changeset/154128.