Summary: | [EFL][WK2] Implement runBeforeUnloadConfirmPanel on EFL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jaehun Lim <ljaehun.lim> | ||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jaehun Lim
2013-01-15 21:01:36 PST
Created attachment 182921 [details]
Patch
Comment on attachment 182921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182921&action=review > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] Add support for onBeforeUnload handling This title is confusing. Please update it. > Source/WebKit2/ChangeLog:12 > + (WebKit): You can remove this, it is likely generated from the namespace. > Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:231 > > + uiClient.runBeforeUnloadConfirmPanel = runBeforeUnloadConfirmPanel; > + For clarity, you should keep those in the order of the PageUIClient. > Source/WebKit2/UIProcess/efl/PageUIClientEfl.h:74 > + static bool runBeforeUnloadConfirmPanel(WKPageRef, WKStringRef, WKFrameRef, const void*); ditto. Comment on attachment 182921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182921&action=review >> Source/WebKit2/ChangeLog:3 >> + [EFL][WK2] Add support for onBeforeUnload handling > > This title is confusing. Please update it. ok. I'll update. >> Source/WebKit2/ChangeLog:12 >> + (WebKit): > > You can remove this, it is likely generated from the namespace. ok. >> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:231 >> + > > For clarity, you should keep those in the order of the PageUIClient. The current function ordering in PageUIClientEfl is confused. I'll fix it together. Created attachment 182935 [details]
Fixed patch
Comment on attachment 182935 [details] Fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=182935&action=review I am confused. You reorder the functions of PagreUIClient but it does not really match the order of WKPageUIClient (createNewPage for example). You ordered PageUIClientEfl.h (which is nice) but the implementaion of PageUIClientEfl::PageUIClientEfl keeps the old order. Please make a separate patch to clean EFL PageUIClient, we land that first, and then we do the implementation for runBeforeUnloadConfirmPanel. > Source/WebKit2/ChangeLog:11 > + > + Implement runBeforeUnloadConfirmPanel() to support window.onbeforeunload. > + And adjsust the ordering of functions to kepp in sync with PageUIClient. > + > + * UIProcess/efl/PageUIClientEfl.cpp: Your patch now also sort the callbacks of PageUIClient correctly. You should have mentioned that in your ChangeLog. (In reply to comment #5) > (From update of attachment 182935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182935&action=review > > I am confused. You reorder the functions of PagreUIClient but it does not really match the order of WKPageUIClient (createNewPage for example). > You ordered PageUIClientEfl.h (which is nice) but the implementaion of PageUIClientEfl::PageUIClientEfl keeps the old order. > > Please make a separate patch to clean EFL PageUIClient, we land that first, and then we do the implementation for runBeforeUnloadConfirmPanel. > > > Source/WebKit2/ChangeLog:11 > > + > > + Implement runBeforeUnloadConfirmPanel() to support window.onbeforeunload. > > + And adjsust the ordering of functions to kepp in sync with PageUIClient. > > + > > + * UIProcess/efl/PageUIClientEfl.cpp: > > Your patch now also sort the callbacks of PageUIClient correctly. You should have mentioned that in your ChangeLog. Thank you for your review. I uploaded another patch. Please see the https://bugs.webkit.org/show_bug.cgi?id=107060. I'll upload new patch for runBeforeUnloadConfirmPanel after fixing style issues. Created attachment 183076 [details]
Fixed patch
Created attachment 186471 [details]
Patch
Comment on attachment 186471 [details] Patch Clearing flags on attachment: 186471 Committed r141848: <http://trac.webkit.org/changeset/141848> All reviewed patches have been landed. Closing bug. |