Closed Bug 1477670 Opened 6 years ago Closed 6 years ago

Actually remove registerContentHandler implementation

Categories

(Firefox :: File Handling, enhancement, P1)

Product:
Firefox
Component:
File Handling
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(4 files)

Bug 1477670 - remove tests for registerContentHandler,
59 bytes, text/x-review-board-request
jkt
: review+
Details
Bug 1477670 - empty out DOM implementation of registerContentHandler,
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1477670 - remove content handler code from browser/,
59 bytes, text/x-review-board-request
florian
: review+
jkt
: review+
Details
Bug 1477670 - remove web feed handling prefs and tidy up user-set prefs if they exist,
59 bytes, text/x-review-board-request
flod
: review+
jkt
: review+
Details 
Per discussion in bug 1369443, I think we can just remove this. Jonathan, who else needs to sign off on that decision? I assume a DOM/webidl peer?
Flags: needinfo?(jkt)
Blocks: 1369443
Depends on: 1460481
Blocks: 1477667
Yeah I think any DOM peer should be ok to sign this off :). Baku was the previous reviewer here.
Flags: needinfo?(jkt)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs (he/him) from comment #2)
> Ploink:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9f03a199e2e6e49870cab34b4e4ae8a4a14961d7

Mostly green besides the startup load test that is angry that I didn't remove WebContentConverterRegistrar. Attaching patches with that addressed.

:baku: I'm deliberately leaving the DOM implementation behind in case we somehow end up shipping with this disabled on 62 and then finding out we need to re-enable it (in which case we can pretend to have the API and just have it no-op, which won't be detectable by sites, AIUI). It already does nothing on Android, and this seems less risky than removing everything outright, though I'm also happy to remove the Navigator.cpp/h definition, the (web)idl definition, and fix up any other wpt tests, if that's what you'd prefer.
Mike, can you confirm that any feed reader definitions in the (localized/regional, so not just MyYahoo in en-US) prefs aren't part of any formal partnerships we have with anyone?
Flags: needinfo?(mozilla)
> Mike, can you confirm that any feed reader definitions in the (localized/regional, so not just MyYahoo in en-US) prefs aren't part of any formal partnerships we have with anyone?

Yes, i can confirm that. This were all added by localizers.
Flags: needinfo?(mozilla)
Comment on attachment 8994671 [details]
Bug 1477670 - remove web feed handling prefs and tidy up user-set prefs if they exist,

https://reviewboard.mozilla.org/r/259182/#review266246

Is there any plan/follow-up to do this for /mobile?
https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/mobile/locales/en-US/chrome/region.properties#25-28
Attachment #8994671 - Flags: review?(francesco.lodolo) → review+
Blocks: 1478219
Comment on attachment 8994669 [details]
Bug 1477670 - empty out DOM implementation of registerContentHandler,

