Closed Bug 851969 Opened 7 years ago Closed 3 years ago

Add Amazon product search suggestions to Awesome Screen

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox48 --- verified
firefox49 --- verified
relnote-firefox --- 48+
fennec + ---

People

(Reporter: cpeterson, Assigned: mkaply)

References

()

Details

Attachments

(2 files)

The following blog post describes how Amazon implements the autocompletion search suggestions on the amazon.com home page:

http://blog.evanrosebrook.com/2011/01/using-amazons-autocomplete-suggestions.html

For example, the following URL with the search term is "Z" (i.e. "&q=Z"):

https://completion.amazon.com/search/complete?search-alias=aps&client=Mozilla-search&mkt=1&q=Z

will return the following JSON product search suggestions:

["z",["zumba","zippo","zero dark thirty","z10","zumba fitness dvd","zelda","zippo lighter","zumba dvd","zapatos adidas","zombie"],[{"nodes":[{"alias":"sporting","name":"Sports & Outdoors"},{"alias":"movies-tv","name":"Movies & TV"},{"alias":"videogames","name":"Video Games"},{"alias":"videogames-tradein","name":"Video Games Trade-in"},{"alias":"moviestv-tradein","name":"Movies & TV Trade-in"},{"alias":"apparel","name":"Clothing & Accessories"}]},{},{},{},{},{},{},{},{},{}],[]]
be careful that the mobile search is not the same as the desktop search.

Desktop search URI for amazon is /s 
Mobile search URI is  /gp/aw/s
This to avoid the lag of redirection that would occur for the user.
sorry, please ignore that previous comment -- I posted in the wrong ticket (was looking for Firefox OS, not FF for Android).
Attached patch Patch v1Splinter Review
This pulls in suggestions from any search engines you have that support it (if they're enabled). Right now we ship wikipedia and google with it enabled (we have a Yahoo plugin in our tree that supports it, but its disabled).

I modified things so that if your default search engine supports suggestions, its shown at the top (rather than the current behavior which shows anything with suggestions at the top of the list). If your default doesn't support suggestions, no search engines are shown at the top of the list. I felt like that gives you an understandable control over what you're seeing, but there might be people out there who really want to see suggestions for Google, but have their default search be Amazon?

Known problems (Hence Feedback and not review):
1.) There's no UI right now to disable search suggestions on a per-engine basis (and nothing in the nsISearchService for it either).
2.) There are also string problems with the current opt-in which includes the "default" search engine's name.

Build at:
http://people.mozilla.com/~wjohnston/suggest.apk
Also includes some separate tweaks to make suggestions only take a single, scrollable row.
Attachment #794227 - Flags: feedback?(bnicholson)
Flags: needinfo?(ibarlow)
(In reply to Wesley Johnston (:wesj) from comment #4)

> I modified things so that if your default search engine supports
> suggestions, its shown at the top (rather than the current behavior which
> shows anything with suggestions at the top of the list).

Have you seen bug 904279? It's actually a bug (that bug) if we return a non-default engine as the suggestions engine.

I'd like to see us get rid of the logic that changes the order based on which search engines might have suggestions, and just always show the engines in the same order, but it sounds like that's what your patch is doing.
Flags: needinfo?(ibarlow)
Oh whoops, didn't mean to un-needinfo myself, just adding Arun as he's thinking about multi search suggestions as well
Has this bug morhped into "Show search suggestions from multiple providers"? If so, we should update the summary.

However, maybe we should move that work into a different bug, since this bug was originally just filed about showing suggestions for Amazon search, which is something different (if we add support for those suggestions, our current UI will show them if Amazon is the default search engine).
Comment on attachment 794227 [details] [diff] [review]
Patch v1

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

::: mobile/android/chrome/content/browser.js
@@ -6496,5 @@
>      });
>  
> -    let suggestTemplate = null;
> -    let suggestEngine = null;
> -    let engine = this.getSuggestionEngine();

Drive-by: You should also remember to remove the getSuggestionEngine method, since this is the only place it's used.
Comment on attachment 794227 [details] [diff] [review]
Patch v1

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

Code mostly looks OK, but I know that Arun has been thinking about how we want to approach multi-provider suggestions, so you might want to talk to him about his plans before doing any more work on this.

