753102 - Refactor GeckoApp into a better base class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We want to remove any UI bits out of GeckoApp and create BrowserApp for gecko-based chrome activities and WebApp for gecko-based chromeless activities.

We plan to use the WebApp activity as a basis for Reader Mode and Webapps support.

Philipp and Rob started work along these lines in Github and we should look at that code as a starting point.

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to Mark Finkle (:mfinkle) from comment #0)
> We want to remove any UI bits out of GeckoApp and create BrowserApp for
> gecko-based chrome activities and WebApp for gecko-based chromeless
> activities.

I think having a WebApp activity base class is great for Webapps, but I would recommend (and it may even be necessary for that) to factor Gecko management entirely out of the activity classes (currently GeckoApp). This will allow usecases like GeckoWebView. Rob and I went for a "Gecko" singleton, but other abstractions might be more useful... There's also the behemoth that's GeckoAppShell which is currently tangled into everything...

Note: This is a lot of work, although IMHO very necessary just for reducing engineering debt. E.g. code in gfx and other places depend entirely on browser-only code (e.g. tabs), so better separation of concerns will lead to cleaner and more reusable pieces.

I'll attach a patch of our work.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: GeckoWebView PoC

Comment 3

[Richard Newman [:rnewman]]

This is relevant to my interests. Please subscribe me to your refactoring newsletter.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This is a bit more.... reserved. Split off from the patch in bug 740586 (applies on top of it).

Really rough sketch that mostly works. Basically does what is necessary to move mBrowserToolbar and mAboutHomeContent out of GeckoApp. Needs mucho mucho MUCHO clean up and testing and other stuff, but wanted to save this off somewhere. Plus, kinda hoping mfinkle will have a chance to look it over and provide any thoughts he has.

Comment 5

[Richard Newman [:rnewman]]

With my reviewer/sheriff hat on: can I suggest you spend the time (in future, at least) to fork refactoring work into dependent bugs, building a stack bottom-up, rather than doing things the other way around and having your refactoring apply on top of a feature patch?

If Bug 740586 has to be backed out, this will have to go, too. And it's much harder to review a refactoring that layers on top of unreviewed/partly tested/WIP code.

</OH THE HUGE MANATEE>

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 628133 [details] [diff] [review]
WIP Patch

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

Driiiiiiiiiive byyyyyyyyyyy.

::: mobile/android/base/App.java.in
@@ +14,5 @@
>  import android.view.MenuItem;
>  import android.util.Log;
>  import android.os.Bundle;
>  
> +public class App extends GeckoAppWithUrlbar {

Can we do better, name-wise? GeckoURLBrowser?

I would encourage you to broadly reconsider the naming of these core classes -- GeckoApp, GeckoAppShell, etc.

The name should reflect what they do. If it's a browser activity, call it BrowserActivity -- if we decide to hide the URL bar for browsing, or show the URL bar differently for web apps, this'll look really silly.

If it's a web app container, call it WebAppContainer.

GeckoAppWithUrlbar tells you very little about what it's *for*, and leaks some information about how it does it ("it shows a URL bar").

::: mobile/android/base/AwesomeBar.java
@@ +110,5 @@
>                            mText.getPaddingBottom() };
>  
>          GeckoStateListDrawable states = new GeckoStateListDrawable();
> +        // This should not depend on having a browserToolbar
> +        // states.initializeFilter(GeckoApp.mBrowserToolbar.getHighlightColor());

Fixing this is eminently suitable for a second part or bug…

::: mobile/android/base/BrowserToolbar.java
@@ +81,5 @@
> +            case CLOSED:
> +                updateTabCountAndAnimate(Tabs.getInstance().getCount());
> +                break;
> +            case START:
> +                if (Tabs.getInstance().isSelectedTab(tab)) {

This phrase occurs all the time.

Have you thought about adding Tab.isSelected()? (Presumably each tab can belong to only a single Tabs, and has a reference to it…?)

Failing that: if Tabs is a singleton, then there's nothing wrong with adding static methods for simplicity:

  Tabs.isSelectedTab(tab);
  Tabs.getCount();

@@ +108,5 @@
> +                break;
> +            case SELECTED:
> +                refresh();
> +                String url = tab.getURL();
> +                setShadowVisibility((url == null) || !url.startsWith("about:"));

Seems like this logic should be bundled somewhere:

  setShadowForURL(url)?

  setShadowVisibility(tab.shouldShadow())?

See the sibling:

  tab.canDoForward();

::: mobile/android/base/DoorHangerPopup.java
@@ +121,5 @@
>              dh.show();
>          }
>  
> +        if (v == null)
> +            showAtLocation(GeckoApp.mAppContext.mGeckoLayout, Gravity.TOP, 0, 0);

