1356346 - (customtabs_geckoview) Overhaul CustomTabsActivity to use GeckoView rather than extending GeckoApp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Dylan Roeh (:droeh) (he/him)]

Ideally we want to have our custom tabs client based on GeckoView, rather than the current solution of using workarounds to allow multiple GeckoApps.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

A first attempt at it. Most front end functionality is preserved, except for the progress bar. Currently running into an issue where switching between multiple custom tabs (and/or Fennec) results in random black tiles being rendered.

Comment 2

[Jan Henning [:JanH]]

I realise this is an early WIP, but some random thoughts:
- If I launch a custom tab first and the main UI afterwards, things are broken in the latter with symptoms similar to (although not exactly the same as) bug 1356563
- We have some code that assumes there's only ever one window and therefore simply uses getMostRecentWindow("navigator:browser") when it needs a window
- Session store doesn't work in the custom tab (presumably because it expects a window with browser.js's BrowserApp and tabs), although with the current implementation that's probably just as well, because if it did work, the custom tab would simply overwrite the user's previous session
- If this approach works out, it should probably be generalised into a base class that can be used for web apps as well
- Add-on compatibility, since GeckoView doesn't implement a number of concepts expected by add-ons (although at least uBlock and Stylish seem to be working)?
- The windows/content browsers opened are never closed until eventually the whole process is killed

