1330821 - Remove unused files from services/fxaccounts/

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Sam Penrose]

Ryan, can you direct Florian to the current owner of the code (especially services/fxaccounts)? Thanks ...

Comment 2

[Ryan Kelly [:rfkelly]]

:markh is likely the best person to answer questions about that code.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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

Comment 4

[Florian Quèze [:florian]]

(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?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

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

Comment 6

[Ryan Kelly [:rfkelly]]

> > 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?

Comment 7

[Shane Tomlinson [: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.

Comment 8

[Florian Quèze [:florian]]

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/.

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch

Comment 10

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 11

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8828580 [details] [diff] [review]
Patch v2

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

I didn't think of that :) Thanks!

Comment 13

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=358080bb3103

Comment 14

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4dc22fcd04354db1dc728754b497a83ca01e34
Bug 1330821 - Remove unused files from services/fxaccounts/, r=markh.

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f678120c11
Backed out changeset ab4dc22fcd04 for OS X S Bustage

Comment 17

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ece477bc26a761831519a9ad9893c504dcd14f03
Bug 1330821 - Remove unused files from services/fxaccounts/, r=markh.

Comment 18

[Florian Quèze [:florian]]

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

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ece477bc26a7