1413739 - Offer Firefox itself as an Assist App instead of the former Search Activity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

(In reply to Nick Alexander :nalexander from comment #25)
> (In reply to Mark from comment #24)
> > I just filed bugs 1406876 and 1407877 and got told Search Activity is going
> > to die. Boo, a tragedy! But I understand.
> > 
> > Can you at least tweak Fennec so that a long press of the Home button brings
> > up Fennec with the cursor in the Awesomebar and the keyboard invoked. Like
> > Samsung Internet Browser does?
> 
> This is a cool idea.  File the ticket, CC me, and we'll triage it?  I didn't
> know such things were possible.

It seems the magic keyword is registering as an "Assist app" [1] and apparently the search activity used to do this. So we'd need to copy whatever magic incantation (presumably some intent filter) is necessary and make sure to handle that intent by opening a new tab (always about:home, or using the user's home page preference?) with editing mode enabled.

[1] Settings -> Apps -> settings icon -> Default apps -> Assist & voice input -> Assist app

Comment 1

[Jan Henning [:JanH]]

Starting from Lollipop, we can specify an icon that gets used when the user launches us as an assist app (see bug 1210242 where we did this for the former search activity and https://bug1210242.bmoattachments.org/attachment.cgi?id=8671541 for an example screenshot). Should I simply use our launcher icon, or do you prefer something else there?

Comment 2

[Carol Huang [:Carol]]

Hi Jan,
I will suggest using our launcher icon.
Could I see a screenshot once we put the icon on the list? :)
Thank you!

Comment 3

[Nevin Chen(Not active on Bugzilla)]

That's cool! 
Ni Max so he knows this.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

The technique for setting our icon is just a straight reimplementation
of bug 1210242.

Because of the way the new tab might be opened from within a
processActionViewIntent Runnable, we can't enter editing mode by simply
listening for an ACTION_ASSIST intent from within BrowserApp, as we need to
enter editing mode *after* the correct tab has already been opened and selected
and BrowserApp doesn't get any hint on when that Runnable might have run.

Instead, we introduce a new tab event, so we can trigger editing mode at the
right time via the tab itself.

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

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

assertUrl wasn't used before and getUrlEditText().getText() returns a
SpannableStringBuilder, so we need to add a toString() conversion there in order
to successfully use it.

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

Comment 6

[Jan Henning [:JanH]]

Attachment: assist_intent_logo.png

(In reply to Carol Huang [:Carol] from comment #2)
> Could I see a screenshot once we put the icon on the list? :)

Voila (I'm using the Hobbes icon for my local build, in case you're wondering).

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

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

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

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

Comment 9

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9f895d7324a29ebe6debbe1c4ce0d386d200c13

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

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

Comment 11

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]: The search activity has been removed, so we should offer a replacement for users that had used Firefox as an assist app when long-pressing the home button.

Comment 12

[Gerry Chang [:gchang]]

Hi :JanH,
We usually don't allow new feature to be added in beta cycle because it's not baked long enough in nightly cycle. In particular, we don't have aurora cycle anymore. I would suggest we let this ride the train. Track 58- and mark 58 won't fix.

Comment 13

[Jan Henning [:JanH]]

You could argue that this is a regression from bug 1221344, though.

Comment 14

[Jan Henning [:JanH]]

Max, if you don't have the time to review this, could you at least pass this off to someone else?

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Hi Carol.
Please take a look after the APK is generated.

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

Remembered that I wanted to extend this to exercise the onNewIntent code path as well.

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

