Update MainActivity handling to not use the Homescreen #815

liuche · 2018-04-14T00:02:26Z

This only changes the calling order from MainActivity, and removes HomeFragment - #768 does the UI changes.

liuche · 2018-04-14T00:04:25Z

@mcomella I did not end up making any of the changes you were suggesting as follow-ups to #507. This ended up being cleaner than I expected, but let me know if you think it should integrate the follow-ups you suggested. We may also be able to do more work as part of #766 .

mcomella

This works but I'm concerned about maintainability:

I don't see how this implementation will support a native Pocket video feed
The back button handling is janky: hitting back on the last web page pops the BrowserFragment from the stack and the session observer recreates it in order to show the overlay. I guess we'll fix that in [Home] Consolidate back button behavior and ensure back behavior is correct #766 though?
I assume we're drawing the overlay - and will be with other views like Pocket video - over a blank WebView. This creates overdraw which will affect performance.

I'd like it if we just came up with one solution to support Pocket and the overlay first but we can land this and iterate on it too.

mcomella · 2018-04-17T21:21:28Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

@@ -232,6 +234,12 @@ public boolean shouldOverrideUrlLoading(AmazonWebView view, String url) {
            return true;
        }

+        // Home screen loads a blank page and shows the overlay
+        if (url.equals(URL_HOME)) {


url can be null: URL_HOME.equals(url)

mcomella · 2018-04-17T21:22:34Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

@@ -232,6 +234,12 @@ public boolean shouldOverrideUrlLoading(AmazonWebView view, String url) {
            return true;
        }

+        // Home screen loads a blank page and shows the overlay
+        if (url.equals(URL_HOME)) {


It's fine to do the simple thing for now but considering we expect other pages to exist here (e.g. the Pocket feed), we'll want to do something more generic in the future (maybe a map from url -> Fragment?)

mcomella · 2018-04-17T21:28:21Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

        if (url.equals(URL_HOME)) {
-            // TODO: show browser overlay
+            view.clearView(); // Deprecated in Android WebView, but AmazonWebView doesn't conform


I don't think we should use deprecated methods: Google has a history of breaking them. Thoughts:

Load about:blank

Override the page content to load our own blank page

If firefox: URL, replace WebView with native content (this is necessary for Pocket videos anyway).

mcomella · 2018-04-17T21:29:51Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

@@ -234,9 +234,10 @@ public boolean shouldOverrideUrlLoading(AmazonWebView view, String url) {
            return true;
        }

-        // Home screen loads a blank page and shows the overlay
+        // Home screen should show a blank webview. Since WebView doesn't (and shouldn't) have a


nit: -> "blank page"? Also, why should the home screen be blank? Why can't we just show the home screen for the homescreen? Is it about clearing the webView resources?

Homescreen isn't separate from BrowserFragment, so the webview is always loaded.

mcomella · 2018-04-17T21:31:33Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

@@ -173,6 +173,13 @@ class BrowserFragment : IWebViewLifecycleFragment() {
        return layout
    }

+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {


nit: Can we leave this in onCreateView?

My understanding of the intended use is that the view doing the inflating overrides onCreateView while its super/sub classes can handle the inflation with onViewCreated: using them both in a single class is really confusing.

mcomella · 2018-04-17T21:35:29Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

@@ -173,6 +173,13 @@ class BrowserFragment : IWebViewLifecycleFragment() {
        return layout
    }

+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {


This will only work when BrowserFragment is first created. What about loading Pocket videos?

I assume that Pocket Videos will just be loading a fragment anyway. This breaks history behavior if users expect "pocket to be a web page" and want to go "back" to it, but since back doesn't work on Youtube anyway (#733), I didn't handle that case here.

mcomella · 2018-04-17T21:42:57Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

@@ -173,8 +173,12 @@ class BrowserFragment : IWebViewLifecycleFragment() {
        return layout
    }

+    private fun isHomepage(): Boolean {


nit: val isUrlEqualToHomepage: Boolean get() = session...

BrowserFragment isn't a URL so I don't love adding accessor methods that should act on URLs to BrowserFragment. Ideally, url would be a class that isn't String so we could add a url.isHomepage property. I would prefer not to propagate these style of accessor methods but since our team is small (and we can sort of avoid broken window theory through tough reviews), it's probably fine for now.

Well, I think this a being a little nitpicky - the current alternative is to have url == HOME_URL everywhere, which I think is worse. At least this is an easy single accessor, so if we want to change it, it's easy to find.

In this case, it's nitpicky, yes. But if the trend continues, BrowserFragment becomes a really long file because it acts as a callthrough for functionality defined elsewhere: this is why the Focus BrowserFragment seems more complicated than ours. I'd like to stop this as soon possible.

mcomella · 2018-04-17T21:44:51Z

app/src/main/java/org/mozilla/focus/home/HomeFragment.kt

-    private lateinit var uiLifecycleCancelJob: Job
-
-    override fun onCreateView(inflater: LayoutInflater?, container: ViewGroup?, savedInstanceState: Bundle?): View? {
-        val rootView = inflater!!.inflate(R.layout.fragment_home, container, false)


Do we need to remove this layout?

mcomella · 2018-04-17T21:50:52Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

@@ -193,7 +197,6 @@ class BrowserFragment : IWebViewLifecycleFragment() {
                webView?.goBack()
                TelemetryWrapper.browserBackControllerEvent()
            }
-            browserOverlay.isVisible -> setOverlayVisibleByUser(false)


Pressing back from the home screen doesn't close the app.

Hmmm yeah this is a little more complicated - short of hardcoding "if ishomepage && !hashistory", I'm not sure how else we should do this. Can handle this in #766 .

mcomella · 2018-04-17T21:57:15Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

@@ -234,9 +234,10 @@ public boolean shouldOverrideUrlLoading(AmazonWebView view, String url) {
            return true;
        }

-        // Home screen loads a blank page and shows the overlay
+        // Home screen should show a blank webview. Since WebView doesn't (and shouldn't) have a
+        // handle to browser UI, this url can't be used to load the overlay.
        if (url.equals(URL_HOME)) {


Do we want the URL bar to display "firefox:home"?

mcomella

This is what I was looking for: nice! The two main issues:

shouldOverrideLoading is called twice
I think we should call ScreenController.showBrowserScreenForUrl instead of creating a session directly.

mcomella · 2018-04-21T00:22:15Z

app/src/main/java/org/mozilla/focus/MainActivity.kt

                }
+                ScreenController.showBrowserScreenForCurrentSession(supportFragmentManager, sessionManager)


nit: This is going to get called twice: once now and once when the session is created. I think you should do this instead:

if (sessions.isEmpty()) { ScreenController.showBrowserScreenForUrl(URL_HOME) } else { ScreenController.showBrowserScreenForCurrentSession(...) }

mcomella · 2018-04-21T00:29:53Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

-        const val URL_HOME = "firefox:home"
+        const val APP_URL_PREFIX = "firefox:"
+        const val HOME_SUFFIX = "home"
+        const val APP_URL_HOME = APP_URL_PREFIX + HOME_SUFFIX


nit: "${APP_URL_PREFIX}home" and rm constant above (since it's only used once).

mcomella · 2018-04-21T00:30:29Z

app/src/main/java/org/mozilla/focus/browser/BrowserFragment.kt

@@ -288,6 +287,13 @@ private class BrowserIWebViewCallback(
    override fun onBlockingStateChanged(isBlockingEnabled: Boolean) {}

    override fun onLongPress(hitTarget: IWebView.HitTarget) {}
+    override fun shouldInterceptRequest(url: String) {
+        launch(UI) {


nit: make a comment that this may not be called from the UI thread and that's why we launch

mcomella · 2018-04-21T00:44:50Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FirefoxAmazonWebView.kt

@@ -112,6 +112,7 @@ internal class FirefoxAmazonWebView(
        // called by webview when clicking on a link, and not when opening a new page for the
        // first time using loadUrl().
        if (!client.shouldOverrideUrlLoading(this, url)) {
+            callback?.shouldInterceptRequest(url)


This gets called twice: here and in shouldInterceptRequest. I don't think this call is necessary but if you don't, the screen flashes when loadUrl is initially called: I think this is because shouldInterceptRequest gets called from a background thread so the webView has time to blank (white) before it displays the overlay while calling shouldInterceptRequest from loadUrl is on the UI thread and happens ASAP. One hacky way to fix this is to set the default color of the WebView.

I think we should try to remove the redundant call but I'm not sure the best way to fix the flash-in issue.

Hmm, okay - I'll file a follow-up for this (#841), but take it out for now. We could possibly do this in onViewCreated for BrowserFragment, now that we handle that for other cases too.

mcomella · 2018-04-21T00:45:41Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

        return super.shouldInterceptRequest(view, request);
    }

+    private ByteArrayInputStream makeEmptyDataStream() {


nit: -> getBlankPageStream or getBlankPageInputStream()

mcomella · 2018-04-21T00:46:45Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

+            switch (request) {
+                case APP_URL_HOME:
+                    // Home screen should show a blank webview behind the overlay, but keep the url.
+                    callback.shouldInterceptRequest(request);


nit: -> onShouldInterceptRequest. If you don't expect it to replace the method, you should make it clear this is a callback.

mcomella · 2018-04-21T00:47:00Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

@@ -111,9 +114,24 @@ public void onLoadResource(AmazonWebView view, String url) {
    // WebResourceRequest was added in API21 and there is no equivalent AmazonWebResourceRequest
    @Override
    public AmazonWebResourceResponse shouldInterceptRequest(AmazonWebView view, String request) {
+        if ((request != null && request.startsWith(APP_URL_PREFIX))) {


nit: This if has an extra set of parenthesis.

mcomella · 2018-04-21T01:23:15Z

app/src/amazonWebview/java/org/mozilla/focus/webview/FocusWebViewClient.java

+                    // Home screen should show a blank webview behind the overlay, but keep the url.
+                    callback.shouldInterceptRequest(request);
+                    final ByteArrayInputStream homeDataStream = makeEmptyDataStream();
+                    return new AmazonWebResourceResponse("text/html", "utf-8", homeDataStream);


nit: add a comment explaining why we blank the page.

I assumed that this was sufficient:
// Home screen should show a blank webview behind the overlay, but keep the url.

…n homescreen.

liuche · 2018-04-23T11:57:08Z

TC errors, but we need to do a string export to get rid of localized strings that were removed.

liuche requested a review from mcomella April 14, 2018 00:02

liuche force-pushed the rm-homescreen branch 3 times, most recently from 61faf4e to 4b5ebc3 Compare April 17, 2018 01:55

mcomella approved these changes Apr 17, 2018

View reviewed changes

liuche force-pushed the rm-homescreen branch from 4b5ebc3 to 7f2fcd4 Compare April 20, 2018 23:06

liuche requested a review from mcomella April 20, 2018 23:35

mcomella approved these changes Apr 21, 2018

View reviewed changes

mcomella reviewed Apr 21, 2018

View reviewed changes

liuche added 10 commits April 23, 2018 11:35

Issue mozilla-mobile#762 - Show BrowserFragment on empty session.

4733093

No issue: fix typo Visibile

16bf19f

Issue mozilla-mobile#762 - Show overlay as "home" screen.

b2cc743

Issue mozilla-mobile#762 - Remove references to Home.

9b3d4fe

Issue mozilla-mobile#762 - Remove HomeFragment.

c930a40

No issue: Remove unused getItemAtPosition call.

9abab70

Issue mozilla-mobile#762 - Add handling for app "firefox" urls.

f51ddbb

Issue mozilla-mobile#762 - Remove unused fragment_home.xml

103ca73

Issue mozilla-mobile#762 - Prevent menu button from closing overlay o…

dee12b8

…n homescreen.

Closes mozilla-mobile#762 - Address feedback.

e335195

liuche force-pushed the rm-homescreen branch from 7f2fcd4 to e335195 Compare April 23, 2018 11:42

liuche merged commit 46a5e28 into mozilla-mobile:master Apr 23, 2018

liuche deleted the rm-homescreen branch April 23, 2018 11:57

mcomella mentioned this pull request Apr 23, 2018

Lint busted on master: regression from removing home layouts? #845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MainActivity handling to not use the Homescreen #815

Update MainActivity handling to not use the Homescreen #815

liuche commented Apr 14, 2018

liuche commented Apr 14, 2018

mcomella left a comment •

edited

mcomella Apr 17, 2018

mcomella Apr 17, 2018

mcomella Apr 17, 2018

mcomella Apr 17, 2018

liuche Apr 19, 2018 •

edited

mcomella Apr 17, 2018

mcomella Apr 17, 2018

liuche Apr 19, 2018

mcomella Apr 17, 2018

liuche Apr 20, 2018 •

edited

mcomella Apr 21, 2018

mcomella Apr 17, 2018

mcomella Apr 17, 2018

liuche Apr 20, 2018

mcomella Apr 17, 2018

mcomella left a comment

mcomella Apr 21, 2018

mcomella Apr 21, 2018

mcomella Apr 21, 2018

mcomella Apr 21, 2018

liuche Apr 23, 2018 •

edited

mcomella Apr 21, 2018

mcomella Apr 21, 2018

mcomella Apr 21, 2018

mcomella Apr 21, 2018

liuche Apr 23, 2018 •

edited

liuche commented Apr 23, 2018

		}
		ScreenController.showBrowserScreenForCurrentSession(supportFragmentManager, sessionManager)

Update MainActivity handling to not use the Homescreen #815

Update MainActivity handling to not use the Homescreen #815

Conversation

liuche commented Apr 14, 2018

liuche commented Apr 14, 2018

mcomella left a comment • edited

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

liuche Apr 19, 2018 • edited

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

liuche Apr 20, 2018 • edited

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

mcomella left a 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

liuche Apr 23, 2018 • edited

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

liuche Apr 23, 2018 • edited

Choose a reason for hiding this comment

liuche commented Apr 23, 2018

mcomella left a comment •

edited

liuche Apr 19, 2018 •

edited

liuche Apr 20, 2018 •

edited

liuche Apr 23, 2018 •

edited

liuche Apr 23, 2018 •

edited