A few weeks ago, I created an etherpad with some concerns here: https://etherpad.mozilla.org/wMC375bLW3. It's nice that you made this patch so we can test some of those concerns. As expected, there isn't much delay showing suggestions on 4G. There also weren't really any issues when I tried 3G and wifi, so that's good.

On 1X, however, it's a different story. From my brief experiments, it takes 4-6 seconds just for a single query to appear. The second query takes around 2 seconds longer to come in, so depending on the order, Google suggestions take longer to receive with this patch. It also feels weird having different engine populate their results several seconds apart, and this will be worse for every additional suggestion engine installed. Interestingly, even without this patch, Chrome suggestions appear about twice as fast on 1X, so there may be optimizations we can try here (or perhaps this is SPDY?).

Aside from performance issues, figuring out a good opt-in UI might be tricky....

::: mobile/android/base/home/BrowserSearch.java
@@ +363,5 @@
>  
> +        for (int i = 0; i < mSuggestClients.size(); i++) {
> +            SuggestClient client = mSuggestClients.get(i);
> +            SuggestionLoaderCallbacks cb = new SuggestionLoaderCallbacks(client);
> +            mSuggestionLoaderCallbacks.add(cb);

Don't think you want to do this -- this adds a new SuggestionLoaderCallbacks instance to an ever-growing list every time a filter happens!

I think what you wanted to do is check to see if the index is set in mSuggestionLoaderCallbacks. If it is, just call restartLoader() with the existing callback without creating a new one. If it's not, create a new one and add it to the list so it can be reused in the future.

@@ +370,5 @@
>      }
>  
> +    private void setSuggestions(SuggestClient client, ArrayList<String> suggestions) {
> +        int index = getIndexForClient(client);
> +        mSearchEngines.get(index).suggestions = suggestions;

I think you could call getIndexForClient() only once, which would be when you initialize the loader callbacks object. Then, if SuggestionLoaderCallbacks takes a SearchEngine rather than a SuggestClient, we could just use the engine here directly instead of looking it up every time.

@@ +383,5 @@
>              return;
>          }
>  
>          try {
> +

Drop this line

@@ +669,4 @@
>                  return ROW_STANDARD;
> +            } else {
> +                final SearchEngine engine = mSearchEngines.get(engineIndex);
> +                if (engine.suggestions != null && engine.suggestions.size() > 0 && mSuggestionsEnabled) {

I'm not sure this will work correctly -- the fix for bug 815937 assumed that there was no more than one suggestion provider (see https://bugzilla.mozilla.org/show_bug.cgi?id=815937#c1 for the ugly details). What was happening was that the rows were being populated in some order, then recycled in reverse order. Using only one suggestion row was the workaround for that, so if we have more than one suggestion row, I imagine we'll have this bug again.

I'd try this patch on a 2.2 device to see what happens. Assuming the glitch is still there, we might have to assign each suggestion row its own view type, or figure out some other way to solve that bug.

@@ +756,2 @@
>  
> +            // if the default (first) search engine, has suggestions, show it at the

Grammar nits: capitalize "if", remove first comma, and finish the comment

@@ +767,5 @@
>  
>              // Return search engine index
>              return position - resultCount;
>          }
> +

Drop this line

::: mobile/android/chrome/content/browser.js
@@ +2511,5 @@
>  
>      return browser;
>    },
>  
> +  

Remove this

@@ +6491,5 @@
>          name: engine.name,
>          identifier: engine.identifier,
>          iconURI: (engine.iconURI ? engine.iconURI.spec : null),
>          hidden: engine.hidden,
> +        immutable: immutableEngines.indexOf(engine) != -1,

This isn't being used anywhere.
Attachment #794227 - Flags: feedback?(bnicholson) → feedback+
Also, I see suggestions only for Google and Wikipedia, not Amazon, which is what this bug is about. Should I be seeing Amazon suggestions too?
Heh. No. I hijacked this bug. I'll move it, and put together a build with 4 engines that have suggestions to see just how awesome that is (we have Yahoo in the tree with suggestions, its just not enabled yet).
Blocks: 909536
No longer blocks: 909536
In the continuous quest to keep iOS engines more in-sync with Android, I noticed that we don't have Amazon suggestions for Android, but we have them for iOS [1].

Can we simply add [1] to Android's amazondotcom.xml? That'll be one less iOS-specific overlay that we need :)

