| Summary: | Add RegisterProtocolHandlerClient to the Modules/protocolhandler | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, japhet, jochen, morrita, ojan, rakuco, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Bug Depends on: | 90766 | ||||||||||||||||||||||||
| Bug Blocks: | 79327 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Gyuyoung Kim
2012-07-10 22:46:42 PDT
Created attachment 151662 [details]
Working in Progress - Please do not review
Please don't make client supplementable. It limits flexibility of the client lifetime. It would be preferable to introduce a controller class which is supplementable. Created attachment 151867 [details] Patch for EFL ,GTK and QT port This patch is based on patch of Bug 90766. Because, RegisterProtocolhandlerClient can be added after landing the patch of Bug 90766. I'd like to remove the patch of Bug 90766 in this patch after it is landed. For now, this patch is to support EFL, GTK and QT ports first. Updated patch for MAC, Chromium and Win ports is going to be submitted again. In addition, this patch also supports that RegisterProtocolhandlerClient is to be supplementable. (In reply to comment #2) > Please don't make client supplementable. It limits flexibility of the client lifetime. > It would be preferable to introduce a controller class which is supplementable. If you think so, next patch will use controller. Created attachment 151876 [details]
Patch
I add RegisterProtocolHandlerController to Modules/protocolhandler as well as this patch is only for EFL, GTK and QT for now. This patch is still based on Bug 90766 to test build break. Created attachment 151881 [details]
Patch
Bug 90766 is being landed. So, I'm going to submit a patch for all ports. Comment on attachment 151881 [details] Patch Attachment 151881 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13198696 Comment on attachment 151881 [details]
Patch
r- for cr-linux redness.
I noticed that we can get rid of RegisterProtocolHandlerController by
making NavigatorRegisterProtocolHandler RefCountedSupplement<Page> instead of
Supplement<Navigator>. Even though NavigatorRegisterProtocolHandler has m_frame, no one uses it. So it doesn't need any frame/document specific state and can be a page-supplement.
Created attachment 152185 [details]
Patch
(In reply to comment #10) > (From update of attachment 151881 [details]) > r- for cr-linux redness. > > I noticed that we can get rid of RegisterProtocolHandlerController by > making NavigatorRegisterProtocolHandler RefCountedSupplement<Page> instead of > Supplement<Navigator>. Even though NavigatorRegisterProtocolHandler has m_frame, no one uses it. So it doesn't need any frame/document specific state and can be a page-supplement. Hajime, I change NavigatorRegisterProtocolHandler is based on RefCountedSupplement<Page> in new patch. But, I'm not sure whether this patch is able to cover your suggestion. If this patch still has something wrong, could you let me know what is your suggestion more detail ? Comment on attachment 152185 [details] Patch Attachment 152185 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13238096 New failing tests: fast/dom/register-protocol-handler.html Created attachment 152234 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 152260 [details]
Patch
Comment on attachment 152260 [details] Patch Attachment 152260 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13242144 New failing tests: WebFrameTest.GetFullHtmlOfPage Created attachment 152279 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 152384 [details]
Patch
In WebFrameTest.GetFullHtmlOfPage case, there is no test fail on my local. Hmm, something strange. Looks like it passed this time, so maybe it was just flaky. (In reply to comment #18) > Created an attachment (id=152384) [details] > Patch Morrita, could you review this patch again ? Comment on attachment 152384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152384&action=review Looks good. Added some trivial comments. You could have implemented RegisterProtocolHandlerClient in ChromeClientImpl instead of adding new class. It's fine to choose either way. But using multiple inheritance will be smaller. > Source/WebKit/ChangeLog:13 > + Please explain the change of these build files. > Source/WebKit/efl/ChangeLog:10 > + protocol handlers, virtual functions should be moved to RegisterProtocolHandlerClient. Please explain the change of WebKit/efl rather than something generic. > Source/WebKit/gtk/ChangeLog:11 > + Ditto. > Source/WebKit2/ChangeLog:10 > + protocol handlers, virtual functions should be moved to RegisterProtocolHandlerClient. Ditto. > Source/WebCore/Modules/protocolhandler/RegisterProtocolHandlerClient.h:38 > +public: Need virtual (empty) destructor. Created attachment 152692 [details]
Patch
Comment on attachment 152692 [details]
Patch
If you got r+ once, you can rewrite "Reviewed by" line and set cq+ instead of asking r? again.
"webkit-patch land-safely" will take care of these for you.
(In reply to comment #24) > (From update of attachment 152692 [details]) > If you got r+ once, you can rewrite "Reviewed by" line and set cq+ instead of asking r? again. Yes, I know it very well. But, You didn't set r+ on this bug. > "webkit-patch land-safely" will take care of these for you. Ok, I will use this command. (In reply to comment #25) > (In reply to comment #24) > > (From update of attachment 152692 [details] [details]) > > If you got r+ once, you can rewrite "Reviewed by" line and set cq+ instead of asking r? again. > > Yes, I know it very well. But, You didn't set r+ on this bug. Woa. I thought I did. I'm sorry for that. Comment on attachment 152692 [details] Patch Clearing flags on attachment: 152692 Committed r122810: <http://trac.webkit.org/changeset/122810> All reviewed patches have been landed. Closing bug. |