Bug 1356346 (customtabs_geckoview)

Overhaul CustomTabsActivity to use GeckoView rather than extending GeckoApp

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: droeh, Assigned: droeh)

Tracking

(Depends on 3 bugs)

53 Branch
Firefox 56
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Assignee

Description

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

Comment 1

2 years ago
Posted patch GeckoView-based custom tabs (obsolete) — Splinter Review
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.
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...
Assignee

Comment 3

2 years ago
Posted patch GeckoView-based custom tabs (obsolete) — Splinter Review
Rebased with minor updates, but no fix for black tiles or dead tabs in Fennec yet.
Attachment #8858100 - Attachment is obsolete: true
(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...
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Assignee

Comment 6

2 years ago
(In reply to Jan Henning [:JanH] from comment #5)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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!
Assignee

Comment 7

2 years ago
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
Assignee

Comment 8

2 years ago
(In reply to Dylan Roeh (:droeh) from comment #6)
> (In reply to Jan Henning [:JanH] from comment #5)
> > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Assignee

Comment 9

2 years ago
(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) (jwillcox@mozilla.com) 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.
Assignee

Comment 10

2 years ago
Posted patch GeckoView-based custom tabs (obsolete) — Splinter Review
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.
Attachment #8859580 - Attachment is obsolete: true
Assignee

Comment 11

2 years ago
Posted patch GeckoView-based custom tabs (obsolete) — Splinter Review
Rebased. Still two show-stoppers at this point: white screen (with multiple geckoviews), and dead tabs in Fennec if launched after a custom tab.
Attachment #8861124 - Attachment is obsolete: true
Alias: customtabs_geckoview
Assignee

Comment 12

2 years ago
Posted patch GeckoView-based custom tabs (obsolete) — Splinter Review
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.
Attachment #8864232 - Attachment is obsolete: true
Attachment #8868583 - Flags: review?(nchen)
Attachment #8868583 - Flags: review?(cnevinchen)
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"
Attachment #8868583 - Flags: review?(nchen) → feedback+
Assignee

Comment 14

2 years ago
Updated to address Jim's suggestions.
Attachment #8868583 - Attachment is obsolete: true
Attachment #8868583 - Flags: review?(cnevinchen)
Attachment #8869029 - Flags: review?(nchen)
Attachment #8869029 - Flags: review?(cnevinchen)
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 :)
Attachment #8868583 - Flags: review?(walkingice0204)
Assignee

Comment 16

2 years ago
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.
Attachment #8869029 - Flags: review?(cnevinchen) → review?(walkingice0204)
Assignee

Updated

2 years ago
Attachment #8868583 - Flags: review?(walkingice0204)
Assignee

Comment 17

2 years ago
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 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)`
Attachment #8869029 - Flags: review?(nchen) → review+
(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 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.
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?
Since we original planned to target 55, NI Joe for discussing landing plan.
Flags: needinfo?(jcheng)
(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.
(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.
Assignee

Comment 25

2 years ago
(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.
No longer blocks: 1365868
Depends on: 1365868
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?
Attachment #8869029 - Flags: review?(walkingice0204) → review+
(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.
Assignee

Comment 28

2 years ago
Updated version of the patch at Jim's request; carrying over r+'s
Attachment #8869029 - Attachment is obsolete: true
Attachment #8873117 - Flags: review+

Comment 29

2 years ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb1cfd59d00
Overhaul CustomTabsActivity.java to use GeckoView rather than GeckoApp. r=jchen, walkingice

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5eb1cfd59d00
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: needinfo?(jcheng)
Assignee

Comment 31

2 years ago
Reopening; we backed this out in bug 1369393 and will re-land in 56.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 32

2 years ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fa4f730bf6
Overhaul CustomTabsActivity.java to use GeckoView rather than GeckoApp. r=jchen, walkingice
https://hg.mozilla.org/mozilla-central/rev/00fa4f730bf6
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Dylan can you land part 2 from bug 1367081 as discussed in bug 1367081 comment 9?
Flags: needinfo?(droeh)
Assignee

Comment 35

2 years ago
(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.
Flags: needinfo?(droeh)
Target Milestone: Firefox 55 → Firefox 56
Depends on: 1356545
Depends on: 1388058
Depends on: 1366770
Depends on: 1388724
Depends on: 1369050
Depends on: 1388734
Depends on: 1355735

Updated

2 years ago
Depends on: 1394404
Depends on: 1395557
Depends on: 1395570
Depends on: 1395573
Depends on: 1395577
Depends on: 1395582
Depends on: 1397798
Depends on: 1398508

Updated

2 years ago
Depends on: 1375141
You need to log in before you can comment on or make changes to this bug.