Closed Bug 904279 Opened 6 years ago Closed 6 years ago

Changing default search engine doesn't change order of engines in awesomescreen

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- verified
firefox26 --- verified
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a regression from the behavior before bug 892113 landed.
This isn't a regression from Bug 892113 - this seems to be a bug in the way search suggestions are handled.
The existing code would add the configured suggestion engine (which seems to always be Google) to the list before the default engine, even if search suggestions were disabled.

So while changing the default engine was changing the ordering, Google was always being placed at the head of the queue, followed by your default engine. Tapping on the suggestion from Google would take you to a search on your default engine.

With this patch, if search suggestions are disabled then your engine appears at the top of the list. If suggestions are enabled, the suggestion engine (Google) always appears ahead of the default engine providing suggestions which, when tapped, are searched for using the default engine configured.

Is this the desired behaviour? At least the patch is nice and simple.
Attachment #789279 - Flags: review?(margaret.leibovic)
Comment on attachment 789279 [details] [diff] [review]
Don't display suggestion engine ahead of default engine if suggestions are disabled.

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

I'm confused about how this fixes things... it looks like suggestEngine could only be the current engine or default engine:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6548

I'm not actually sure how currentEngine differs from defaultEngine, but it seems like they're probably the same in the builds we're testing:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3572

I'm also having trouble reliably reproducing this problem with suggestions enabled or disabled, so I feel like it may not actually be related to this. I feel like I've seen this issue with search suggestions enabled, which wouldn't be the case if this is the source of the problem.
Attachment #789279 - Flags: review?(margaret.leibovic) → review-
Chris is right that this isn't a regression from his bug, I've seen it on release now, although it's hard to reproduce.

Cc'ing bnicholson, since he's familiar with this code as well.
Blocks: 730445
No longer blocks: 892113
tracking-fennec: --- → ?
Keywords: regressionsteps-wanted
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
tracking-fennec: + → 26+
What's happening here is that the defaultEngine is getting updated, but the currentEngine isn't. So, if the default engine is Google, then we switch it to Twitter, getSuggestionEngine will still end up returning Google:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6563

Questions:

Brian, why do we have this check for currentEngine? Why not just check the defaultEngine?

Gavin, when and how does currentEngine change? After restarting the browser, I find that currentEngine is what I would expect (the new default engine I chose).
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(bnicholson)
(In reply to :Margaret Leibovic from comment #4)
> What's happening here is that the defaultEngine is getting updated, but the
> currentEngine isn't. So, if the default engine is Google, then we switch it
> to Twitter, getSuggestionEngine will still end up returning Google:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6563

Is that because Twitter doesn't support suggestions?

currentEngine/defaultEngine at the search service level aren't linked in any way, and don't change by themselves (they only change if a consumer like Fennec sets them).
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > What's happening here is that the defaultEngine is getting updated, but the
> > currentEngine isn't. So, if the default engine is Google, then we switch it
> > to Twitter, getSuggestionEngine will still end up returning Google:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > browser.js#6563
> 
> Is that because Twitter doesn't support suggestions?

Correct, Twitter doesn't support suggestions (right now only Google supports suggestions), but even if it did, we would be checking currentEngine first and give priority to returning that. So that's not what's causing this problem.

> currentEngine/defaultEngine at the search service level aren't linked in any
> way, and don't change by themselves (they only change if a consumer like
> Fennec sets them).