https://reviewboard.mozilla.org/r/259178/#review266256
Attachment #8994669 - Flags: review?(amarchesini) → review+
(In reply to Francesco Lodolo [:flod] from comment #10)
> Comment on attachment 8994671 [details]
> Bug 1477670 - remove web feed handling prefs and tidy up user-set prefs if
> they exist,
> 
> https://reviewboard.mozilla.org/r/259182/#review266246
> 
> Is there any plan/follow-up to do this for /mobile?
> https://searchfox.org/mozilla-central/rev/
> bdfd20ef30d521b57d5b6feeda71325e8b4cad66/mobile/locales/en-US/chrome/region.
> properties#25-28

Uhhh... this is interesting. I don't think this code runs on mobile. I could be wrong because I haven't double-checked. But it'd surprise me, because I'm 99% sure that the registerProtocolHandler code doesn't run on mobile, either, and it's in the same file...
(In reply to :Gijs (he/him) from comment #12)
> Uhhh... this is interesting. I don't think this code runs on mobile. I could
> be wrong because I haven't double-checked. But it'd surprise me, because I'm
> 99% sure that the registerProtocolHandler code doesn't run on mobile,
> either, and it's in the same file...

Those keys were added in bug 852828, but beyond that I'm quite lost (e.g., I wouldn't exactly know how to access that feature in Fennec)
https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/FeedHandler.js
(In reply to Francesco Lodolo [:flod] from comment #13)
> (In reply to :Gijs (he/him) from comment #12)
> > Uhhh... this is interesting. I don't think this code runs on mobile. I could
> > be wrong because I haven't double-checked. But it'd surprise me, because I'm
> > 99% sure that the registerProtocolHandler code doesn't run on mobile,
> > either, and it's in the same file...
> 
> Those keys were added in bug 852828, but beyond that I'm quite lost (e.g., I
> wouldn't exactly know how to access that feature in Fennec)
> https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/
> FeedHandler.js

Oh, wow. So yeah, on Fennec you can still subscribe to feeds with "My Yahoo!" (or whatever localized RSS reader is included in the mobile versions of that .properties file for other locales). But there's no feed preview, users can't customize the list of supported RSS readers with which they can subscribe (unless you count manually futzing about with about:config on your mobile device...), and there's no telemetry that'd give us an idea of if people use this. You'd have to discover the menu > page > subscribe... path (only available on sites that advertise feed support). Seems the item in the location bar long press menu (as mentioned in that bug) is gone already.

I'll file a separate bug for Fennec to consider what they want to do with it. AFAICT there's no support for registerContentHandler on fennec - the only implementor of the contractid requested from DOM is the one I'm removing in this bug, and that file is not included on fennec, so this bug won't affect Fennec in any way.
Filed bug 1478320 for the mobile side of things.
> But there's no feed preview, users can't customize the list of supported RSS readers with which they can subscribe

Yeah there is a code path I think that can make those feed reader prefs auto open (for desktop and android), it might have been broken along the way of updates though last year.
Comment on attachment 8994668 [details]
Bug 1477670 - remove tests for registerContentHandler,

https://reviewboard.mozilla.org/r/259176/#review266332

It would also be good to remove any test that uses: dom.registerContentHandler.enabled I see 3 mentions on dxr. Other than that this looks great.
Attachment #8994668 - Flags: review?(jkt) → review+
> I'm deliberately leaving the DOM implementation behind in case we somehow end up shipping with this disabled on 62 and then finding out we need to re-enable it

Sorry I missed this, feel free to ignore my comment regarding removing "dom.registerContentHandler.enabled" from tests as this makes a lot of sense.
Comment on attachment 8994670 [details]
Bug 1477670 - remove content handler code from browser/,

https://reviewboard.mozilla.org/r/259180/#review266306

Nice code removal, r+ because this makes sense to remove, but I haven't searched for code becoming dead due to this patch, or for ways to improve the remaining code, as my understanding from our IRC discussion in #fx-team is that most of what remains is going away in the next patch anyway.

::: browser/base/content/browser-feeds.js:511
(Diff revision 1)
>    _getReaderForType(feedType) {
>      let prefs = Services.prefs;
>      let handler = "bookmarks";
> -    let url;
>      // eslint-disable-next-line mozilla/use-default-preference-values
>      try {

[can be ignored] This try block could also be changed to use a default pref value as the second parameter.

::: browser/base/content/browser-feeds.js:559
(Diff revision 1)
>          this._setPref(actionPref, settings.action);
>          const readerPref = getPrefReaderForType(settings.feedType);
>          this._setPref(readerPref, settings.reader);
> -        handler = null;
>  
>          switch (settings.reader) {

[can be ignored] Did you mean to remove this switch statement too?

::: browser/components/feeds/BrowserFeeds.manifest:1
(Diff revision 1)
>  # This component must restrict its registration for the app-startup category

This comment seems obsolete.

::: browser/components/feeds/BrowserFeeds.manifest
(Diff revision 1)
>  contract @mozilla.org/browser/feeds/result-service;1 {2376201c-bbc6-472f-9b62-7548040a61c6}
>  component {49bb6593-3aff-4eb3-a068-2712c28bd58e} FeedWriter.js
>  contract @mozilla.org/browser/feeds/result-writer;1 {49bb6593-3aff-4eb3-a068-2712c28bd58e}
>  component {792a7e82-06a0-437c-af63-b2d12e808acc} WebContentConverter.js
>  contract @mozilla.org/embeddor.implemented/web-content-handler-registrar;1 {792a7e82-06a0-437c-af63-b2d12e808acc}
> -category app-startup WebContentConverter service,@mozilla.org/embeddor.implemented/web-content-handler-registrar;1 application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} application={aa3c5121-dab2-40e2-81ca-7ea25febc110}

I think this means you can remove https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/browser/base/content/test/performance/browser_startup.js#34 and close bug 1369443.

::: browser/components/feeds/FeedConverter.js:172
(Diff revision 1)
>          let handler = Services.prefs.getCharPref(getPrefActionForType(feed.type), "ask");
>  
>          if (handler != "ask") {
>            if (handler == "reader")
>              handler = Services.prefs.getCharPref(getPrefReaderForType(feed.type), "bookmarks");
>            switch (handler) {

[can be ignored] Looks like this switch can be simplified too.

::: browser/components/feeds/WebContentConverter.js:334
(Diff revision 1)
> -     [Ci.nsIWebContentConverterService,
> +     [Ci.nsIWebContentHandlerRegistrar,
> -      Ci.nsIWebContentHandlerRegistrar,
> -      Ci.nsIObserver,
>        Ci.nsIFactory]),
>  
>    _xpcom_categories: [{

This seems obsolete too. Actually, I wonder if _xpcom_categories in general isn't a leftover from Mozilla 1.* .
Attachment #8994670 - Flags: review?(florian) → review+
> (In reply to Florian Quèze [:florian] from comment #19)
> my understanding from our IRC discussion in #fx-team
> is that most of what remains is going away in the next patch anyway.

Correct. I'm hoping to also land a fix for bug 1477669 in 63, though I'll doublecheck with Marco to see that we don't end up in a situation where we've half-finished removing things and don't manage the rest for 63, especially as I'm on PTO for 2 of the weeks remaining for 63. Though it's not super bad to ship without the feed previewer but *with* live bookmark support, ideally we should do all of the removals in 1 release so it's clear for users.

> ::: browser/components/feeds/BrowserFeeds.manifest
> (Diff revision 1)
> >  contract @mozilla.org/browser/feeds/result-service;1 {2376201c-bbc6-472f-9b62-7548040a61c6}
> >  component {49bb6593-3aff-4eb3-a068-2712c28bd58e} FeedWriter.js
> >  contract @mozilla.org/browser/feeds/result-writer;1 {49bb6593-3aff-4eb3-a068-2712c28bd58e}
> >  component {792a7e82-06a0-437c-af63-b2d12e808acc} WebContentConverter.js
> >  contract @mozilla.org/embeddor.implemented/web-content-handler-registrar;1 {792a7e82-06a0-437c-af63-b2d12e808acc}
> > -category app-startup WebContentConverter service,@mozilla.org/embeddor.implemented/web-content-handler-registrar;1 application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} application={aa3c5121-dab2-40e2-81ca-7ea25febc110}
> 
> I think this means you can remove
> https://searchfox.org/mozilla-central/rev/
> bdfd20ef30d521b57d5b6feeda71325e8b4cad66/browser/base/content/test/
> performance/browser_startup.js#34 and close bug 1369443.

Yes, I committed this as part of the test removals, but I guess that particular test change should actually probably live in this commit.

> ::: browser/components/feeds/WebContentConverter.js:334
> (Diff revision 1)
> > -     [Ci.nsIWebContentConverterService,
> > +     [Ci.nsIWebContentHandlerRegistrar,
> > -      Ci.nsIWebContentHandlerRegistrar,
> > -      Ci.nsIObserver,
> >        Ci.nsIFactory]),
> >  
> >    _xpcom_categories: [{
> 
> This seems obsolete too. Actually, I wonder if _xpcom_categories in general
> isn't a leftover from Mozilla 1.* .

https://searchfox.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#57-77 suggests this used to be done by XPCOMUtils. But I don't see any support left for that. Looks like support was removed in bug 568691 for mozilla 2.0 (Fx4) and we've been cargo-culting this ... around since then. Even recently added to marionette code, where AFAICT it does absolutely nothing...

I'll file a separate bug on that.
Priority: -- → P1
Comment on attachment 8994671 [details]
Bug 1477670 - remove web feed handling prefs and tidy up user-set prefs if they exist,

https://reviewboard.mozilla.org/r/259182/#review266678
Attachment #8994671 - Flags: review?(jkt) → review+
Comment on attachment 8994670 [details]
Bug 1477670 - remove content handler code from browser/,

https://reviewboard.mozilla.org/r/259180/#review266682

Looks great! thanks
Attachment #8994670 - Flags: review?(jkt) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5b714ea69f17
remove tests for registerContentHandler, r=jkt
https://hg.mozilla.org/integration/autoland/rev/04ce1d287d58
empty out DOM implementation of registerContentHandler, r=baku
https://hg.mozilla.org/integration/autoland/rev/3abafc9e0915
remove content handler code from browser/, r=florian,jkt
https://hg.mozilla.org/integration/autoland/rev/452156f0fc6d
remove web feed handling prefs and tidy up user-set prefs if they exist, r=flod,jkt
Depends on: 1479417
Blocks: 1499321
No longer blocks: 1478219
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: