Bug 23429 - Frame Refactor: Move focusWindow/unfocusWindow/shouldClose/scheduleClose to Chrome
Summary: Frame Refactor: Move focusWindow/unfocusWindow/shouldClose/scheduleClose to C...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Holger Freyther
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-20 03:56 PST by Holger Freyther
Modified: 2010-06-10 17:03 PDT (History)
0 users

See Also:


Attachments
Carry out the move. (9.37 KB, patch)
2009-01-20 04:39 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Carry out the move (13.59 KB, patch)
2009-02-09 15:09 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Move stuff to Chrome.. (11.35 KB, patch)
2009-02-09 15:14 PST, Holger Freyther
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2009-01-20 03:56:10 PST
Move the above named methods from WebCore::Frame to WebCore::Chrome.
Comment 1 Holger Freyther 2009-01-20 04:39:23 PST
Created attachment 26859 [details]
Carry out the move.

The statusbar methods were left in Frame. They require more research, e.g. if the per frame statusbar text needs to be safed.
Comment 2 Nikolas Zimmermann 2009-02-06 16:52:45 PST
Comment on attachment 26859 [details]
Carry out the move.

Looks fine, long live refactoring, r=me :-)
Comment 3 Holger Freyther 2009-02-09 15:09:35 PST
Created attachment 27496 [details]
Carry out the move

Add null check for Frame::page in the FrameLoaderClient. shouldClose will be true when we have no page or chrome()->shouldClose will return true. This should resemble the current state with Frame::shouldClose.
Comment 4 Holger Freyther 2009-02-09 15:14:53 PST
Created attachment 27497 [details]
Move stuff to Chrome..

Unmangled version?
Comment 5 Darin Adler 2009-02-09 15:33:11 PST
Comment on attachment 27497 [details]
Move stuff to Chrome..

My intent here for these functions was to have the top-frame check be in DOMWindow, and the functions in Page not take a Frame*. I don't think it makes sense to "close" a non-main Frame. Could you make that change?
Comment 6 Holger Freyther 2009-02-09 15:36:33 PST
(In reply to comment #5)
> (From update of attachment 27497 [details] [review])
> My intent here for these functions was to have the top-frame check be in
> DOMWindow, and the functions in Page not take a Frame*. I don't think it makes
> sense to "close" a non-main Frame. Could you make that change?
> 

Sure.
Comment 7 Adam Roben (:aroben) 2009-02-09 15:41:23 PST
Comment on attachment 27497 [details]
Move stuff to Chrome..

> +        Like the many other Chrome methods take a Frame* pointer. Update the
> +        FrameLoader and the DOMWindow call site. In the FrameLoader it is assumed
> +        that the page is around...

This last sentence doesn't seem necessary anymore -- there's no code in FrameLoader in this patch that assumes there's a Page.

You need to patch WebView::shouldClose in WebKit/win/WebView.cpp as well.

r=me
Comment 8 Darin Adler 2009-02-09 15:57:01 PST
Comment on attachment 27497 [details]
Move stuff to Chrome..

I'm going to change this back to review- since I've requested a change.
Comment 9 Eric Seidel (no email) 2009-04-29 14:51:26 PDT
Comment on attachment 26859 [details]
Carry out the move.

Clearing review flag to remove it from the commit queue.
Comment 10 Eric Seidel (no email) 2009-04-29 14:51:26 PDT
Comment on attachment 26859 [details]
Carry out the move.

Clearing review flag to remove it from the commit queue.