RESOLVED FIXED 73176
Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'.
https://bugs.webkit.org/show_bug.cgi?id=73176
Summary Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'.
Dongwoo Joshua Im (dwim)
Reported 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.
Attachments
Patch (26.69 KB, patch)
2011-12-04 23:55 PST, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Patch (25.88 KB, patch)
2011-12-05 01:35 PST, Dongwoo Joshua Im (dwim)
no flags
Patch (19.71 KB, patch)
2011-12-07 15:39 PST, Dongwoo Joshua Im (dwim)
no flags
Patch (19.71 KB, patch)
2011-12-07 18:26 PST, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Patch (22.61 KB, patch)
2012-01-10 19:23 PST, Dongwoo Joshua Im (dwim)
fishd: review-
webkit.review.bot: commit-queue-
Patch (27.93 KB, patch)
2012-05-23 01:36 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
Patch (13.91 KB, patch)
2012-05-23 04:28 PDT, Dongwoo Joshua Im (dwim)
no flags
Patch (29.71 KB, patch)
2012-05-23 18:20 PDT, Dongwoo Joshua Im (dwim)
no flags
Patch (29.71 KB, patch)
2012-05-23 18:27 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
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
Patch (30.91 KB, patch)
2012-05-23 20:20 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit-queue-
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
Patch (32.52 KB, patch)
2012-05-23 21:44 PDT, Dongwoo Joshua Im (dwim)
abarth: review-
Patch (62.40 KB, patch)
2012-05-29 04:51 PDT, Dongwoo Joshua Im (dwim)
no flags
Patch (62.37 KB, patch)
2012-05-29 04:53 PDT, Dongwoo Joshua Im (dwim)
buildbot: commit-queue-
Patch (62.21 KB, patch)
2012-05-30 00:09 PDT, Dongwoo Joshua Im (dwim)
abarth: review-
Patch (56.76 KB, patch)
2012-06-01 23:50 PDT, Dongwoo Joshua Im (dwim)
no flags
Patch (56.65 KB, patch)
2012-06-03 19:43 PDT, Dongwoo Joshua Im (dwim)
no flags
Patch (58.25 KB, patch)
2012-06-04 01:43 PDT, Dongwoo Joshua Im (dwim)
no flags
patch for landing (60.54 KB, patch)
2012-06-04 21:49 PDT, Dongwoo Joshua Im (dwim)
no flags
Alexey Proskuryakov
Comment 1 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.
Dongwoo Joshua Im (dwim)
Comment 2 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
WebKit Review Bot
Comment 3 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
Dongwoo Joshua Im (dwim)
Comment 4 2011-12-05 01:35:47 PST
WebKit Review Bot
Comment 5 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
Tony Chang
Comment 6 2011-12-05 10:46:23 PST
+koz who implemented registerProtocolHandler. Maybe he can do an informal review.
Tony Chang
Comment 7 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?
Adam Barth
Comment 8 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?
Dongwoo Joshua Im (dwim)
Comment 9 2011-12-07 15:39:18 PST
Dongwoo Joshua Im (dwim)
Comment 10 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.
Dongwoo Joshua Im (dwim)
Comment 11 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.
Adam Barth
Comment 12 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?
Dongwoo Joshua Im (dwim)
Comment 13 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.
Dongwoo Joshua Im (dwim)
Comment 14 2011-12-07 18:26:33 PST
WebKit Review Bot
Comment 15 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
Dongwoo Joshua Im (dwim)
Comment 16 2012-01-10 19:23:02 PST
WebKit Review Bot
Comment 17 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.
WebKit Review Bot
Comment 18 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
Darin Fisher (:fishd, Google)
Comment 19 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.
Gyuyoung Kim
Comment 20 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.
Adam Barth
Comment 21 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.
Dongwoo Joshua Im (dwim)
Comment 22 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.
Dongwoo Joshua Im (dwim)
Comment 23 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.
WebKit Review Bot
Comment 24 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
Dongwoo Joshua Im (dwim)
Comment 25 2012-05-23 04:28:06 PDT
Dongwoo Joshua Im (dwim)
Comment 26 2012-05-23 18:20:04 PDT
Created attachment 143693 [details] Patch I've re-based the patch.
WebKit Review Bot
Comment 27 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.
Dongwoo Joshua Im (dwim)
Comment 28 2012-05-23 18:27:56 PDT
WebKit Review Bot
Comment 29 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
WebKit Review Bot
Comment 30 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
Dongwoo Joshua Im (dwim)
Comment 31 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.
WebKit Review Bot
Comment 32 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
WebKit Review Bot
Comment 33 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
Dongwoo Joshua Im (dwim)
Comment 34 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.
Dongwoo Joshua Im (dwim)
Comment 35 2012-05-24 18:52:58 PDT
Do I need to split this patch as one WebCore patch and one EFL port patch?
Adam Barth
Comment 36 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.
Dongwoo Joshua Im (dwim)
Comment 37 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.
Dongwoo Joshua Im (dwim)
Comment 38 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";
Adam Barth
Comment 39 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.
Dongwoo Joshua Im (dwim)
Comment 40 2012-05-29 04:51:03 PDT
Created attachment 144521 [details] Patch I've fixed the patch regarding the Adam's comments.
Dongwoo Joshua Im (dwim)
Comment 41 2012-05-29 04:53:02 PDT
Build Bot
Comment 42 2012-05-29 05:00:17 PDT
Dongwoo Joshua Im (dwim)
Comment 43 2012-05-30 00:09:59 PDT
Adam Barth
Comment 44 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.
Dongwoo Joshua Im (dwim)
Comment 45 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.
Dongwoo Joshua Im (dwim)
Comment 46 2012-06-01 23:50:49 PDT
Created attachment 145435 [details] Patch I've fixed the patch regarding abarth's comments.
Gyuyoung Kim
Comment 47 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.
Dongwoo Joshua Im (dwim)
Comment 48 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.
Gyuyoung Kim
Comment 49 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
Dongwoo Joshua Im (dwim)
Comment 50 2012-06-04 01:43:05 PDT
Created attachment 145541 [details] Patch I've added some description.
WebKit Review Bot
Comment 51 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.
Dongwoo Joshua Im (dwim)
Comment 52 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.
Adam Barth
Comment 53 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. :)
Gyuyoung Kim
Comment 54 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.
Adam Barth
Comment 55 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.
WebKit Review Bot
Comment 56 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
Dongwoo Joshua Im (dwim)
Comment 57 2012-06-04 21:49:06 PDT
Created attachment 145693 [details] patch for landing
WebKit Review Bot
Comment 58 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>
WebKit Review Bot
Comment 59 2012-06-05 04:32:49 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.