Remove registerContentHandler()

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: annevk, Assigned: jkt)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 2

2 years ago
It's been removed from HTML now.

web-platform-tests:

  https://github.com/w3c/web-platform-tests/pull/7349
(Assignee)

Comment 3

a year ago
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
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → jkt
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 8

a year ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
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)
(Assignee)

Comment 12

a year ago
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 14

a year ago
mozreview-review
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).
(Assignee)

Comment 16

a year ago
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)
(Assignee)

Comment 18

a year ago
mozreview-review
Comment on attachment 8938592 [details]
Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

https://reviewboard.mozilla.org/r/209232/#review217794
(Assignee)

Comment 19

a year ago
mozreview-review
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)
Comment hidden (mozreview-request)
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
(Assignee)

Updated

a year ago
Attachment #8938592 - Flags: review?(amarchesini)

Comment 23

a year ago
mozreview-review
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+

Comment 24

a year ago
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

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/189a02700a27
Status: NEW → RESOLVED
Last Resolved: a year ago
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!
(Assignee)

Comment 27

a year ago
Looks great to me thank you!
(Assignee)

Updated

a year ago
Depends on: 1460481
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.