1398169 - Remove registerContentHandler()

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 2

[Anne (:annevk)]

It's been removed from HTML now.

web-platform-tests:

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

Comment 3

[Jonathan Kingston [:jkt] he/him]

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 4

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1398169 - Use pref to disable registerContentHandler in non stable builds.

Review commit: https://reviewboard.mozilla.org/r/209232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209232/

Comment 5

[Jonathan Kingston [:jkt] he/him]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209232/diff/1-2/

Comment 6

[Dão Gottwald [:dao]]

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?

Comment 7

[Jonathan Kingston [:jkt] he/him]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209232/diff/2-3/

Comment 8

[Anne (:annevk)]

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.

Comment 9

[Jonathan Kingston [:jkt] he/him]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209232/diff/3-4/

Comment 10

[Jonathan Kingston [:jkt] he/him]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209232/diff/4-5/

Comment 11

[Jonathan Kingston [:jkt] he/him]

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!

Comment 12

[Jonathan Kingston [:jkt] he/him]

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.

Comment 13

[Mike Taylor [:miketaylr]]

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

The bits in all.js lgtm. Thanks!

Comment 14

[Dão Gottwald [:dao]]

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.

Comment 15

[Mike Taylor [:miketaylr]]

(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).

Comment 16

[Jonathan Kingston [:jkt] he/him]

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.

Comment 17

[Dão Gottwald [:dao]]

(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

Comment 18

[Jonathan Kingston [:jkt] he/him]

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

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

Comment 19

[Jonathan Kingston [:jkt] he/him]

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

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

Comment 20

[Mozilla Autoland]

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 21

[Jonathan Kingston [:jkt] he/him]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209232/diff/5-6/

Comment 22

[Mozilla Autoland]

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

Comment 23

[Andrea Marchesini [:baku]]

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

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/189a02700a27
Use pref to disable registerContentHandler in non stable builds. r=baku,dao

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/189a02700a27

Comment 26

[Chris Mills [:cmills]]

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!

Comment 27

[Jonathan Kingston [:jkt] he/him]

Looks great to me thank you!

Comment 28

[Kohei Yoshino]

Have posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/navigator-registercontenthandler-has-been-removed/