Closed Bug 1362112 Opened 4 years ago Closed 4 years ago

String is missing from the aboutUrlClassifier.properties

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: marko.andrejic93, Assigned: dimi)

References

Details

Attachments

(1 file)

There's a string on about:url-classifer page, "success" that's been missing from the file aboutUrlClassifier.properties. It represent Last update status.

I noticed this when I tested localization and saw thatit was still on english even I translated everything on about:url-classifer page.
Priority: -- → P2
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Comment on attachment 8864726 [details]
Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

https://reviewboard.mozilla.org/r/136368/#review139466

::: commit-message-0b255:1
(Diff revision 1)
> +Bug 1362112 - String is missing from the aboutUrlClassifier.properties. r?francois

nit: commit messages are supposed to describe the fix, not the underlying bug, so I would suggest:

    Add missing string in aboutUrl... r?francois

::: toolkit/components/url-classifier/content/listmanager.js:583
(Diff revision 1)
>    this.updateCheckers_[updateUrl] =
>      new G_Alarm(BindToObject(this.checkForUpdates, this, updateUrl),
>                  this.updateInterval, false);
>  
>    Services.obs.notifyObservers(null, "safebrowsing-update-finished",
> -                               "update error(" + result + ")");
> +                               "update error:" + result);

probably needs a space after the `:`

::: toolkit/components/url-classifier/content/listmanager.js:611
(Diff revision 1)
>    this.updateCheckers_[updateUrl] =
>      new G_Alarm(BindToObject(this.checkForUpdates, this, updateUrl),
>                  delay, false);
>  
>    Services.obs.notifyObservers(null, "safebrowsing-update-finished",
> -                               "download error(" + status + ")");
> +                               "download error:" + status);

ditto

::: toolkit/content/aboutUrlClassifier.js:98
(Diff revision 1)
> +    } else if (aData.startsWith("download error")) {
> +      let err = aData.split(":")[1];
> +      elem.childNodes[0].nodeValue =
> +        bundle.formatStringFromName("downloadError", [aData.split(":")[1]], 1);
> +    } else {
> +      elem.childNodes[0].nodeValue = bundle.GetStringFromName("UnknownResult");

Maybe in this case we should display the exact error even if it's not translated?

::: toolkit/content/aboutUrlClassifier.js:99
(Diff revision 1)
> +      let err = aData.split(":")[1];
> +      elem.childNodes[0].nodeValue =
> +        bundle.formatStringFromName("downloadError", [aData.split(":")[1]], 1);
> +    } else {
> +      elem.childNodes[0].nodeValue = bundle.GetStringFromName("UnknownResult");
> +    } 

nit: trailing whitespace

::: toolkit/locales/en-US/chrome/global/aboutUrlClassifier.properties:23
(Diff revision 1)
>  
>  CannotUpdate = cannot update
> +
> +success = success
> +
> +updateError = update error(%S)

nit: space before `(`

::: toolkit/locales/en-US/chrome/global/aboutUrlClassifier.properties:25
(Diff revision 1)
> +
> +success = success
> +
> +updateError = update error(%S)
> +
> +downloadError = download error(%S)

ditto

::: toolkit/locales/en-US/chrome/global/aboutUrlClassifier.properties:27
(Diff revision 1)
> +
> +updateError = update error(%S)
> +
> +downloadError = download error(%S)
> +
> +UnknownResult = unknown result

not needed if you agree with my other comment
Attachment #8864726 - Flags: review?(francois) → review-
Comment on attachment 8864726 [details]
Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

https://reviewboard.mozilla.org/r/136368/#review139466

> Maybe in this case we should display the exact error even if it's not translated?

Agree, thanks for suggestion.
Comment on attachment 8864726 [details]
Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

https://reviewboard.mozilla.org/r/136368/#review139476

Thanks Dimi!
Attachment #8864726 - Flags: review?(francois) → review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35f1edbebb52
Add missing string in aboutUrlClassifier.properties. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35f1edbebb52
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Not vital, but spotted a missing newline in aboutUrlClassifier.properties during l10n.
 (In reply to Ton from comment #8)
> Not vital, but spotted a missing newline in aboutUrlClassifier.properties
> during l10n.

I'll fix this in bug 1360480, thanks for reminding :)
You need to log in before you can comment on or make changes to this bug.