Implement OS query parsing and user search choice

VERIFIED FIXED in Firefox 40

Status

()

defect
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: clarkbw, Assigned: Dolske)

Tracking

(Depends on 1 bug, {user-doc-needed})

unspecified
Firefox 42
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40 verified, firefox41 verified, firefox42 verified)

Details

Attachments

(4 attachments, 3 obsolete attachments)

This bug implements the Firefox component that handles URL input, determines if this is the correct search URL coming from the OS and extracts the search terms so they can be applied to the users preferred search engine.
(Assignee)

Comment 1

4 years ago
Posted patch Patch v.1 (obsolete) — Splinter Review
A few notes:

* This is currently limited to Windows-only, do we want to limit it further? Windows 10 needs it, presumably XP does not, and I don't know about Win7/8.

* AFAIK there's no way to exactly know that the search comes from Windows, all we know is that someone asked Firefox to open a URL. So there's a small risk that we'll end up redirecting such URLs that are not from a search you just did -- for example, if someone else does a search and emails you the Bing link, we'd rewrite it. [I think we could actually improve this -- one of the URL params we get from the search appears to be a timestamp, so we could check to see if the search was triggered within the last few seconds.]

* Most Firefox searches include, in the constructed URL, a param indicating the context of the search (ie, what piece of UI the user used). Unfortunately we don't have an existing value for "from the OS" or "from the command line", and Yahoo returns suboptimal results when it doesn't recognize the context, so we'll need to fix that still.
Attachment #8626201 - Flags: feedback?(jaws)
(Assignee)

Comment 2

4 years ago
Also:

* Need to do some more investigation / testing / outreach to see if Windows uses different URLs in other locales.
Comment on attachment 8626201 [details] [diff] [review]
Patch v.1

Review of attachment 8626201 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, nice turnaround here :)

