Closed Bug 1238712 Opened 4 years ago Closed 4 years ago

Remove duplication between nsWindowsShellService, nsGnomeShellService, and nsMacShellService

Categories

(Firefox :: Shell Integration, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

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 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 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 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 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?
Attachment #8706703 - Flags: review?(gijskruitbosch+bugs)
(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.
(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!
Ah, okay, on the same page now. I'll make that change.
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/
Attachment #8706703 - Flags: review?(gijskruitbosch+bugs)
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 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 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
Attachment #8706703 - Flags: review?(gijskruitbosch+bugs) → review+
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
Flags: needinfo?(jaws)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15d929c5bf36
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
Depends on: 1286627
You need to log in before you can comment on or make changes to this bug.