Bug 120382

Summary: Make it less awkward to check if a Frame is the main Frame.
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebCore Misc.Assignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, ggaren, glenn, japhet, kangil.han, kling, kondapallykalyan, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andreas Kling 2013-08-27 16:07:28 PDT
Make it less awkward to check if a Frame is the main Frame.
Comment 1 Andreas Kling 2013-08-27 16:12:01 PDT
Created attachment 209817 [details]
Patch
Comment 2 WebKit Commit Bot 2013-08-27 17:08:15 PDT
Comment on attachment 209817 [details]
Patch

Clearing flags on attachment: 209817

Committed r154715: <http://trac.webkit.org/changeset/154715>
Comment 3 WebKit Commit Bot 2013-08-27 17:08:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Geoffrey Garen 2013-08-27 17:11:07 PDT
Style nit: "frame is main frame (frame)" is kind of a mouthful. Do we really need to say "frame" so many times? How about "page->isMainFrame(frame())" instead?
Comment 5 Andreas Kling 2013-08-27 18:04:03 PDT
(In reply to comment #4)
> Style nit: "frame is main frame (frame)" is kind of a mouthful. Do we really need to say "frame" so many times? How about "page->isMainFrame(frame())" instead?

I dunno. I semi-actively chose it over page->isMainFrame(frame) because "page is main frame" sounds wrong out loud.

FWIW the decision making process was:
23:56:54 <kling> Page::isMainFrame(frame)
23:56:55 <kling> OR
23:56:59 <kling> Page::frameIsMainFrame(frame)
23:58:02 <anttik> Page::frameIsMainFrameOfPage(frame)
23:58:35 <kling> Page::frameIsMainFrameOfPageAndNotSubFrame()
23:58:37 <kling> :|
23:58:43 <anttik> :|

Page::frameIsMain()? Meh..
Comment 6 Darin Adler 2013-08-27 18:05:31 PDT
Why is this better than adding an isMainFrame function to the Frame class?
Comment 7 Andreas Kling 2013-08-27 18:07:48 PDT
(In reply to comment #6)
> Why is this better than adding an isMainFrame function to the Frame class?

My reasoning was that this idiom:

if (page && &page->mainFrame() == &someFrame)
    page->thisOrThat();

Would turn into this:

if (someFrame.isMainFrame())
    page->thisOrThat();

Where it's no longer immediately obvious that it's OK to dereference the "page" pointer.
Comment 8 Darin Adler 2013-08-27 18:10:57 PDT
Gotta say these call sites still look really awkward.
Comment 9 Andreas Kling 2013-08-27 18:15:47 PDT
(In reply to comment #8)
> Gotta say these call sites still look really awkward.

I only said "less" awkward :(

Another reason I dislike Frame::isMainFrame() is that it doesn't make explicit which Page it's the main frame of.
And what do we do when Frame::m_page is null? (I suppose we're not the main frame anymore, since there'd be no Page to be the main frame of. But what if we're still alive, but we don't have an owner element? etc.)
Comment 10 Darin Adler 2013-08-27 18:19:36 PDT
FrameView now has isMainFrame. Tim Horton added it within the last week or two.

There are annoyingly similar ways to do almost the same check. Some code checks that frame.tree().parent() is 0 instead of an isMainFrame check. Other code actually checks that &frame == &frame.tree().top().
Comment 11 Andreas Kling 2013-08-27 18:25:07 PDT
(In reply to comment #10)
> FrameView now has isMainFrame. Tim Horton added it within the last week or two.
> 
> There are annoyingly similar ways to do almost the same check. Some code checks that frame.tree().parent() is 0 instead of an isMainFrame check. Other code actually checks that &frame == &frame.tree().top().

I don't feel strongly about which method is used to check. I do think that we should use the same method everywhere.

The reason I ended up checking against Page::mainFrame() here is because of the call sites that wanted to do additional "non-null business" with the Page.

In my perfect world, Frame::m_page would never be null, and we'd have Frame::isMainFrame(). :|
Comment 12 Darin Adler 2013-08-27 18:28:55 PDT
(In reply to comment #11)
> The reason I ended up checking against Page::mainFrame() here is because of the call sites that wanted to do additional "non-null business" with the Page.

For that I’d suggest version of the page() function on Frame that returns 0 if it’s not the main frame of that page. Not sure how to name it, but I’d want it to return the page, not a boolean.