1238712 - Remove duplication between nsWindowsShellService, nsGnomeShellService, and nsMacShellService

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Much of the different shell service implementations is duplicated and has to be updated on each change. Also, it is not all necessary that this code be written in C++. We should use JS where possible and reasonable.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42bc352092a0

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70eef69c5940

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4455378fadfe

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d81700bf092

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

https://reviewboard.mozilla.org/r/30441/#review27219

::: browser/components/shell/ShellService.jsm:18
(Diff revision 4)
> +  _shellService: null,
> +  get shellService() {
> +    if (!this._shellService) {
> +      try {
> +        this._shellService = Components.classes["@mozilla.org/browser/shell-service;1"]
> +                                       .getService(Components.interfaces.nsIShellService);
> +      } catch (e) { }
> +    }
> +    return this._shellService;
> +  },

You could replace all of this with, after the definition of ShellServiceInternal, doing:

XPCOMUtils.defineLazyServiceGetter(ShellServiceInternal, "shellService", "@mozilla.org/browser/shell-service;1", Ci.nsIShellService);

::: browser/components/shell/ShellService.jsm:36
(Diff revision 4)
> +  get canSetDesktopBackground() {

I don't think anything calls this right now. You've not updated nsContextMenu.js

::: browser/components/shell/ShellService.jsm:44
(Diff revision 4)
> +        return this.shellService.canSetDesktopBackground;

Hm. The getter here doesn't QI to Ci.nsIGNOMEShellService, and you moved the definition of canSetDesktopBackground there. So I think this will actually be undefined in the current code when run on Linux. You should ensure that in this case you QI the shellService before using that attribute.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1028
(Diff revision 4)
> +    if (!shellService) {
> +      try {
> -      shellService = Cc["@mozilla.org/browser/shell-service;1"]
> +        shellService = Cc["@mozilla.org/browser/shell-service;1"]
> -                       .getService(Ci.nsIShellService);
> +                         .getService(Ci.nsIShellService);
> -    } catch (ex) {
> +      } catch (ex) {
> -      this._log.error("_isDefaultBrowser - Could not obtain shell service", ex);
> +        this._log.error("_isDefaultBrowser - Could not obtain shell service", ex);
> -      return null;
> +        return null;
> -    }
> +      }
> +    }

Why keep this? There shouldn't be any cases where we don't ship the JSM, right?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #9)
> ::: browser/components/shell/ShellService.jsm:18
> (Diff revision 4)
> > +  _shellService: null,
> > +  get shellService() {
> > +    if (!this._shellService) {
> > +      try {
> > +        this._shellService = Components.classes["@mozilla.org/browser/shell-service;1"]
> > +                                       .getService(Components.interfaces.nsIShellService);
> > +      } catch (e) { }
> > +    }
> > +    return this._shellService;
> > +  },
> 
> You could replace all of this with, after the definition of
> ShellServiceInternal, doing:
> 
> XPCOMUtils.defineLazyServiceGetter(ShellServiceInternal, "shellService",
> "@mozilla.org/browser/shell-service;1", Ci.nsIShellService);

I'm not sure I follow. The goal of this code is to allow consumers to use ShellService.jsm as the replacement for the service. If the consumer requested an attribute or function that was previously defined in the shell-service, then they would now call it through ShellService.jsm, where it is either now implemented in JS or forwarded to the shell-service. It is opaque to the consumer if they used ShellService.jsm.
 
> ::: browser/components/shell/ShellService.jsm:36
> (Diff revision 4)
> > +  get canSetDesktopBackground() {
> 
> I don't think anything calls this right now. You've not updated
> nsContextMenu.js

This was updated by changing utilityOverlay.js' getShellService() function to return a reference to the ShellService.jsm object instead of the shell-service. Due to how I implemented the forwarding as described above, consumers of the shell-service won't need to be updated.

> ::: browser/components/shell/ShellService.jsm:44
> (Diff revision 4)
> > +        return this.shellService.canSetDesktopBackground;
> 
> Hm. The getter here doesn't QI to Ci.nsIGNOMEShellService, and you moved the
> definition of canSetDesktopBackground there. So I think this will actually
> be undefined in the current code when run on Linux. You should ensure that
> in this case you QI the shellService before using that attribute.

Thanks, this is now fixed locally.

> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1028
> (Diff revision 4)
> > +    if (!shellService) {
> > +      try {
> > -      shellService = Cc["@mozilla.org/browser/shell-service;1"]
> > +        shellService = Cc["@mozilla.org/browser/shell-service;1"]
> > -                       .getService(Ci.nsIShellService);
> > +                         .getService(Ci.nsIShellService);
> > -    } catch (ex) {
> > +      } catch (ex) {
> > -      this._log.error("_isDefaultBrowser - Could not obtain shell service", ex);
> > +        this._log.error("_isDefaultBrowser - Could not obtain shell service", ex);
> > -      return null;
> > +        return null;
> > -    }
> > +      }
> > +    }
> 
> Why keep this? There shouldn't be any cases where we don't ship the JSM,
> right?

Call it a mix of trying not to change whatever debugging/diagnostic code that Telemetry apparently wanted and also doing it this way because Telemetry is defined in /toolkit but the ShellService.jsm is defined in /browser.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > ::: browser/components/shell/ShellService.jsm:18
> > (Diff revision 4)
> > > +  _shellService: null,
> > > +  get shellService() {
> > > +    if (!this._shellService) {
> > > +      try {
> > > +        this._shellService = Components.classes["@mozilla.org/browser/shell-service;1"]
> > > +                                       .getService(Components.interfaces.nsIShellService);
> > > +      } catch (e) { }
> > > +    }
> > > +    return this._shellService;
> > > +  },
> > 
> > You could replace all of this with, after the definition of
> > ShellServiceInternal, doing:
> > 
> > XPCOMUtils.defineLazyServiceGetter(ShellServiceInternal, "shellService",
> > "@mozilla.org/browser/shell-service;1", Ci.nsIShellService);
> 
> I'm not sure I follow. The goal of this code is to allow consumers to use
> ShellService.jsm as the replacement for the service. If the consumer
> requested an attribute or function that was previously defined in the
> shell-service, then they would now call it through ShellService.jsm, where
> it is either now implemented in JS or forwarded to the shell-service. It is
> opaque to the consumer if they used ShellService.jsm.

Yes, I understand that. I was simply saying that you manually implemented a lazy getter for the shell-service as a "shellService" property onto ShellService.jsm, and that you could instead use XPCOMUtils to define that lazy getter and save yourself the code (plus, it would be slightly more performant because XPCOMUtils should redefine the property to avoid the getter once the service has been fetched once).

> > ::: browser/components/shell/ShellService.jsm:36
> > (Diff revision 4)
> > > +  get canSetDesktopBackground() {
> > 
> > I don't think anything calls this right now. You've not updated
> > nsContextMenu.js
> 
> This was updated by changing utilityOverlay.js' getShellService() function
> to return a reference to the ShellService.jsm object instead of the
> shell-service. Due to how I implemented the forwarding as described above,
> consumers of the shell-service won't need to be updated.

Oh, oops, I missed that. Thanks for clarifying!

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Ah, okay, on the same page now. I'll make that change.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5374a5998e3f

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c3155f69a08

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26df22dab4e3

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

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

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8706703 [details]
MozReview Request: Bug 1238712 - Move duplicated shell-service code to a shared JSM. r?gijs

https://reviewboard.mozilla.org/r/30441/#review27503

Comment 20

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

applying 26df22dab4e3
patching file browser/components/nsBrowserGlue.js
Hunk #2 FAILED at 173
1 out of 2 hunks FAILED -- saving rejects to file browser/components/nsBrowserGlue.js.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/15d929c5bf361329a9c95c7ff0bf0e44429568b6
Bug 1238712 - Move duplicated shell-service code to a shared JSM. r=gijs

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15d929c5bf36

Comment 23

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected