Closed Bug 1398169 Opened 7 years ago Closed 6 years ago

Remove registerContentHandler()

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: annevk, Assigned: jkt)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.
59 bytes, text/x-review-board-request
dao
: review+
baku
: review+
Details 
We're the only browser that implements it and we don't even follow the standard for it.

I've proposed it be removed from the standard: https://github.com/whatwg/html/pull/3027.
Seems not an urgent thing to me, but it helps us clean up several long-standing issues ... feel free to let me know if we should get it prioritized.
Priority: -- → P3
It's been removed from HTML now.

web-platform-tests:

  https://github.com/w3c/web-platform-tests/pull/7349
This looks like it could probably be replaced by the Web Share Target API[1] if we have any intent to work on that?
I'm aware this isn't designed for feeds however feels related enough especially as the API doesn't suggest a requirement of a HTML document etc.

If a website was able to register itself as a sharing target then we could show them in our reader preview.

[1] https://github.com/WICG/web-share-target/blob/master/docs/explainer.md
Assignee: nobody → jkt
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review215050

::: browser/base/content/browser-feeds.js:604
(Diff revision 2)
>          this._setPref(readerPref, settings.reader);
>          handler = null;
>  
>          switch (settings.reader) {
>            case "web":
> -            // This is a web set URI by content using window.registerContentHandler()
> +            // This is the default web handlers set to handler prefs "browser.contentHandlers.types.0.type"

I'm having a hard time reading and understanding this sentence...

::: dom/webidl/Navigator.webidl:85
(Diff revision 2)
> -  void registerContentHandler(DOMString mimeType, DOMString url, DOMString title);
>    // NOT IMPLEMENTED
>    //DOMString isProtocolHandlerRegistered(DOMString scheme, DOMString url);
>    //DOMString isContentHandlerRegistered(DOMString mimeType, DOMString url);
>    //void unregisterProtocolHandler(DOMString scheme, DOMString url);
>    //void unregisterContentHandler(DOMString mimeType, DOMString url);

Remove mentions of isContentHandlerRegistered and unregisterContentHandler as well?
Attachment #8938592 - Flags: review?(dao+bmo) → review+
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review215346

The web-platform-tests expectation changes look good. I can't really review though.
Attachment #8938592 - Flags: review?(annevk)
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

Hey :dao,

This patch has changed to use a pref instead to disable the code, it makes it a very different patch but mozreview has carried over your r+. Could you check it again pleas?

Mike, The changes were made based upon your feedback on dev-platform, is there anything else you would like in the patch?

Thanks both!
Attachment #8938592 - Flags: review+
Attachment #8938592 - Flags: feedback?(miket)
Attachment #8938592 - Flags: feedback?(dao+bmo)
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

Seems I can r? you on bugzilla here after removing it.
Attachment #8938592 - Flags: feedback?(dao+bmo) → review?(dao+bmo)
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

The bits in all.js lgtm. Thanks!
Attachment #8938592 - Flags: feedback?(miket) → feedback+
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review217072

::: modules/libpref/init/all.js:115
(Diff revision 5)
>  
> +// Remove navigator.registerContentHandler
> +#ifdef EARLY_BETA_OR_EARLIER
> +pref("dom.registerContentHandler.enabled", false);
> +#else
> +pref("dom.registerContentHandler.enabled", true);

I don't understand what you're doing here. AIUI, the pref should be false regardless of the release channel. Also, please make sure to file a followup on removing this pref again later.
Attachment #8938592 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8938592 [details]
> Bug 1398169 - Use pref to disable registerContentHandler in non stable
> builds.
> 
> https://reviewboard.mozilla.org/r/209232/#review217072
> 
> ::: modules/libpref/init/all.js:115
> (Diff revision 5)
> >  
> > +// Remove navigator.registerContentHandler
> > +#ifdef EARLY_BETA_OR_EARLIER
> > +pref("dom.registerContentHandler.enabled", false);
> > +#else
> > +pref("dom.registerContentHandler.enabled", true);
> 
> I don't understand what you're doing here. AIUI, the pref should be false
> regardless of the release channel. Also, please make sure to file a followup
> on removing this pref again later.

The idea is to unship on non-release for a few releases to see if we get any reports of high-profile bustage (and eventually, remove the whole thing if nothing comes thru).
I will file a follow up for the removal in stable, however I just want to check you are ok with the current plan this is just being removed from nightly and beta to test the web-compat impact. I have emailed the dev-platform list to inform them of this change too.
Flags: needinfo?(dao+bmo)
(In reply to Jonathan Kingston [:jkt] from comment #16)
> I will file a follow up for the removal in stable, however I just want to
> check you are ok with the current plan this is just being removed from
> nightly and beta to test the web-compat impact. I have emailed the
> dev-platform list to inform them of this change too.

ok
Flags: needinfo?(dao+bmo)
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review217794
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review217796
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ac46cf095fd9 -d 5f6417128acb: rebasing 441882:ac46cf095fd9 "Bug 1398169 - Use pref to disable registerContentHandler in non stable builds. r=dao" (tip)
merging dom/webidl/Navigator.webidl
merging modules/libpref/init/all.js
merging testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/historical.window.js.ini
warning: conflicts while merging testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/historical.window.js.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 6 changes to 6 files
remote: 
remote: WebIDL file dom/webidl/Navigator.webidl altered in changeset 1f637b927c35 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Attachment #8938592 - Flags: review?(amarchesini)
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review218142
Attachment #8938592 - Flags: review?(amarchesini) → review+
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/189a02700a27
Use pref to disable registerContentHandler in non stable builds. r=baku,dao
https://hg.mozilla.org/mozilla-central/rev/189a02700a27
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I've documented this by adding an entry to our experimental features page, since it is not in release yet; see the bottom of the following table.

https://developer.mozilla.org/en-US/Firefox/Experimental_features#DOM

Let me kn ow if this looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Looks great to me thank you!
Depends on: 1460481
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: