Bug 90940 – Add RegisterProtocolHandlerClient to the Modules/protocolhandler

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

In order to move protocol handler to the modules, client interfaces need to be moved to the Modules/protocolhandler as well. Patch is coming.

[Gyuyoung Kim]

Created attachment 151662 [details]
Working in Progress - Please do not review

[Hajime Morrita]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

Created attachment 151876 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 151881 [details]
Patch

[Gyuyoung Kim]

Bug 90766 is being landed. So, I'm going to submit a patch for all ports.

[WebKit Review Bot]

Comment on attachment 151881 [details]
Patch

Attachment 151881 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13198696

[Hajime Morrita]

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.

[Gyuyoung Kim]

Created attachment 152185 [details]
Patch

[Gyuyoung Kim]

(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 ?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Gyuyoung Kim]

Created attachment 152260 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Gyuyoung Kim]

Created attachment 152384 [details]
Patch

[Gyuyoung Kim]

In WebFrameTest.GetFullHtmlOfPage case,  there is no test fail on my local. Hmm, something strange.

[Adam Barth]

Looks like it passed this time, so maybe it was just flaky.

[Gyuyoung Kim]

(In reply to comment #18)
> Created an attachment (id=152384) [details]
> Patch

Morrita, could you review this patch again ?

[Hajime Morrita]

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.

[Gyuyoung Kim]

Created attachment 152692 [details]
Patch

[Hajime Morrita]

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.

[Gyuyoung Kim]

(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.

[Hajime Morrita]

(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.

[WebKit Review Bot]

Comment on attachment 152692 [details]
Patch

Clearing flags on attachment: 152692

Committed r122810: <http://trac.webkit.org/changeset/122810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.