762230 - Add-ons not listed when resetting Sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

When you go to reset Sync and replace all your data, the UI gives a list of things that will be deleted. It doesn't show add-ons because we forgot to implement it when add-on sync landed.

Comment 1

[Allison Naaktgeboren :ally]

incidentially, I don't see tabs on the list when I reproduce this, Ill see about adding that one too

Comment 2

[Allison Naaktgeboren :ally]

turns out tabs don't show up because they don't go away on a client wipe

Comment 3

[Allison Naaktgeboren :ally]

Attachment: logic & strings for addon count part 1/1, v0

only potential gotcha is the back compat string issue, 600141. After talking with connor, we should not have to worry as we have new strings in the .properties file, so we shouldn't hit that. 

However, we might consider putting it in anyway so that the addons entry is consistent with all the other blocks of code/related string bundles and less confusing to future developers/localizers. 

I can see an argument either way.

The topmost comments will be removed before submission

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 655078 [details] [diff] [review]
logic & strings for addon count part 1/1, v0

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

I just looked at style and nits.

::: browser/base/content/sync/setup.js
@@ +859,4 @@
>      return valid;
>    },
>  
> +  // anaaktge-note- localization note that for new engine names we won't support the "wrong" syntax ie 

Nit: trailing whitespace.

@@ +859,5 @@
>      return valid;
>    },
>  
> +  // anaaktge-note- localization note that for new engine names we won't support the "wrong" syntax ie 
> +  // 600141 wont apply to new strings

Nit: "Bug 600141 won't..." We need to include "bug" so we can search for known bugs in the tree.