https://reviewboard.mozilla.org/r/196466/#review209726

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1087
(Diff revision 2)
>       * Opens a new tab and loads either about:home or, if PREFS_HOMEPAGE_FOR_EVERY_NEW_TAB is set,
>       * the user's homepage.
>       */
>      @RobocopTarget
>      public Tab addTab() {
> -        return loadUrl(getHomepageForNewTab(mAppContext), Tabs.LOADURL_NEW_TAB);
> +        return addTab(0);

Maybe LOADURL_NONE?

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1095
(Diff revision 2)
>      public Tab addPrivateTab() {
> -        return loadUrl(getHomepageForNewTab(mAppContext), Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE);
> +        return addTab(Tabs.LOADURL_PRIVATE);
> +    }
> +
> +    public Tab addTab(int flags) {
> +        return loadUrl(getHomepageForNewTab(mAppContext), flags | Tabs.LOADURL_NEW_TAB);

In my two cents, now Tabs class have too many ways to add atab. addTab..() and also loadUrl..(). I think it'll be better if we do this in one of those loadUrl(....) methods. How do you think?

Comment 18

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

https://reviewboard.mozilla.org/r/196468/#review209730

Comment 19

[Nevin Chen(Not active on Bugzilla)]

APK is here
https://queue.taskcluster.net/v1/task/FO2oUwU6QheL5Ov9bo76OA/runs/0/artifacts/public/build/target.apk

Comment 20

[Jan Henning [:JanH]]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

https://reviewboard.mozilla.org/r/196466/#review209726

> Maybe LOADURL_NONE?

Yes, somehow I overlooked the existence of that.

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

https://reviewboard.mozilla.org/r/196466/#review209726

> In my two cents, now Tabs class have too many ways to add atab. addTab..() and also loadUrl..(). I think it'll be better if we do this in one of those loadUrl(....) methods. How do you think?

I don't know - I think it's a nice shorthand so callers don't have to worry about what URL to use when opening a new tab.

Comment 22

[Jan Henning [:JanH]]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

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

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

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

Comment 24

[Jan Henning [:JanH]]

The Robocop test now tests both the onCreate and the onNewIntent code paths:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a3a4b9c6dc9a10e6c1c245c9b2d892d25505fe3

Comment 25

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

https://reviewboard.mozilla.org/r/196466/#review210480

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1099
(Diff revision 3)
>  
>      public Tab addPrivateTab() {
> -        return loadUrl(getHomepageForNewTab(mAppContext), Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE);
> +        return addTab(Tabs.LOADURL_PRIVATE);
> +    }
> +
> +    public Tab addTab(int flags) {

Please hlep add a comment here for the reason "e.g. it's a shorthand so callers don't have to worry about what URL to use when opening a new tab."

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

https://reviewboard.mozilla.org/r/196468/#review210484

Thanks!

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8925256 [details]
Bug 1413739 - Part 1 - Offer Firefox itself as an Assist App.

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

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8925257 [details]
Bug 1413739 - Part 2 - Simple test for handling of an ACTION_ASSIST intent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196468/diff/4-5/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/151826976a7d
Part 1 - Offer Firefox itself as an Assist App. r=nechen
https://hg.mozilla.org/integration/autoland/rev/92d7837be2db
Part 2 - Simple test for handling of an ACTION_ASSIST intent. r=nechen

Comment 30

[Jan Henning [:JanH]]

Release Note Request (optional, but appreciated)
[Why is this notable]: This used to be possible via our Search Activity, which was removed in bug 1221344. This bug restores this functionality (open an activity where you can start a search with one of the search engines installed in Firefox) via Firefox itself.
[Affects Firefox for Android]: Only
[Suggested wording]: You can register Firefox as an Assist App to start a search by long-pressing the home button.
[Links (documentation, blog post, etc)]: none

Comment 31

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/151826976a7d
https://hg.mozilla.org/mozilla-central/rev/92d7837be2db

Comment 32

[Mark]

This is great, I love it.

I've filed bug 1423566 re seeing the tabs tray instead of the new tab, I think it would be great to fix that too to polish it up.

thanks for doing this

Mark

Comment 33

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

Relnote added for 59.0b1 as New: "Added Firefox as an Assist App so users can start a search by long-pressing the home button"