1360068 - (pwa_geckoview) Use GeckoView in PWA standalone mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

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

This would alleviate a lot of the nasty interactions with Fennec and I think provide a better overall experience.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1360068-1.0-Add-GeckoView-activity-for-standalon.patch

WIP

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1360068-2.0-Use-GeckoView-activity-for-standalon.patch

WIP

Comment 3

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

Attachment: GeckoView-based web apps

This overhauls WebAppActivity to use GeckoView rather than extending from GeckoApp. Depends partly on layout changes made in the patch for bug 1356346, which will be landed simultaneously.

Comment 4

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

Comment on attachment 8871303 [details] [diff] [review]
GeckoView-based web apps

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

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +100,5 @@
>  
>          final View customView = actionBar.getCustomView();
>          mUrlView = (TextView) customView.findViewById(R.id.webapps_action_bar_url);
>  
> +        setGeckoInterface(new BaseGeckoInterface(this));

`BaseGeckoInterface` is gone and you don't need to call `setGeckoInterface` here anymore.

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8871303 [details] [diff] [review]
GeckoView-based web apps

Big apologies, due to my photon commitments I havent been able to review this as thoroughly as I would have liked, however I generally agree with with the move towards geckoview so I do not want to block its progress.

-        EventDispatcher.getInstance().registerUiThreadListener(this,
-                "Website:AppEntered",
-                "Website:AppLeft",
-                null);

Can we not do this in the same manner using geckoview? I find the need for Fennec to reimplement logic we hold inside gecko problematic and would like to see us improve the code sharing and less duplicating bugs.


+    private boolean isInScope(String url) {
+        if (mScope == null) {
+            return true;
+        }
+
.....
+
+        if (scopeSegments.size() > urlSegments.size()) {
+            return false;
+        }

This is wrong, if scope is not defined then the default scope is the path of the start_url, ie if http://example.org/app/index.html is installed then http://example.org/app/hello/path/ is within scope

The scope issue definitely needs fixed, I will give this thorough testing and follow up with bugs + fixes when it lands, thanks for the work

Comment 6

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

Thanks for the review! One thing:

(In reply to Dale Harvey (:daleharvey) from comment #5)
> This is wrong, if scope is not defined then the default scope is the path of
> the start_url, ie if http://example.org/app/index.html is installed then
> http://example.org/app/hello/path/ is within scope

I thought this was the case too, but according to the spec[0], the correct behavior in absence of specified scope is to consider everything in scope. It might be worth considering ignoring the spec here given how odd that behavior is, but at any rate it's an easy enough change to make if we decide to do so. Of course there are probably other cases where scope handling is broken here (relative scopes, if nothing else).

[0] https://www.w3.org/TR/appmanifest/#dfn-navigation-scope -- "If scopeURL is undefined (i.e., it is unbounded because of an error or it was not declared in the manifest), return true."

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9ceef9b280
Overhaul WebAppActivity to use GeckoView rather than GeckoApp. r=jchen,daleharvey

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e9ceef9b280

Comment 9

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

This got backed out and will re-land in 56.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e85d16a5e2
Overhaul WebAppActivity to use GeckoView rather than GeckoApp. r=jchen,daleharvey

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/a4e85d16a5e2