Closed Bug 904230 Opened 6 years ago Closed 6 years ago

Make nsIWinMetroUtils available as a service in Services.jsm as 'Services.metro'

Categories

(Firefox for Metro Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v1 (WIP) (obsolete) — Splinter Review
See bug 866304 comment 35 - we'd like to make nsIWinMetroUtils available as 'Services.metro'

See also bug 778949 I think?
Attached patch Patch v2 (obsolete) — Splinter Review
I'll do some additional testing on this since there are a lot of consumers, but so far this seems to work just fine.
Attachment #789163 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Patch v3 (obsolete) — Splinter Review
Pretty sure nothing has changed between v2 and v3, but just in case I'm uploading a new version

I did probably more testing than was warranted for this patch, and I've also pushed to try:
  https://tbpl.mozilla.org/?tree=Try&rev=4b21aa75d7ec


This patch technically needs reviewers from metro, sync, and toolkit. I've chosen mbrubeck, rnewman, and dolske, respectively.
Attachment #793356 - Attachment is obsolete: true
Attachment #794309 - Flags: review?(rnewman)
Attachment #794309 - Flags: review?(mbrubeck)
Attachment #794309 - Flags: review?(dolske)
Comment on attachment 794309 [details] [diff] [review]
Patch v3

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

r+ on the browser/base/ and toolkit/ changes with an ifdef.

::: toolkit/modules/Services.jsm
@@ +74,5 @@
>    ["DOMRequest", "@mozilla.org/dom/dom-request-service;1", "nsIDOMRequestService"],
>    ["focus", "@mozilla.org/focus-manager;1", "nsIFocusManager"],
>    ["uriFixup", "@mozilla.org/docshell/urifixup;1", "nsIURIFixup"],
> +  ["blocklist", "@mozilla.org/extensions/blocklist;1", "nsIBlocklistService"],
> +  ["metro", "@mozilla.org/windows-metroutils;1", "nsIWinMetroUtils"],

Should probably have a |#ifdef MOZ_METRO| around this. I see we're doing the same for android up above.
Attachment #794309 - Flags: review?(dolske) → review+
Comment on attachment 794309 [details] [diff] [review]
Patch v3

Note: this removes the stub implementation for non-Windows platforms.  It'd be nice to restore this at some point, but not necessary for landing (since that's not a supported or tested build config).
Attachment #794309 - Flags: review?(mbrubeck) → review+
Comment on attachment 794309 [details] [diff] [review]
Patch v3

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2786,5 @@
>      if (update.showPrompt) {
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because the " +
>            "update snippet specified showPrompt");
>        this._showPrompt(update);
> +      if (!Services.metro.immersive) {

You might need to do "if (Services.metro && Services.metro.immersive)" here to avoid breaking non-Windows platforms.  Or wrap this in a #ifdef XP_WIN #ifdef MOZ_METRO like the previous hunk.

@@ +2796,5 @@
>      if (!getPref("getBoolPref", PREF_APP_UPDATE_AUTO, true)) {
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because silent " +
>            "install is disabled");
>        this._showPrompt(update);
> +      if (!Services.metro.immersive) {

same here
Comment on attachment 794309 [details] [diff] [review]
Patch v3

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

::: browser/base/content/sync/setup.js
@@ +420,5 @@
>  
>  #ifdef XP_WIN
>  #ifdef MOZ_METRO
>    _securelyStoreForMetroSync: function(weaveEmail, weavePassword, weaveKey) {
> +    Services.metro.storeSyncInfo(weaveEmail, weavePassword, weaveKey);

Just inline the new one-line _securelyStoreForMetroSync. It's only used once, and the call site is already #ifdef'ed.
Attachment #794309 - Flags: review?(rnewman) → review+
Attached patch Patch v4Splinter Review
Carrying forward the various r+s

(In reply to Justin Dolske [:Dolske] from comment #3)
[...]
> Should probably have a |#ifdef MOZ_METRO| around this. I see we're doing the
> same for android up above.

Done!

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
[...]
> You might need to do "if (Services.metro && Services.metro.immersive)" here
> to avoid breaking non-Windows platforms.  Or wrap this in a #ifdef XP_WIN
> #ifdef MOZ_METRO like the previous hunk.

It turns out that the appropriate condition is "if (!Services.metro || !Services.metro.immersive)". I've updated both locations to look like this.

(In reply to Richard Newman [:rnewman] from comment #6)
> Just inline the new one-line _securelyStoreForMetroSync. It's only used
> once, and the call site is already #ifdef'ed.

Done!


Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=61110d3758a6
Attachment #794309 - Attachment is obsolete: true
Attachment #796217 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/55bb504ba5f4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.