Closed Bug 1320072 Opened 3 years ago Closed 3 years ago

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

Categories

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

50 Branch
Unspecified
Android
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: u583025, Assigned: maliu)

References

Details

Attachments

(2 files)

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.
OS: Unspecified → Android
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.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Priority: -- → P3
Priority: P3 → P5
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.
Actually... this is something for the roadmap (and not triage). Barbara, what do you think?
tracking-fennec: ? → ---
Flags: needinfo?(bbermes)
Priority: P5 → P3
Assignee: nobody → max
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.
Flags: needinfo?(bbermes) → needinfo?(s.kaspari)
Yeah, compared with other Intent based features this should be pretty easy.
Flags: needinfo?(s.kaspari)
Any progress on this?
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?
Attachment #8818831 - Flags: review?(s.kaspari) → review+
(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?
Flags: needinfo?(s.kaspari)
Comment on attachment 8818831 [details]
Bug 1320072 - Support direct search from intent,

Sure! Transforming the NI into a review request.
Flags: needinfo?(s.kaspari)
Attachment #8818831 - Flags: review+ → review?(s.kaspari)
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.
Attachment #8818831 - Flags: review?(s.kaspari) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d24c7655825c
Support direct search from intent, r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d24c7655825c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I don't believe that PROCESS_TEXT should have been added here. 

See bug 1360670.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[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.
I've verified that this patch backs out cleanly (no conflicts).
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?
From more discussion on irc with Sebastian and Mike, Mike will get backout patch ready for all channels.
The backout patch applies cleanly to release and beta. I'm putting together a separate patch for inbound that I will check in.
I was wrong about inbound. I had a local patch. Once reviewed, I'll check in there.
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
Attachment #8863460 - Flags: approval-mozilla-release?
Attachment #8863460 - Flags: approval-mozilla-beta?
Comment on attachment 8863460 [details]
Bug 1320072 - Backout intent change - broke partner Google test.

via IRC
Attachment #8863460 - Flags: review?(snorp) → review+
Did the backout land on release? The flags were set to "fixed" so the sheriffs wouldn't see this.
Flags: needinfo?(mozilla)
Comment on attachment 8863460 [details]
Bug 1320072 - Backout intent change - broke partner Google test.

Land this for a dot release
Attachment #8863460 - Flags: approval-mozilla-release?
Attachment #8863460 - Flags: approval-mozilla-release+
Attachment #8863460 - Flags: approval-mozilla-beta?
Attachment #8863460 - Flags: approval-mozilla-beta+
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.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7894c708b6f
Backout intent change - broke partner Google test. r=snorp
Duplicate of this bug: 1360670
(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.
> 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.
This is an ex-Search Activity now (bug 1221344).
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
Removing leave-open keyword from resolved bugs, per :sylvestre.
You need to log in before you can comment on or make changes to this bug.