@@ +931,5 @@
>            document.getElementById("prefsWipe").hidden = true;
>          }
>  
> +	      if (Weave.Engines.get("addons").enabled) {
> +	        let ids = Weave.Engines.get("addons")._store.getAllIDs();

I will bitrot this in bug 785225.

@@ +934,5 @@
> +	      if (Weave.Engines.get("addons").enabled) {
> +	        let ids = Weave.Engines.get("addons")._store.getAllIDs();
> +	        let blessedcount = 0;
> +	        for each (let i in ids) {
> +	          if(i) {

Nit: Space between "if" and "("

@@ +936,5 @@
> +	        let blessedcount = 0;
> +	        for each (let i in ids) {
> +	          if(i) {
> +	            blessedcount++;
> +            }

Nit: Identation

@@ +942,5 @@
> +	        // 600141 does not apply, as this does not have to support existing strings
> +	        document.getElementById("addonCount").value =
> +            PluralForm.get(blessedcount,
> +                           this._stringBundle.GetStringFromName("addonsCount.label"))
> +                      .replace("#1", blessedcount);

Nit: If you have a computed argument that takes a lot of screen real estate to obtain, just assign it to a variable and pass the variable into the function call. This makes the code cleaner.

Comment 5

[Allison Naaktgeboren :ally]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 655078 [details] [diff] [review]
> logic & strings for addon count part 1/1, v0
> 
> Review of attachment 655078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +931,5 @@
> >            document.getElementById("prefsWipe").hidden = true;
> >          }
> >  
> > +	      if (Weave.Engines.get("addons").enabled) {
> > +	        let ids = Weave.Engines.get("addons")._store.getAllIDs();
> 
> I will bitrot this in bug 785225.

Er, ok. So what would you like me to do to address that?


> 
> @@ +942,5 @@
> > +	        // 600141 does not apply, as this does not have to support existing strings
> > +	        document.getElementById("addonCount").value =
> > +            PluralForm.get(blessedcount,
> > +                           this._stringBundle.GetStringFromName("addonsCount.label"))
> > +                      .replace("#1", blessedcount);
> 
> Nit: If you have a computed argument that takes a lot of screen real estate
> to obtain, just assign it to a variable and pass the variable into the
> function call. This makes the code cleaner.

I agree!  However, (idk if you can tell this from the patch) this is consistent with all the other blocks in the logical set, and philikon was always firm about consistency with the file trumping all. Do you want me to adjust all 5 blocks to be cleaner or keep this consistent-but-ugly? connor?

Comment 6

[Allison Naaktgeboren :ally]

Attachment: addons count part 1/1, v1

nits that don't run afoul of file consistency addressed, including nasty soft tabs. nit that does, awaiting guidance from reviewer. :/

does not address:
- bitrotting question
- long ugly line

Comment 7

[Gregory Szorc [:gps]]

(In reply to :Ally Naaktgeboren from comment #5)
> (In reply to Gregory Szorc [:gps] from comment #4)

> > I will bitrot this in bug 785225.
> 
> Er, ok. So what would you like me to do to address that?

It depends when you land. If you land before bug 785225, you don't need to do anything. If you land after, you'll need to update your patch. Basically, we should coordinate.
 
 
> > @@ +942,5 @@
> > > +	        // 600141 does not apply, as this does not have to support existing strings
> > > +	        document.getElementById("addonCount").value =
> > > +            PluralForm.get(blessedcount,
> > > +                           this._stringBundle.GetStringFromName("addonsCount.label"))
> > > +                      .replace("#1", blessedcount);
> > 
> > Nit: If you have a computed argument that takes a lot of screen real estate
> > to obtain, just assign it to a variable and pass the variable into the
> > function call. This makes the code cleaner.
> 
> I agree!  However, (idk if you can tell this from the patch) this is
> consistent with all the other blocks in the logical set, and philikon was
> always firm about consistency with the file trumping all. Do you want me to
> adjust all 5 blocks to be cleaner or keep this consistent-but-ugly? connor?

IMO, consistency is more about indentation, variable naming (camelCase vs underscores), etc. Sloppy code is sloppy code: if you touch a line that has sloppy code, do everyone a favor and clean it up.

If clean-up is a large patch, please do it as a part 0. Bonus points are earned if the part 0 cleans up all instances of that sloppiness in a file.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 655691 [details] [diff] [review]
addons count part 1/1, v1

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

::: browser/locales/en-US/chrome/browser/syncSetup.properties
@@ +34,5 @@
>  passwordsCount.label        = #1 password;#1 passwords
> +# LOCALIZATION NOTE (addonsCount.label):
> +# Semi-colon list of plural forms. See:
> +# http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 is the number of passwords (was %S for a short while, use #1 instead, even if both work)

I'm not sure why you duplicated the link that's a few lines above, and the comment is misleading (and still says passwords!), since %S will not work with the code above.  I'd just say something like "#1 is the number of add-ons, see the link above for forms" and leave the middle two lines out.

Comment 9

[Allison Naaktgeboren :ally]

Attachment: hg-friendly version of the patch

while I wait for my hg privs to be re-enabled, I can haz check-in?

Comment 10

[Richard Newman [:rnewman]]

On current mozilla-inbound:

patching file browser/base/content/sync/setup.xul
Hunk #1 FAILED at 455
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/sync/setup.xul.rej
patching file browser/locales/en-US/chrome/browser/syncSetup.properties
Hunk #1 FAILED at 26
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/syncSetup.properties.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

I'll leave this for someone else to address in the morning.

Comment 11

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b71c9d5c55da 

Qa, should be short & sweet to test.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 656651 [details] [diff] [review]
hg-friendly version of the patch

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

::: browser/locales/en-US/chrome/browser/syncSetup.properties
@@ +33,5 @@
>  # #1 is the number of passwords (was %S for a short while, use #1 instead, even if both work)
>  passwordsCount.label        = #1 password;#1 passwords
> +# LOCALIZATION NOTE (addonsCount.label):
> +# #1 is the number of add-ons, see the link above for forms
> +addonsCount.label        = #1 addon;#1 addons

ITYM "add-on"

Comment 14

[Gregory Szorc [:gps]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> ::: browser/locales/en-US/chrome/browser/syncSetup.properties
> @@ +33,5 @@
> >  # #1 is the number of passwords (was %S for a short while, use #1 instead, even if both work)
> >  passwordsCount.label        = #1 password;#1 passwords
> > +# LOCALIZATION NOTE (addonsCount.label):
> > +# #1 is the number of add-ons, see the link above for forms
> > +addonsCount.label        = #1 addon;#1 addons
> 
> ITYM "add-on"

Yes.

Ally: please fix this ASAP. Land directly in inbound. Do not pass Go. Do not collect $200.