867517 - Gecko-based WebView for Android

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch

Comment 1

[Chris Lord [:cwiiis]]

Comment on attachment 744052 [details] [diff] [review]
WIP patch

Adding patch flag.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch

added touch event support

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: refactor patch

most of this patch is working around places where we access GeckoApp.mAppContext from outside GeckoApp.

I eventually want to make GeckoApp.mAppContext private and create interfaces for anything in GeckoApp that needs to be accessed. The /*FIXME*/'s are places where we will need those interfaces. There are others but this is the minimal change to get a WebView working.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for a gecko based webview

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for an example embed app

we should probably land something like this in embedding/android

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: refactor patch

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: refactor patch

This renames GeckoApp.mAppContext to GeckoApp.sAppContext (since it is static after all) and makes it private.

It also adds a method to GeckoAppShell called getGeckoInterface() which right now returns a GeckoApp. The next step is to create a minimal interface for it to return.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: refactor patch

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 752959 [details] [diff] [review]
refactor patch

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>-        GeckoApp.mAppContext.invalidateOptionsMenu();
>+        GeckoAppShell.getGeckoInterface().invalidateOptionsMenu();

I think we should eventually move this to a different mechanism, but fine for now. BrowserToolbar is a child of BrowserApp and should have a better way to communicate, rather than via a static class interface.

>diff --git a/mobile/android/base/BrowserToolbarLayout.java b/mobile/android/base/BrowserToolbarLayout.java

>-                    ((BrowserApp)GeckoApp.mAppContext).refreshToolbarHeight();
>+                    ((BrowserApp)GeckoAppShell.getContext()).refreshToolbarHeight();

Same

>diff --git a/mobile/android/base/ContextGetter.java b/mobile/android/base/ContextGetter.java

>+package org.mozilla.gecko;
>+import android.content.Context;
>+
>+interface ContextGetter {
>+  Context getContext();
>+}

I'd love to see us kill this, but we can't for now and this at least makes access more controlled.

>diff --git a/mobile/android/base/FormAssistPopup.java b/mobile/android/base/FormAssistPopup.java

>-                (InputMethodManager) GeckoApp.mAppContext.getSystemService(Context.INPUT_METHOD_SERVICE);
>+                (InputMethodManager) GeckoAppShell.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);

