1048525 - Use SuggestClient from MC.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[Eric Edens [:eedens]]

We should remove the temporarily-bundled version of SuggestClient in favor of the SuggestClient in org.mozilla.gecko.home.

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.patch

Here's the associated PR: https://github.com/mozilla/fennec-search/pull/54

Comment 2

[:Margaret Leibovic]

Comment on attachment 8471166 [details] [diff] [review]
bug-1048525-fix.patch

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

Nice.

Comment 3

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/645f5cfe12b4

Comment 4

[Eric Edens [:eedens]]

https://github.com/mozilla/fennec-search/commit/ecd51b62630fef543399fc594e7c4ceb36ee03f1

Comment 5

[Chenxia Liu [:liuche]]

Backed out at eedens's request, because of crash when Gecko isn't running.

https://hg.mozilla.org/integration/fx-team/rev/07f040c1f11c

Comment 6

[Eric Edens [:eedens]]

Revert in Github:

https://github.com/mozilla/fennec-search/commit/6c7d5ba014cb9811e0f1b83758e2ca91d8487dfb

The root issue was the static init in SuggestClient [1]. This init works when gecko is running, but fails with a NPE otherwise.

[1] https://lxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SuggestClient.java#31

Comment 7

[:Margaret Leibovic]

bnicholson, what do you think we should do about this? I think we should just have a fallback user agent string. I feel like the user agent isn't too important in this case, because it's not like we're getting web content.

Right now we're just sending an empty string with search activity requests... I wonder if that's an adequate solution.

Comment 8

[Eric Edens [:eedens]]

Alternatively, we can inject the UA through the constructor. 

Then, for the search activity, we can use any UA -- blank, the webview's, something else ...

Comment 9

[Brian Nicholson (:bnicholson)]

Assuming we plan to eventually replace WebView with GeckoView, I would just bypass GeckoInterface for now with a comment:

// This should go through GeckoInterface to get the UA, but the search activity
// doesn't use a GeckoView yet. Until it does, get the UA directly.
String USER_AGENT = HardwareUtils.isTablet() ? AppConstants.USER_AGENT_FENNEC_TABLET :
                                               AppConstants.USER_AGENT_FENNEC_MOBILE;

Comment 10

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v2.patch

Thanks Brian!



PR: https://github.com/mozilla/fennec-search/pull/57
Try: https://tbpl.mozilla.org/?tree=Try&rev=54599c90adf5

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8471800 [details] [diff] [review]
bug-1048525-fix.v2.patch

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

::: mobile/android/base/home/SuggestClient.java
@@ +32,5 @@
>      private static final String LOGTAG = "GeckoSuggestClient";
> +
> +    // This should go through GeckoInterface to get the UA, but the search activity
> +    // doesn't use a GeckoView yet. Until it does, get the UA directly.
> +    String USER_AGENT = HardwareUtils.isTablet() ? AppConstants.USER_AGENT_FENNEC_TABLET :

This should be 'private static final'.

Comment 12

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v3.patch

bnicholson, good catch! Your code snippet was so nice I just copied and pasted it. :)

Comment 13

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v4.patch

Fixed code tabbing.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8471814 [details] [diff] [review]
bug-1048525-fix.v4.patch

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

Looks good to me, though we should probably move SuggestClient out of the home package since it's more general-purpose.

Comment 15

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> Comment on attachment 8471814 [details] [diff] [review]
> bug-1048525-fix.v4.patch
> 
> Review of attachment 8471814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, though we should probably move SuggestClient out of the
> home package since it's more general-purpose.

Yeah, I've had the same thought. Do you think we should just move it to /base? Or /util maybe?

Comment 16

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #15)
> Yeah, I've had the same thought. Do you think we should just move it to
> /base? Or /util maybe?

I'd vote /base since since it seems more like its own component than a helper util, but I don't have strong opinions either way.

Comment 17

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v5.patch

Changed location of SuggestClient to /base. I'll post a try build when the tree is open.

PR: https://github.com/mozilla/fennec-search/pull/57/files

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8472649 [details] [diff] [review]
bug-1048525-fix.v5.patch

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

Looks like you ended up copying the old SuggestClient without your changes!

Comment 19

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v6.patch

> Looks like you ended up copying the old SuggestClient without your changes!

Oh hell. Thanks for catching that! Here's a fresh patch that includes the changes.

Try: https://tbpl.mozilla.org/?tree=Try&rev=35790a387068
PR: https://github.com/mozilla/fennec-search/pull/57/files

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 8473079 [details] [diff] [review]
bug-1048525-fix.v6.patch

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

LGTM

Comment 21

[:Margaret Leibovic]

Comment on attachment 8473079 [details] [diff] [review]
bug-1048525-fix.v6.patch

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

The try run looks green, so I can this for you.

Comment 22

[Eric Edens [:eedens]]

https://github.com/mozilla/fennec-search/commit/65d9f086c22e07d6054340503c618fd66475c966

Comment 23

[Eric Edens [:eedens]]

Attachment: bug-1048525-fix.v7.patch

Rebased for landing

Comment 24

[:Margaret Leibovic]

Comment on attachment 8473382 [details] [diff] [review]
bug-1048525-fix.v7.patch

https://hg.mozilla.org/integration/fx-team/rev/e0ffa340816c

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0ffa340816c