Closed Bug 1330821 Opened 3 years ago Closed 3 years ago

Remove unused files from services/fxaccounts/

Categories

(Core Graveyard :: Identity, defect)

53 Branch
defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 1 obsolete file)

My test in bug 1316187 reports the following files as shipped but unreferenced:

resource://gre/modules/identity/FirefoxAccounts.jsm
resource://gre/modules/identity/Identity.jsm
resource://gre/modules/identity/MinimalIdentity.jsm
resource://gre/modules/FxAccountsOAuthClient.jsm
resource://gre/modules/FxAccountsPush.js

I'm not familiar enough with the history of this code to be able to figure out what's dead code that should be removed, and what's still in use.
Ryan, can you direct Florian to the current owner of the code (especially services/fxaccounts)? Thanks ...
Flags: needinfo?(rfkelly)
:markh is likely the best person to answer questions about that code.
Flags: needinfo?(rfkelly)
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> My test in bug 1316187 reports the following files as shipped but
> unreferenced:
> 
> resource://gre/modules/identity/FirefoxAccounts.jsm
> resource://gre/modules/identity/Identity.jsm
> resource://gre/modules/identity/MinimalIdentity.jsm

IIRC, these are all from b2g and probably can be removed.

> resource://gre/modules/FxAccountsOAuthClient.jsm

I can't find a caller in our tree, but I wonder if pocket it using it?

> resource://gre/modules/FxAccountsPush.js

This is used and is loaded due to it being an xpcom service.
(In reply to Mark Hammond [:markh] from comment #3)

> > resource://gre/modules/FxAccountsOAuthClient.jsm
> 
> I can't find a caller in our tree, but I wonder if pocket it using it?

Pocket is also in our tree (browser/extensions/pocket), do you mean a special different add-on?

> > resource://gre/modules/FxAccountsPush.js
> 
> This is used and is loaded due to it being an xpcom service.

This file is packaged twice: once in the components/ folder and once in the modules/ folder, and whitelisted in browser/installer/allowed-dupes.mn. It's the modules/ version that's reported as unreferenced by the test here. Maybe http://searchfox.org/mozilla-central/source/services/fxaccounts/moz.build#30 is just a copy/paste accident?
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> Pocket is also in our tree (browser/extensions/pocket), do you mean a
> special different add-on?

hrm, no - I meant that one. So I guess that's probably b2g too and can die.

> Maybe
> http://searchfox.org/mozilla-central/source/services/fxaccounts/moz.build#30
> is just a copy/paste accident?

Ah, yes, I suspect that is the case and that line can be removed.
> > resource://gre/modules/FxAccountsOAuthClient.jsm
> I can't find a caller in our tree, but I wonder if pocket it using it?

I don't believe Pocket uses this either.  In fact I seem to recall us removing support for "webchannel oauth clients" on the server recently, because no-one was using it al ll.  Shane, can you confirm my hazy recollection here?
Flags: needinfo?(stomlinson)
(In reply to Ryan Kelly [:rfkelly] from comment #6)
> > > resource://gre/modules/FxAccountsOAuthClient.jsm
> > I can't find a caller in our tree, but I wonder if pocket it using it?
> 
> I don't believe Pocket uses this either.  In fact I seem to recall us
> removing support for "webchannel oauth clients" on the server recently,
> because no-one was using it al ll.  Shane, can you confirm my hazy
> recollection here?

Aren't you on vacation?

Yes, we completely remove support for WebChannel based OAuth reliers.
Flags: needinfo?(stomlinson)
Actually, there are already patches in bug 1313045 to handle toolkit/identity (Mark could you help that bug move forward please? :-)), so I'll keep this bug to handle only the 2 files from services/fxaccounts/.
Summary: Remove unused files from toolkit/identity and services/fxaccounts/ → Remove unused files from services/fxaccounts/
Attached patch Patch (obsolete) — Splinter Review
Attachment #8828460 - Flags: review?(markh)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Comment on attachment 8828460 [details] [diff] [review]
Patch

Review of attachment 8828460 [details] [diff] [review]:
-----------------------------------------------------------------

I think this will break test_push_service.js, which imports FxAccountsPush.js as a module :( I suspect that's the underlying reason it is listed as a module even though it's not imported by the runtime. That looks tricky to adjust as the test wants to instantiate the service with various mocks.
Attachment #8828460 - Flags: review?(markh)
Attached patch Patch v2Splinter Review
Attachment #8828580 - Flags: review?(markh)
Attachment #8828460 - Attachment is obsolete: true
Comment on attachment 8828580 [details] [diff] [review]
Patch v2

Review of attachment 8828580 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't think of that :) Thanks!
Attachment #8828580 - Flags: review?(markh) → review+
had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=70656112&repo=mozilla-inbound&lineNumber=3488

04:14:50     INFO -  ERROR: The following duplicated files are not allowed:

04:14:50     INFO -  NightlyDebug.app/Contents/Resources/components/FxAccountsPush.js

04:14:50     INFO -  NightlyDebug.app/Contents/Resources/modules/FxAccountsPush.js
Flags: needinfo?(florian)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f678120c11
Backed out changeset ab4dc22fcd04 for OS X S Bustage
(In reply to Carsten Book [:Tomcat] from comment #15)
> had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=70656112&repo=mozilla-
> inbound&lineNumber=3488
> 
> 04:14:50     INFO -  ERROR: The following duplicated files are not allowed:
> 
> 04:14:50     INFO - 
> NightlyDebug.app/Contents/Resources/components/FxAccountsPush.js
> 
> 04:14:50     INFO - 
> NightlyDebug.app/Contents/Resources/modules/FxAccountsPush.js

I relanded without the browser/installer/allowed-dupes.mn change, which will need to land a couple days later I think.
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/ece477bc26a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1352982
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.