Issue #282: Process CustomTab intent #307

csadilek · 2018-06-19T06:28:03Z

This processes CustomTab intents. We currently only use the toolbarColor config. I've filed #306 for the remaining properties.

This only works with WebView right now until the issues mentioned in #158 are all resolved.

codecov · 2018-06-19T06:47:00Z

Codecov Report

Merging #307 into master will increase coverage by 0.07%.
The diff coverage is 90.32%.

@@             Coverage Diff              @@
##             master     #307      +/-   ##
============================================
+ Coverage     76.88%   76.95%   +0.07%     
- Complexity      686      693       +7     
============================================
  Files           100      101       +1     
  Lines          2570     2591      +21     
  Branches        374      377       +3     
============================================
+ Hits           1976     1994      +18     
- Misses          386      388       +2     
- Partials        208      209       +1

Impacted Files	Coverage Δ	Complexity Δ
...java/mozilla/components/concept/toolbar/Toolbar.kt	`41.93% <0%> (-0.69%)`	`0 <0> (ø)`
...s/browser/session/storage/DefaultSessionStorage.kt	`89.55% <100%> (+1.31%)`	`19 <0> (+1)`	⬆️
...zilla/components/browser/session/SessionManager.kt	`89.55% <100%> (-1.36%)`	`32 <4> (+3)`
...java/mozilla/components/browser/session/Session.kt	`75.67% <100%> (+0.67%)`	`11 <2> (+2)`	⬆️
...zilla/components/feature/session/SessionFeature.kt	`72.72% <100%> (ø)`	`6 <1> (ø)`	⬇️
...illa/components/feature/session/SessionUseCases.kt	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
...omponents/browser/session/tab/CustomTabsService.kt	`100% <100%> (ø)`	`2 <2> (?)`
.../components/feature/session/EngineViewPresenter.kt	`71.42% <50%> (-11.91%)`	`3 <1> (ø)`
...mponents/feature/session/SessionIntentProcessor.kt	`90.47% <92.3%> (-1.84%)`	`6 <0> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51a6558...7246611. Read the comment docs.

pocmo · 2018-06-19T15:30:10Z

...er/session/src/main/java/mozilla/components/browser/session/storage/DefaultSessionStorage.kt

@@ -110,12 +110,12 @@ class DefaultSessionStorage(
            json.put(VERSION_KEY, VERSION)
            json.put(SELECTED_SESSION_KEY, sessionManager.selectedSession.id)

-            sessionManager.sessions.forEach({ session ->
+            sessionManager.sessions.filter { it.customTabConfig == null }.forEach { session ->


I wonder if this should be the default for sessions (return the filtered list). A lot of things are going to need the filtered list (e.g. tabs tray) and so far nothing in Focus required all sessions. We only look up specific sessions by ID for custom tabs.

OK, I will update.

pocmo · 2018-06-19T15:32:31Z

...ts/browser/session/src/main/java/mozilla/components/browser/session/tab/CustomTabsService.kt

+import android.support.customtabs.ICustomTabsCallback
+import android.support.customtabs.ICustomTabsService
+
+class CustomTabsService : Service() {


I wonder if we should really put this into session. Some of the implementations here will require access to more things. How about moving this to feature-session? Or should we create something like feature-customtabs where we can add more things later on?

Hm, not sure, let's file a follow-up and refactor this later?

Okay, let's file a follow-up :) 👍

pocmo · 2018-06-19T15:34:24Z

components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/BrowserToolbar.kt

+        displayToolbar.setBackgroundColor(color)
+        editToolbar.setBackgroundColor(color)
+    }
+


This shouldn't be needed as BrowserToolbar is already a View. I had the same problem with EngineView which is just an interface but sometimes I want to use it as a View. I added this helper to it:

fun asView(): View = this as View

How about we add something like that to Toolbar? With that we do not need to mirror all View methods that we may want to access.

OK, looks good to me.

pocmo · 2018-06-19T15:35:46Z

...ents/feature/session/src/main/java/mozilla/components/feature/session/EngineViewPresenter.kt

+    override fun onSessionAdded(session: Session) {
+        if (session.customTabConfig != null) {
+            renderSession(session)
+        }


Maybe we should add Session.isCustomTab(): Boolean :)

pocmo · 2018-06-19T15:36:19Z

components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt

-        fun invoke(url: String) {
-            sessionManager.getOrCreateEngineSession().loadUrl(url)
+        fun invoke(url: String, session: Session = sessionManager.selectedSession) {
+            sessionManager.getOrCreateEngineSession(session).loadUrl(url)
        }


Ah, nice! Now this has the default parameter like the other methods here. :)

pocmo · 2018-06-19T15:39:52Z

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

+        session.customTabConfig?.toolbarColor?.let {
+            toolbar.setBackgroundColor(it)
+        }
+        // TODO Apply remaining configurations: https://github.com/mozilla-mobile/android-components/issues/306


I wonder if this should be done in the callback: Afaik this only works if the callback is set before the custom tab config is set, right? Otherwise the callback won't get executed, or? Especially as the config will never change I wonder if we should just do this on session selection / initializeView?

pocmo · 2018-06-19T15:41:02Z

samples/browser/src/main/java/org/mozilla/samples/browser/MainActivity.kt

    }

    override fun onResume() {
        super.onResume()

        sessionFeature?.start()
        toolbarFeature?.start()
+
+        components.sessionIntentProcessor.process(intent)


onResume() seems to be wrong. This would be called whenever the app gets to the foreground.

onCreate() (and maybe onNewIntent()?)

@pocmo the problem I am having here is that I need the features to start before the intent is being processed. I am not able to make this work with onCreate or onNewIntent, since we have to start in onResume. Any ideas?

I assume because you need onCustomTabConfigChanged() to be called in ToolbarPresenter? As mentioned above: I think you want to update the toolbar from initializeView() and not onCustomTabConfigChanged().

@pocmo no (I actually missed that comment before). The reason is that we want the ToolbarPresenter to be started, so it's an active Session observer.

We need to know (at least currently) when a CustomTab session was added or another session selected when we're processing the intent.

Yeah, we need to fix our presenter. The pattern for all those UI components should be: (1) Display current state, (2) update on changes. Looks like we never implemented (1) here. :)

OK, this is addressed now. I took a step back and I don't think we should start/stop features in onPause/onResume but onStart/onStop instead. This is what we do in Fenix. Then this whole type of problem goes away. I think It's reasonable to assume that all features are started when we're processing intents. So, this is cleaner, imo, and actually works :).

This may work for now, but it will fall apart for the same reasons again: I think the assumption that all features exist, are started and subscribed at all times is wrong. It's very well possible that we are going to process intents while the toolbar is not even on the screen and we are somewhere in settings. Imagine other components like the tabs tray. There's no need to create, start and subscribe the feature if it's not visible. I think all components will need to display the current state on start and update if there are changes during their lifetime.

Apart from that this we would still process the same Intent multiple times in onStart(). Try this:

Set sample browser as default browser

Click on a link in another app to open the browser

Press "home" button of device

Switch back to app

-> We process the Intent every time the app is shown again. Right now this means we reload the page every time. Once we start opening tabs we will open new tabs every time.

The only way to avoid that is to process the Intent in onCreate() and only if savedInstanceState is null - meaning this activity wasn't just re-created.

We can also land this and continue in a follow-up?

OK, I will try something. My view on the problem is that the presenters observe and act on changes only after they started. So, they might miss events. However, it should be possible to act on events that are still relevant (that we know occurred and haven't been acted on) when the presenters are started.

pocmo · 2018-06-20T08:56:20Z

components/browser/session/src/main/java/mozilla/components/browser/session/SessionManager.kt

     */
    val sessions: List<Session>
+        get() = synchronized(values) { values.filter { it.customTabConfig == null }.toList() }


I think toList() may not be necessary here as filter() should already return a new list? Not sure about the type though.

pocmo · 2018-06-20T08:58:04Z

components/browser/session/src/main/java/mozilla/components/browser/session/SessionManager.kt

+    /**
+     * Returns a list of all active sessions.
+     */
+    val allSessions: List<Session>


nit: For the SessionManager I would also accept just all? 🙃

pocmo · 2018-06-20T16:03:22Z

...ents/feature/session/src/main/java/mozilla/components/feature/session/EngineViewPresenter.kt

@@ -20,8 +20,7 @@ class EngineViewPresenter(
     * Start presenter and display data in view.
     */
    fun start() {
-        renderSession(sessionManager.selectedSession)
-
+        sessionManager.all.forEach { handleNewSession(it) }


I do not understand why we loop over all sessions here and then later render it if it's a custom tab or selected. Isn't this the same as the previous code that just rendered the selected session? With that we may render multiple things if there are multiple sessions (selected + multiple custom tabs) and just settle on the last one.

I think the underlying problem you are trying to solve is that we never display a custom tab right now because it is never the selected one(*) [at least if there's already one session]?

Essentially Custom Tabs and the browser will live in different activities. So I guess what we will need to do is to tell renderView to either render "whatever is selected" or a specific (custom tab) session. That's what we essentially do in Focus with BrowserFragment.

All this makes me think whether we want to delay displaying custom tabs in the sample browser until we have made the separation at the activity level.

(*) We actually should make sure that a custom tab doesn't get selected because this doesn't make any sense. But that's something for another issue.

pocmo · 2018-06-20T16:14:57Z

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

@@ -24,6 +24,7 @@ class ToolbarPresenter(
    fun start() {
        session = sessionManager.selectedSession

+        sessionManager.all.forEach { onSessionAdded(it) }


Here I also do not understand why we loop over all sessions and then display the state based on the last custom tab we saw (in onSessionAdded()) what may or may not be what we actually display.

This probably has the same problem as EngineViewPresenter: I think we actually want to display the currently selected session (main browser activity) or a specific session (custom tab activities)

csadilek · 2018-06-21T03:26:21Z

@pocmo OK, this is updated now with the discussed changes:

Created a separate activity to handle intents, reusing our SessionIntentProcessor and sharing code between the two activity types
Made sure there's only one instance of our components

This works fine now, but it uncovered one problem (which is fixed here as well):

It is now possible for the browser to be closed and re-opened using the same JVM (a CustomTab might still be around keeping the process alive). When the app is re-opened, it does not need to restore the sessions from storage (we have a running process with the sessions in memory, now that we have a single SessionManager). However, we have a new instance of our Presenter, which wants to render the selected session but nothing called loadUrl to make sure it's properly rendered. loadUrl is only called when the session is added/linked (e.g. when restored from storage or newly created). I fixed this by moving the loadUrl call from sessionManager.link to the Presenter. I never quite liked that this happened as a side-effect of link anyway. So, this seems cleaner to me and works well.

Curious what you think.

pocmo

This is nice! 👍

pocmo · 2018-06-21T12:51:52Z

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt


-        this.session = session
+        activeSession = session
        session.register(this)



Do we need to check here whether we were started with a specific session and then ignore selection changes? Most of the time this won't be a problem as the selection won't change while the custom tab activity is in the foreground - but you may be able to have both activities visible using Android's split screen feature.

Or should we just not register an observer on the session manager in onStart() if we got a session? We only listen for a selection change right now.

pocmo · 2018-06-21T12:56:12Z

samples/browser/src/main/java/org/mozilla/samples/browser/IntentReceiverActivity.kt

+    override fun onCreate(savedInstanceState: Bundle?) {
+        components.sessionIntentProcessor.process(intent)
+        super.onCreate(savedInstanceState)
+    }


Oh, interesting... the IntentReceiverActivity is a BrowserActivity instead of launching it? Won't this duplicate the browser if there are multiple non-custom-tab VIEW intents or if we launch the app normally and then send a normal intent? Anyhow, at this point I would prefer merging this and investigating this later.

pocmo · 2018-06-21T13:00:06Z

It is now possible for the browser to be closed and re-opened using the same JVM (a CustomTab might still be around keeping the process alive). When the app is re-opened, it does not need to restore the sessions from storage (we have a running process with the sessions in memory, now that we have a single SessionManager). However, we have a new instance of our Presenter, which wants to render the selected session but nothing called loadUrl to make sure it's properly rendered. loadUrl is only called when the session is added/linked (e.g. when restored from storage or newly created). I fixed this by moving the loadUrl call from sessionManager.link to the Presenter. I never quite liked that this happened as a side-effect of link anyway. So, this seems cleaner to me and works well.

That's interesting. Can we file an issue for that too? Now we may create an engine session but we are not going to load the page in it unless we render it. I wonder if this may have other side effects later on.

Now that you mention that I also wonder if there are problems whenever we end up with no session (because there may be only a custom tab one when we launch the browser the first time) because a bunch of things always expect a selected session to exist. Anyhow, let's get this in first!

pocmo · 2018-06-21T17:11:49Z

samples/browser/src/main/java/org/mozilla/samples/browser/CustomTabActivity.kt

+
+    override fun onCreate(savedInstanceState: Bundle?) {
+        super.onCreate(savedInstanceState)
+    }


nit: This is redundant and you can just leave the class empty :)

csadilek requested a review from pocmo June 19, 2018 06:28

csadilek force-pushed the issue-282 branch 2 times, most recently from 8c75a33 to afbb929 Compare June 19, 2018 06:29

pocmo reviewed Jun 19, 2018

View reviewed changes

csadilek force-pushed the issue-282 branch 8 times, most recently from e8c17d2 to 1cd420b Compare June 19, 2018 19:46

csadilek changed the title ~~Issue 282: Process CustomTab intent~~ Issue #282: Process CustomTab intent Jun 19, 2018

pocmo reviewed Jun 20, 2018

View reviewed changes

csadilek force-pushed the issue-282 branch 3 times, most recently from f3f763f to 02b72a5 Compare June 20, 2018 15:42

pocmo reviewed Jun 20, 2018

View reviewed changes

csadilek force-pushed the issue-282 branch 3 times, most recently from 3ac3eea to 18329fd Compare June 21, 2018 03:09

csadilek force-pushed the issue-282 branch 2 times, most recently from 002993e to 2b80bdb Compare June 21, 2018 03:37

pocmo approved these changes Jun 21, 2018

View reviewed changes

csadilek force-pushed the issue-282 branch 2 times, most recently from a45eb07 to 913ac8d Compare June 21, 2018 17:01

pocmo approved these changes Jun 21, 2018

View reviewed changes

csadilek force-pushed the issue-282 branch 2 times, most recently from ae0b4f7 to e39db2d Compare June 21, 2018 17:47

Issue mozilla-mobile#282: Process CustomTab intents

7246611

csadilek force-pushed the issue-282 branch from e39db2d to 7246611 Compare June 21, 2018 17:52

csadilek merged commit 343decf into mozilla-mobile:master Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #282: Process CustomTab intent #307

Issue #282: Process CustomTab intent #307

csadilek commented Jun 19, 2018

codecov bot commented Jun 19, 2018 •

edited

Loading

pocmo Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 19, 2018

pocmo Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 19, 2018

pocmo Jun 19, 2018

pocmo Jun 19, 2018

pocmo Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 19, 2018

csadilek Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 19, 2018

csadilek Jun 19, 2018

pocmo Jun 20, 2018

pocmo Jun 20, 2018 •

edited

Loading

csadilek Jun 20, 2018

pocmo Jun 20, 2018

pocmo Jun 20, 2018

pocmo Jun 20, 2018

pocmo Jun 20, 2018

csadilek commented Jun 21, 2018 •

edited

Loading

pocmo left a comment

pocmo Jun 21, 2018

pocmo Jun 21, 2018

pocmo commented Jun 21, 2018

pocmo Jun 21, 2018

Issue #282: Process CustomTab intent #307

Issue #282: Process CustomTab intent #307

Conversation

csadilek commented Jun 19, 2018

codecov bot commented Jun 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadilek commented Jun 21, 2018 • edited Loading

pocmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo commented Jun 21, 2018

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2018 •

edited

Loading

pocmo Jun 20, 2018 •

edited

Loading

csadilek commented Jun 21, 2018 •

edited

Loading