Comment on attachment 744052 [details] [diff] [review] WIP patch Adding patch flag.

added touch event support

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.

we should probably land something like this in embedding/android

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 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.

updated based on the changes to the refactor patch

sorry, should have been tagged with leave open

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.

(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.

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 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?

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.

(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

this patch is on top of Kats's patch

(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 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.

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

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).

(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