Closed Bug 1276696 Opened 3 years ago Closed 3 years ago

Custom Tabs Prototype

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
We will need to dispatch Intent objects to different destinations (Browser, Custom Tab and eventually
progressive web apps).

The logic of TabQueueDispatcher is folded into this new activity.

Review commit: https://reviewboard.mozilla.org/r/56362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56362/
This activity doesn't do much yet and just has a layout including a GeckoView.
Some basic behavior from GeckoApp is working though: Context menus, floating
text selection, snackbars.

Review commit: https://reviewboard.mozilla.org/r/56366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56366/
Currently this service does not implement any of the callbacks. However it is
necessary to be detected as an app supporting custom tabs.

Review commit: https://reviewboard.mozilla.org/r/56368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56368/
@snorp: This is the current state. In theory this could land. It's behind a build flag and disabled by default.

It works as long as the custom tab activity is the only one. As soon as you open multiple custom tabs or switch between the browser and a custom tab we just render white pages everywhere. In addition to that I saw some JS errors (Messaging.jsm complains that there's already a listener for some messages and there was also some SessionStore stuff going on.). From the frontend side there's all the customization missing so far; this is only a full screen browser view right now.
Flags: needinfo?(snorp)
I think we should try to land this behind a build flag, or even a run-time flag on Nightly only if possible. That would allow Google to start working to add support for Firefox to the support library.
Assignee: nobody → s.kaspari
I'm ok with landing this behind the NIGHTLY flag for now. We can address the platform badness after.
Flags: needinfo?(snorp)
We should *not* land this behind the Nightly flag: This will just break Nightly when opening the Browser and a custom tab. So most of the time you will have annoying white screens. That's even for Nightly too broken.

(In reply to :Margaret Leibovic from comment #7)
> I think we should try to land this behind a build flag, or even a run-time
> flag on Nightly only if possible. That would allow Google to start working
> to add support for Firefox to the support library.

What support does Google need to add? The patches work as-is with for example Google search and Google+. I didn't test a variety of apps though.
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> We should *not* land this behind the Nightly flag: This will just break
> Nightly when opening the Browser and a custom tab. So most of the time you
> will have annoying white screens. That's even for Nightly too broken.
> 
> (In reply to :Margaret Leibovic from comment #7)
> > I think we should try to land this behind a build flag, or even a run-time
> > flag on Nightly only if possible. That would allow Google to start working
> > to add support for Firefox to the support library.
> 
> What support does Google need to add? The patches work as-is with for
> example Google search and Google+. I didn't test a variety of apps though.

They mostly wanted to be able to test Firefox's custom tabs capability. If we land this disabled behind a build flag, we could still give them a custom APK to test.
Comment on attachment 8758008 [details]
Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56362/diff/1-2/
Attachment #8758008 - Attachment description: MozReview Request: Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents. r? → Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents.
Attachment #8758009 - Attachment description: MozReview Request: Bug 1276696 - Add custom tabs support library. r? → Bug 1276696 - Add custom tabs support library.
Attachment #8758010 - Attachment description: MozReview Request: Bug 1276696 - Add CustomTabsActivity based on GeckoApp. r? → Bug 1276696 - Add CustomTabsActivity based on GeckoApp.
Attachment #8758011 - Attachment description: MozReview Request: Bug 1276696 - Add dummy service for custom tabs. r? → Bug 1276696 - Add dummy service for custom tabs.
Attachment #8758012 - Attachment description: MozReview Request: Bug 1276696 - Put custom tabs support behind a build flag. r? → Bug 1276696 - Put custom tabs support behind a build flag.
Attachment #8758008 - Flags: review?(michael.l.comella)
Attachment #8758009 - Flags: review?(michael.l.comella)
Attachment #8758010 - Flags: review?(michael.l.comella)
Attachment #8758011 - Flags: review?(michael.l.comella)
Attachment #8758012 - Flags: review?(michael.l.comella)
Comment on attachment 8758009 [details]
Bug 1276696 - Add custom tabs support library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56364/diff/1-2/
Comment on attachment 8758010 [details]
Bug 1276696 - Add CustomTabsActivity based on GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56366/diff/1-2/
Comment on attachment 8758011 [details]
Bug 1276696 - Add dummy service for custom tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56368/diff/1-2/
Comment on attachment 8758012 [details]
Bug 1276696 - Put custom tabs support behind a build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56370/diff/1-2/
Comment on attachment 8758008 [details]
Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents.

https://reviewboard.mozilla.org/r/56362/#review55546

Cool cleanup! You've got some try failures though.
Attachment #8758008 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8758009 [details]
Bug 1276696 - Add custom tabs support library.

https://reviewboard.mozilla.org/r/56364/#review55548

Do we want to put this behind a build flag?

I assume we won't include the code if we compile with this unconditionally on the classpath, but I'm afraid we might include some resources (i.e. of the R.java kind) – I'm not sure how this works.
Attachment #8758009 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8758010 [details]
Bug 1276696 - Add CustomTabsActivity based on GeckoApp.

https://reviewboard.mozilla.org/r/56366/#review55550

::: mobile/android/branding/unofficial/res/layout/customtabs_activity.xml:1
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>

nit: license

::: mobile/android/branding/unofficial/res/layout/customtabs_activity.xml:4
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>
> +<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +    android:id="@+id/root_layout"
> +    android:orientation="vertical" android:layout_width="match_parent"

nit: move second attr to nl

::: mobile/android/branding/unofficial/res/layout/customtabs_activity.xml:16
(Diff revision 2)
> +
> +    <view class="org.mozilla.gecko.GeckoApp$MainLayout"
> +        android:id="@+id/main_layout"
> +        android:layout_width="match_parent"
> +        android:layout_height="match_parent"
> +        android:background="@android:color/transparent">

@null may be slightly more performant, depending on context, but I haven't looked at the code.
Attachment #8758010 - Flags: review?(michael.l.comella) → review+
Attachment #8758011 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8758012 [details]
Bug 1276696 - Put custom tabs support behind a build flag.

https://reviewboard.mozilla.org/r/56370/#review55554

r+, caveat to my already raised build concerns.
Attachment #8758012 - Flags: review?(michael.l.comella) → review+
https://reviewboard.mozilla.org/r/56366/#review55550

> @null may be slightly more performant, depending on context, but I haven't looked at the code.

This is copied straight from gecko_app.xml. I'll file a new bug for this. If there's a performance gain possible then our main layout should benefit from that too. :)
Comment on attachment 8758008 [details]
Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56362/diff/2-3/
Comment on attachment 8758009 [details]
Bug 1276696 - Add custom tabs support library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56364/diff/2-3/
Comment on attachment 8758010 [details]
Bug 1276696 - Add CustomTabsActivity based on GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56366/diff/2-3/
Comment on attachment 8758011 [details]
Bug 1276696 - Add dummy service for custom tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56368/diff/2-3/
Comment on attachment 8758012 [details]
Bug 1276696 - Put custom tabs support behind a build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56370/diff/2-3/
https://reviewboard.mozilla.org/r/56364/#review55548

Mhmh. Currently the build flag disables the feature but it's still getting compiled and therefore we need the library. I expect Proguard to strip most of the code after building if the feature is disabled.

Now that platform is looking into getting this feature ready for prime time it seems to be helpful to always build (and verify) this - but have it disabled until it's ready.

Let's see what the build metrics report after this lands: Does it affect APK size? If so then let's see if we need to take action.
Meh. Build issues. Proguard strips some things and some of the remaining bits don't build. Awesome!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e339f562aa81
Comment on attachment 8758009 [details]
Bug 1276696 - Add custom tabs support library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56364/diff/3-4/
Comment on attachment 8758010 [details]
Bug 1276696 - Add CustomTabsActivity based on GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56366/diff/3-4/
Comment on attachment 8758011 [details]
Bug 1276696 - Add dummy service for custom tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56368/diff/3-4/
Comment on attachment 8758012 [details]
Bug 1276696 - Put custom tabs support behind a build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56370/diff/3-4/
Attachment #8762134 - Attachment is obsolete: true
Comment on attachment 8758008 [details]
Bug 1276696 - Introduce new LauncherActivity that will handle all incoming Intents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56362/diff/3-4/
Comment on attachment 8758009 [details]
Bug 1276696 - Add custom tabs support library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56364/diff/4-5/
Comment on attachment 8758010 [details]
Bug 1276696 - Add CustomTabsActivity based on GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56366/diff/4-5/
Comment on attachment 8758011 [details]
Bug 1276696 - Add dummy service for custom tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56368/diff/4-5/
Comment on attachment 8758012 [details]
Bug 1276696 - Put custom tabs support behind a build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56370/diff/4-5/
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed07ec207c76
Introduce new LauncherActivity that will handle all incoming Intents. r=mcomella
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08f7952eeba
Add custom tabs support library. r=mcomella
https://hg.mozilla.org/integration/mozilla-inbound/rev/a449393cc44f
Add CustomTabsActivity based on GeckoApp. r=mcomella
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa393509b3ce
Add dummy service for custom tabs. r=mcomella
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0aee891a7a
Put custom tabs support behind a build flag. r=mcomella
You need to log in before you can comment on or make changes to this bug.