941868 - Rework special favicon loading, retrieval, and caching

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Richard Newman [:rnewman]]

Forking Bug 938153. We need to rework how "special" favicons are stored and retrieved, and -- presuming that we'll need them Real Soon after startup -- we should preload them in the favicon cache.

That will allow us to show the about:home icon instantly, not several seconds after launching Firefox.

ckitching adds:

---
13:24:51 < ckitching> rnewman: If you do this right, you also get to remove the block of annoying special-casing logic from the cache which will speed a few things up.
13:25:00 < ckitching> As right now it has to go "Try favicon url. Now try page url. Now give up"
13:25:13 < ckitching> And the page url is only ever used in the case of bundled icons.
13:26:54 < ckitching> rnewman: *also* note: Bug 921433
---

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 0: remove magic constant URLs.

foldme

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8336432 [details] [diff] [review]
Part 0: remove magic constant URLs.

Step one of fixing favicon storage horrors for special pages is to get all of this logic in one place.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 0: remove magic URLs. v2

*sigh*

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8336438 [details] [diff] [review]
Part 0: remove magic URLs. v2

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

Nice style cleanup! :) I'd prefer it in a separate patch but I'll live.

r+ with the AboutPages.APPS fix.

::: mobile/android/base/AboutPages.java
@@ +11,5 @@
> +    public static final String DOWNLOADS       = "about:downloads";
> +    public static final String HOME            = "about:home";
> +    public static final String PRIVATEBROWSING = "about:privatebrowsing";
> +    public static final String READER          = "about:reader";
> +    public static final String UPDATER         = "about:";

This is the same page as "about:firefox". Perhaps that should be added too?

---

nit: These vars can also better be named *_URL, as it's technically more correct, but it should be fine the way it is.