If the FormAssistPopup life time is limited to the GeckoApp, we could pass the Context into the constructor.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>     public MenuInflater getMenuInflater() {
>         if (Build.VERSION.SDK_INT >= 11)
>-            return new GeckoMenuInflater(mAppContext);
>+            return new GeckoMenuInflater(sAppContext);

I often wonder why we just don't pass "this" into these functions?

>-                mMenuPanel = new MenuPanel(mAppContext, null);
>+                mMenuPanel = new MenuPanel(sAppContext, null);

Same

>-            GeckoMenu gMenu = new GeckoMenu(mAppContext, null);
>+            GeckoMenu gMenu = new GeckoMenu(sAppContext, null);

Same

>-                        Toast.makeText(GeckoApp.mAppContext, R.string.bookmark_added, Toast.LENGTH_SHORT).show();
>+                        Toast.makeText(GeckoApp.sAppContext, R.string.bookmark_added, Toast.LENGTH_SHORT).show();

Same

>                         final Runnable startCallback = new Runnable() {
>                             @Override
>                             public void run() {
>-                                GeckoApp.mAppContext.runOnUiThread(new Runnable() {
>+                                GeckoApp.sAppContext.runOnUiThread(new Runnable() {

Change: Can we switch to "ThreadUtils.postToUiThread"

>                         final Runnable stopCallback = new Runnable() {
>                             @Override
>                             public void run() {
>-                                GeckoApp.mAppContext.runOnUiThread(new Runnable() {
>+                                GeckoApp.sAppContext.runOnUiThread(new Runnable() {

Same

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>     public static void enableSensor(int aSensortype) {
>         SensorManager sm = (SensorManager)
>-            GeckoApp.mAppContext.getSystemService(Context.SENSOR_SERVICE);
>+            getContext().getSystemService(Context.SENSOR_SERVICE);

Change: Can we get the GeckoInterface in a local variable here? Then return early if it's null. If not reuse the local variable in the code below.

>     public static void disableSensor(int aSensortype) {
>         SensorManager sm = (SensorManager)
>-            GeckoApp.mAppContext.getSystemService(Context.SENSOR_SERVICE);
>+            getContext().getSystemService(Context.SENSOR_SERVICE);

Change: Same

>     static void onXreExit() {

>+        if (getGeckoInterface() != null) {
>+            if (gRestartScheduled) {
>+                getGeckoInterface().doRestart();
>+            } else {
>+                getGeckoInterface().getActivity().finish();
>+            }
>         }
>-
>         Log.d(LOGTAG, "Killing via System.exit()");
>         System.exit(0);

Change: Keep the blank line

>diff --git a/mobile/android/base/GeckoEditable.java b/mobile/android/base/GeckoEditable.java

>-        LayerView v = GeckoApp.mAppContext.getLayerView();
>+        LayerView v = GeckoAppShell.getLayerView();
>         mListener = GeckoInputConnection.create(v, this);

>                 // InputConnectionHandler.onCreateInputConnection
>-                LayerView v = GeckoApp.mAppContext.getLayerView();
>+                LayerView v = GeckoAppShell.getLayerView();

Just thinking out loud. If we want to support multiple GeckoViews in a single app, we'll need to flip this relationship around. The view will need to hold a editable. We use this pattern a lot too.

>diff --git a/mobile/android/base/MenuPanel.java b/mobile/android/base/MenuPanel.java

>         // Restrict the height to 75% of the screen-height. heightPixels changes during rotation.
>-        DisplayMetrics metrics = GeckoApp.mAppContext.getResources().getDisplayMetrics();
>+        DisplayMetrics metrics = GeckoAppShell.getContext().getResources().getDisplayMetrics();

Another example of a Context that should be passed via constructor?

>diff --git a/mobile/android/base/PromptService.java b/mobile/android/base/PromptService.java

>     PromptService() {
>-        mInflater = LayoutInflater.from(GeckoApp.mAppContext);
>+        mInflater = LayoutInflater.from(GeckoAppShell.getContext());

Another example of a Context that should be passed via constructor? This whole file really.

>diff --git a/mobile/android/base/SiteIdentityPopup.java b/mobile/android/base/SiteIdentityPopup.java

>     private SiteIdentityPopup() {
>-        super(GeckoApp.mAppContext);
>+        super(GeckoAppShell.getContext());
> 
>-        mResources = GeckoApp.mAppContext.getResources();
>+        mResources = GeckoAppShell.getContext().getResources();

Another example of a Context that should be passed via constructor?

>diff --git a/mobile/android/base/SuggestClient.java b/mobile/android/base/SuggestClient.java

>-    private static final String USER_AGENT = GeckoApp.mAppContext.getDefaultUAString();
>+    private static final String USER_AGENT = GeckoAppShell.getGeckoInterface().getDefaultUAString();

Better way?

>diff --git a/mobile/android/base/ThumbnailHelper.java b/mobile/android/base/ThumbnailHelper.java

>-        mPendingWidth = new AtomicInteger((int)GeckoApp.mAppContext.getResources().getDimension(R.dimen.tab_thumbnail_width));
>+        mPendingWidth = new AtomicInteger((int)GeckoAppShell.getContext().getResources().getDimension(R.dimen.tab_thumbnail_width));

Another example of a Context that should be passed via constructor?

>-                byte[] thumbnail = BrowserDB.getThumbnailForUrl(GeckoApp.mAppContext.getContentResolver(), url);
>+                byte[] thumbnail = BrowserDB.getThumbnailForUrl(GeckoAppShell.getContext().getContentResolver(), url);

Same

>     private boolean shouldUpdateThumbnail(Tab tab) {
>-        return (Tabs.getInstance().isSelectedTab(tab) || GeckoApp.mAppContext.areTabsShown());
>+        return (Tabs.getInstance().isSelectedTab(tab) || (GeckoAppShell.getGeckoInterface() != null && GeckoAppShell.getGeckoInterface().areTabsShown()));

Ouch. Seems like we should be doing this differently.

>diff --git a/mobile/android/base/awesomebar/AllPagesTab.java b/mobile/android/base/awesomebar/AllPagesTab.java

>-                        mSuggestClient = new SuggestClient(GeckoApp.mAppContext, suggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
>+                        mSuggestClient = new SuggestClient(GeckoAppShell.getContext(), suggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);

I would have thought this code (as part of a View) has easy access to an existing Context

>diff --git a/mobile/android/base/gfx/LayerView.java b/mobile/android/base/gfx/LayerView.java

>-            LayerView layerView = GeckoApp.mAppContext.getLayerView();
>+            LayerView layerView = GeckoAppShell.getLayerView();

I would have thought this code (as a View) has easy access to an existing Context


This is mostly a rename refactor, so we are not changing behavior. There are a few things I think you should change ("Change:"). The rest are good fodder for followups. Looking at the code affected by the patch makes it easy to see some "problem areas" I think we should address in the near future.

Please get a Try run. We'll need to do a lot of testing too, just to be sure nothing got borked.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for a gecko based webview

updated based on the changes to the refactor patch

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

sorry, should have been tagged with leave open

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 753914 [details] [diff] [review]
patch for a gecko based webview

>diff --git a/mobile/android/base/GeckoView.java b/mobile/android/base/GeckoView.java

>+public class GeckoView extends LayerView implements GeckoEventListener, TouchEventInterceptor, ContextGetter {
>+  static GeckoThread sGeckoThread;

How does this affect the goal of multiple GeckoViews in an application? Will we need to address this when we try to add that support?

>+  public GeckoView(Context context, AttributeSet attributes) {

>+    // TODO Auto-generated constructor stub

nit: Remove

>+    //getHolder().addCallback(this);

nit: Needed? If not, remove

>+    Intent intent = null;
>+    GeckoAppShell.setContextGetter(this);
>+    if (context instanceof Activity) {
>+      intent = ((Activity) context).getIntent();

Should we even want to do this in the view? Shouldn't this be handled from outside?

>+      Tabs tabs = Tabs.getInstance();
>+      tabs.attachToActivity((Activity) context);

I think we want to move the basics of Tabs and Tab into GeckoView as child systems, not external singletons.

>+    GeckoProfile profile = GeckoProfile.get(context);
>+    BrowserDB.initialize(profile.getName());

I wonder if this too should be handled outside the view.

>+    GeckoAppShell.registerEventListener("Gecko:Ready", this);

This would need to be re-examined when we support multiple GeckoViews too.

>+    sGeckoThread = new GeckoThread(intent, "http://mit.edu");

OMG

>+  public void handleMessage(String event, JSONObject message) {

>+      Tab tab = tabs.loadUrl("http://mit.edu", Tabs.LOADURL_NEW_TAB);
>+      int id = tab.getId();
>+      tabs.selectTab(id);

>+      Tab selectedTab = Tabs.getInstance().getSelectedTab();

Can't you use the tab you loaded above? "tab"

>+    public boolean onInterceptTouchEvent(View view, MotionEvent event) {
>+    public boolean onTouch(View view, MotionEvent event) {

We should get Kats to take a look at this part.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> >+    public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> >+    public boolean onTouch(View view, MotionEvent event) {
> 
> We should get Kats to take a look at this part.

I think we can lift this part out of GeckoApp and into LayerView (or somewhere else in gfx/) now and then it wouldn't need to be duplicated across GeckoApp and this new GeckoView. Also, WebAppImpl disables overscroll by calling setOverScrollMode(View.OVER_SCROLL_NEVER) which is also probably something you want in a standalone GeckoView. It probably makes more sense to make that the default behaviour and modify BrowserApp to turn on overscroll explicitly. I can provide patches for that.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Move the touch dispatching into LayerView

This moves the code to dispatch touch events from GeckoApp into LayerView so that it doesn't need to be duplicated. I left the overscroll stuff as-is because in the end changing that behaviour didn't actually simplify the code, just moved it around.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 755534 [details] [diff] [review]
Move the touch dispatching into LayerView

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

this is good. Another option is to move the logic into GeckoView and have GeckoApp use the GeckoView. Thoughts?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

I prefer doing this because it does a better job of encapsulating this behaviour with other code that depends on it. Ideally I also want to move the GeckoAppShell.notifyDefaultPrevented function into gfx/ somewhere as well. This code will not be needed once we switch over to the APZC so it doesn't make sense to expose it to GeckoView.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> Comment on attachment 753914 [details] [diff] [review]
> patch for a gecko based webview
> 
> >diff --git a/mobile/android/base/GeckoView.java b/mobile/android/base/GeckoView.java
> 
> >+public class GeckoView extends LayerView implements GeckoEventListener, TouchEventInterceptor, ContextGetter {
> >+  static GeckoThread sGeckoThread;
> 
> How does this affect the goal of multiple GeckoViews in an application? Will
> we need to address this when we try to add that support?
I think we'll always have one GeckoThread so holding it staticly is appropriate. I'll buy an argument that GeckoAppShell should manage it though.

> 
> >+  public GeckoView(Context context, AttributeSet attributes) {
> 
> >+    // TODO Auto-generated constructor stub
> 
> nit: Remove
> 
> >+    //getHolder().addCallback(this);
> 
> nit: Needed? If not, remove
> 
> >+    Intent intent = null;
> >+    GeckoAppShell.setContextGetter(this);
> >+    if (context instanceof Activity) {
> >+      intent = ((Activity) context).getIntent();
> 
> Should we even want to do this in the view? Shouldn't this be handled from
> outside?
Outside where? We need an intent to initialize GeckoThread with an intent. That said, we probably don't want the Activity's intent.

> 
> >+      Tabs tabs = Tabs.getInstance();
> >+      tabs.attachToActivity((Activity) context);
> 
> I think we want to move the basics of Tabs and Tab into GeckoView as child
> systems, not external singletons.

makes sense

> 
> >+    GeckoProfile profile = GeckoProfile.get(context);
> >+    BrowserDB.initialize(profile.getName());
> 
> I wonder if this too should be handled outside the view.
Where would you suggest?

> 
> >+    GeckoAppShell.registerEventListener("Gecko:Ready", this);
> 
> This would need to be re-examined when we support multiple GeckoViews too.
I don't think so. A second GeckoView would be like a second window. We'd only have one gecko thread to spin up. We would need to check checkLaunchState(GeckoThread.LaunchState.GeckoRunning) and trigger the handler code though.
> 
> >+    sGeckoThread = new GeckoThread(intent, "http://mit.edu");
> 
> OMG
good test site
> 
> >+  public void handleMessage(String event, JSONObject message) {
> 
> >+      Tab tab = tabs.loadUrl("http://mit.edu", Tabs.LOADURL_NEW_TAB);
> >+      int id = tab.getId();
> >+      tabs.selectTab(id);
> 
> >+      Tab selectedTab = Tabs.getInstance().getSelectedTab();
> 
> Can't you use the tab you loaded above? "tab"
maybe

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for a gecko based webview

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for a gecko based webview

this patch is on top of Kats's patch

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brad Lassey [:blassey] from comment #20)

> > >+    Intent intent = null;
> > >+    GeckoAppShell.setContextGetter(this);
> > >+    if (context instanceof Activity) {
> > >+      intent = ((Activity) context).getIntent();
> > 
> > Should we even want to do this in the view? Shouldn't this be handled from
> > outside?
> Outside where? We need an intent to initialize GeckoThread with an intent.
> That said, we probably don't want the Activity's intent.

The GeckoViewChrome interface impl? Yeah, that's a future thing.

> > >+    GeckoProfile profile = GeckoProfile.get(context);
> > >+    BrowserDB.initialize(profile.getName());
> > 
> > I wonder if this too should be handled outside the view.
> Where would you suggest?

GeckoViewChrome interface impl maybe

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 755720 [details] [diff] [review]
patch for a gecko based webview

>diff --git a/mobile/android/base/GeckoView.java b/mobile/android/base/GeckoView.java

>+import java.util.Random;
>+ 
>+import android.app.Activity;
>+import android.content.Context;
>+import android.graphics.Canvas;
>+import android.graphics.Paint;
>+import android.os.Bundle;
>+import android.view.SurfaceHolder;
>+import android.view.SurfaceView;
>+import android.util.AttributeSet;
>+import android.util.Log;
>+import android.content.Intent;
>+import org.mozilla.gecko.util.*;
>+import org.mozilla.gecko.db.*;
>+import org.mozilla.gecko.gfx.*;
>+import android.os.Handler;
>+import org.json.JSONObject;
>+import android.view.*;
>+import android.graphics.*;
>+import android.net.Uri;
>+import android.content.res.TypedArray;

Take a look at GeckoApp.java for the "preferred" import style:
* Order: org.mozilla., org.json., android. and java.
* Blank lines bewteen them
* Alpha ordered

>+public class GeckoView extends LayerView implements GeckoEventListener, ContextGetter {

Java files are 4 space indents. This file is mostly 2 space indents

r+ with those changes

Note to people following along: This patch gets something in the tree that we can start to experiment with more. We'd like to get a basic API proposal moving along. We'd also like to get a JAR packaging system setup to allow devs to use it in their own apps.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d6963f47888
https://hg.mozilla.org/mozilla-central/rev/a1a60bc1d6b7

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

For future work, we should create a meta bug, link this bug to it, and make new bugs for any new work (linking those to the meta bug too).

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #26)
> For future work, we should create a meta bug, link this bug to it, and make
> new bugs for any new work (linking those to the meta bug too).

Filed bug 880107