1366214 - The result not found message should be "Sorry! There are no results in Options for " on Windows

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Evan Tseng [:evanxd]]

The result not found message should be "Sorry! There are no results in Options for " on Linux/Windows.

The spec: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/218928235

Comment 1

[Evan Tseng [:evanxd]]

Hi Mike,

Do you know how we load different string keys (or dtd files) for different OS platforms to fix this issue?

Thanks.

Comment 2

[Evan Tseng [:evanxd]]

I think we could detect OS platform and load different string keys, like

```
let xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"]
                           .getService(Components.interfaces.nsIXULRuntime);
let message = "";

if (xulRuntime == "Windows" || xulRuntime == "Linux") {
  // Load the correct string for Windows and Linux platforms.
  message = strings.getFormattedString("searchResults.sorryMessageWin", [query]);
} else {
  message = strings.getFormattedString("searchResults.sorryMessage", [query]);
}
```

This approach almost likes [1].

What do you think of the above solution? Is it OK to us?

[1]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/preferences.xul#60-71

Comment 3

[Francesco Lodolo [:flod]]

Note that only Windows use Options, while Linux *and* Mac use Preferences. 

Not sure if this helps, but we have code around already doing that, e.g. http://searchfox.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.xhtml#67-71

Comment 4

[Evan Tseng [:evanxd]]

Attachment: Bug 1366214 - Add a new result not found string for Windows.

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

Comment 5

[Evan Tseng [:evanxd]]

(In reply to Francesco Lodolo [:flod] from comment #3)
> Note that only Windows use Options, while Linux *and* Mac use Preferences. 
> 
> Not sure if this helps, but we have code around already doing that, e.g.
> http://searchfox.org/mozilla-central/source/browser/base/content/abouthome/
> aboutHome.xhtml#67-71

Hi Francesco,

Thanks for reminding, we only use "Options" on Windows.

Comment 6

[Evan Tseng [:evanxd]]

Hi Mike,

Could you help review the patch?

Thanks.

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

https://reviewboard.mozilla.org/r/141056/#review144992

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:261
(Diff revision 1)
>  removeContainerButton2=Don’t remove this Container
>  
>  # Search Results Pane
>  # LOCALIZATION NOTE %S will be replaced by the word being searched
> +searchResults.sorryMessageWin=Sorry! There are no results in Options for “%S”.
>  searchResults.sorryMessage2=Sorry! There are no results in Preferences for “%S”.

At this point I would rename this string as searchResults.sorryMessageUnix for clarity.

The current string (with 2) hasn't been pushed out to localizers yet because I'm waiting for the correct strings to land.

Comment 8

[Evan Tseng [:evanxd]]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

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

Comment 9

[Evan Tseng [:evanxd]]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

https://reviewboard.mozilla.org/r/141056/#review144992

> At this point I would rename this string as searchResults.sorryMessageUnix for clarity.
> 
> The current string (with 2) hasn't been pushed out to localizers yet because I'm waiting for the correct strings to land.

Hi Flod,

Good point. I've updated it.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

https://reviewboard.mozilla.org/r/141056/#review145998

r=me with the following change

::: browser/components/preferences/in-content/findInPage.js:243
(Diff revision 2)
>          let noResultsEl = document.querySelector(".no-results-message");
>          noResultsEl.hidden = false;
>  
>          let strings = this.strings;
> -        document.getElementById("sorry-message").textContent =
> -          strings.getFormattedString("searchResults.sorryMessage2", [query]);
> +
> +        document.getElementById("sorry-message").textContent = Services.appinfo.OS === "WINNT" ?

AppConstants.jsm is loaded already, so please use `AppConstants.platform == "win"` instead since it is more concise.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

https://reviewboard.mozilla.org/r/141056/#review146418

r=me with jaws' issue fixed.

Comment 12

[Evan Tseng [:evanxd]]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

https://reviewboard.mozilla.org/r/141056/#review146868

::: browser/components/preferences/in-content/findInPage.js:243
(Diff revision 2)
>          let noResultsEl = document.querySelector(".no-results-message");
>          noResultsEl.hidden = false;
>  
>          let strings = this.strings;
> -        document.getElementById("sorry-message").textContent =
> -          strings.getFormattedString("searchResults.sorryMessage2", [query]);
> +
> +        document.getElementById("sorry-message").textContent = Services.appinfo.OS === "WINNT" ?

Sure. Let's do it.

Comment 13

[Evan Tseng [:evanxd]]

Comment on attachment 8869892 [details]
Bug 1366214 - Add a new result not found string for Windows.

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

Comment 14

[Evan Tseng [:evanxd]]

Updated the patch for review comments. Let's land it after the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51b6f43650e3

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a1143c269e5
Add a new result not found string for Windows. r=jaws,mconley

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3a1143c269e5

Comment 17

[Hani Yacoub, Desktop QA]

Build ID: 20170529030204
User Agent:Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64 and Windows 7 x32.