[1] https://github.com/mozilla/firefox-ios/blob/be7e3f8bfd4933f8a3af305d515e4a101efd8350/Client/Assets/Search/SearchPlugins/en/amazondotcom.xml#L14
tracking-fennec: --- → ?
mkaply, do you know if there anything blocking this, can we just do this?

I would ask mconnor, but I bet he's on parental leave right now :)
Flags: needinfo?(mozilla)
Bug 997970 added search suggestions to the en-US Firefox.

It looks like Amazon communicated with Bryan Clark. Maybe he can offer some insight as to if there was a reason mobile was excluded and if we should take this?
Flags: needinfo?(mozilla) → needinfo?(clarkbw)
I don't think mobile was included in the discussion at the time.  To keep accurate metrics Amazon wanted to use specific params to indicate where the searches were coming from (esp locales / locations) and so I'd assume they have similar requirements for mobile.  That said, I think you could add it in now while you begin the discussion with them via bizdev in parallel to get any updated search params.
Flags: needinfo?(clarkbw)
Depends on: 1269481
I don't think this needs to track a specific release, but it sounds good to do. We should figure out how to move this forward.
tracking-fennec: ? → +
Flags: needinfo?(bbermes)
Definitely something to support and help one of the most used activities on the web: online shopping.

Aha card created.
Flags: needinfo?(bbermes)
mkaply, do you want to own making this happen?
Flags: needinfo?(mozilla)
(In reply to :Margaret Leibovic from comment #18)
> mkaply, do you want to own making this happen?

Assuming we're just talking about adding the line into the XML file, yes.
Flags: needinfo?(mozilla)
This simply adds the autocomplete URL to the search XML.

I've verified this gives us Amazon autocomplete.
Attachment #8752320 - Flags: review?(margaret.leibovic)
Attachment #8752320 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/f47209159b74
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Margaret: Do you think it's worth trying to uplift to 48?
Flags: needinfo?(margaret.leibovic)
(In reply to Mike Kaply [:mkaply] from comment #23)
> Margaret: Do you think it's worth trying to uplift to 48?

Sure, this is low risk. Let's do it!
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8752320 [details] [diff] [review]
Add Amazon suggestions to XML

Approval Request Comment
[Feature/regressing bug #]: Adding Amazon suggestions to awesome screen
[User impact if declined]: None
[Describe test coverage new/current, TreeHerder]: current
[Risks and why]: Low risk.
[String/UUID change made/needed]: None

This is extremely low risk and adds a nice user feature.
Attachment #8752320 - Flags: approval-mozilla-aurora?
Assignee: nobody → mozilla
Comment on attachment 8752320 [details] [diff] [review]
Add Amazon suggestions to XML

Sure, why not ;)
Attachment #8752320 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Add Amazon product search suggestions to Awesomescreen Search
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Relnote 49+ - added "Add Amazon product search suggestions to Awesomescreen Search"
(In reply to Marcia Knous [:marcia - use ni] from comment #29)
> Relnote 49+ - added "Add Amazon product search suggestions to Awesomescreen
> Search"

This was uplifted to 48.
Flags: needinfo?(mozillamarcia.knous)
(In reply to Marcia Knous [:marcia - use ni] from comment #29)
> Relnote 49+ - added "Add Amazon product search suggestions to Awesomescreen
> Search"

Removed relnote for 49.
Flags: needinfo?(mozillamarcia.knous)
QA Contact: mihai.ninu
This is part of 48 release notes:
"Amazon product search suggestions added to Awesomescreen Search"
I can confirm it is correctly implemented on all builds Nightly (50.0a1 / 2016-07-25), Aurora (49.0a2 / 2016-07-25) and Beta (48.0b10). 
Since this feature is ready to be shipped with 48, can it be moved to the Aha 48 section , from the 49?
Ninu's work can be tracked here- https://wiki.mozilla.org/QA/Fennec/Add_Amazon_product_search_suggestions_to_Awesome_Screen 

@Barbara - should we still move the card or it is too late as it is outdated already?

Setting this as VERIFIED FIXED as all versions were checked and the flags moved accordingly.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bbermes)
I can't move the card into a past release, but we have the title indicating that this was uplifted to 48. Is that sufficient? https://mozilla.aha.io/features/FENN-503
Flags: needinfo?(bbermes)
Yes, thank you Barbara!
You need to log in before you can comment on or make changes to this bug.