(In reply to Dylan Roeh (:droeh) from comment #1)
> Most front end functionality is preserved, except for
> the progress bar.

Some things that are currently planned/under development and which would have to be reimplemented as well by this:
- Context menu -> Open link in new tab -> pressing "Switch" will open a new tab and switch to our main UI
- The ability to take a Gecko-side tab and simply display it in another activity (i.e. not just opening the same URL in the target activity, which would lose quite a bit of state), e.g. for switching from custom tabs to the full UI, or for switching to/from the Web App UI when entering/leaving a web app. This would probably need something like the Desktop code for moving browsers between windows...

Comment 3

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

Rebased with minor updates, but no fix for black tiles or dead tabs in Fennec yet.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jan Henning [:JanH] from comment #2)
> I realise this is an early WIP, but some random thoughts:
> - If I launch a custom tab first and the main UI afterwards, things are
> broken in the latter with symptoms similar to (although not exactly the same
> as) bug 1356563

Yeah, Dylan is investigating that here, so maybe we'll know more about what's going on there soon.

> - We have some code that assumes there's only ever one window and therefore
> simply uses getMostRecentWindow("navigator:browser") when it needs a window

Yeah, that's bad. The main place I think we have that is in the session store code, but...

> - Session store doesn't work in the custom tab (presumably because it
> expects a window with browser.js's BrowserApp and tabs), although with the
> current implementation that's probably just as well, because if it did work,
> the custom tab would simply overwrite the user's previous session

Exactly, we don't want SessionStore doing anything to GeckoView windows. One thing we've talked about is using a windowtype other than navigator:browser in GeckoView to avoid any accidental getMostRecentWindow("navigator:browser") stuff.

> - If this approach works out, it should probably be generalised into a base
> class that can be used for web apps as well

Definitely! FennecView would hook up Fennec's prompt implementation, history storage, password provider, etc. Lots of the stuff needed for that is not implemented in GV yet, however.

> - Add-on compatibility, since GeckoView doesn't implement a number of
> concepts expected by add-ons (although at least uBlock and Stylish seem to
> be working)?

For the most part we're punting on that issue for the time being, but we should ensure that things don't just blow up. As you said, addons that have content scripts should mostly just work. It's things that add browser UI elements that are problematic.

> - The windows/content browsers opened are never closed until eventually the
> whole process is killed

Wow, that sounds like a bad bug. The window should be closed when you finish the custom tab activity and the GeckoView is torn down. Dylan, can you verify?

> 
> (In reply to Dylan Roeh (:droeh) from comment #1)
> > Most front end functionality is preserved, except for
> > the progress bar.
> 
> Some things that are currently planned/under development and which would
> have to be reimplemented as well by this:
> - Context menu -> Open link in new tab -> pressing "Switch" will open a new
> tab and switch to our main UI
> - The ability to take a Gecko-side tab and simply display it in another
> activity (i.e. not just opening the same URL in the target activity, which
> would lose quite a bit of state), e.g. for switching from custom tabs to the
> full UI, or for switching to/from the Web App UI when entering/leaving a web
> app. This would probably need something like the Desktop code for moving
> browsers between windows...

Yeah, we should look at how that gets done. Probably just reparent the <browser> element? I wonder if something like that would be useful for pure GeckoView applications -- moving the content from one GV to another. I can't think of many good uses...

Comment 5

[Jan Henning [:JanH]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Wow, that sounds like a bad bug. The window should be closed when you finish
> the custom tab activity and the GeckoView is torn down. Dylan, can you
> verify?

I think I didn't see any "domwindowclosed" notifications, but I'd have to check again to make sure there was no error in my logging that caused me to miss them.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

(In reply to Jan Henning [:JanH] from comment #5)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > Wow, that sounds like a bad bug. The window should be closed when you finish
> > the custom tab activity and the GeckoView is torn down. Dylan, can you
> > verify?
> 
> I think I didn't see any "domwindowclosed" notifications, but I'd have to
> check again to make sure there was no error in my logging that caused me to
> miss them.

I'll check into this shortly, thanks for catching all of these!

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Also, for anyone who doesn't want to build, an apk with geckoview custom tabs enabled is available here: https://www.dropbox.com/s/wzkmv61th7ny3ih/fennec-55.0a1.en-US.android-arm.apk?dl=0

Comment 8

[Dylan Roeh (:droeh) (he/him)]

(In reply to Dylan Roeh (:droeh) from comment #6)
> (In reply to Jan Henning [:JanH] from comment #5)
> > (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > > Wow, that sounds like a bad bug. The window should be closed when you finish
> > > the custom tab activity and the GeckoView is torn down. Dylan, can you
> > > verify?
> > 
> > I think I didn't see any "domwindowclosed" notifications, but I'd have to
> > check again to make sure there was no error in my logging that caused me to
> > miss them.
> 
> I'll check into this shortly, thanks for catching all of these!

I'm getting domwindowclosed messages (specifically, checking in SessionStore.observe) when I finish a CustomTabsActivity.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

(In reply to Dylan Roeh (:droeh) from comment #8)
> (In reply to Dylan Roeh (:droeh) from comment #6)
> > (In reply to Jan Henning [:JanH] from comment #5)
> > > (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > > > Wow, that sounds like a bad bug. The window should be closed when you finish
> > > > the custom tab activity and the GeckoView is torn down. Dylan, can you
> > > > verify?
> > > 
> > > I think I didn't see any "domwindowclosed" notifications, but I'd have to
> > > check again to make sure there was no error in my logging that caused me to
> > > miss them.
> > 
> > I'll check into this shortly, thanks for catching all of these!
> 
> I'm getting domwindowclosed messages (specifically, checking in
> SessionStore.observe) when I finish a CustomTabsActivity.

Er, to clarify, one domwindowclosed message per custom tab I close, of course.

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

Rebased again. I've temporarily removed some functionality (getDoorHangerOverlay and getTextSelectPresenter; they were overrides of GeckoApp functions) unfortunately. I'll add disentangling that stuff from GeckoApp (if it's not too difficult/ugly to do so) to the to-do list for this.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

Rebased. Still two show-stoppers at this point: white screen (with multiple geckoviews), and dead tabs in Fennec if launched after a custom tab.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

Alright, this is looking good enough to land, I think.

In terms of front end, the action bar should more or less behave the same as it does in the GeckoApp-based implementation; the progress bar is gone (good riddance); the text selection and long click context menus are not yet working.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8868583 [details] [diff] [review]
GeckoView-based custom tabs

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

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +118,5 @@
> +
> +        final GeckoProfile profile = GeckoProfile.get(getApplicationContext());
> +
> +        GeckoThread.initMainProcess(profile, /* args */ null, /* debugging */ false);
> +        GeckoThread.launch();

Don't need this block

@@ +124,5 @@
> +        if (intent != null) {
> +            final Uri u = intent.getData();
> +            mGeckoView.loadUri(u.toString());
> +        } else {
> +            mGeckoView.loadUri("http://mozilla.org");

Probably don't want this

@@ +385,5 @@
>      /**
>       * Update loading progress of current page
>       *
>       * @param isLoading to indicate whether ProgressBar should be visible or not
>       * @param progress  value of loading progress in percent, should be 0 - 100.

Update comment.

@@ +423,5 @@
>      private void onOpenInClicked() {
> +        final Intent intent = new Intent();
> +        intent.setData(Uri.parse(mCurrentUrl));
> +        intent.setAction(Intent.ACTION_VIEW);
> +        startActivity(intent);

Do we need to keep the `finish()` here?

@@ +482,5 @@
>          }
>          return null;
>      }
> +
> +    private class Navigation implements GeckoView.NavigationListener {

Why not implement these in CustomTabsActivity itself?

::: mobile/android/chrome/geckoview/geckoview.xul
@@ +6,5 @@
>  <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
>  
>  <window id="main-window"
>          onload="startup();"
> +        windowtype="navigator:embeddedbrowser"

"navigator:embedded-browser" or maybe just "navigator:geckoview"

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs v1.1

Updated to address Jim's suggestions.

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8868583 [details] [diff] [review]
GeckoView-based custom tabs

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

Love this feature. But if we want to land this patch, some front-end work still needs to be done. 
I've added some comments, but :walkingice is the owner of CustomTabActivity. So I'll transfer this review request to him.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ -62,5 @@
>      private final SparseArrayCompat<PendingIntent> menuItemsIntent = new SparseArrayCompat<>();
>      private GeckoPopupMenu popupMenu;
>      private View doorhangerOverlay;
>      private ActionBarPresenter actionBarPresenter;
> -    private ProgressBar mProgressView;

Why don't you want the ProgressBar?

@@ +86,4 @@
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          super.onCreate(savedInstanceState);
>  

It's great to get rid of GeckoApp, but what it does in onCreate() and other lifecycle call backs are still valuable. e.g. DoorHanger[1].
If we really want to land this patch, those efforts to are required.




[1]  http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1673

@@ -232,5 @@
> -            actionBarPresenter.update(tab);
> -        }
> -
> -        updateMenuItemForward();
> -    }

We still need forward feature, right?

@@ -331,5 @@
>  
> -    @Override
> -    protected ActionModePresenter getTextSelectPresenter() {
> -        return new ActionModePresenter() {
> -            private ActionMode mMode;

Why don't we want text selection ?

@@ +392,3 @@
>          if (menuItemControl != null) {
>              Drawable icon = menuItemControl.getIcon();
> +            if (isLoading) {

CustomTab has its own UI design of ProgressBar, which I think we should align the App's UX so should keep it.

::: mobile/android/base/resources/layout/customtabs_activity.xml
@@ -31,4 @@
>          android:layout_below="@id/actionbar"
>          android:layout_height="match_parent"
> -        android:background="@android:color/transparent">
> -

It's great we can finally have a cleaner UI layout :)

Comment 16

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8869029 [details] [diff] [review]
GeckoView-based custom tabs v1.1

Moving review request to the up-to-date version of the patch.

Comment 17

[Dylan Roeh (:droeh) (he/him)]

Thanks for the feedback, Nevin. To respond to a few things:

* Right now, the plan is to drop the progress bar. Using Tabs for progress is not very accurate anyways, and it would require some changes to GeckoView to implement this.
* Doorhangers and text selection are things I plan to add back in after this lands -- I want to get this in place as fast as possible so that any new front end development going forward happens on the GeckoView-based custom tabs.
* We still call updateMenuItemForward(), we just do it from onCanGoForward rather than anything Tabs-related.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8869029 [details] [diff] [review]
GeckoView-based custom tabs v1.1

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

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +53,2 @@
>  
> +public class CustomTabsActivity extends AppCompatActivity 

Extra space at the end

@@ +54,5 @@
> +public class CustomTabsActivity extends AppCompatActivity 
> +                                implements GeckoMenu.Callback,
> +                                           GeckoView.NavigationListener,
> +                                           GeckoView.ProgressListener,
> +                                           GeckoView.ContentListener {

Alphabetical order

@@ +476,5 @@
> +    /* GeckoView.NavigationListener */
> +    @Override
> +    public void onLocationChange(GeckoView view, String url) {
> +        mCurrentUrl = url;
> +        actionBarPresenter.update(mCurrentTitle, mCurrentUrl, mSecurityStatus);

Since it's repeated a bunch of times, I think it'd be good to put this into one update method that doesn't take arguments.

@@ +496,5 @@
> +    public void onPageStart(GeckoView view, String url) {
> +        mCurrentUrl = url;
> +        mCanStop = true;
> +        actionBarPresenter.update(mCurrentTitle, mCurrentUrl, mSecurityStatus);
> +        updateProgress(true);

`updateProgress(/* isLoading */ true)`

@@ +502,5 @@
> +
> +    @Override
> +    public void onPageStop(GeckoView view, boolean success) {
> +        mCanStop = false;
> +        updateProgress(false);

`updateProgress(/* isLoading */ false)`

Comment 19

[Nevin Chen(Not active on Bugzilla)]

(In reply to Dylan Roeh (:droeh) from comment #17)
> * Right now, the plan is to drop the progress bar. Using Tabs for progress
> is not very accurate anyways, and it would require some changes to GeckoView
> to implement this.
> * Doorhangers and text selection are things I plan to add back in after this
> lands -- I want to get this in place as fast as possible so that any new
> front end development going forward happens on the GeckoView-based custom
> tabs.
> * We still call updateMenuItemForward(), we just do it from onCanGoForward
> rather than anything Tabs-related.


Hi Dylan
Since Customtab need to be shipped in 55, we can't land this patch right now or we'll cause regression (or fail to deliver what we have promised)

Btw, since GeckoApp do lots of stuff when it implement GeckoAppSehll.GeckoInterface[1]
I think you also need to port those implementation to here. I can imagine there will be lot of front-end effort.

[1] http://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#1647

Comment 20

[Julian Chu [:walkingice]]

Comment on attachment 8869029 [details] [diff] [review]
GeckoView-based custom tabs v1.1

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

::: mobile/android/base/java/org/mozilla/gecko/customtabs/ActionBarPresenter.java
@@ -101,5 @@
> -        final String title = tab.getTitle();
> -        final String url = tab.getBaseDomain();
> -
> -        // Do not update CustomView immediately. If this method be invoked rapidly several times,
> -        // only apply last one.

Keep this comment could help people understand why we send message to handler, rather than change custom-view directly.

@@ -209,5 @@
> -            mIconView.setImageLevel(mode.ordinal());
> -            mIdentityPopup.setSiteIdentity(identity);
> -
> -            if (mode == SecurityModeUtil.Mode.LOCK_SECURE) {
> -                // Lock-Secure is special case. Keep its original green color.

please keep this comment, since this is requirement from designer

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ -99,4 @@
>  
> -        final String host = getReferrerHost();
> -        recordCustomTabUsage(host);
> -        sendTelemetry();

these two methods were removed, should we send Telemetry?

@@ +111,5 @@
> +        settings.setBoolean(GeckoViewSettings.USE_MULTIPROCESS, false);
> +
> +        if (intent != null) {
> +            final Uri u = intent.getData();
> +            mGeckoView.loadUri(u.toString());

we can use `intent.getDataString()` to get url, also use `TextUtils.isEmpty()` to check whether the intent really has dara uri.

Comment 21

[Julian Chu [:walkingice]]

By the patch I see using GeckoView will make lots logic cleaner, and I like it! I know it is still WIP, and try to list UI differences between GV-based and GeckoApp based CustomTabs Activity.

* So far there is no Doorhanger, ProgressBar, Context Menu, Text-Selection(ActionBar).
* SiteIdentity only have 2 states.(LOCK_SECURE for globe-icon, and invisible).

According to conversation in slack and previous comments, above stuffs will be added back.

I also followed Jan's comment #2, to launch CustomTabs then Fennec, Fennec cannot load page. (And back key doesn't work.). Also, I am not sure can we complete bug 1356188 after migrate to GeckoView based CustomTabs?

Comment 22

[Julian Chu [:walkingice]]

Since we original planned to target 55, NI Joe for discussing landing plan.

Comment 23

[Jan Henning [:JanH]]

(In reply to Julian Chu [:walkingice] from comment #21)
> Also, I am not sure can we
> complete bug 1356188 after migrate to GeckoView based CustomTabs?

It's still possible, just needs a different approach. It's no longer quite as simple (at least comparing the high level description, not sure what problems might crop up in practice if we actually tried this with the GeckoApp-based implementation) as changing the tab's nominal type and then selecting it in BrowserApp because GeckoView doesn't have tabs as such, but it can be done:
As mentioned above, the Gecko-side <browser> needs to be moved to the main "navigator:browser" window used for BrowserApp's tabs (probably using the same approach Desktop uses for moving tabs between windows) and then we need to create both a Gecko and a Java tab object based on that <browser>, which we can then select for display in BrowserApp.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jan Henning [:JanH] from comment #23)
> (In reply to Julian Chu [:walkingice] from comment #21)
> > Also, I am not sure can we
> > complete bug 1356188 after migrate to GeckoView based CustomTabs?
> 
> It's still possible, just needs a different approach. It's no longer quite
> as simple (at least comparing the high level description, not sure what
> problems might crop up in practice if we actually tried this with the
> GeckoApp-based implementation) as changing the tab's nominal type and then
> selecting it in BrowserApp because GeckoView doesn't have tabs as such, but
> it can be done:
> As mentioned above, the Gecko-side <browser> needs to be moved to the main
> "navigator:browser" window used for BrowserApp's tabs (probably using the
> same approach Desktop uses for moving tabs between windows) and then we need
> to create both a Gecko and a Java tab object based on that <browser>, which
> we can then select for display in BrowserApp.

Yup, and this is the same thing desktop does (modulo the java bits) when you move a tab between two windows, so it should be possible to base whatever we want off of that. I don't personally think this "tab migration" feature is a shipping requirement, though it would certainly be nice.

Comment 25

[Dylan Roeh (:droeh) (he/him)]

(In reply to Julian Chu [:walkingice] from comment #21)
> I also followed Jan's comment #2, to launch CustomTabs then Fennec, Fennec
> cannot load page. (And back key doesn't work.). Also, I am not sure can we
> complete bug 1356188 after migrate to GeckoView based CustomTabs?

This will be fixed by bug 1365599, but that needs to land simultaneously with GeckoView-based custom tabs and PWAs.

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Sorry to interrupt but let's keep in mind on the release process.
June5 is the pre-beta sign-off date for features to ride on 55, and currently customtab has been raised "at risk" given 2 unresolved P1 bugs (which I think may not be P1).
My point is, at this moment between 55 nightly ends and 56 starts, shall we have the change pending for it requires longer testing cycle to stabilize? 
I think we can ship the GeckoApp version of custom tab and PWA first.
That's more realistic, isn't it?

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #26)
> I think we can ship the GeckoApp version of custom tab and PWA first.
> That's more realistic, isn't it?

I do not believe we can ship the GeckoApp implementation, as it has intractable bugs regarding how it interacts with Gecko and Fennec.

Comment 28

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView-based custom tabs

Updated version of the patch at Jim's request; carrying over r+'s

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb1cfd59d00
Overhaul CustomTabsActivity.java to use GeckoView rather than GeckoApp. r=jchen, walkingice

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5eb1cfd59d00

Comment 31

[Dylan Roeh (:droeh) (he/him)]

Reopening; we backed this out in bug 1369393 and will re-land in 56.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fa4f730bf6
Overhaul CustomTabsActivity.java to use GeckoView rather than GeckoApp. r=jchen, walkingice

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/00fa4f730bf6

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Dylan can you land part 2 from bug 1367081 as discussed in bug 1367081 comment 9?

Comment 35

[Dylan Roeh (:droeh) (he/him)]

(In reply to (pto) Jim Chen [:jchen] [:darchons] from comment #34)
> Dylan can you land part 2 from bug 1367081 as discussed in bug 1367081
> comment 9?

Done.