::: mobile/android/base/BrowserApp.java
@@ +2185,5 @@
>              return true;
>          }
>  
>          if (itemId == R.id.apps) {
> +            Tabs.getInstance().loadUrlInTab(AboutPages.ADDONS);

AboutPages.APPS (you have to add it).

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8336438 [details] [diff] [review]
Part 0: remove magic URLs. v2

Moved Part 0 to separate bug.

Comment 6

[Richard Newman [:rnewman]]

I've got this working.

TODOs for self:

* Implement non-eviction for bitmaps themselves, not just for page mappings.
* Eliminate now-redundant delayed about:home favicon load.
* Remove icons from distribution bookmarks.json?
* Move on to fixing the massive redundant calls we have (Bug 941844 -- but also extends to setFavicon, which is just silly).
* For some reason we don't *display* the favicon until a little later in the load, even though we set it early.
* Explore whether we can get higher-res favicons in the tree. My HTC One displays 76x76 images, and we only ship 64x64 and 32x32.
* Make sure we're not regressing startup time by moving this favicon load work earlier in the startup process.
* Simplify favicon cache according to Comment 0.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 1: add NonEvictingLruCache. v1

Step 1: a variant of LRU cache that doesn't evict some entries. Used to keep persistent page-to-favicon mappings.

Comment 8

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

(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 8336438 [details] [diff] [review]
> Part 0: remove magic URLs. v2
> 
> Moved Part 0 to separate bug.

Which bug? I'd like to r+ right away.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Richard Newman [:rnewman] from comment #5)
> > Comment on attachment 8336438 [details] [diff] [review]
> > Part 0: remove magic URLs. v2
> > 
> > Moved Part 0 to separate bug.
> 
> Which bug? I'd like to r+ right away.

See Dependencies. Michael already reviewed and I dropped it into fx-team before I went to bed.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8336606 [details] [diff] [review]
Part 1: add NonEvictingLruCache. v1

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

lgtm.

::: mobile/android/base/favicons/Favicons.java
@@ +53,5 @@
>      private static final Map<Integer, LoadFaviconTask> sLoadTasks = Collections.synchronizedMap(new HashMap<Integer, LoadFaviconTask>());
>  
>      // Cache to hold mappings between page URLs and Favicon URLs. Used to avoid going to the DB when
>      // doing so is not necessary.
> +    private static final NonEvictingLruCache<String, String> sPageURLMappings = new NonEvictingLruCache<String, String>(PAGE_URL_MAPPINGS_TO_STORE);

nit: PAGE_URL_MAPPINGS_TO_STORE -> NUM_PAGE_URL_MAPPINGS_TO_STORE

::: mobile/android/base/favicons/NonEvictingLruCache.java
@@ +36,5 @@
> +
> +    public void put(K key, V value) {
> +        evictable.put(key, value);
> +    }
> +    

nit: whitespace.

Comment 11

[Richard Newman [:rnewman]]

I landed Part 1, with the cache moved into util/ for future reuse.

https://hg.mozilla.org/integration/fx-team/rev/b660e7e10184

Comment 12

[Matt Brubeck (:mbrubeck)]

part 1: https://hg.mozilla.org/mozilla-central/rev/b660e7e10184

Comment 13

[Richard Newman [:rnewman]]

See the message trace I recorded in Bug 941844 Comment 11.

It looks to me like we should *only* be modifying favicons thusly:

* In one of Content:StateChange or LOCATION_CHANGE, either blank the icon or set it if we know it
* In PAGE_SHOW, show the icon.

Instead, we have favicon setting code scattered throughout multiple different messages, which is a godawful mess and doesn't work correctly.

Nearly done...

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 2: load and cache certain preloaded favicons on launch.* * *

Removing logging.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Part 3: remove favicon from about:home's HTML content.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Part 4: don't evict built-in bitmaps, either.

Comment 17

[Richard Newman [:rnewman]]

Attachment: Part 5: tidying.

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 6: don't fetch about:home's favicon later in the load cycle.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 2: load and cache certain preloaded favicons on launch.

Comment 20

[Richard Newman [:rnewman]]

I am quite inclined to leave this debug logging in place:

11-25 14:22:35.163 D/GeckoToolbar(15079): onTabChanged: FAVICON
11-25 14:22:35.163 D/GeckoToolbar(15079): setFavicon(android.graphics.Bitmap@41e66058)
11-25 14:22:35.163 D/GeckoBrowserApp(15079): BrowserApp.onTabChanged: 1: FAVICON
11-25 14:22:35.163 D/GeckoToolbar(15079): onTabChanged: TITLE
11-25 14:22:35.163 D/GeckoBrowserApp(15079): BrowserApp.onTabChanged: 1: TITLE
11-25 14:22:35.163 D/GeckoToolbar(15079): onTabChanged: LOADED
11-25 14:22:35.163 D/GeckoBrowserApp(15079): BrowserApp.onTabChanged: 1: LOADED
11-25 14:22:35.163 D/GeckoToolbar(15079): onTabChanged: PAGE_SHOW
11-25 14:22:35.163 D/GeckoBrowserApp(15079): BrowserApp.onTabChanged: 1: PAGE_SHOW

because (a) it's debug, (b) it'll be handy tying log messages to event handlers, and (c) it'll remind us just how awfully verbose our code is.

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 8338030 [details] [diff] [review]
Part 2: load and cache certain preloaded favicons on launch.

I will file follow-ups for the two TODOs.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8338025 [details] [diff] [review]
Part 3: remove favicon from about:home's HTML content.

I think this has some context that might warrant mfinkle's eyes, too.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 8338028 [details] [diff] [review]
Part 5: tidying.

The only caller of a tab change event that doesn't pass a valid Tab is RESTORED, so this patch (a) adds logging should that constraint be violated, complete with a nice stack, and (b) short-circuits in a couple of places, which avoids a bunch of listener calls that aren't going to match a switch statement anyway.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338030 [details] [diff] [review]
Part 2: load and cache certain preloaded favicons on launch.

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

I'd like to hear more about the browser.js change.

Additionally, I don't understand the Favicon caching system all that well so I think it would be a good idea for someone else with that knowledge to also do this review. Alternatively, I can try to dig in-depth a bit more, but it might take me a while.

::: mobile/android/base/AboutPages.java
@@ +39,5 @@
>          }
>          return url.startsWith(READER);
>      }
> +
> +    private static final String[] DEFAULT_ICON_PAGES = new String[]{

nit: String[] {

@@ +51,5 @@
> +        UPDATER
> +    };
> +
> +    /**
> +     * Callers must not modify the returned array.

Potential idea: Make DEFAULT_ICON_PAGES an enum with a url field. That way you can retrieve DEFAULT_ICON_PAGES.values() and not have to worry if the original array is modified (though I presume this is creating a new array under the hood every time and thus not very efficient?).

@@ +64,5 @@
> +            return false;
> +        }
> +
> +        // TODO: it'd be quicker to not compare the "about:" part every time.
> +        for (int i = 0; i < DEFAULT_ICON_PAGES.length; ++i) {

With the enum idea, you can also add to a Set in the constructor of each page to do constant time lookups, if you don't mind the memory use, rather than iterating through all of the values (makes the "about:" comparison obsolete too).

::: mobile/android/base/GeckoApp.java
@@ +1382,5 @@
> +    protected Tab loadHomePage(int flags) {
> +        Tab tab = Tabs.getInstance().loadUrl(AboutPages.HOME, flags);
> +        Log.d(LOGTAG, "Setting about: tab favicon inline.");
> +        tab.updateFavicon(getAboutPageFavicon(AboutPages.HOME));
> +        return tab;

I'm not sure if this breaks the separation of concerns too much (I don't think so), but if you put this logic in Tab.loadUrl, you don't have to worry about devs not using the `loadHomePage` method and thus messing up favicon loading.

::: mobile/android/base/favicons/Favicons.java
@@ +34,5 @@
>  public class Favicons {
>      private static final String LOGTAG = "GeckoFavicons";
>  
> +    // A magic URL representing the app's own favicon, used for about: pages.
> +    private static final String BUILT_IN_FAVICON = "about:favicon";

Rename -> BUILT_IN_FAVICON_URL

What happens when you try to open this url in the browser?

@@ +102,5 @@
>          return NOT_LOADING;
>      }
>  
>      /**
> +     * Only returns a value if the entire path is cached.

What's a value? I'd prefer if you were more explicit and said, "Returns a Bitmap only if the entire path is cached; returns null otherwise." or similar.

@@ +104,5 @@
>  
>      /**
> +     * Only returns a value if the entire path is cached.
> +     */
> +    public static Bitmap getCachedFaviconForSize(final String pageURL, int targetSize) {

Is this `getSizeFaviconForPageFromLocalSynchronzied`? Perhaps it should be named as such.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +740,5 @@
>                                      mActivity.getString(R.string.one_tab));
>      }
>  
>      public void setProgressVisibility(boolean visible) {
> +        Log.d(LOGTAG, "setProgressVisibility: " + visible);

I assume leaving these log statements in is intentional?

@@ +923,5 @@
>          setTitle(title);
>      }
>  
>      private void setFavicon(Bitmap image) {
> +        Log.d(LOGTAG, "setFavicon(" + image + ")");

Ditto.

::: mobile/android/chrome/content/browser.js
@@ +3424,5 @@
>        }
>  
>        case "DOMLinkAdded": {
> +        if (docURI == "about:home") {
> +            // Nope!

Why not? Since I don't know what happens when this event occurs (which is another problem) and the code is too complicated to skim, it would be good to add the reasons to return in the comment (or an explanation above of what this event handler is trying to do in the first place).

Comment 25

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #24)

> Additionally, I don't understand the Favicon caching system all that well so
> I think it would be a good idea for someone else with that knowledge to also
> do this review. Alternatively, I can try to dig in-depth a bit more, but it
> might take me a while.

mfinkle will probably drive by.

> (though I presume this is creating a new array
> under the hood every time and thus not very efficient?).

enum.values() does copy the array, yes. And here we're not using the enum constants themselves, so we don't win anything from string interning.

> With the enum idea, you can also add to a Set in the constructor of each
> page to do constant time lookups, if you don't mind the memory use, rather
> than iterating through all of the values (makes the "about:" comparison
> obsolete too).

The goal here is the fastest possible solution that *doesn't* spread this logic as shitty string comparisons around the codebase.

 
> I'm not sure if this breaks the separation of concerns too much (I don't
> think so), but if you put this logic in Tab.loadUrl, you don't have to worry
> about devs not using the `loadHomePage` method and thus messing up favicon
> loading.

Yeah, I thought about that, but I decided on this to avoid Tab (a) having to check what kind of thing its argument was, and (b) knowing how to get a favicon for about pages. I want Tab to be more stupid, not less.


> Rename -> BUILT_IN_FAVICON_URL
> 
> What happens when you try to open this url in the browser?

Same as if you were to do so now -- "La dirección no es válida". This is never exposed to the outside world; it's just a sentinel that we use to represent the set of hard-coded favicon sizes in our JAR.


> > +    public static Bitmap getCachedFaviconForSize(final String pageURL, int targetSize) {
> 
> Is this `getSizeFaviconForPageFromLocalSynchronzied`? Perhaps it should be
> named as such.

It's not; the LruCache of page mappings is thread-safe, and the favicon cache itself (to which we delegate the rest of the work) is independently thread-safe. There is no failure possible here.

> I assume leaving these log statements in is intentional?

Yes. It's probably our most useful debug-level logging!


> >        case "DOMLinkAdded": {
> > +        if (docURI == "about:home") {
> > +            // Nope!
> 
> Why not? Since I don't know what happens when this event occurs (which is
> another problem) and the code is too complicated to skim, it would be good
> to add the reasons to return in the comment (or an explanation above of what
> this event handler is trying to do in the first place).

Fair.

Essentially, about:home is both a piece of Java UI and a totally empty XHTML document. We give not one fuck about what happens with the document, we just don't want it to be an error page that flickers into view when you first load a real page.

Muffling events here avoids needless redisplays in the absence of Part 3. Consider this a mild form of "Nope" for *all* browser.js event handling of about:home.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338025 [details] [diff] [review]
Part 3: remove favicon from about:home's HTML content.

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

r- because of my insecurity with the mLastFavicon code.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +122,2 @@
>      private ImageButton mFavicon;
> +    

nit: whitespace.

@@ +750,5 @@
>          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (visible) {
>              mFavicon.setImageResource(R.drawable.progress_spinner);
> +            mLastFavicon = null;

I haven't tracked down all the logic, but I think the throbber will run between all page loads, causing this lastFavicon to be invalidated on every page load - have you seen this working?

@@ +935,5 @@
>              return;
> +        }
> +
> +        if (image == mLastFavicon) {
> +            Log.d(LOGTAG, "No need to set favicon!");

nit: Be more specific, e.g. "Given favicon is the same as the previous favicon so there is no need to set the favicon."

::: mobile/android/chrome/content/aboutHome.xhtml
@@ -15,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>  <head>
>    <title>&abouthome.title;</title>
> -  <link rel="icon" type="image/png" sizes="64x64" href="chrome://branding/content/favicon64.png" />

I see what you were doing with browser.js now - you should probably explain in the comment in browser.js.

Comment 27

[Richard Newman [:rnewman]]

Attachment: Part 2: load and cache certain preloaded favicons on launch. v3

Comment 28

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #26)

> r- because of my insecurity with the mLastFavicon code.

You could have just asked ;P

> I haven't tracked down all the logic, but I think the throbber will run
> between all page loads, causing this lastFavicon to be invalidated on every
> page load - have you seen this working?

D/GeckoToolbar( 5347): onTabChanged: LOCATION_CHANGE
D/GeckoToolbar( 5347): setFavicon(null)
D/GeckoToolbar( 5347): setProgressVisibility: false
D/GeckoToolbar( 5347): setFavicon(null)
D/GeckoToolbar( 5347): No need to set favicon!
I/GeckoToolbar( 5347): zerdatime 774864927 - Throbber stop
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: LOCATION_CHANGE
D/GeckoToolbar( 5347): onTabChanged: SECURITY_CHANGE
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: SECURITY_CHANGE
D/GeckoToolbar( 5347): onTabChanged: PAGE_SHOW
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: PAGE_SHOW
D/GeckoToolbar( 5347): onTabChanged: VIEWPORT_CHANGE
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: VIEWPORT_CHANGE
D/GeckoToolbar( 5347): onTabChanged: STOP
D/GeckoToolbar( 5347): setProgressVisibility: false
D/GeckoToolbar( 5347): setFavicon(android.graphics.Bitmap@41e83c98)
I/GeckoToolbar( 5347): zerdatime 774865042 - Throbber stop
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: STOP
D/GeckoToolbar( 5347): onTabChanged: TITLE
D/GeckoBrowserApp( 5347): BrowserApp.onTabChanged: 0: TITLE
D/GeckoToolbar( 5347): onTabChanged: FAVICON
D/GeckoToolbar( 5347): setFavicon(android.graphics.Bitmap@41e83c98)
D/GeckoToolbar( 5347): No need to set favicon!


> I see what you were doing with browser.js now - you should probably explain
> in the comment in browser.js.

kk.

Comment 29

[Richard Newman [:rnewman]]

Attachment: Part 3: remove favicon from about:home's HTML content. v2

Log message improved, whitespace.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #25)
> enum.values() does copy the array, yes. And here we're not using the enum
> constants themselves, so we don't win anything from string interning.

Except that the underlying array will not get modified by accident.

> The goal here is the fastest possible solution that *doesn't* spread this
> logic as shitty string comparisons around the codebase.

It could still be wrapped with the same method (i.e. isDefaultIconPage) - for such a small array, however, perhaps the performance gains from a Set to array iteration are negligible (if they're even better).

> > What happens when you try to open this url in the browser?
> 
> Same as if you were to do so now -- "La dirección no es válida". This is
> never exposed to the outside world; it's just a sentinel that we use to
> represent the set of hard-coded favicon sizes in our JAR.

Then I fear what would happen if "about:favicon" is ever actually created as a page. Perhaps you want to make the sentinel a more obvious sentinel? (e.g. "about:favicon_sentinel")

Comment 31

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

Comment on attachment 8338027 [details] [diff] [review]
Part 4: don't evict built-in bitmaps, either.

The complexity of our Favicon code continues to increase

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #25)
> Essentially, about:home is both a piece of Java UI and a totally empty XHTML
> document. We give not one fuck about what happens with the document, we just
> don't want it to be an error page that flickers into view when you first
> load a real page.
> 
> Muffling events here avoids needless redisplays in the absence of Part 3.
> Consider this a mild form of "Nope" for *all* browser.js event handling of
> about:home.

(Without really digging into the code and if I'm understanding this correctly) I'm not sure I agree with special casing about:home in this way. From my (minimal) knowledge of how about:home is loaded, we try not to special case it more than it has to be. If this issue is only caused because part 3 is not yet applied, I'd rather part 3 gets folded in here than than to include the special case.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Which pages did you go to for the logcat output in comment 28?

Comment 34

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #32)

> (Without really digging into the code and if I'm understanding this
> correctly) I'm not sure I agree with special casing about:home in this way.
> From my (minimal) knowledge of how about:home is loaded, we try not to
> special case it more than it has to be.

Well, about:home *is* a special case in that it's our only content that has a page URL but is not rendered by all of this DOM stuff.

A future work item is probably to special-case about:home and not even parse it off the disk.

> If this issue is only caused because
> part 3 is not yet applied, I'd rather part 3 gets folded in here than than
> to include the special case.

wfm.

Comment 35

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #33)
> Which pages did you go to for the logcat output in comment 28?

This is on first launch, about:home:

11-25 18:08:58.342 D/GeckoToolbar( 8076): onTabChanged: SELECTED
11-25 18:08:58.352 D/GeckoToolbar( 8076): setFavicon(android.graphics.Bitmap@41e543a8)
11-25 18:08:58.352 D/GeckoToolbar( 8076): setProgressVisibility: false
11-25 18:08:58.352 D/GeckoToolbar( 8076): setFavicon(android.graphics.Bitmap@41e543a8)
11-25 18:08:58.352 D/GeckoToolbar( 8076): No need to set favicon!

...

11-25 18:09:01.606 D/GeckoToolbar( 8076): onTabChanged: VIEWPORT_CHANGE
11-25 18:09:01.606 D/GeckoBrowserApp( 8076): BrowserApp.onTabChanged: 0: VIEWPORT_CHANGE
11-25 18:09:01.606 D/GeckoToolbar( 8076): onTabChanged: STOP
11-25 18:09:01.606 D/GeckoToolbar( 8076): setProgressVisibility: false
11-25 18:09:01.606 D/GeckoToolbar( 8076): setFavicon(android.graphics.Bitmap@41e9b4a8)
11-25 18:09:01.606 D/GeckoToolbar( 8076): No need to set favicon!
11-25 18:09:01.606 I/GeckoToolbar( 8076): zerdatime 776554361 - Throbber stop

Then tapping a top site that fails to load, but still has a favicon:

11-25 18:10:31.932 D/GeckoToolbar( 8076): onTabChanged: STOP
11-25 18:10:31.932 D/GeckoToolbar( 8076): setProgressVisibility: false
11-25 18:10:31.932 D/GeckoToolbar( 8076): setFavicon(null)
11-25 18:10:31.932 D/GeckoToolbar( 8076): No need to set favicon!
11-25 18:10:31.932 I/GeckoToolbar( 8076): zerdatime 776644694 - Throbber stop
11-25 18:10:31.932 D/GeckoBrowserApp( 8076): BrowserApp.onTabChanged: 0: STOP
11-25 18:10:31.992 D/GeckoToolbar( 8076): onTabChanged: MENU_UPDATED
11-25 18:10:31.992 D/GeckoBrowserApp( 8076): BrowserApp.onTabChanged: 0: MENU_UPDATED
11-25 18:10:31.992 D/GeckoToolbar( 8076): onTabChanged: TITLE
11-25 18:10:32.002 D/GeckoBrowserApp( 8076): BrowserApp.onTabChanged: 0: TITLE
11-25 18:10:32.002 D/GeckoToolbar( 8076): onTabChanged: MENU_UPDATED
11-25 18:10:32.002 D/GeckoBrowserApp( 8076): BrowserApp.onTabChanged: 0: MENU_UPDATED
11-25 18:10:32.002 D/GeckoToolbar( 8076): onTabChanged: LOCATION_CHANGE
11-25 18:10:32.002 D/GeckoToolbar( 8076): setFavicon(null)
11-25 18:10:32.002 D/GeckoToolbar( 8076): No need to set favicon!
11-25 18:10:32.002 D/GeckoToolbar( 8076): setProgressVisibility: false
11-25 18:10:32.002 D/GeckoToolbar( 8076): setFavicon(null)
11-25 18:10:32.002 D/GeckoToolbar( 8076): No need to set favicon!
...


Many ordinary page loads are short-circuited by the prior clause:

        if (Tabs.getInstance().getSelectedTab().getState() == Tab.STATE_LOADING) {
            return;
        }


The nice thing is that the favicon bitmap is *already* sticking around in the cache (and in mFavicon, if it doesn't need scaling), so this is basically free... and it avoids making scaling calls or setting the view's bitmap, which isn't cheap.

Comment 36

[Richard Newman [:rnewman]]

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

> The complexity of our Favicon code continues to increase

On the plus side, now no longer rely on an async task culminating in a JAR load and database storage for built-in favicons. And we have a mechanism for not using the DB for any distribution or built-in icons, too.

Comment 37

[Richard Newman [:rnewman]]

Attachment: Part 2: load and cache certain preloaded favicons on launch. v4

Misplaced a null check when I moved that logic into Tabs.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #35)
> 11-25 18:10:32.002 D/GeckoToolbar( 8076): setProgressVisibility: false
> 11-25 18:10:32.002 D/GeckoToolbar( 8076): setFavicon(null)
> 11-25 18:10:32.002 D/GeckoToolbar( 8076): No need to set favicon!

I see: setProgressVisibility(false) calls setFavicon. However, I noticed we call setFavicon several times, including before we call setProgressVisibility - I wonder if some of these setFavicon calls are unnecessary and can be merged into setProgressVisibility.

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338027 [details] [diff] [review]
Part 4: don't evict built-in bitmaps, either.

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

lgtm, fix nits if you like.

::: mobile/android/base/favicons/cache/FaviconCache.java
@@ +221,5 @@
>          boolean isAborting = false;
>  
>          try {
>              // If we don't have it in the cache, it certainly isn't a known failure.
> +            // Non-evictable favicons are never failed.

nit: -> "Non-evictable favicons are never failed so we don't check mPermanentBackingMap" or similar.

@@ +583,5 @@
>          boolean abortingRead = false;
>  
>          // Not using setMostRecentlyUsed, because the elements are known to be new. This can be done
>          // without taking the write lock, via the magic of the reordering semaphore.
>          mReorderingSemaphore.acquireUninterruptibly();

nit: This *might* be cleaner if you only acquired the read lock when permanently != true, and acquire the write lock directly later for permanently == true.

Alternatively, just separating the two code paths with one `if (permanently)` at the top, despite the minor code duplication (namely, `finishWrite()`).

@@ +660,5 @@
>              mCurrentSize.set(0);
>              mBackingMap.clear();
>              mOrdering.clear();
> +
> +            // Note that we neither clear, nor track the size of, the permanent map.

nit: I'd put this above mCurrentSize.set(0).

Comment 40

[Richard Newman [:rnewman]]

Attachment: Part 2: load and cache certain preloaded favicons on launch. v5

Folded in Part 3, eliminated browser.js change.

Comment 41

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #39)

> nit: This *might* be cleaner if you only acquired the read lock when
> permanently != true...

I thought about it, but:

Note that even if we read from the permanent map, we're also doing things like storing secondaries. Rather than spend the (probably substantial) time to ensure that all of the guarantees of this class hold if we don't take the semaphore, I opted to simply always take the same path.

I would rather wait for this code to stabilize, and then see if this can be optimized if it needs to be, rather than do it here.

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338028 [details] [diff] [review]
Part 5: tidying.

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

lgtm.

::: mobile/android/base/BrowserApp.java
@@ +197,5 @@
>  
>      @Override
>      public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
> +        if (tab == null) {
> +            // Only RESTORED is allowed a null tab.

nit: Why? Be more specific!

@@ +199,5 @@
>      public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
> +        if (tab == null) {
> +            // Only RESTORED is allowed a null tab.
> +            if (msg != Tabs.TabEvents.RESTORED) {
> +                Log.w(LOGTAG, "Ignoring null tab in onTabChanged for " + msg, new RuntimeException("Null tab."));

As per IRC, perhaps this should throw.

::: mobile/android/base/Tabs.java
@@ +564,5 @@
>      public void notifyListeners(final Tab tab, final TabEvents msg, final Object data) {
> +        if (tab == null &&
> +            msg != TabEvents.RESTORED) {
> +            Log.e(LOGTAG, "notifyListeners: tab is null!", new RuntimeException("Null tab."));
> +            return;

Ditto to BrowserApp comments.

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338863 [details] [diff] [review]
Part 2: load and cache certain preloaded favicons on launch. v5

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

lgtm, though should the commit message be a one-liner?

::: mobile/android/base/Tabs.java
@@ +753,5 @@
> +     * we fetch with that size.
> +     *
> +     * This method completes on the calling thread.
> +     */
> +    private Bitmap getAboutPageFavicon(final String url) {

I caught you changing things! :P

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8338029 [details] [diff] [review]
Part 6: don't fetch about:home's favicon later in the load cycle.

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

r+ with the three non-nit changes. Granted, I don't fully understand the PAGE_SHOW/LOCATION_CHANGE interaction, but it seems okay.

::: mobile/android/base/BrowserApp.java
@@ +214,4 @@
>              case LOCATION_CHANGE:
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
>                      maybeCancelFaviconLoad(tab);
> +                    loadFavicon(tab);

This calls maybeCancelFaviconLoad so the line above is unnecessary.

::: mobile/android/base/Tab.java
@@ +628,5 @@
>          final String oldUrl = getURL();
>          mEnteringReaderMode = ReaderModeUtils.isEnteringReaderMode(oldUrl, uri);
> +
> +        if (TextUtils.equals(oldUrl, uri)) {
> +            Log.i(LOGTAG, "Ignoring location change event: URIs are the same.");

return here? (resolve before landing)

@@ +644,5 @@
>          }
>  
>          setContentType(message.getString("contentType"));
> +
> +        // This only applies if the location has actually changed.

Clarify, as per IRC.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1601,5 @@
>                  tab.toggleReaderMode();
>              }
> +            return;
> +        }
> +        if (event.equals("Reader:LongClick")) {

nit: I kind of prefer this the way it was (plus no churn).

Comment 45

[Richard Newman [:rnewman]]

Renumbered after folding 3 into 2:

   https://hg.mozilla.org/integration/fx-team/rev/b5a3121b05c1
   https://hg.mozilla.org/integration/fx-team/rev/cffd41d1d7e8
   https://hg.mozilla.org/integration/fx-team/rev/a164ef0013e7
   https://hg.mozilla.org/integration/fx-team/rev/192103a30df7

Comment 46

[Richard Newman [:rnewman]]

Landed a typo from removing the early return. Sadface.

https://hg.mozilla.org/integration/fx-team/rev/9f446888b2d2

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5a3121b05c1
https://hg.mozilla.org/mozilla-central/rev/cffd41d1d7e8
https://hg.mozilla.org/mozilla-central/rev/a164ef0013e7
https://hg.mozilla.org/mozilla-central/rev/192103a30df7
https://hg.mozilla.org/mozilla-central/rev/9f446888b2d2

Comment 48

[Richard Newman [:rnewman]]

I backed out the last two changesets (the former Part 6, now named Part 5, and its typo followup) for causing Bug 943879/Bug 943887.

   https://hg.mozilla.org/integration/fx-team/rev/08218ec17a37
   https://hg.mozilla.org/integration/fx-team/rev/9047f1cc94ca

This causes the about:home favicon to load way sooner than it did, and to flicker for an instant when it would previously have loaded (because the load task is no longer eliminated).

Comment 49

[Richard Newman [:rnewman]]

This is the uncontroversial part of the last part:

https://hg.mozilla.org/integration/fx-team/rev/2296ea325107

Coincidentally, it removes the flicker in the URL bar favicon. It does so by discarding some duplicate work, though, so I want to continue with the end goal of this bug. I'll do so in a follow-up.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2296ea325107

Comment 51

[Richard Newman [:rnewman]]

Comment on attachment 8336606 [details] [diff] [review]
Part 1: add NonEvictingLruCache. v1

*batch approval comment*

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 940049. Precise source unknown.

User impact if declined: 
  Favicons will sometimes not load on first page load.

Testing completed (on m-c, etc.): 
  Baking in Nightly for ages.

Risk to taking this patch (and alternatives if risky): 
  Some. This is not a trivial set of patches, but it's not incredibly complex, and we're happy with it in Nightly. The alternatives are to spend a bunch of time figuring out exactly what this patch changes that fixes the behavior on Aurora, or to ship with Bug 940049.

  This also buys a dramatic first-launch visible perf win, so that's a plus.

String or IDL/UUID changes made by this patch:
  None.

Comment 52

[Richard Newman [:rnewman]]

Aaron verified an Aurora build with these patches applied.

Comment 53

[Richard Newman [:rnewman]]

Comment on attachment 8336606 [details] [diff] [review]
Part 1: add NonEvictingLruCache. v1

Updating approval request to follow fx27.

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8336606 [details] [diff] [review]
Part 1: add NonEvictingLruCache. v1

It's early in beta and we have two extra weeks with our users for this version so let's go ahead, thanks for the early nomination.

Comment 55

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/releases/mozilla-beta/rev/12583a256cc5
   https://hg.mozilla.org/releases/mozilla-beta/rev/dbe0e1ca219a
   https://hg.mozilla.org/releases/mozilla-beta/rev/3cf413884fdd
   https://hg.mozilla.org/releases/mozilla-beta/rev/8e41eadc884f
   https://hg.mozilla.org/releases/mozilla-beta/rev/f1dbbbab2724