1320072 - Firefox doesn't integrate with Android's Web Search "Intent"

Status: RESOLVED WONTFIX

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

Description

[u583025]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161114144856

Steps to reproduce:

Android has something called a Web Search "Intent" - https://developer.android.com/reference/android/content/Intent.html#ACTION_WEB_SEARCH
It can take keywords from apps (like text from an ebook reader say) and searches them in any app that follows the Web Search "Intent".


I have an ebook reader app which has implemented the web search intent to be passed on to apps which support it, like Google's own Search app. Moreover, Bing, Opera Browser, Dolphin Browser and others support it too. Firefox doesn't

Here is the GIF showcasing the same - http://imgur.com/9NAYQwK

Now considering that Firefox already has a Search Activity, which acts as Assists and acts separate from the browser, it would be great if the Search Activity followed the Intent and gave the desired search results. 


Actual results:

Firefox should have read the Android Web Search "Intent" and be displayed among the options for when an Intent is asked for.


Expected results:

Firefox/ Firefox Search Activity don't show up as a  browser/search app that can read the Android Web Search Intent.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Shouldn't be too hard. Flagging for triage: Maybe a front-end developer has time to implement this. This could potentially improve engagement a bit.

Comment 2

[u583025]

Considering that all famous browsers and search apps use search "intent" since a long time and many 3rd party apps have integrated the ability to launch the intent, I think it would improve Firefox quite noticeably.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Actually... this is something for the roadmap (and not triage). Barbara, what do you think?

Comment 4

[Barbara Bermes [:barbara] - NI please!]

Definitely something to put on the roadmap for "Intent" work. I see other Intent plans for more important for now, unless it's easy to do (work-wise/effort-wise)?

Please include probes to understand the usage of this intent.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, compared with other Intent based features this should be pretty easy.

Comment 6

[u583025]

Any progress on this?

Comment 7

[Max Liu [:maliu]]

Attachment: Bug 1320072 - Support direct search from intent,

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

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

https://reviewboard.mozilla.org/r/98772/#review99378

LGTM. I'm just not sure about the state when the activity is getting restored.

::: mobile/android/search/java/org/mozilla/search/SearchActivity.java:180
(Diff revision 1)
> -        if (savedInstanceState != null) {
> +        final String queryString = getQueryString();
> +        if (!TextUtils.isEmpty(queryString)) {
> +            searchBar.setText(queryString);
> +            onSearch(queryString);
> +        } else if (savedInstanceState != null) {

I wonder if the block checking savedInstanceState should be before this one?

What if we start the activity with a search intent, perform some other searches, leave the activity and later return (activity gets restored): Will be run the initial query again instead of restoring the last query?

Comment 9

[Max Liu [:maliu]]

Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

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

Comment 10

[Max Liu [:maliu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> Comment on attachment 8818831 [details]
> ::: mobile/android/search/java/org/mozilla/search/SearchActivity.java:180
> (Diff revision 1)
> > -        if (savedInstanceState != null) {
> > +        final String queryString = getQueryString();
> > +        if (!TextUtils.isEmpty(queryString)) {
> > +            searchBar.setText(queryString);
> > +            onSearch(queryString);
> > +        } else if (savedInstanceState != null) {
> 
> I wonder if the block checking savedInstanceState should be before this one?
> 
> What if we start the activity with a search intent, perform some other
> searches, leave the activity and later return (activity gets restored): Will
> be run the initial query again instead of restoring the last query?

I guess you are correct, but there could also be another problem. If we always take savedInstanceState prior than incoming query, then new query is not been handled as long as savedInstanceState exist. So I made some modification on the patch. Would you help to take a look please?

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

Sure! Transforming the NI into a review request.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

https://reviewboard.mozilla.org/r/98772/#review105288

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:79
(Diff revision 2)
>              Log.w(LOGTAG, "Couldn't get intent extras.", e);
>              return null;
>          }
>      }
>  
> +    public Bundle getExtras() {

nit: This is not the convention in this file yet, but I'd either return an empty Bundle in the exception case or add @Nullable to the method signature.

Comment 13

[Max Liu [:maliu]]

Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d24c7655825c
Support direct search from intent, r=sebastian

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Mike Kaply [:mkaply]]

I don't believe that PROCESS_TEXT should have been added here. 

See bug 1360670.

Comment 17

[Mike Kaply [:mkaply]]

We are going to need to back out this change for a Firefox 53.0.2.

The changes it made to Android integration are affecting our preloads.

We need to spend more time investigating these integration points.

Comment 18

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: Our partners are having issues with these changes on their preloads because of how it changes our Android integration.

Comment 19

[Mike Kaply [:mkaply]]

I've verified that this patch backs out cleanly (no conflicts).

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Should we back this out from other branches? We can respin a new Fennec quickly, but I'd like to make sure we know we're doing the right thing. 

Are we likely to break something else from the backout?

How can we handle QA with/for distribution partners in a better way?

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

From more discussion on irc with Sebastian and Mike, Mike will get backout patch ready for all channels.

Comment 22

[Mike Kaply [:mkaply]]

Attachment: Bug 1320072 - Backout intent change - broke partner Google test.

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

Comment 23

[Mike Kaply [:mkaply]]

The backout patch applies cleanly to release and beta. I'm putting together a separate patch for inbound that I will check in.

Comment 24

[Mike Kaply [:mkaply]]

I was wrong about inbound. I had a local patch. Once reviewed, I'll check in there.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 8863460 [details]
Bug 1320072 - Backout intent change - broke partner Google test.

This is a backout, it shouldn't need review in hindsight.

Approval Request Comment
[Feature/Bug causing the regression]: This bug (1320072)
[User impact if declined]: Firefox won't ship on new devices
[Is this code covered by automated tests?]: No (it wasn't before either)
[Has the fix been verified in Nightly?]: Hasn't been backed out there yet.
[Needs manual test from QE? If yes, steps to reproduce]: Verify that Firefox is not in the Web Search when long pressing text in other apps.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Backs out new code.
[String changes made/needed]: None

Comment 26

[Mike Kaply [:mkaply]]

Comment on attachment 8863460 [details]
Bug 1320072 - Backout intent change - broke partner Google test.

via IRC

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Did the backout land on release? The flags were set to "fixed" so the sheriffs wouldn't see this.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8863460 [details]
Bug 1320072 - Backout intent change - broke partner Google test.

Land this for a dot release

Comment 29

[Mike Kaply [:mkaply]]

Not sure what to do with the flags here now.

The original bug is not fixed.

I've backed this out of beta and release.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7894c708b6f
Backout intent change - broke partner Google test. r=snorp

Comment 31

[Ryan VanderMeulen [:RyanVM]]

For posterity:
https://hg.mozilla.org/releases/mozilla-beta/rev/014fa3e151f8
https://hg.mozilla.org/releases/mozilla-release/rev/44981f78f602

Comment 33

[Carsten Book [:Tomcat]]

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

Comment 34

[u583025]

(In reply to Mike Kaply [:mkaply] from comment #29)
> Not sure what to do with the flags here now.
> 
> The original bug is not fixed.
> 
> I've backed this out of beta and release.

Yeah, I'd still like to have this feature if possible. Its very handy.

Comment 35

[Mike Kaply [:mkaply]]

> Yeah, I'd still like to have this feature if possible. Its very handy.

Totally understand. We're trying to figure out how to make it happen.

Comment 36

[Jan Henning [:JanH]]

This is an ex-Search Activity now (bug 1221344).

Comment 37

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.