::: browser/components/nsBrowserContentHandler.js
@@ +727,5 @@
>        while ((ar = cmdLine.handleFlagWithParam("url", false))) {
>          var uri = resolveURIInternal(cmdLine, ar);
> +
> +        // XXX how robust to make this against minor URL variations?
> +        if (rewriteWinSearch && uri.spec.startsWith("https://www.bing.com/search")) {

We can check if it is win10 and higher by using AppConstants.isPlatformAndVersionAtLeast("win", "10")

@@ +742,5 @@
> +              // XXX doSearch() should set this 3rd arg too
> +              uri = submission.uri;
> +            }
> +          } catch (e) {
> +            Components.utils.reportError("Couldn't redirect Windows search: " + e);

Can we add telemetry to track this?

::: browser/locales/en-US/chrome/browser/preferences/search.dtd
@@ +10,5 @@
>  <!ENTITY provideSearchSuggestions.accesskey    "s">
>  
> +<!ENTITY redirectSearchCaption.label "Windows Search">
> +<!ENTITY redirectWindowsSearch.label "Use my default search engine for searches from Windows">
> +<!ENTITY redirectWindowsSearch.accesskey "w">

W should be capitalized.
Attachment #8626201 - Flags: feedback?(jaws) → feedback+
(In reply to Justin Dolske [:Dolske] from comment #1)
> * This is currently limited to Windows-only, do we want to limit it further?
> Windows 10 needs it, presumably XP does not, and I don't know about Win7/8.

I'll try to look into this, however probably a lower priority as Windows 10 is a free upgrade for all Win 7+ users so if we did enable it for others it would likely be a shrinking set of users.

> * AFAIK there's no way to exactly know that the search comes from Windows,
> all we know is that someone asked Firefox to open a URL. So there's a small
> risk that we'll end up redirecting such URLs that are not from a search you
> just did -- for example, if someone else does a search and emails you the
> Bing link, we'd rewrite it. [I think we could actually improve this -- one
> of the URL params we get from the search appears to be a timestamp, so we
> could check to see if the search was triggered within the last few seconds.]

Could we create a Telemetry probe that calculates the diff between the ts and now?  Not sure that's the best measure but it might let us see how often there is an anomaly.

> * Most Firefox searches include, in the constructed URL, a param indicating
> the context of the search (ie, what piece of UI the user used).
> Unfortunately we don't have an existing value for "from the OS" or "from the
> command line", and Yahoo returns suboptimal results when it doesn't
> recognize the context, so we'll need to fix that still.

Joanne, is there an alternate code we should use here?  And if not or not available, what context code should we default to?
Flags: needinfo?(jnagel)
Matej and Winston, we need this string before EOW.  Can you coordinate and leave feedback?  See bug 1175223 for more context.

This is the preference string located in the Search category of the preferences:

[x] "Use my default search engine for searches from Windows"

I'll ask Dolske to upload a screenshot, for now this is fairly close except for the text: http://cl.ly/image/0j1Z3A0h0h3X
Flags: needinfo?(matej)
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #5)
> Matej and Winston, we need this string before EOW.  Can you coordinate and
> leave feedback?  See bug 1175223 for more context.
> 
> This is the preference string located in the Search category of the
> preferences:
> 
> [x] "Use my default search engine for searches from Windows"
> 
> I'll ask Dolske to upload a screenshot, for now this is fairly close except
> for the text: http://cl.ly/image/0j1Z3A0h0h3X

The other quirk here is that this would only be "used from the Windows start menu" if Firefox was set as the default browser. Should that be mentioned here? Or should there be a link/way from the Search page to set Firefox as the default browser?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> The other quirk here is that this would only be "used from the Windows start
> menu" if Firefox was set as the default browser. Should that be mentioned
> here? Or should there be a link/way from the Search page to set Firefox as
> the default browser?

Should we check that we are the default browser here?
(Assignee)

Comment 8

4 years ago
Posted patch Patch v.2 (obsolete) — Splinter Review
Attachment #8626201 - Attachment is obsolete: true
Attachment #8626601 - Flags: review?(jaws)
(Assignee)

Comment 9

4 years ago
Posted image Screenshot of patch v.2 (obsolete) —
Comment on attachment 8626601 [details] [diff] [review]
Patch v.2

Review of attachment 8626601 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserContentHandler.js
@@ +736,5 @@
> +          try {
> +            var url = uri.QueryInterface(Components.interfaces.nsIURL);
> +            var params = new URLSearchParams(url.query);
> +            // XXX search -> weather -> click -> gets form=WNSFC2
> +            if (params.get("form") == "WNSGPH") {

Can you put a comment here saying where WNSGPH comes from?

nit, don't forget to remove the XXX comments

@@ +740,5 @@
> +            if (params.get("form") == "WNSGPH") {
> +              var term = params.get("q");
> +              var ss = Components.classes["@mozilla.org/browser/search-service;1"]
> +                                 .getService(nsIBrowserSearchService);
> +              var submission = ss.defaultEngine.getSubmission(term, null, "searchbar");

Will using "searchbar" here confuse any of our telemetry data?

::: browser/components/preferences/in-content/search.xul
@@ +9,5 @@
>        <preference id="browser.search.hiddenOneOffs"
>                    name="browser.search.hiddenOneOffs"
>                    type="unichar"/>
>  
> +      <!-- Redirect Windows Search -->

This comment seems redundant.

::: browser/locales/en-US/chrome/browser/preferences/search.dtd
@@ +8,5 @@
>  
>  <!ENTITY provideSearchSuggestions.label        "Provide search suggestions">
>  <!ENTITY provideSearchSuggestions.accesskey    "s">
>  
> +<!ENTITY redirectWindowsSearch.label "Use my default search engine for searches from Windows">

I think maybe we should mention "Requires Firefox as the default browser". But maybe that can just be described in a SUMO article about the feature?
Attachment #8626601 - Flags: review?(jaws) → review+
(Assignee)

Comment 11

4 years ago
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #7)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > The other quirk here is that this would only be "used from the Windows start
> > menu" if Firefox was set as the default browser. Should that be mentioned
> > here? Or should there be a link/way from the Search page to set Firefox as
> > the default browser?
> 
> Should we check that we are the default browser here?

AFAIK Windows just passes off the Bing URL to whatever your default browser is. So we don't need to add any explicit checks -- it won't go to us unless we're the default.
(Assignee)

Updated

4 years ago
Depends on: 1177443
(In reply to Justin Dolske [:Dolske] from comment #11)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #7)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > > The other quirk here is that this would only be "used from the Windows start
> > > menu" if Firefox was set as the default browser. Should that be mentioned
> > > here? Or should there be a link/way from the Search page to set Firefox as
> > > the default browser?
> > 
> > Should we check that we are the default browser here?
> 
> AFAIK Windows just passes off the Bing URL to whatever your default browser
> is. So we don't need to add any explicit checks -- it won't go to us unless
> we're the default.

I understood the question to be "Should we check that we are the default browser here *before* we show this checkbox?" The checkbox doesn't make much sense when Firefox is not the default browser.
Flags: needinfo?(dolske)
Flags: needinfo?(clarkbw)
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #5)
> Matej and Winston, we need this string before EOW.  Can you coordinate and
> leave feedback?  See bug 1175223 for more context.
> 
> This is the preference string located in the Search category of the
> preferences:
> 
> [x] "Use my default search engine for searches from Windows"
> 
> I'll ask Dolske to upload a screenshot, for now this is fairly close except
> for the text: http://cl.ly/image/0j1Z3A0h0h3X

Hi Bryan,

What does "searches from Windows" mean? Does that still mean within Firefox? Or is there another way to search in Windows when you're not in the browser?

Also, would this only appear in Windows? If so, I'm wondering why we need the distinction.

Thanks.
Flags: needinfo?(matej)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> I understood the question to be "Should we check that we are the default
> browser here *before* we show this checkbox?" The checkbox doesn't make much
> sense when Firefox is not the default browser.

That is what I was wondering.  If the option doesn't make sense when we aren't the default browser, should hide it? I can be persuaded otherwise but I think we should, otherwise toggling it has no effect.

@phsla: what do you think?
Flags: needinfo?(clarkbw) → needinfo?(philipp)
Good questions.

(In reply to Matej Novak [:matej] from comment #13)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #5)
> > Matej and Winston, we need this string before EOW.  Can you coordinate and
> > leave feedback?  See bug 1175223 for more context.
> > 
> > This is the preference string located in the Search category of the
> > preferences:
> > 
> > [x] "Use my default search engine for searches from Windows"
> > 
> > I'll ask Dolske to upload a screenshot, for now this is fairly close except
> > for the text: http://cl.ly/image/0j1Z3A0h0h3X
> 
> Hi Bryan,
> 
> What does "searches from Windows" mean? Does that still mean within Firefox?
> Or is there another way to search in Windows when you're not in the browser?

This options only effects searches coming from Windows via the Start Menu or Cortana.  Other searches within the browser are not effected.  You can see the main point of entry in the screenshot of attachment 8626603 [details] where it says "Ask me anything" in the bottom left; this changes to say "Search..."

Here's the step by step: (on Windows 10 machine)
* Press Start button
* Start typing search query (a search input titled "Search the web and Windows" is focused by default)
* Press enter with the query "Whimsy"
* A new tab is opened in Firefox for the search "Whimsy" (Firefox is opened because it is your default browser)

> Also, would this only appear in Windows? If so, I'm wondering why we need
> the distinction.

Yes, this option would only appear for Windows 10 users.  The distinction may not be needed but then I don't know how to phrase where searches come from.
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #4)
> > * Most Firefox searches include, in the constructed URL, a param indicating
> > the context of the search (ie, what piece of UI the user used).
> > Unfortunately we don't have an existing value for "from the OS" or "from the
> > command line", and Yahoo returns suboptimal results when it doesn't
> > recognize the context, so we'll need to fix that still.
> 
> Joanne, is there an alternate code we should use here?  And if not or not
> available, what context code should we default to?

Commenting to myself!  Joanne is working on an alternate code, I'll file a followup bug for that but we shouldn't consider it blocking for now.
Flags: needinfo?(jnagel)
Uh, what I meant was... bug 1177443 is already filed for the followup and shouldn't block this work.
Comment on attachment 8626601 [details] [diff] [review]
Patch v.2

Review of attachment 8626601 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserContentHandler.js
@@ +731,5 @@
>        while ((ar = cmdLine.handleFlagWithParam("url", false))) {
>          var uri = resolveURIInternal(cmdLine, ar);
> +
> +        // XXX how robust to make this against minor URL variations?
> +        if (redirectWinSearch && uri.spec.startsWith("https://www.bing.com/search")) {

Actually, I think we should be bail out of this if the default search engine is set to Bing, and I don't see anything in this patch that does this. Specifically, it may be likely that Microsoft has special interactions or results that are triggered by the Cortana search, and our redirect here will invalidate those.
Attachment #8626601 - Flags: feedback-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8626601 [details] [diff] [review]
> Patch v.2
> 
> Review of attachment 8626601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserContentHandler.js
> @@ +731,5 @@
> >        while ((ar = cmdLine.handleFlagWithParam("url", false))) {
> >          var uri = resolveURIInternal(cmdLine, ar);
> > +
> > +        // XXX how robust to make this against minor URL variations?
> > +        if (redirectWinSearch && uri.spec.startsWith("https://www.bing.com/search")) {
> 
> Actually, I think we should be bail out of this if the default search engine
> is set to Bing, and I don't see anything in this patch that does this.
> Specifically, it may be likely that Microsoft has special interactions or
> results that are triggered by the Cortana search, and our redirect here will
> invalidate those.

I checked in with Joanne specifically about this and we want it to run for all searches, including Bing.
Priority: -- → P1
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> Good questions.

Thanks, Bryan. That's very helpful.

I guess we could clarify and call out the Start Menu and Cortana, but that would get long, so the string as is WFM.
(In reply to Matej Novak [:matej] from comment #20)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> > Good questions.
> 
> Thanks, Bryan. That's very helpful.
> 
> I guess we could clarify and call out the Start Menu and Cortana, but that
> would get long, so the string as is WFM.

Should we maybe change the string in settings to »Use this search engine for searches from Windows«?
Since it is visually grouped with the default selection, I think that would be a little less jargon.
Flags: needinfo?(philipp) → needinfo?(matej)
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #14)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > I understood the question to be "Should we check that we are the default
> > browser here *before* we show this checkbox?" The checkbox doesn't make much
> > sense when Firefox is not the default browser.
> 
> That is what I was wondering.  If the option doesn't make sense when we
> aren't the default browser, should hide it? I can be persuaded otherwise but
> I think we should, otherwise toggling it has no effect.
> 
> @phsla: what do you think?

Oops, didn't even realize that I was needinfo'd here...
Could we use this as an entry point for setting the default browser? 
Perhaps a line of text next to the checkbox that says something like »This only works if Firefox is your default browser. [Make Firefox my Default]«
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #22)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #14)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > > I understood the question to be "Should we check that we are the default
> > > browser here *before* we show this checkbox?" The checkbox doesn't make much
> > > sense when Firefox is not the default browser.
> > 
> > That is what I was wondering.  If the option doesn't make sense when we
> > aren't the default browser, should hide it? I can be persuaded otherwise but
> > I think we should, otherwise toggling it has no effect.
> > 
> > @phsla: what do you think?
> 
> Oops, didn't even realize that I was needinfo'd here...
> Could we use this as an entry point for setting the default browser? 
> Perhaps a line of text next to the checkbox that says something like »This
> only works if Firefox is your default browser. [Make Firefox my Default]«

I think that's a good idea, but feeling like we should call this v2 work.
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #21)
> (In reply to Matej Novak [:matej] from comment #20)
> > (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> > > Good questions.
> > 
> > Thanks, Bryan. That's very helpful.
> > 
> > I guess we could clarify and call out the Start Menu and Cortana, but that
> > would get long, so the string as is WFM.
> 
> Should we maybe change the string in settings to »Use this search engine for
> searches from Windows«?
> Since it is visually grouped with the default selection, I think that would
> be a little less jargon.

Yes, I like that. "Search engine" is a big jargony as well, but it's probably OK in this context.
Flags: needinfo?(matej)
(Assignee)

Comment 25

4 years ago
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #14)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > I understood the question to be "Should we check that we are the default
> > browser here *before* we show this checkbox?" The checkbox doesn't make much
> > sense when Firefox is not the default browser.
> 
> That is what I was wondering.  If the option doesn't make sense when we
> aren't the default browser, should hide it? I can be persuaded otherwise but
> I think we should, otherwise toggling it has no effect.

The flip side is that it's weird to have a pref that sometimes disappears depending on non-obviously related setting elsewhere. And can't be toggled without temporarily making Firefox your default. [We _could_ disable it and add a small explanatory text ("Firefox can only do this when it's your default browser", but that's still weird to do for similar reasons.]

I'm inclined to leave it as-is, and investigate a better default browser tie-in in as a possible followup. At worst its purpose might be a little unclear, but it's not incorrect.
Flags: needinfo?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #25)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #14)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > > I understood the question to be "Should we check that we are the default
> > > browser here *before* we show this checkbox?" The checkbox doesn't make much
> > > sense when Firefox is not the default browser.
> > 
> > That is what I was wondering.  If the option doesn't make sense when we
> > aren't the default browser, should hide it? I can be persuaded otherwise but
> > I think we should, otherwise toggling it has no effect.
> 
> The flip side is that it's weird to have a pref that sometimes disappears
> depending on non-obviously related setting elsewhere. And can't be toggled
> without temporarily making Firefox your default. [We _could_ disable it and
> add a small explanatory text ("Firefox can only do this when it's your
> default browser", but that's still weird to do for similar reasons.]
> 
> I'm inclined to leave it as-is, and investigate a better default browser
> tie-in in as a possible followup. At worst its purpose might be a little
> unclear, but it's not incorrect.

Agreed, I appreciate the simplest route.
(Assignee)

Comment 27

4 years ago
Posted patch Patch v.3Splinter Review
Updated with previous nits.

I also noticed that the search box generates a slightly different URL when doing a Cortana voice search, so I made that work too. When a term is typed, we get "form=WNSGPH" in the Bing URL. But when it's done by voice (ie, clicking the microphone), it's "FORM=WNSBOX".

I'm on Win10 preview build 10158, not sure if this changed or if we didn't actually test searching when playing around with the voice recognition earlier.
Attachment #8626601 - Attachment is obsolete: true
Attachment #8626603 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
https://hg.mozilla.org/mozilla-central/rev/5d12635ce6be
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Comment 31

4 years ago
Seems not work for me. I'm on Windows 10 build 10162 x64 edition and latest firefox nightly x64 build. When typing "test" in search box on taskbar and type "search", firefox still opens a page from bing. But I use Google as my default search engine for firefox.
Flags: needinfo?(dolske)
(Assignee)

Comment 32

4 years ago
(In reply to leichixian from comment #31)
> Seems not work for me. I'm on Windows 10 build 10162 x64 edition and latest
> firefox nightly x64 build.

Are you _sure_ you're on a build where this landed? IE, you see the "Use this search engine for searches from Windows" checkbox ala attachment 8630255 [details]?

> When typing "test" in search box on taskbar and
> type "search", firefox still opens a page from bing. But I use Google as my
> default search engine for firefox.

What's the full Bing URL that loads for you?
Flags: needinfo?(dolske) → needinfo?(leichixian)

Comment 33

4 years ago
(In reply to Justin Dolske [:Dolske] from comment #32)
> (In reply to leichixian from comment #31)
> > Seems not work for me. I'm on Windows 10 build 10162 x64 edition and latest
> > firefox nightly x64 build.
> 
> Are you _sure_ you're on a build where this landed? IE, you see the "Use
> this search engine for searches from Windows" checkbox ala attachment
> 8630255 [details]?
> 
> > When typing "test" in search box on taskbar and
> > type "search", firefox still opens a page from bing. But I use Google as my
> > default search engine for firefox.
> 
> What's the full Bing URL that loads for you?

I can confirm that this function is enabled in my nightly. I saw the checkbox in preference. The full URL when I type "test" in search box on Win 10 is this: https://cn.bing.com/search?q=test&form=WNSGPH&qs=8C&cvid=32189a7862ca4802a69a11d28b02f537&pq=test&sbts=1436402830947&nclid=McwAP2RTH41GAxcv6GK%2Fsg%3D%3D&ts=1436402821236
Flags: needinfo?(leichixian)

Comment 34

4 years ago
(In reply to leichixian from comment #33)
> (In reply to Justin Dolske [:Dolske] from comment #32)
> > (In reply to leichixian from comment #31)
> > > Seems not work for me. I'm on Windows 10 build 10162 x64 edition and latest
> > > firefox nightly x64 build.
> > 
> > Are you _sure_ you're on a build where this landed? IE, you see the "Use
> > this search engine for searches from Windows" checkbox ala attachment
> > 8630255 [details]?
> > 
> > > When typing "test" in search box on taskbar and
> > > type "search", firefox still opens a page from bing. But I use Google as my
> > > default search engine for firefox.
> > 
> > What's the full Bing URL that loads for you?
> 
> I can confirm that this function is enabled in my nightly. I saw the
> checkbox in preference. The full URL when I type "test" in search box on Win
> 10 is this:
> https://cn.bing.com/
> search?q=test&form=WNSGPH&qs=8C&cvid=32189a7862ca4802a69a11d28b02f537&pq=test
> &sbts=1436402830947&nclid=McwAP2RTH41GAxcv6GK%2Fsg%3D%3D&ts=1436402821236

And I can't switch to www.bing.com because Microsoft detects users IP to serve them country specific pages. I'm in China and I can only visit www.bing.com using tools usually used by Chinese to pass across the Great Firewall.
(Assignee)

Updated

4 years ago
Depends on: 1182308
(Assignee)

Comment 35

4 years ago
(In reply to leichixian from comment #33)

> I can confirm that this function is enabled in my nightly. I saw the
> checkbox in preference. The full URL when I type "test" in search box on Win
> 10 is this:
> https://cn.bing.com/
> search?q=test&form=WNSGPH&qs=8C&cvid=32189a7862ca4802a69a11d28b02f537&pq=test
> &sbts=1436402830947&nclid=McwAP2RTH41GAxcv6GK%2Fsg%3D%3D&ts=1436402821236

Thanks! Let's take this to bug 1182308 to fix. Right now we're strictly matching "https://www.bing.com", and should loosen that to make it work for cases like this.

If anyone else has examples of other searches/locales not working, please file a new bug blocking this (1177237) bug.
(Assignee)

Comment 36

4 years ago
Comment on attachment 8630251 [details] [diff] [review]
Patch v.3

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Users miss an opportunity to have Windows 10 searches use their default/expected search engine, and Mozilla misses an opportunity to talk about user search choice during the attention Windows 10 launch.
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Main risks are that we rewrite a search we shouldn't, or fail to rewrite a search we should. The current patch is conservative (which lead to the bug 1182308 followup).
[String/UUID change made/needed]: 1 string (and an access key). Per separate email thread, this is known as a late string change on Aurora, and an exceptional change on Beta that will require special outreach to get partial localization.
Attachment #8630251 - Flags: approval-mozilla-beta?
Attachment #8630251 - Flags: approval-mozilla-aurora?
Pike, Flod: Can we take these string changes for uplift to Aurora/Beta? Please comment.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(In reply to Ritu Kothari (:ritu) from comment #37)
> Pike, Flod: Can we take these string changes for uplift to Aurora/Beta?
> Please comment.

I guess we *have to* for Aurora (and the sooner it lands the better), for Beta I asked a question in the email thread but never got an answer from the product side.

> User's perspective: I assume we're going to ship with the pref enabled by
> default, because it seems the most logical thing to do. If a search opens in
> Firefox, and the user intentionally set is as default browser, it should use the
> default search engine I use in Firefox, not Bing. Using Bing with Firefox being
> set to a different default search engine sounds like an edge case, worth
> shipping Fx40 without the UI to enable it.

For sure we've been very bad in pushing this discussion to an end, given that it started over 2 weeks ago.
Flags: needinfo?(francesco.lodolo)
Lawrence is on the email thread, and still asking questions, I think we should defer to him to make the actual approval decision, instead of getting more people on the email thread, if that's OK.
Flags: needinfo?(l10n)
Comment on attachment 8630251 [details] [diff] [review]
Patch v.3

l10n has ok'd to uplift this change to Aurora even though it contains string changes.
Attachment #8630251 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting QE team to verify this feature/fix to ensure it works as expected.
Flags: qe-verify+
We got overruled, and so be it.
Comment on attachment 8630251 [details] [diff] [review]
Patch v.3

OK, let's do it then.
Attachment #8630251 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 44

4 years ago
QA Contact: cornel.ionce
Backed out from beta for mass bustage. Friendly reminder that you need to watch your pushes when landing on the release branches.
https://treeherder.mozilla.org/logviewer.html#?job_id=429363&repo=mozilla-beta

02:23:29 INFO - JavaScript error: jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/components/nsBrowserContentHandler.js, line 671: TypeError: AppConstants.isPlatformAndVersionAtLeast is not a function 

https://hg.mozilla.org/releases/mozilla-beta/rev/04f0d7265e3e
Flags: needinfo?(dolske)
(In reply to Axel Hecht [:Pike] from comment #42)
> We got overruled, and so be it.

I think overruled implies that you made a decision/call but in comment 39 you requested the relman make this call.

In a separate e-mail thread (let's merge the discussion here) I asked you "Have you spoken with any localizers about this? If not, can you and please confirm which locales will be able to localize this string in time for the 40 release?"

I followed this up with "The value in knowing which locales we can get is that this can inform our decision about taking the string. I do not want to ship an English string in any of our locales that keep up-to-date with localization. (The string will clearly look out of place in these builds.) If we can't get coverage for French, German, Italian, Russian, etc. (you tell me the important list), we may decide not to ship the pref or to ship without the pref entirely in these locales. Can you please poll your localizers to get some information on whether they can turn around this request in the next couple of weeks?"

If I wasn't clear, I do not want to ship the string partially localized for locales that we typically have very good coverage. This to me is not great (as in great or dead). My intention with this request is to try and understand what our l10n story will be wrt this string if we go ahead. Code landings are not final. (In fact this one bounced on Beta.) The string can be removed if we need it to be before we release.

Flod has informed the l10n teams about this change:

"I sent an email out to dev-l10n to warn about the strings right after they landed.

In case you wonder how to track how many locales will have a translation, this should be a good place (updated 3 times a day)"
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/preferences/search.dtd:redirectWindowsSearch.label&repo=beta

This is good as we've informed the teams. Rather than just wait and see what comes back, can you ask the localizers which teams think they will be able to handle this? Or, better yet, can you directly ask the teams that have the most substantial user bases if they will be able to localize the string so that we can make an informed decision about shipping this pref in 40?
Flags: needinfo?(l10n)

Comment 48

4 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #47)
> (In reply to Axel Hecht [:Pike] from comment #42)
> > We got overruled, and so be it.
> 
> I think overruled implies that you made a decision/call but in comment 39
> you requested the relman make this call.
> 
> In a separate e-mail thread (let's merge the discussion here) I asked you
> "Have you spoken with any localizers about this? If not, can you and please
> confirm which locales will be able to localize this string in time for the
> 40 release?"
> 
> I followed this up with "The value in knowing which locales we can get is
> that this can inform our decision about taking the string. I do not want to
> ship an English string in any of our locales that keep up-to-date with
> localization. (The string will clearly look out of place in these builds.)
> If we can't get coverage for French, German, Italian, Russian, etc. (you
> tell me the important list), we may decide not to ship the pref or to ship
> without the pref entirely in these locales. Can you please poll your
> localizers to get some information on whether they can turn around this
> request in the next couple of weeks?"
> 
> If I wasn't clear, I do not want to ship the string partially localized for
> locales that we typically have very good coverage. This to me is not great
> (as in great or dead). My intention with this request is to try and
> understand what our l10n story will be wrt this string if we go ahead. Code
> landings are not final. (In fact this one bounced on Beta.) The string can
> be removed if we need it to be before we release.
> 
> Flod has informed the l10n teams about this change:
> 
> "I sent an email out to dev-l10n to warn about the strings right after they
> landed.
> 
> In case you wonder how to track how many locales will have a translation,
> this should be a good place (updated 3 times a day)"
> https://transvision.mozfr.org/string/?entity=browser/chrome/browser/
> preferences/search.dtd:redirectWindowsSearch.label&repo=beta
> 
> This is good as we've informed the teams. Rather than just wait and see what
> comes back, can you ask the localizers which teams think they will be able
> to handle this? Or, better yet, can you directly ask the teams that have the
> most substantial user bases if they will be able to localize the string so
> that we can make an informed decision about shipping this pref in 40?

Well, I guess l10n team can do little if they just translate strings. If you push all the things develop team should do to l10n team, we may need to build a special version to a lot of locals, which certainly enlarge the work of upgrade and bug fixing.
The new search UI proved that we're not good at enabling features by locale. We used a pref and it ended up being displayed on Linux for all locales, and had issues in review.

Pocket proved that shipping things in English (or a subset of locales) is confusing for users. People read PR and news and don't understand why the feature X is missing in their build.

Personally I don't consider "not shipping the pref" a viable option at this point. We asked localizers to work on beta[1], and some of them already did. I only wish we had a clear response (and position) from the product side 2 weeks ago when we started the discussion. That alone would have doubled the time available for localization.

At this point we simply need to check how many locales we'll get and do a post-mortem to figure how to do approach these requests (not like we did).

[1] https://groups.google.com/d/msg/mozilla.dev.l10n/HTMf6w7PU7I/KCem_yNdEM0J
When it comes down to surveying some communities, I don't think it's helpful for me to be in the way of you getting the answers. At best, you just get your answers later than you could. I don't have much better contacts than what's listed on https://wiki.mozilla.org/L10n:Teams, you should feel free to send them questions directly. Feel free to CC me, too.
Flags: needinfo?(l10n)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Backed out from beta for mass bustage. Friendly reminder that you need to
> watch your pushes when landing on the release branches.
> https://treeherder.mozilla.org/logviewer.html#?job_id=429363&repo=mozilla-
> beta
> 
> 02:23:29 INFO - JavaScript error:
> jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/
> components/nsBrowserContentHandler.js, line 671: TypeError:
> AppConstants.isPlatformAndVersionAtLeast is not a function 
> 
> https://hg.mozilla.org/releases/mozilla-beta/rev/04f0d7265e3e

This is because https://hg.mozilla.org/mozilla-central/rev/8a504c99c379 was not uplifted to 40. We don't want to uplift all of that patch to 40, but we could uplift the AppConstants.jsm changes to 40.
(Assignee)

Comment 52

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Backed out from beta for mass bustage. Friendly reminder that you need to
> watch your pushes when landing on the release branches.
> https://treeherder.mozilla.org/logviewer.html#?job_id=429363&repo=mozilla-
> beta

Ah, nuts. I even did try pushes, but looks like I only tested the Aurora build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec48c49b51fa
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f250e485fa63

No points for "it's the thought that counts", obviously. :/
Flags: needinfo?(dolske)
(Assignee)

Updated

4 years ago
Attachment #8633933 - Attachment description: Patch for aurora/beta → Patch for aurora
(Assignee)

Comment 53

4 years ago
Added the AppConstants.jsm change noted in comment 51.
Attachment #8634305 - Flags: review?(jaws)
Comment on attachment 8634305 [details] [diff] [review]
Patch for beta

Review of attachment 8634305 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8634305 - Flags: review?(jaws) → review+
(Assignee)

Updated

4 years ago
Depends on: 1184737
Tested on a MS PRO 2 device and with a desktop computer, both running Windows 10 build 10240 using Nightly 42.0a1 and Aurora 41.0a2.
"Use this search engine for searches from Windows" option is correctly displayed on about:preferences#search. The option can be toggled correctly.
When Firefox is the default browser, if the option is not selected the search will be performed with Bing. If the option is selected, the search is correctly performed using the Firefox default search engine.
The option is also displayed if Firefox is not the default browser and it's toggling is not affecting the searches.
Also verified with custom search engines and got the expected results with each of them.

Covered the "en-US/de/es-CL/fr/it/zh-TW" locales.

Waiting for Firefox 40 beta 5 before closing this.
Also confirming this fix for Firefox 40 beta 7, build ID: 20150723165742.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

4 years ago
Depends on: 1215948

Updated

3 years ago
Depends on: 1194283
(Assignee)

Updated

3 years ago
Depends on: 1287622
You need to log in before you can comment on or make changes to this bug.