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
Andreas Kling
2013-08-27 16:07:28 PDT
Created attachment 209817 [details]
Patch
Comment on attachment 209817 [details] Patch Clearing flags on attachment: 209817 Committed r154715: <http://trac.webkit.org/changeset/154715> All reviewed patches have been landed. Closing bug. 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? (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.. Why is this better than adding an isMainFrame function to the Frame class? (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. Gotta say these call sites still look really awkward. (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.) 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(). (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(). :| (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. |