I would encourage you to prune use of the global GeckoApp as you go.

Every time you see a `.mFoo`, you're encouraging a tight coupling.

Why not have this method take a GeckoLayout argument, or have this class retain a reference to a GeckoLayout instance?

@@ +138,1 @@
>          Log.i(LOGTAG, "Showing the DoorHangerPopup");

info logging is overkill here.

::: mobile/android/base/GeckoApp.java
@@ +469,5 @@
>          return true;
>      }
>  
> +    void toggleChrome(Boolean aShow) { }
> +    void focusChrome() { }

Style: separate lines for these. And why Boolean rather than boolean?

@@ +613,5 @@
>      }
>  
> +    void postLocationChange(final Tab tab, Boolean sameDocument) { }
> +    void updatePopups(final Tab tab) {
> +      mDoorHangerPopup.updatePopup();

Indentation.

@@ +1131,5 @@
>  
>      void handleDocumentStop(int tabId, boolean success, final String uri) {
>          final Tab tab = Tabs.getInstance().getTab(tabId);
>          if (tab == null)
>              return;

What does this condition indicate? It doesn't seem cheerful…

@@ +1133,5 @@
>          final Tab tab = Tabs.getInstance().getTab(tabId);
>          if (tab == null)
>              return;
>  
>          tab.setState(success ? Tab.STATE_SUCCESS : Tab.STATE_ERROR);

tab.setSuccess(success);

Let Tab own its own internal state.

@@ +1200,2 @@
>      void handleLinkAdded(final int tabId, String rel, final String href) {
> +        if (rel.indexOf("[icon]") == -1)

What is this, JavaScript? :D

if (!rel.contains("[icon]"))

@@ +1200,3 @@
>      void handleLinkAdded(final int tabId, String rel, final String href) {
> +        if (rel.indexOf("[icon]") == -1)
> +          return;

Indentation.

(2 spaces is my style, but Fennec opted for the side of evil.)

@@ +1515,5 @@
> +            Log.i(LOGTAG, "xxx Window:Show");
> +            if (getIsWebapp())
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Show", "webapps.xul"));
> +            else
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Show", "browser.xul"));

protected String getWindowXUL() {
  return (getIsWebapp() ? "webapps.xul" : "browser.xul");
}

final String xul = getWindowXUL();
GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Show", xul));

::: mobile/android/base/GeckoAppWithUrlbar.java
@@ +63,5 @@
> +
> +abstract public class GeckoAppWithUrlbar extends GeckoApp {
> +    private static final String LOGTAG = "GeckoAppWithUrlbar";
> +
> +    public static BrowserToolbar mBrowserToolbar;

I have qualms about the number of statics in use here. Why is mAboutHomeContent non-static, but the toolbar is?

@@ +68,5 @@
> +    private AboutHomeContent mAboutHomeContent;
> +
> +    @Override
> +    public void onTabChanged(Tab tab, Tabs.TabEvents msg) {
> +        boolean callSuper = true;

This is evil. Don't do this.

Call super, or don't… but let the superclass decide what to do. When you're using this flag you're relying on *this* class knowing the internal implementation choices of the superclass; your ability to alter them independently has been destroyed.

Fix the superclass, add an argument to indicate some stable domain-specific flag, or add an intermediate method in the superclass, overridden here, to do the updatePopup stuff correctly.

@@ +128,5 @@
> +        // If tab is not loading and the favicon is updated, we
> +        // want to load the image straight away. If tab is still
> +        // loading, we only load the favicon once the page's content
> +        // is fully loaded (see handleContentLoaded()).
> +        if (tab.getState() != Tab.STATE_LOADING) {

tab.isReady()? tab.isLoaded()? tab.isLoading()?

Seems you have lots of opportunity here to enrich the meaning of the code…

@@ +142,5 @@
> +    void handleClearHistory() {
> +        updateAboutHomeTopSites();
> +    }
> +
> +    @Override void toggleChrome(Boolean aShow) {

s/Boolean/boolean

@@ +144,5 @@
> +    }
> +
> +    @Override void toggleChrome(Boolean aShow) {
> +        if (aShow) mBrowserToolbar.show();
> +        else mBrowserToolbar.hide();

Style.

@@ +147,5 @@
> +        if (aShow) mBrowserToolbar.show();
> +        else mBrowserToolbar.hide();
> +    }
> +
> +    @Override void updatePopups(final Tab tab) {

Style: annotations on a separate line, always. (Throughout.)

@@ +195,5 @@
> +    @Override void postSecurityChange(final Tab tab) {
> +        mMainHandler.post(new Runnable() { 
> +            public void run() {
> +                if (Tabs.getInstance().isSelectedTab(tab))
> +                    mBrowserToolbar.setSecurityMode(tab.getSecurityMode());

This is a really pervasive pattern: some static class handles a tab event by asking the tab collection if this one is selected, and then pokes at some other object using some internal state of the tab. Can we think of a better way to do this?

@@ +228,5 @@
> +
> +    @Override void postSetContentView() {
> +        LinearLayout actionBar;
> +
> +        if (Build.VERSION.SDK_INT >= 11 && !getIsWebapp()) {

This is voodoo. Please comment.

And also: having a boolean switch method like this scattered through the code is a recipe for bugs.

Rather than having "getIsWebapp", which is a boolean method that returns true for WebApp and false for GeckoApp, *have some methods* that each of the App classes can override. For example, how 'bout WebApp has something like

  @Override
  void postSetContentView() {
    super.postSetContentView();
    actionBar = (LinearLayout) findViewById(R.id.browser_toolbar);

    // Webapps don't have a toolbar.
  }

and GeckoBrowserApp has:
  
  @Override
  void postSetContentView() {
    super.postSetContentView();
    if (Build.VERSION.SDK_INT >= 11) {
      actionBar = (LinearLayout) GeckoActionBar.getCustomView(this);
    } else {
      actionBar = (LinearLayout) findViewById(R.id.browser_toolbar);
    }

    mBrowserToolbar = new BrowserToolbar(mAppContext);
    mBrowserToolbar.from(actionBar);
  }

? See what I mean?

getIsWebapp should die.

@@ +291,5 @@
> +        } else {
> +            mBrowserToolbar.updateTabCount(1);
> +        }
> +
> +        mBrowserToolbar.setProgressVisibility(isExternalURL || (mRestoreMode != GeckoAppShell.RESTORE_NONE));

Need to spend some time deciding where all of GeckoAppShell's pseudo-enums should live.

@@ +351,5 @@
> +
> +            GeckoActionBar.setBackgroundDrawable(this, background);
> +            GeckoActionBar.setCustomView(this, actionBar);
> +        } else {
> +            Log.i(LOGTAG, "Is a webapp " + getIsWebapp());

This is a perfect illustration of my earlier point.

@@ +365,5 @@
> +        // Cancel pending favicon load task
> +        mFavicons.cancelFaviconLoad(faviconLoadId);
> +
> +        // Reset favicon load state
> +        tab.setFaviconLoadId(Favicons.NOT_LOADING);

This is all backward. Whenever you see get..foo..set, you're violating encapsulation.

Why are we not simply saying:

  tab.cancelFaviconLoad()

, and letting the tab implementation decide how to do all this? The tab is the thing loading the favicon; it should be holding a reference to whatever mFavicons is, tracking its load ID, managing its state, etc.

Comment 7

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

Take a look at bug 753217 in which Brian is moving a lot of code out of GeckoApp and into Tabs/Tab. He is also making some Tab.selected() changes too. Let's get that code moving again.

Comment 8

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

(In reply to Richard Newman [:rnewman] from comment #6)

> > +public class App extends GeckoAppWithUrlbar {
> 
> Can we do better, name-wise? GeckoURLBrowser?
> 
> I would encourage you to broadly reconsider the naming of these core classes
> -- GeckoApp, GeckoAppShell, etc.
> 
> The name should reflect what they do. If it's a browser activity, call it
> BrowserActivity -- if we decide to hide the URL bar for browsing, or show
> the URL bar differently for web apps, this'll look really silly.
> 
> If it's a web app container, call it WebAppContainer.

Agree. I think BrowserActivity and WebActivity (or WebAppActivity) would fit better.

> Have you thought about adding Tab.isSelected()? (Presumably each tab can
> belong to only a single Tabs, and has a reference to it…?)
> 
> Failing that: if Tabs is a singleton, then there's nothing wrong with adding
> static methods for simplicity:
> 
>   Tabs.isSelectedTab(tab);
>   Tabs.getCount();
> 
>   setShadowVisibility(tab.shouldShadow())?
> 
> See the sibling:
> 
>   tab.canDoForward();

Most of this should be handled in bug 753217. The "tab.shouldShadow" is not, but should be added.

> >          tab.setState(success ? Tab.STATE_SUCCESS : Tab.STATE_ERROR);
> 
> tab.setSuccess(success);
> 
> Let Tab own its own internal state.

Tab.mState is not boolean.

> > +            if (getIsWebapp())
> > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Show", "webapps.xul"));
> > +            else
> > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Show", "browser.xul"));
> 
> protected String getWindowXUL() {
>   return (getIsWebapp() ? "webapps.xul" : "browser.xul");
> }

I like this change, but I also worry about having 2 different XUL windows. There will be much overlap.

> > +        if (tab.getState() != Tab.STATE_LOADING) {
> 
> tab.isReady()? tab.isLoaded()? tab.isLoading()?
> 
> Seems you have lots of opportunity here to enrich the meaning of the code…

Since Tab.mState is not boolean, I dislike the proliferation of methods needed to handle checking and setting the state. The enum-ish style works better IMO.

> > +        // Cancel pending favicon load task
> > +        mFavicons.cancelFaviconLoad(faviconLoadId);
> > +
> > +        // Reset favicon load state
> > +        tab.setFaviconLoadId(Favicons.NOT_LOADING);
> 
> This is all backward. Whenever you see get..foo..set, you're violating
> encapsulation.
> 
> Why are we not simply saying:
> 
>   tab.cancelFaviconLoad()
> 
> , and letting the tab implementation decide how to do all this? The tab is
> the thing loading the favicon; it should be holding a reference to whatever
> mFavicons is, tracking its load ID, managing its state, etc.

I don't like coupling the Tab to all of our DB/Storage (favicons, history, bookmarks). I don't like the current pattern very much myself and Brian is working to liberate some of it, but I don't want Tab to be the kitchen sink like GeckoApp has become. We need to find some balance. You could say Tab has two purposes: Display things in the UI and (possibly) saving things to a DB. Example is Tab.setTitle(...) - we do not want this to save the title every time it's called and using a Tab.setTitle(string title, boolean saveToDB) is not the answer.

FWIW, Brian's changes to this issue are to move the message listeners into Tab, so Tab.handleTitleChanged(...) would call setTitle(newTitle) and updateHistory(...) - trying to find a good balance.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #8)

> > tab.setSuccess(success);
> > 
> > Let Tab own its own internal state.
> 
> Tab.mState is not boolean.

Oh, I know -- but Tab is the class that should know how to translate from some arbitrary notion of "success" into side-effects that affect its internal state.

If the notion of "success" is specific to this calling code, then it should call some appropriate _informative_ combination of methods on Tab, such as Tab.onFooLoaded() + Tab.onFooConnectionClosed() (or whatever).

That is, having this calling code know about Tab.mState is a violation of encapsulation. If you add a new Tab state enumeration, you should never have to modify GeckoApp; right now you would.

> I like this change, but I also worry about having 2 different XUL windows.
> There will be much overlap.

Yup. Indication of a certain amount of brokenness in the overall design? Something to think about going forward.
 
> > Seems you have lots of opportunity here to enrich the meaning of the code…
> 
> Since Tab.mState is not boolean, I dislike the proliferation of methods
> needed to handle checking and setting the state. The enum-ish style works
> better IMO.

I'm just concerned about having dozens of call sites that apply varying rules to compute a state enumeration, poking at this internal state of the tab. Tab is being treated like a struct, not an object.

I think it'd be worth precisely defining the states and state transitions of a tab, and then have callers like this poke Tab through those defined transition interfaces.

That is, rather than something like

  Is the tab loading? Did I just finish a favicon load? Well, that means we transition to STATE_FOO... let's update a member variable! Sure hope I remembered to synchronize on the right monitor... and what if I throw?

you'd have

  Hey tab: your favicon just finished loading. Update accordingly.

and the Tab code can handle validation of current state, synchronization, logging, and shifting to the right state flag.

> > , and letting the tab implementation decide how to do all this? The tab is
> > the thing loading the favicon; it should be holding a reference to whatever
> > mFavicons is, tracking its load ID, managing its state, etc.
> 
> I don't like coupling the Tab to all of our DB/Storage (favicons, history,
> bookmarks). I don't like the current pattern very much myself and Brian is
> working to liberate some of it, but I don't want Tab to be the kitchen sink
> like GeckoApp has become.

Nope, I quite agree.

> We need to find some balance. You could say Tab
> has two purposes: Display things in the UI and (possibly) saving things to a
> DB. Example is Tab.setTitle(...) - we do not want this to save the title
> every time it's called and using a Tab.setTitle(string title, boolean
> saveToDB) is not the answer.

I agree.

The general approach I've taken to this kind of thing is to use delegates; either a constructor-time delegate that provides e.g., persistence (TabStorageDelegate), or one passed in on a call (TabFaviconProvider).

This dramatically simplifies testing (you can use mock delegates), and it allows you to easily partition behavior and formalize interfaces. Of course, it adds more classes and interfaces, but c'est la Java.

The tradeoff is that you might *always* be using the same kind of delegate to do the same kind of work, in which case we're verging on architecture astronautics. But I find the flexibility for unit testing to be quite persuasive.

> FWIW, Brian's changes to this issue are to move the message listeners into
> Tab, so Tab.handleTitleChanged(...) would call setTitle(newTitle) and
> updateHistory(...) - trying to find a good balance.

That's probably a decent tradeoff; you're essentially using the message listeners to implement the delegate calls. You probably still want a reference to some provider/delegate to implement storage backends and such. Can't say for sure without exploring a little more.

Comment 10

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #9)
> > I like this change, but I also worry about having 2 different XUL windows.
> > There will be much overlap.
> 
> Yup. Indication of a certain amount of brokenness in the overall design?
> Something to think about going forward.

This was already gone. Just a relic from a first attempt at "chromeless" windows when we were running everything in the same process.

The tab stuff is interesting, but for this patch I'm primarily concerned with getting BrowserToolbar out of... everything. We can keep this open and do more tabs refactoring though, or open some separate bugs....

Comment 11

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #10)

> The tab stuff is interesting, but for this patch I'm primarily concerned
> with getting BrowserToolbar out of... everything. We can keep this open and
> do more tabs refactoring though, or open some separate bugs....

Yeah, I'm totally fine with incremental improvements over time. Was just dumping my thoughts as I read the code; I don't expect you to hold the train for me to pile on all of my baggage :D

Follow-ups are generally the best way to go for bugs that take a while. Perhaps morph this into a meta bug?

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 2

This addresses... some comments. And gets BrowserToolbar out of GeckoApp and everything else its in right now. Its still based on top of the chromeless windows stuff (bug 740586), which isn't ready, so I may have to rebase it but wanted to start reviews early.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated to latest m-c

Just merged this up to latest m-c.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated to latest m-c, try 2

Missed some stuff in the earlier patch.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch

This will bitrot daily I'm sure. I'll try to do a bit better at unbitrotting it .

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated to trunk again

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: un-bitrotted

Updated to m-c tip

Comment 18

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

Comment on attachment 631444 [details] [diff] [review]
un-bitrotted


>+    private int mColor = -1;
>+    public int getHighlightColor() {
>+        if (mColor < 0) {
>+            // Get the device's highlight color
>+           TypedArray typedArray;
>+    
>+            if (Build.VERSION.SDK_INT >= 11) {            
>+                typedArray = obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            } else {
>+                ContextThemeWrapper wrapper  = new ContextThemeWrapper(this, android.R.style.TextAppearance);
>+                typedArray = wrapper.getTheme().obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            }
>+    
>+            mColor = typedArray.getColor(typedArray.getIndex(0), 0);
>+            typedArray.recycle();
>+        }
>+        return mColor;
>+    }

This code was removed. We don't need it anymore, I think.

I just realized that the code moving to BrowserApp from GeckoApp could be out of date. I think we need to do a more "from scratch un-bitrot"

I will do a review of this patch for general content and organization, but let's get a new patch up too.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I've been doing essentially from-scratch -- any hunks that fail I've been looking at the original code in the previous GeckoApp and moving the new/updated code as needed.

Comment 20

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

Comment on attachment 631444 [details] [diff] [review]
un-bitrotted

Vlad - OK, with that in mind I'll do the review so we can get this change landed. 

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

>+    private int mColor = -1;
>+    public int getHighlightColor() {
>+        if (mColor < 0) {
>+            // Get the device's highlight color
>+           TypedArray typedArray;
>+    
>+            if (Build.VERSION.SDK_INT >= 11) {            
>+                typedArray = obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            } else {
>+                ContextThemeWrapper wrapper  = new ContextThemeWrapper(this, android.R.style.TextAppearance);
>+                typedArray = wrapper.getTheme().obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            }
>+    
>+            mColor = typedArray.getColor(typedArray.getIndex(0), 0);
>+            typedArray.recycle();
>+        }
>+        return mColor;
>+    }

Remove this. It's not used as far as I can tell.

>             return;
> 
>         tab.setReaderEnabled(true);
>-
>-        mMainHandler.post(new Runnable() {
>-            public void run() {
>-                if (Tabs.getInstance().isSelectedTab(tab))
>-                    mBrowserToolbar.setReaderVisibility(tab.getReaderEnabled());
>-            }
>-        });
>     }
> 
>-    void handleLoadError(final int tabId, final String uri, final String title) {
>-        final Tab tab = Tabs.getInstance().getTab(tabId);
>-        if (tab == null)
>-            return;
>-    
>-        // When a load error occurs, the URLBar can get corrupt so we reset it
>-        mMainHandler.post(new Runnable() {
>-            public void run() {
>-                if (Tabs.getInstance().isSelectedTab(tab)) {
>-                    mBrowserToolbar.refresh();
>-                    invalidateOptionsMenu();
>-                }
>-            }
>-        });
>+    void handleLoadError(final int tabId, final String uri, final String title) { }
>+
>+    void handlePageShow(final int tabId) { }
>+
>+    private String getDefaultProfileName() {
>+        return "default";

>             // This key should be profile-dependent. For now, we're simply hardcoding
>             // the "default" profile here.
>-            String keyName = packageName + ".default.startup_version";
>+            String profileName = getDefaultProfileName();
>+            if (profileName == null)
>+                profileName = "default";
>+            String keyName = packageName + "." + profileName + ".startup_version";

Is this still needed? Is it needed for separate profiles for web apps?

>+    protected void finishProfileMigration() {
>+    }

nit: Sometimes we use this style (braces on separate lines) and sometimes we use braces on the same line. Not a big deal, we can clean up style in a followup.

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 631444 [details] [diff] [review]
un-bitrotted

There's a few bugs here I see that I want to work out first.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch

Somewhere along the way I lost the event listener in BrowserToolbar.java. I went through this pretty slowly to make sure that every deleted line was being replaced by something somewhere, but this is bit more than just moving code around.

Works well in testing. Trying some other devices as well.

Comment 23

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

Comment on attachment 632004 [details] [diff] [review]
Patch


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

>+abstract public class BrowserApp extends GeckoApp {
>+    private static final String LOGTAG = "BrowserApp";

I know it might be weird, but we prefix all logtags with "Gecko" so let's make this "GeckoBrowserApp"


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

>+    private int mColor = -1;
>+    public int getHighlightColor() {
>+        if (mColor < 0) {
>+            // Get the device's highlight color
>+           TypedArray typedArray;
>+    
>+            if (Build.VERSION.SDK_INT >= 11) {            
>+                typedArray = obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            } else {
>+                ContextThemeWrapper wrapper  = new ContextThemeWrapper(this, android.R.style.TextAppearance);
>+                typedArray = wrapper.getTheme().obtainStyledAttributes(new int[] { android.R.attr.textColorHighlight });
>+            }
>+    
>+            mColor = typedArray.getColor(typedArray.getIndex(0), 0);
>+            typedArray.recycle();
>+        }
>+        return mColor;
>+    }

I don't see this used anymore. Sriram removed it IIRC.

Comment 24

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1268c57588

Comment 25

[Wesley Johnston (:wesj)]

backed out for bustage (grr bitrot under this...)
https://hg.mozilla.org/integration/mozilla-inbound/rev/906fc6bf33d8

Comment 26

[Wesley Johnston (:wesj)]

and back in!
https://hg.mozilla.org/integration/mozilla-inbound/rev/9af545c94146

Comment 27

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9af545c94146

Comment 28

[Wesley Johnston (:wesj)]

Comment on attachment 632004 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): No bug. Just a refactor
User impact if declined: No impact, but this holds up landing other user facing patches on Aurora
Testing completed (on m-c, etc.): Has been on mc for a few weeks. No regressions found that I know of.
Risk to taking this patch (and alternatives if risky): Moves around a lot of code, but doesn't change much. All behavior should be the same.
String or UUID changes made by this patch: None.

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 632004 [details] [diff] [review]
Patch

[Triage Comment]
Although not typically something we approve, we understand that this will be necessary to uplift FF16 tablet fixes like bug 765805 in the future. Approved for Aurora 15.

Comment 30

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ca4bbb128d1