Closed Bug 1360068 (pwa_geckoview) Opened 3 years ago Closed 2 years ago

Use GeckoView in PWA standalone mode

Categories

(Firefox for Android :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: snorp, Assigned: droeh)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

This would alleviate a lot of the nasty interactions with Fennec and I think provide a better overall experience.
Depends on: 1364471
Blocks: 1365868
Blocks: 1366098
Alias: pwa_geckoview
No longer blocks: 1365868
Depends on: 1365868
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.
Assignee: nobody → droeh
Attachment #8871303 - Flags: review?(nchen)
Attachment #8871303 - Flags: review?(dale)
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.
Attachment #8871303 - Flags: review?(nchen) → review+
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
Attachment #8871303 - Flags: review?(dale) → review+
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."
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9ceef9b280
Overhaul WebAppActivity to use GeckoView rather than GeckoApp. r=jchen,daleharvey
https://hg.mozilla.org/mozilla-central/rev/3e9ceef9b280
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This got backed out and will re-land in 56.
Depends on: 1371796
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e85d16a5e2
Overhaul WebAppActivity to use GeckoView rather than GeckoApp. r=jchen,daleharvey
https://hg.mozilla.org/mozilla-central/rev/a4e85d16a5e2
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 55 → Firefox 56
Depends on: 1398067
Depends on: 1414084
You need to log in before you can comment on or make changes to this bug.