WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(17)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 117856
[details]
Patch
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
Created
attachment 118284
[details]
Patch
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
Created
attachment 118303
[details]
Patch
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
Created
attachment 121962
[details]
Patch
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
Created
attachment 143530
[details]
Patch
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
Created
attachment 143696
[details]
Patch
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
Created
attachment 144523
[details]
Patch
Build Bot
Comment 42
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
Dongwoo Joshua Im (dwim)
Comment 43
2012-05-30 00:09:59 PDT
Created
attachment 144730
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug