Closed Bug 1024975 Opened 8 years ago Closed 8 years ago

[browser] Ignore searchplugins/metrolist.txt in


(Firefox for Metro Graveyard :: Build Config, defect)

Not set


(firefox32 fixed, firefox33 fixed)

Firefox 33
Tracking Status
firefox32 --- fixed
firefox33 --- fixed


(Reporter: flod, Assigned: flod)




(1 file, 1 obsolete file)

Bug 1018257 removed checks on Metro localization files, but we're still requiring searchplugins/metrolist.txt, that prevents us from deleting the file in l10n repositories.
Attached patch bug1024975.patch (obsolete) — Splinter Review
I'm open to suggestion for the Python syntax: I find it hard to read in short format + 2 conditions, I could also ignore "else:" and just return "error" after checking for files to ignore in /browser
Attachment #8439841 - Flags: feedback?(l10n)
Comment on attachment 8439841 [details] [diff] [review]

Review of attachment 8439841 [details] [diff] [review]:

Yeah, I think you're right that restructuring makes this more readable.

I'd take it one step further, and not use an else:, but
if blah: return "ignore"
if foo: return "ignore"
return "error"

::: browser/locales/
@@ +23,5 @@
> +    if (re.match(r"searchplugins\/.+\.xml", path) or
> +        path == "searchplugins/metrolist.txt"):
> +      return "ignore"
> +    else:
> +      return "error"

I'd drop the else: and just close the 
if not entity:
  return "error"
Attachment #8439841 - Flags: feedback?(l10n) → feedback+
Agreed, that was my first version. I think it also makes clearer that we always return at that point in the cycle.
Carrying over f+ from previous patch, asking Gavin for official review.
Attachment #8439841 - Attachment is obsolete: true
Attachment #8439847 - Flags: review?(
Comment on attachment 8439847 [details] [diff] [review]

I hereby declare that :Pike and :flod are official browser peers for the purposes of reviewing browser/locales/ Please feel free to review each others work here without any additional review. As mentioned previously, I am happy to sign-off on any policy changes if you deem it necessary. (As straightforward followup to bug 1018257, this shouldn't require any additional sign-off IMO.)
Attachment #8439847 - Flags: review?( → review+
Thanks Gavin, I'll be happy to stop nagging you for this :-)
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Comment on attachment 8439847 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: localizers won't be able to remove all metro files on mozilla-aurora, since only a part of the checks has been removed before last merge day (bug 1018257).
Testing completed (on m-c, etc.): yes on m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #8439847 - Flags: approval-mozilla-aurora?
Comment on attachment 8439847 [details] [diff] [review]

Aurora approval granted.
Attachment #8439847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.