Bug 73176 - Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'.
Summary: Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 88163
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-27 17:54 PST by Dongwoo Joshua Im (dwim)
Modified: 2018-09-27 08:23 PDT (History)
22 users (show)

See Also:


Attachments
Patch (26.69 KB, patch)
2011-12-04 23:55 PST, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2011-12-05 01:35 PST, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (19.71 KB, patch)
2011-12-07 15:39 PST, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (19.71 KB, patch)
2011-12-07 18:26 PST, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (22.61 KB, patch)
2012-01-10 19:23 PST, Dongwoo Joshua Im (dwim)
fishd: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (27.93 KB, patch)
2012-05-23 01:36 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2012-05-23 04:28 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (29.71 KB, patch)
2012-05-23 18:20 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (29.71 KB, patch)
2012-05-23 18:27 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (662.92 KB, application/zip)
2012-05-23 20:12 PDT, WebKit Review Bot
no flags Details
Patch (30.91 KB, patch)
2012-05-23 20:20 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (657.25 KB, application/zip)
2012-05-23 21:29 PDT, WebKit Review Bot
no flags Details
Patch (32.52 KB, patch)
2012-05-23 21:44 PDT, Dongwoo Joshua Im (dwim)
abarth: review-
Details | Formatted Diff | Diff
Patch (62.40 KB, patch)
2012-05-29 04:51 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (62.37 KB, patch)
2012-05-29 04:53 PDT, Dongwoo Joshua Im (dwim)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (62.21 KB, patch)
2012-05-30 00:09 PDT, Dongwoo Joshua Im (dwim)
abarth: review-
Details | Formatted Diff | Diff
Patch (56.76 KB, patch)
2012-06-01 23:50 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (56.65 KB, patch)
2012-06-03 19:43 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (58.25 KB, patch)
2012-06-04 01:43 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
patch for landing (60.54 KB, patch)
2012-06-04 21:49 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongwoo Joshua Im (dwim) 2011-11-27 17:54:37 PST
Regarding the HTML5 specification, 
(http://dev.w3.org/html5/spec/Overview.html#custom-handlers)
two APIs are added for Custom Scheme Handler.

One is 'isProtocolHandlerRegistered' to query whether the specific URL is registered or not. 

The other is 'unregisterProtocolHandler' to remove the registered URL.

I'm preparing the patch to add these APIs now,
I'll upload in a few days.

Thanks.
Comment 1 Alexey Proskuryakov 2011-11-28 10:35:07 PST
Since this is a new feature, please send an e-mail to webkit-dev to discuss, so that interested parties could comment on implementation strategy, as well as on whether we want this feature in WebKit.
Comment 2 Dongwoo Joshua Im (dwim) 2011-12-04 23:55:10 PST
Created attachment 117850 [details]
Patch

I'm uploading the first patch of this bug.

I've discussed in webkit-dev about this APIs.
https://lists.webkit.org/pipermail/webkit-dev/2011-November/018687.html
Comment 3 WebKit Review Bot 2011-12-05 00:03:36 PST
Comment on attachment 117850 [details]
Patch

Attachment 117850 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10736206
Comment 4 Dongwoo Joshua Im (dwim) 2011-12-05 01:35:47 PST
Created attachment 117856 [details]
Patch
Comment 5 WebKit Review Bot 2011-12-05 01:42:08 PST
Comment on attachment 117856 [details]
Patch

Attachment 117856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10689648
Comment 6 Tony Chang 2011-12-05 10:46:23 PST
+koz who implemented registerProtocolHandler.  Maybe he can do an informal review.
Comment 7 Tony Chang 2011-12-05 10:46:47 PST
Comment on attachment 117856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1024
> +unsigned HTMLInputElement::height() const
> +{
> +    return m_inputType->height();
> +}

Did you mean to include these changes in this patch?
Comment 8 Adam Barth 2011-12-05 10:54:17 PST
Comment on attachment 117856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review

This patch has a bunch of unrelated changes.

> Source/WebCore/page/Chrome.cpp:366
> +String Chrome::isProtocolHandlerRegistered(const String& scheme, const String& baseURL, const String& url)
> +{
> +    return m_client->isProtocolHandlerRegistered(scheme, baseURL, url);
> +}
> +void Chrome::unregisterProtocolHandler(const String& scheme, const String& baseURL, const String& url)
> +{
> +    m_client->unregisterProtocolHandler(scheme, baseURL, url);
> +}

Functions should have blank lines between them.

> Source/WebCore/page/Navigator.cpp:290
> +    String declined = String("declined");

You don't need to call the string constructor explicitly.

> Source/WebCore/page/Navigator.cpp:297
> +    if (!document)
> +        return declined;

This can never occur.

> Source/WebCore/page/Navigator.cpp:299
> +    String baseURL = document->baseURL().baseAsString();

Why the base URL?

> Source/WebCore/page/Navigator.cpp:309
> +    Page* page = m_frame->page();
> +    if (!page)
> +        return declined;

Why not check for this at the top?
Comment 9 Dongwoo Joshua Im (dwim) 2011-12-07 15:39:18 PST
Created attachment 118284 [details]
Patch
Comment 10 Dongwoo Joshua Im (dwim) 2011-12-07 15:45:24 PST
(In reply to comment #7)
> (From update of attachment 117856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:1024
> > +unsigned HTMLInputElement::height() const
> > +{
> > +    return m_inputType->height();
> > +}
> 
> Did you mean to include these changes in this patch?

It mixed with other patch. 
I've uploaded revised patch.
Comment 11 Dongwoo Joshua Im (dwim) 2011-12-07 15:50:02 PST
(In reply to comment #8)
> (From update of attachment 117856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review
> 
> This patch has a bunch of unrelated changes.
> 
> > Source/WebCore/page/Chrome.cpp:366
> > +String Chrome::isProtocolHandlerRegistered(const String& scheme, const String& baseURL, const String& url)
> > +{
> > +    return m_client->isProtocolHandlerRegistered(scheme, baseURL, url);
> > +}
> > +void Chrome::unregisterProtocolHandler(const String& scheme, const String& baseURL, const String& url)
> > +{
> > +    m_client->unregisterProtocolHandler(scheme, baseURL, url);
> > +}
> 
> Functions should have blank lines between them.
> 

Yes.

> > Source/WebCore/page/Navigator.cpp:290
> > +    String declined = String("declined");
> 
> You don't need to call the string constructor explicitly.
> 

I fixed this.

> > Source/WebCore/page/Navigator.cpp:297
> > +    if (!document)
> > +        return declined;
> 
> This can never occur.
> 

I removed these lines.

> > Source/WebCore/page/Navigator.cpp:299
> > +    String baseURL = document->baseURL().baseAsString();
> 
> Why the base URL?
> 

The baseURL might be needed at browser side.
The "registerProtocolHandler" also pass the baseURL as one of the parameters.

> > Source/WebCore/page/Navigator.cpp:309
> > +    Page* page = m_frame->page();
> > +    if (!page)
> > +        return declined;
> 
> Why not check for this at the top?

I moved this to the top.
Comment 12 Adam Barth 2011-12-07 15:54:22 PST
Comment on attachment 117856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review

>>> Source/WebCore/page/Navigator.cpp:299
>>> +    String baseURL = document->baseURL().baseAsString();
>> 
>> Why the base URL?
> 
> The baseURL might be needed at browser side.
> The "registerProtocolHandler" also pass the baseURL as one of the parameters.

Needed for what?
Comment 13 Dongwoo Joshua Im (dwim) 2011-12-07 16:30:24 PST
(In reply to comment #12)
> (From update of attachment 117856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117856&action=review
> 
> >>> Source/WebCore/page/Navigator.cpp:299
> >>> +    String baseURL = document->baseURL().baseAsString();
> >> 
> >> Why the base URL?
> > 
> > The baseURL might be needed at browser side.
> > The "registerProtocolHandler" also pass the baseURL as one of the parameters.
> 
> Needed for what?

Regarding the Chromium port implementation,
they use the baseURL to resolve the URL.

You can find the source code in RenderView::registerProtocolHandler function, 
in render_view.cc file in the Chromium source code.

IMHO, this kind of check is also needed for these two new APIs.
Comment 14 Dongwoo Joshua Im (dwim) 2011-12-07 18:26:33 PST
Created attachment 118303 [details]
Patch
Comment 15 WebKit Review Bot 2011-12-07 20:03:44 PST
Comment on attachment 118303 [details]
Patch

Attachment 118303 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10807043
Comment 16 Dongwoo Joshua Im (dwim) 2012-01-10 19:23:02 PST
Created attachment 121962 [details]
Patch
Comment 17 WebKit Review Bot 2012-01-10 19:25:12 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 18 WebKit Review Bot 2012-01-10 19:44:36 PST
Comment on attachment 121962 [details]
Patch

Attachment 121962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11207025
Comment 19 Darin Fisher (:fishd, Google) 2012-01-11 12:41:05 PST
Comment on attachment 121962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121962&action=review

> Source/WebKit/chromium/public/WebViewClient.h:332
> +    // Registers a new URL handler for the given protocol.

This comment is wrong.  It should say that the protocol handler association is removed.

> Source/WebKit/chromium/public/WebViewClient.h:334
> +                                         const WebString& baseUrl,

nit: indentation
nit: baseUrl -> baseURL

per the style guide, acronyms should be lowercase if they are at the start of the word or
all uppercase if they appear elsewhere.
Comment 20 Gyuyoung Kim 2012-01-11 19:28:41 PST
Comment on attachment 121962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121962&action=review

> Source/WebCore/loader/EmptyClients.h:5
> + * Copyright (C) 2011 Samsung Electronics. All rights reserved.

Change 2011 with 2012

> Source/WebCore/page/Chrome.h:4
> + * Copyright (C) 2011 Samsung Electronics. All rights reserved.

ditto.

> Source/WebCore/page/ChromeClient.h:4
> + * Copyright (C) 2011 Samsung Electronics. All rights reserved.

ditto.

> Source/WebCore/page/ChromeClient.h:144
> +

I think this is unneeded line.

> Source/WebCore/page/Navigator.cpp:7
> + *  Copyright (C) 2011 Samsung Electronics. All rights reserved.

ditto.

> Source/WebCore/page/Navigator.h:3
> +    Copyright (C) 2011 Samsung Electronics. All rights reserved.

ditto.
Comment 21 Adam Barth 2012-03-15 10:52:16 PDT
Comment on attachment 121962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121962&action=review

> Source/WebCore/page/Chrome.cpp:374
> +String Chrome::isProtocolHandlerRegistered(const String& scheme, const String& baseURL, const String& url)
> +{
> +    return m_client->isProtocolHandlerRegistered(scheme, baseURL, url);
> +}
> +
> +void Chrome::unregisterProtocolHandler(const String& scheme, const String& baseURL, const String& url)
> +{
> +    m_client->unregisterProtocolHandler(scheme, baseURL, url);
> +}

This functions are unnecessary.  The ChromeClient is visible publicly.  You can just call methods on it directly.  (Feel free to remove Chrome::registerProtocolHandler as well.)

> Source/WebCore/page/Navigator.cpp:289
> +String Navigator::isProtocolHandlerRegistered(const String& scheme, const String& url, ExceptionCode& ec)

I thought we moved Navigator::registerProtocolHandler out of Navigator.cpp.

> Source/WebCore/page/Navigator.cpp:292
> +        return "declined";

This should be a constant so you don't need to repeat it four times.
Comment 22 Dongwoo Joshua Im (dwim) 2012-03-15 18:53:00 PDT
(In reply to comment #21)
> (From update of attachment 121962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121962&action=review
> 
> > Source/WebCore/page/Chrome.cpp:374
> > +String Chrome::isProtocolHandlerRegistered(const String& scheme, const String& baseURL, const String& url)
> > +{
> > +    return m_client->isProtocolHandlerRegistered(scheme, baseURL, url);
> > +}
> > +
> > +void Chrome::unregisterProtocolHandler(const String& scheme, const String& baseURL, const String& url)
> > +{
> > +    m_client->unregisterProtocolHandler(scheme, baseURL, url);
> > +}
> 
> This functions are unnecessary.  The ChromeClient is visible publicly.  You can just call methods on it directly.  (Feel free to remove Chrome::registerProtocolHandler as well.)

Then, I will remove the three functions from Chrome.h/cpp.

> 
> > Source/WebCore/page/Navigator.cpp:289
> > +String Navigator::isProtocolHandlerRegistered(const String& scheme, const String& url, ExceptionCode& ec)
> 
> I thought we moved Navigator::registerProtocolHandler out of Navigator.cpp.
> 
> > Source/WebCore/page/Navigator.cpp:292
> > +        return "declined";
> 
> This should be a constant so you don't need to repeat it four times.

OK.
Comment 23 Dongwoo Joshua Im (dwim) 2012-05-23 01:36:15 PDT
Created attachment 143498 [details]
Patch

I've added the EFL port implementation of these APIs in the patch.
Comment 24 WebKit Review Bot 2012-05-23 02:22:17 PDT
Comment on attachment 143498 [details]
Patch

Attachment 143498 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12773147
Comment 25 Dongwoo Joshua Im (dwim) 2012-05-23 04:28:06 PDT
Created attachment 143530 [details]
Patch
Comment 26 Dongwoo Joshua Im (dwim) 2012-05-23 18:20:04 PDT
Created attachment 143693 [details]
Patch

I've re-based the patch.
Comment 27 WebKit Review Bot 2012-05-23 18:23:26 PDT
Attachment 143693 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebKit/chromium/src/ChromeClientImpl.cpp:503:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Dongwoo Joshua Im (dwim) 2012-05-23 18:27:56 PDT
Created attachment 143696 [details]
Patch
Comment 29 WebKit Review Bot 2012-05-23 20:12:40 PDT
Comment on attachment 143696 [details]
Patch

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

New failing tests:
fast/dom/navigator-detached-no-crash.html
fast/dom/unregister-protocol-handler.html
Comment 30 WebKit Review Bot 2012-05-23 20:12:46 PDT
Created attachment 143711 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 31 Dongwoo Joshua Im (dwim) 2012-05-23 20:20:07 PDT
Created attachment 143713 [details]
Patch

I've add the expected result for the chromium port.
That's why build bot gave cq- to previous patch.
Comment 32 WebKit Review Bot 2012-05-23 21:29:47 PDT
Comment on attachment 143713 [details]
Patch

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

New failing tests:
fast/dom/navigator-detached-no-crash.html
Comment 33 WebKit Review Bot 2012-05-23 21:29:54 PDT
Created attachment 143724 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 34 Dongwoo Joshua Im (dwim) 2012-05-23 21:44:03 PDT
Created attachment 143726 [details]
Patch

I add LayoutTests/platform/chromium/fast/dom/navigator-detached-no-crash-expected.txt file into the patch.

This is the first time I write a patch related to the chromium port, 
so that I made some mistakes.
Comment 35 Dongwoo Joshua Im (dwim) 2012-05-24 18:52:58 PDT
Do I need to split this patch as one WebCore patch and one EFL port patch?
Comment 36 Adam Barth 2012-05-25 09:27:24 PDT
Comment on attachment 143726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143726&action=review

If we were to land this patch as written, it would be wrong on PLATFORM(CHROMIUM).  Instead of breaking other ports, it might make sense to introduce a new ENABLE macro so that those ports can expose these APIs to the web when they work.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:26381
> +                RelativePath="..\page\CustomHandlersState.cpp"

The indent looks wrong here.  I think this file uses tabs.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:504
> +    notImplemented();
> +    return "declined";

This gives the wrong answer.
Comment 37 Dongwoo Joshua Im (dwim) 2012-05-28 18:28:36 PDT
(In reply to comment #36)
> (From update of attachment 143726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143726&action=review
> 
> If we were to land this patch as written, it would be wrong on PLATFORM(CHROMIUM).  Instead of breaking other ports, it might make sense to introduce a new ENABLE macro so that those ports can expose these APIs to the web when they work.
> 

Then, I will use ENABLE_CUSTOM_SCHEME_HANDLER, which is the name of the W3C spec.
After this patch landed, we can remove ENABLE_REGISTER_PROTOCOL_HANDLER, and then use ENABLE_CUSTOM_SCHEME_HANDLER macro only.
Comment 38 Dongwoo Joshua Im (dwim) 2012-05-28 18:34:11 PDT
(In reply to comment #36)
> (From update of attachment 143726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143726&action=review
> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:26381
> > +                RelativePath="..\page\CustomHandlersState.cpp"
> 
> The indent looks wrong here.  I think this file uses tabs.

Ok. I will fix it.

> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:504
> > +    notImplemented();
> > +    return "declined";
> 
> This gives the wrong answer.

Yes.
I think I can change like below, because I will introduce new macro which is disabled in Chromium port.

    notImplemented();
+    ASSERT_NOT_REACHED();
+    return "";
-    return "declined";
Comment 39 Adam Barth 2012-05-28 19:00:51 PDT
> Then, I will use ENABLE_CUSTOM_SCHEME_HANDLER, which is the name of the W3C spec.
> After this patch landed, we can remove ENABLE_REGISTER_PROTOCOL_HANDLER, and then use ENABLE_CUSTOM_SCHEME_HANDLER macro only.

No, we need two ENABLE macros for a while because some ports have implemented the backend for the existing functions but not for the functions you're adding in this patch.
Comment 40 Dongwoo Joshua Im (dwim) 2012-05-29 04:51:03 PDT
Created attachment 144521 [details]
Patch

I've fixed the patch regarding the Adam's comments.
Comment 41 Dongwoo Joshua Im (dwim) 2012-05-29 04:53:02 PDT
Created attachment 144523 [details]
Patch
Comment 42 Build Bot 2012-05-29 05:00:17 PDT
Comment on attachment 144523 [details]
Patch

Attachment 144523 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12844486
Comment 43 Dongwoo Joshua Im (dwim) 2012-05-30 00:09:59 PDT
Created attachment 144730 [details]
Patch
Comment 44 Adam Barth 2012-06-01 00:58:43 PDT
Comment on attachment 144730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144730&action=review

This patch looks like it's in pretty good shape.  I've left you a couple minor comments below.  Once those are addressed, we should be all set.  Thanks!

> Source/WebCore/page/CustomHandlersState.cpp:7
> +    version 2.1 of the License, or (at your option) any later version.

Typically new files are licensed using BSD, but it's up to you.

> Source/WebCore/page/CustomHandlersState.cpp:27
> +String customHandlersStateString(CustomHandlersState state)

Rather than creating a whole new file for this rather simple function, you can declare the CustomHandlersState enum in ChromeClient.h and have the client method return an enum value rather than a string.  Then you can do the translation between enum values and strings in a static function in NavigatorRegisterProtocolHandler.cpp.

> Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:157
> +    if (!document)
> +        return declined;

document can never be 0 here.

> Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:182
> +    if (!document)
> +        return;

ditto

> Source/WebCore/page/NavigatorRegisterProtocolHandler.idl:27
> +#if defined(ENABLE_REGISTER_PROTOCOL_HANDLER) && ENABLE_REGISTER_PROTOCOL_HANDLER

You can just use the [Conditional] attribute on the method.
Comment 45 Dongwoo Joshua Im (dwim) 2012-06-01 03:23:43 PDT
(In reply to comment #44)
> (From update of attachment 144730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144730&action=review
> 
> This patch looks like it's in pretty good shape.  I've left you a couple minor comments below.  Once those are addressed, we should be all set.  Thanks!
> 
> > Source/WebCore/page/CustomHandlersState.cpp:7
> > +    version 2.1 of the License, or (at your option) any later version.
> 
> Typically new files are licensed using BSD, but it's up to you.
> 
> > Source/WebCore/page/CustomHandlersState.cpp:27
> > +String customHandlersStateString(CustomHandlersState state)
> 
> Rather than creating a whole new file for this rather simple function, you can declare the CustomHandlersState enum in ChromeClient.h and have the client method return an enum value rather than a string.  Then you can do the translation between enum values and strings in a static function in NavigatorRegisterProtocolHandler.cpp.
> 

Uhm.. Ok. I will try it.

> > Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:157
> > +    if (!document)
> > +        return declined;
> 
> document can never be 0 here.
> 

Ok. I'll remove this line.

> > Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:182
> > +    if (!document)
> > +        return;
> 
> ditto
> 

Ok. I'll remove this line.

> > Source/WebCore/page/NavigatorRegisterProtocolHandler.idl:27
> > +#if defined(ENABLE_REGISTER_PROTOCOL_HANDLER) && ENABLE_REGISTER_PROTOCOL_HANDLER
> 
> You can just use the [Conditional] attribute on the method.

Ok. I will try it.
Comment 46 Dongwoo Joshua Im (dwim) 2012-06-01 23:50:49 PDT
Created attachment 145435 [details]
Patch

I've fixed the patch regarding abarth's comments.
Comment 47 Gyuyoung Kim 2012-06-02 00:12:33 PDT
Comment on attachment 145435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145435&action=review

> Source/WebKit/efl/ewk/ewk_view_private.h:158
> +Ewk_Custom_Handlers_State ewk_custom_handler_is_protocol_handler_registered(Ewk_Custom_Handler_Data* data);

EFL WK1 decided to divide up ewk_private.h into each file's XXX_private.h file in order to maintain internal functions more easily. So, I think these internal functions should move to ewk_custom_handler_private.h file. I file a bug 88163. Please update this patch after Bug 88163 is landed.
Comment 48 Dongwoo Joshua Im (dwim) 2012-06-03 19:43:03 PDT
Created attachment 145508 [details]
Patch

I've revised the patch regarding the Gyuyoung's patch which had blocked this bug.
Comment 49 Gyuyoung Kim 2012-06-03 19:59:47 PDT
Comment on attachment 145508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145508&action=review

> Source/WebKit/efl/ewk/ewk_custom_handler.cpp:32
> +#if ENABLE(CUSTOM_SCHEME_HANDLER)

Could you write function description as ewk_view.cpp ?

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L2925

> Source/WebKit/efl/ewk/ewk_custom_handler.cpp:36
> +    evas_object_smart_callback_call(data->ewkView, "protocolhandler,isregistered", data);

If you wanna add new signal, you have to write signal description to ewk_view.h.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35
Comment 50 Dongwoo Joshua Im (dwim) 2012-06-04 01:43:05 PDT
Created attachment 145541 [details]
Patch

I've added some description.
Comment 51 WebKit Review Bot 2012-06-04 01:54:32 PDT
Comment on attachment 145541 [details]
Patch

Rejecting attachment 145541 [details] from review queue.

dw.im@samsung.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 52 Dongwoo Joshua Im (dwim) 2012-06-04 02:11:43 PDT
(In reply to comment #51)
> (From update of attachment 145541 [details])
> Rejecting attachment 145541 [details] from review queue.
> 
> dw.im@samsung.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.
> 
> - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
> 
> - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

Uh oh..
I don't know why I check '+' rather than '?' which I actually want to select.
I think that's because... office is too hot.. ;)

Anyway, sorry to make this kind of mistake.
Comment 53 Adam Barth 2012-06-04 11:12:22 PDT
> Anyway, sorry to make this kind of mistake.

No worries.  That's why the bot makes that check.  :)
Comment 54 Gyuyoung Kim 2012-06-04 19:58:13 PDT
Comment on attachment 145541 [details]
Patch

It looks this feature just support to three APIs. register, unregister, check if regitster. As we discuss on private channel, specification just mentioned these functionalities, I'd like to give informal r+ on EFL port layer as initial implementation.
Comment 55 Adam Barth 2012-06-04 20:19:51 PDT
Comment on attachment 145541 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145541&action=review

Great!  Thanks.

> Source/WebCore/page/NavigatorRegisterProtocolHandler.idl:-23
> -        Conditional=REGISTER_PROTOCOL_HANDLER,

I would have left this as Conditional=REGISTER_PROTOCOL_HANDLER|CUSTOM_SCHEME_HANDLER, but this is fine too.
Comment 56 WebKit Review Bot 2012-06-04 21:05:07 PDT
Comment on attachment 145541 [details]
Patch

Rejecting attachment 145541 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 Source/cmakeconfig.h.cmake
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitperl/FeatureList.pm
patching file WebKitLibraries/ChangeLog
patching file WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops
patching file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12895483
Comment 57 Dongwoo Joshua Im (dwim) 2012-06-04 21:49:06 PDT
Created attachment 145693 [details]
patch for landing
Comment 58 WebKit Review Bot 2012-06-05 04:32:40 PDT
Comment on attachment 145693 [details]
patch for landing

Clearing flags on attachment: 145693

Committed r119482: <http://trac.webkit.org/changeset/119482>
Comment 59 WebKit Review Bot 2012-06-05 04:32:49 PDT
All reviewed patches have been landed.  Closing bug.