Sounds like we should stop checking currentEngine there. But I'll wait to hear back from Brian before writing a patch.
(In reply to :Margaret Leibovic from comment #4)
> Brian, why do we have this check for currentEngine? Why not just check the
> defaultEngine?

I think the reasoning for this goes way back to bug 695198. From conversations with Pike, my understanding was that the default engine should never actually change; instead, we should change only the selected engine. Agreements with search providers was given as one of the reasons, but it also made sense conceptually that the engine set by the browser was the true "default" and couldn't be changed by definition.

This coincides with Pike's comment at https://bugzilla.mozilla.org/show_bug.cgi?id=695198#c6; there's also an IRC conversation attached to that bug where we talked about the default vs current engine. That said, I don't know how much of this is still relevant.
Flags: needinfo?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #7)

Thanks for the history. It's awesome you put an IRC convo in that bug!

> (In reply to :Margaret Leibovic from comment #4)
> > Brian, why do we have this check for currentEngine? Why not just check the
> > defaultEngine?
> 
> I think the reasoning for this goes way back to bug 695198. From
> conversations with Pike, my understanding was that the default engine should
> never actually change; instead, we should change only the selected engine.
> Agreements with search providers was given as one of the reasons, but it
> also made sense conceptually that the engine set by the browser was the true
> "default" and couldn't be changed by definition.

Well, in bug 730445, we added a feature to let the user change their default engine, so this isn't true anymore. That doesn't modify currentEngine, but does modify defaultEngine. Maybe we did that wrong? I feel like Gavin would have chimed in if that were the case.

> This coincides with Pike's comment at
> https://bugzilla.mozilla.org/show_bug.cgi?id=695198#c6; there's also an IRC
> conversation attached to that bug where we talked about the default vs
> current engine. That said, I don't know how much of this is still relevant.

Since the discussion in that bug, bug 738818 removed the keyword.URL pref, so I think the decisions in there are no longer relevant.

This logic is going to need to change if we move towards supporting suggestions from multiple search providers (Brian, do you have a bug filed on that?). But right now, we always want the default engine at the top of the list in the UI, and we only ever want to show suggestions for the engine at the top of the list, so I feel like we should just have a check to see if the default engine supports search suggestions.
Keywords: steps-wanted
It seems like this will become obsolete with the work going on in bug 851969, but this fixes the issue in the meantime.

I think that finding any suggestion engine and moving it to the top made more sense before we let the user change their default search engine (and with that the order of the search engines). With that added ability, shifting an arbitrary suggestion engine to the top is just confusing and looks like a bug (the one reported here).

With this patch, we'll only ever show suggestions for the default engine, and the default engine will always be at the top of this list. I suppose we could go into BrowserSearch and get rid of the logic about shifting order for default engines, but I feel lazy about doing that if we're going to change all that logic soon in bug 851969 anyway.
Attachment #789279 - Attachment is obsolete: true
Attachment #794844 - Flags: review?(bnicholson)
Comment on attachment 794844 [details] [diff] [review]
only check defaultEngine for suggestions

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

This looks fine to me, but I'd prefer waiting to land this until Pike or Gavin can confirm that it's OK to change defaultEngine and that the comments in bug 695198 no longer apply. I know bug 730445 has already landed and is in release, but since there's no mention of defaultEngine vs. currentEngine in that bug, I want to make sure it wasn't overlooked.
Attachment #794844 - Flags: review?(bnicholson) → review+
Flags: needinfo?(l10n)
Flags: needinfo?(gavin.sharp)
But doesn't doing this mean that, at least at the moment, if you set your default engine to be any except Google you cannot get suggestions.
Wouldn't it be optimal to secretly use the suggestion engine to provide suggestions, but show the icon of the configured default engine by these suggestions and, when tapped, send the user to their real default engine with the completed search (Even though the actual suggestion was provided by a different engine, in the case that the default engine is not suggestion-capable.
(In reply to Chris Kitching [:ckitching] from comment #11)
> But doesn't doing this mean that, at least at the moment, if you set your
> default engine to be any except Google you cannot get suggestions.
> Wouldn't it be optimal to secretly use the suggestion engine to provide
> suggestions, but show the icon of the configured default engine by these
> suggestions and, when tapped, send the user to their real default engine
> with the completed search (Even though the actual suggestion was provided by
> a different engine, in the case that the default engine is not
> suggestion-capable.

No, I don't think we should do this, mainly because the user won't have any insight into what's going on. Currently we only have one pref to enable/disable suggestions, but when the user chooses to enable that in the UI, we say which engine it is for. If DuckDuckGo is your default engine, I really doubt you would want to be sending your search terms to Google to get suggestions. Doing this without the user being aware would be a privacy fail.

Additionally, search providers probably wouldn't want suggestions they're not controlling to appear next to their logo, unless it was really clear that the suggestions weren't coming from them.

The real solution here is to just wait for bug 851969 to allow suggestions from multiple providers.
(In reply to Brian Nicholson (:bnicholson) from comment #10)
> This looks fine to me, but I'd prefer waiting to land this until Pike or
> Gavin can confirm that it's OK to change defaultEngine and that the comments
> in bug 695198 no longer apply. I know bug 730445 has already landed and is
> in release, but since there's no mention of defaultEngine vs. currentEngine
> in that bug, I want to make sure it wasn't overlooked.

Bug 738818 intentionally made defaultEngine settable in nsSearchService to support desktop Firefox UI's desire to have a single "Selected engine" concept that is used everywhere. So there's no reason for Fennec not to do the same (or a similar) thing, which as I understand it is what's going on here.
Flags: needinfo?(gavin.sharp)
Sounds good, thanks for the info.
Flags: needinfo?(l10n)
Comment on attachment 794844 [details] [diff] [review]
only check defaultEngine for suggestions

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this problem has been around since bug 730445 landed, but it is much more noticeable with bug 892094, which is in 25
User impact if declined: after switching the default search engine, the order of engines in the awesomescreen sometimes won't change until the browser is restarted
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, just reduces the number of engines we check for search suggestions
String or IDL/UUID changes made by this patch: none
Attachment #794844 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fe6833808b5a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment on attachment 794844 [details] [diff] [review]
only check defaultEngine for suggestions

low risk, polish fix.

Requesting QA to help verify once this lands.
Attachment #794844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.