Closed Bug 762230 Opened 8 years ago Closed 8 years ago

Add-ons not listed when resetting Sync

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: gps, Assigned: ally)

References

Details

(Whiteboard: qa+)

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → ally
incidentially, I don't see tabs on the list when I reproduce this, Ill see about adding that one too
turns out tabs don't show up because they don't go away on a client wipe
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
Attachment #655078 - Flags: review?(mconnor)
Attachment #655078 - Flags: feedback?(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.
Attachment #655078 - Flags: feedback?(gps) → feedback+
(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?
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
Attachment #655078 - Attachment is obsolete: true
Attachment #655078 - Flags: review?(mconnor)
Attachment #655691 - Flags: review?(mconnor)
(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 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.
Attachment #655691 - Flags: review?(mconnor) → review+
Blocks: 786779
while I wait for my hg privs to be re-enabled, I can haz check-in?
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71c9d5c55da 

Qa, should be short & sweet to test.
Whiteboard: qa+
https://hg.mozilla.org/mozilla-central/rev/b71c9d5c55da
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 787306
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"
(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.
Blocks: 901488
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.