826357 - Generate Metro-aware Sync client name

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Using Sync as the channel for communication between two Windows profiles will result in two Sync clients being present in the Sync 'constellation'.

Under the current logic, these will have the same name, which is really confusing, and will hit Bug 670474 and Bug 618509.

On Metro, device names should include some meaningful disambiguation, e.g., "Richard's Firefox on Surface Pro (Classic)".

See also Bug 679272.

Comment 1

[Brian R. Bondy [:bbondy]]

This is a problem even without the extra automatic Metro sync setup stuff we've been talking about right?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> This is a problem even without the extra automatic Metro sync setup stuff
> we've been talking about right?

It's a problem in any situation where there are two clients on devices with the same name and/or type, but you're going to hit it 100% of the time, and in an automated situation, perhaps where Metro doesn't have UI to rename the Sync client.

Comment 3

[Jim Mathies [:jimm]]

general->sync 10 bugs

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch

This looks a lot nicer than the current set up we have. On my system I get strings like:

jim's Metro Firefox on Monolith (metro)
jim's Nightly on Monolith (desktop)

where 'Monolith' is the machine name I've given my surface pro. Windows forces users to select this on initial sign in, so people should know it. The only time this name might be unrecognizable is in corporate environments, but I think most corp users are going to know their workstation name.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 744156 [details] [diff] [review]
patch

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

Please test this on a desktop build!

::: services/sync/modules/engines/clients.js
@@ +89,4 @@
>  
> +    let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
> +    // bob's Metro Firefox on somedevice
> +    let appName = syncStrings.get("sync.defaultAccountApplication");

This will throw on desktop. (I think desktop puts this file in chrome://weave/locale/services/sync.properties.)

--
[10:12:34.489] syncStrings.get("sync.defaultAccountApplication");
[10:12:34.491] [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/services-common/stringbundle.js :: <TOP_LEVEL> :: line 97"  data: no]

Please either fix that distinction, or wrap this in a try block.

It also seems to just fail for me if the string doesn't exist:

[10:17:18.476] [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/services-common/stringbundle.js :: <TOP_LEVEL> :: line 97"  data: no]

so definitely be more defensive here.

@@ +89,5 @@
>  
> +    let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
> +    // bob's Metro Firefox on somedevice
> +    let appName = syncStrings.get("sync.defaultAccountApplication");
> +    if (!appName.length) {

Empty strings are falsy in JS, and this will blow up if appName is null, so:

  if (appName) {

is better in every way.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 744156 [details] [diff] [review]
> patch
> 
> Review of attachment 744156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please test this on a desktop build!
> 
> ::: services/sync/modules/engines/clients.js
> @@ +89,4 @@
> >  
> > +    let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
> > +    // bob's Metro Firefox on somedevice
> > +    let appName = syncStrings.get("sync.defaultAccountApplication");
> 
> This will throw on desktop. (I think desktop puts this file in
> chrome://weave/locale/services/sync.properties.)

Ok, np. I was expecting a missing bundle / empty result result on desktop, so I think a try block here is the best route.

Comment 7

[Jim Mathies [:jimm]]

* "result string"

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 745302 [details] [diff] [review]
patch

tested on desktop, works as expected.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 745302 [details] [diff] [review]
patch

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

r+ with nits, but I recommend you use the appropriate definition instead of hard-coding "Firefox" prior to this reaching Aurora.

::: browser/metro/locales/en-US/chrome/sync.properties
@@ +6,5 @@
>  
> +# LOCALIZATION NOTE: Used in the default machine description when a new account
> +# is being set up. Should be unique to Metro, so that it does not conflict
> +# with Desktop. See /services/sync/modules/engines/clients.js getLocalName.
> +sync.defaultAccountApplication=Metro Firefox

Shouldn't this involve brandShortName ("Firefox", "Aurora")?

::: services/sync/modules/engines/clients.js
@@ +89,2 @@
>  
> +    let appName = ""

Trailing semicolon, and you can omit the initialization.

@@ +96,2 @@
>  
> +    if (!appName.length) {

Just 

  if (!appName) {

Comment 11

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #10)
> > +# LOCALIZATION NOTE: Used in the default machine description when a new account
> > +# is being set up. Should be unique to Metro, so that it does not conflict
> > +# with Desktop. See /services/sync/modules/engines/clients.js getLocalName.
> > +sync.defaultAccountApplication=Metro Firefox
> 
> Shouldn't this involve brandShortName ("Firefox", "Aurora")?

Well then it would match the other generated string used by desktop and defeat the purpose of this bug. So I was trying to use something that would be unique to the immersive browser.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Jim Mathies [:jimm] from comment #11)

> > > +sync.defaultAccountApplication=Metro Firefox
> > 
> > Shouldn't this involve brandShortName ("Firefox", "Aurora")?
> 
> Well then it would match the other generated string used by desktop and
> defeat the purpose of this bug. So I was trying to use something that would
> be unique to the immersive browser.

"Involve", not "be" -- that is, I would be expecting "Metro Nightly".

(And regardless, we really shouldn't be hard-coding the "Firefox" string anywhere except in official branding files.)

Comment 13

[Jim Mathies [:jimm]]

Attachment: name v.2

Updated. Also swapped out 'Metro' for 'Modern'.

So in metro land:

"jim's Modern Aurora on Monolith"

and on desktop:

"jim's Aurora on Monolith"

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 745811 [details] [diff] [review]
name v.2

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

::: services/sync/modules/engines/clients.js
@@ +93,5 @@
> +    try {
> +      let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
> +      appName = syncStrings.getFormattedString("sync.defaultAccountApplication", [brandName]);
> +    } catch (ex) {}
> +    appName = !appName ? brandName : appName;

appName = appName || brandName;

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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