1362112 - String is missing from the aboutUrlClassifier.properties

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Marko Andrejić]

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.

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

Review commit: https://reviewboard.mozilla.org/r/136368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136368/

Comment 2

[François Marier [:francois]]

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

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8864726 [details]
Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136368/diff/1-2/

Comment 4

[Dimi Lee [ooo 16.04 - 21.04］]

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 5

[François Marier [:francois]]

Comment on attachment 8864726 [details]
Bug 1362112 - Add missing string in aboutUrlClassifier.properties.

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

Thanks Dimi!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35f1edbebb52
Add missing string in aboutUrlClassifier.properties. r=francois

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35f1edbebb52

Comment 8

[Ton [:Tonnes]]

Not vital, but spotted a missing newline in aboutUrlClassifier.properties during l10n.

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

 (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 :)