WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
101199
Disable registerProtocolHandler on Android
https://bugs.webkit.org/show_bug.cgi?id=101199
Summary
Disable registerProtocolHandler on Android
Miguel Garcia
Reported
2012-11-05 02:35:43 PST
Disable registerProtocolHandler on Android
Attachments
Patch
(1.36 KB, patch)
2012-11-05 02:36 PST
,
Miguel Garcia
no flags
Details
Formatted Diff
Diff
Patch
(1.38 KB, patch)
2012-11-05 02:56 PST
,
Miguel Garcia
no flags
Details
Formatted Diff
Diff
Patch
(1.46 KB, patch)
2012-11-05 05:24 PST
,
Miguel Garcia
no flags
Details
Formatted Diff
Diff
Patch
(1.48 KB, patch)
2012-11-05 05:46 PST
,
Miguel Garcia
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Miguel Garcia
Comment 1
2012-11-05 02:36:48 PST
Created
attachment 172300
[details]
Patch
Peter Beverloo
Comment 2
2012-11-05 02:49:10 PST
Comment on
attachment 172300
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=172300&action=review
Thanks for the patch, Miguel! Please find two nits in-line. You may also want to consider requesting commit-queue, which is useful as you're not a committer yet yourself. You can do this in the patch' Details section, or by supplying the "--request-commit" argument to webkit-patch upload.
> Source/WebKit/chromium/ChangeLog:3 > + Disable registerProtocolHandler on Android
Since this patch only touches Chromium-specific code, it's best to prefix the bug title with "[Chromium]", especially since there is no more Android port in WebKit.
> Source/WebKit/chromium/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
Any line with "(OOPS!)" will make presubmit unhappy. The only exception here is the "Reviewed by NOBODY (OOPS!)" line, which will be filled in automatically by the tools. In this case, it's important for the reviewer and other members of the Chromium project to understand why registerProtocolHandler is being disabled for Android. A bit of rationale can help here, so something like the following text would be clearer: "Chromium for Android has been exposing registerProtocolHandler, but the feature wasn't actually wired up internally. Disable the feature to avoid breaking feature detection until we can implement it properly."
Miguel Garcia
Comment 3
2012-11-05 02:56:12 PST
Created
attachment 172304
[details]
Patch
Miguel Garcia
Comment 4
2012-11-05 02:57:36 PST
(In reply to
comment #3
)
> Created an attachment (id=172304) [details] > Patch
Thanks for the comments Peter! Uploaded a new patch with your suggestions
Julien Chaffraix
Comment 5
2012-11-05 04:56:29 PST
Comment on
attachment 172304
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=172304&action=review
LGTM, Adam is usually your man for such changes but it is simple enough.
> Source/WebKit/chromium/ChangeLog:8 > + Disalble registerProtocolHandler since it is not properly implemented (yet) and could confuse developers trying to use it in android.
Typo: disalble. Also let's be precise! Peter's phrasing was more accurate from this perspective as it mentioned that it is a platform issue (reading your wording I thought it was an issue in the feature).
Miguel Garcia
Comment 6
2012-11-05 05:24:03 PST
Created
attachment 172317
[details]
Patch
Miguel Garcia
Comment 7
2012-11-05 05:25:30 PST
Thanks for the quick review! Changed to use Peter's wording instead. (In reply to
comment #6
)
> Created an attachment (id=172317) [details] > Patch
Miguel Garcia
Comment 8
2012-11-05 05:46:32 PST
Created
attachment 172322
[details]
Patch
Peter Beverloo
Comment 9
2012-11-05 05:48:32 PST
Comment on
attachment 172322
[details]
Patch Thank you Miguel! Setting cq+.
WebKit Review Bot
Comment 10
2012-11-05 06:28:17 PST
Comment on
attachment 172322
[details]
Patch Clearing flags on attachment: 172322 Committed
r133465
: <
http://trac.webkit.org/changeset/133465
>
WebKit Review Bot
Comment 11
2012-11-05 06:28:22 PST
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