Closed Bug 943515 Opened 8 years ago Closed 8 years ago

Fix conflicting naming conventions in favicons.Favicons

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: mcomella, Assigned: jdover)

References

Details

(Whiteboard: [mentor=mcomella][lang=java])

Attachments

(2 files, 10 obsolete files)

13.29 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
10.86 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
There are two conflicting naming conventions in favicons.Favicons - `getFaviconForSize` and `getSizedFavicon*`. These should be merged, one way or another.
Whiteboard: [mentor=mcomella][lang=java]
bug 941868 will land another such method so I'm marking this blocking.
Depends on: 941868
Ah. And if memory serves the cache has similar inconsistency in naming vs. favicons.Favicons, too. (As well as possibly internally).
Assignee: nobody → jdover
Attachment #8357293 - Attachment is obsolete: true
Attachment #8357295 - Flags: review?(michael.l.comella)
Status: NEW → ASSIGNED
Comment on attachment 8357295 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

Was idly browing my bugmail and noticed something scary. Thought I'd comment.

::: mobile/android/base/BrowserApp.java
@@ -784,5 @@
>              if (url == null || title == null) {
>                  return true;
>              }
>  
> -            final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);

^- There's no need to change this line, is there? You've just reflowed it.

::: mobile/android/base/favicons/Favicons.java
@@ +166,5 @@
>      }
>  
> +    public static int getSizedFavicon(String pageURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> +        return getSizedFavicon(pageURL, getFaviconUrlForPageUrl(pageURL), targetSize, flags, listener);
> +    }

What's going on here?

@@ -220,5 @@
>      }
>  
> -    public static int getSizedFaviconForPageFromLocal(final String pageURL, final OnFaviconLoadedListener callback) {
> -        return getSizedFaviconForPageFromLocal(pageURL, sDefaultFaviconSize, callback);
> -    }

Why are you killing this? It's not unused, is it?

::: mobile/android/base/home/TopSitesPage.java
@@ +621,5 @@
>  
>              // If we have no thumbnail, attempt to show a Favicon instead.
>              LoadIDAwareFaviconLoadedListener listener = new LoadIDAwareFaviconLoadedListener(view);
> +            final int loadId = Favicons.getSizedFavicon(url, Integer.MAX_VALUE, 
> +                    LoadFaviconTask.FLAG_PERSIST, listener);

getSizedFaviconForPageFromLocal and getSizedFavicon do quite different things. The former attempts to load a favicon from the cache/database only, but the latter will try to use the network too.
This change, then, will cause a rather radical change in behaviour (Which I don't think was the intention?)

I realise "getSizedFaviconForPageFromLocal" is an irritatingly long name for a method, but it is descriptive of what it does (And that's not the same thing as getSizedFavicon).

::: mobile/android/base/home/TwoLinePageRow.java
@@ +230,5 @@
>  
>          // Blank the Favicon, so we don't show the wrong Favicon if we scroll and miss DB.
>          mFavicon.clearImage();
> +        mLoadFaviconJobId = Favicons.getSizedFavicon(url, Integer.MAX_VALUE,
> +                LoadFaviconTask.FLAG_PERSIST, mFaviconListener);

Likewise, this change will cause it to try downloading a favicon, when the previous implementation did not.
Attachment #8357295 - Attachment is obsolete: true
Attachment #8357295 - Flags: review?(michael.l.comella)
Attachment #8357366 - Attachment is obsolete: true
(In reply to Chris Kitching [:ckitching] from comment #5)
> Comment on attachment 8357295 [details] [diff] [review]
> Refactor favicon accessors for naming and usage consistency.
> 
> Review of attachment 8357295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Was idly browing my bugmail and noticed something scary. Thought I'd comment.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ -784,5 @@
> >              if (url == null || title == null) {
> >                  return true;
> >              }
> >  
> > -            final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> 
> ^- There's no need to change this line, is there? You've just reflowed it.

Wanted to keep it consistent with the < 100 characters rule as I was following in the next line.

> 
> ::: mobile/android/base/favicons/Favicons.java
> @@ +166,5 @@
> >      }
> >  
> > +    public static int getSizedFavicon(String pageURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> > +        return getSizedFavicon(pageURL, getFaviconUrlForPageUrl(pageURL), targetSize, flags, listener);
> > +    }
> 
> What's going on here?
> 
> @@ -220,5 @@
> >      }
> >  
> > -    public static int getSizedFaviconForPageFromLocal(final String pageURL, final OnFaviconLoadedListener callback) {
> > -        return getSizedFaviconForPageFromLocal(pageURL, sDefaultFaviconSize, callback);
> > -    }
> 
> Why are you killing this? It's not unused, is it?
> 
> ::: mobile/android/base/home/TopSitesPage.java
> @@ +621,5 @@
> >  
> >              // If we have no thumbnail, attempt to show a Favicon instead.
> >              LoadIDAwareFaviconLoadedListener listener = new LoadIDAwareFaviconLoadedListener(view);
> > +            final int loadId = Favicons.getSizedFavicon(url, Integer.MAX_VALUE, 
> > +                    LoadFaviconTask.FLAG_PERSIST, listener);
> 
> getSizedFaviconForPageFromLocal and getSizedFavicon do quite different
> things. The former attempts to load a favicon from the cache/database only,
> but the latter will try to use the network too.
> This change, then, will cause a rather radical change in behaviour (Which I
> don't think was the intention?)
> 
> I realise "getSizedFaviconForPageFromLocal" is an irritatingly long name for
> a method, but it is descriptive of what it does (And that's not the same
> thing as getSizedFavicon).

I understand the distinction now. In general, I think there is probably a simpler way of exposing these different depths of caching and different return methods. I think this could be accomplished with a single point of entry to retrieve a favicon with 'depth' flag (enum) which would allow the Favicon service to decide how to handle the request. The only public method signature would be something like this:

public static int getFavicon(String pageUrl, @Nullable String faviconUrl, int size, Favicon.CacheDepth depth, OnFaviconLoadedListener listener)

The Nullable faviconUrl would allow the Favicon service to fallback and determine the URL on its own. I don't have a full understanding of how those URLs are needed within the favicon DB for lookup, so this may not work. Thoughts?

> 
> ::: mobile/android/base/home/TwoLinePageRow.java
> @@ +230,5 @@
> >  
> >          // Blank the Favicon, so we don't show the wrong Favicon if we scroll and miss DB.
> >          mFavicon.clearImage();
> > +        mLoadFaviconJobId = Favicons.getSizedFavicon(url, Integer.MAX_VALUE,
> > +                LoadFaviconTask.FLAG_PERSIST, mFaviconListener);
> 
> Likewise, this change will cause it to try downloading a favicon, when the
> previous implementation did not.
Comment on attachment 8357295 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

I agree with ckitching's comments. If you have any questions about the review comments, please feel free to ask!

::: mobile/android/base/favicons/Favicons.java
@@ +110,5 @@
>       * page URL to favicon URL, and the favicon URL to in-memory bitmaps.
>       *
>       * Returns null otherwise.
>       */
> +    public static Bitmap getSizedFaviconForPageFromCache(final String pageURL, int targetSize) {

"ForPage" - nice catch! This is a good addition!

@@ +178,5 @@
>       * @param targetSize The desired size of the returned Favicon.
>       * @return The cached Favicon, rescaled to be as close as possible to the target size, if any exists.
>       *         null if no applicable Favicon exists in the cache.
>       */
> +    protected static Bitmap getSizedFaviconFromCache(String faviconURL, int targetSize) {

Why did you change the access level here?

@@ +196,5 @@
>       * @param targetSize Target size of the desired Favicon to pass to the cache query
>       * @param callback Callback to fire with the result.
>       * @return The job ID of the spawned async task, if any.
>       */
> +    protected static int getSizedFaviconForPageFromLocal(final String pageURL, final int targetSize, final OnFaviconLoadedListener callback) {

Again, why the access level change?
Attachment #8357295 - Attachment is obsolete: false
(In reply to Josh Dover from comment #8)
> (In reply to Chris Kitching [:ckitching] from comment #5)
> > ^- There's no need to change this line, is there? You've just reflowed it.
> 
> Wanted to keep it consistent with the < 100 characters rule as I was
> following in the next line.

Unless something has changed, we don't have a line length limit - it's up to your best judgement about when to wrap (I usually wrap to 100 though).

That being said, it is generally better not to wrap lines you are not changing because you're affecting the `hg blame` history - but again, sometimes readability wins. Use your judgement.

> In general, I think there is probably a simpler way of exposing these
> different depths of caching and different return methods.

An interesting idea, however, there might be some harrowing complications due to the fact that some methods return ints and others return the Bitmaps directly. If ckitching thinks it's a reasonable refactor, we can do it, though I'd suggest filing a new bug (and perhaps putting it on the back burner because it'll take some time and is *probably* less important right now).

Also, not in my review, but I noticed some inconsistences in "URL" vs. "Url" that can be fixed up too (I'm honestly not sure which one follows our conventions, if we have one).
(In reply to Josh Dover from comment #8)
> The Nullable faviconUrl would allow the Favicon service to fallback and determine the URL on its own.

Also, I'm a bit heistant to use null to indicate state when the method could just be overloaded, but that's more subjective.

To be explicit, I do like the depth flag idea. But, after thinking about it, it's a complicated refactor for (probably) little gain. i.e.:

getSizedFavicon(..., CacheDepth.LOCAL) // vs.
getSizedFaviconFromLocal(...)

I'll let ckitching decide if it's worthwhile, but in any case, I like the proactiveness. :)
(In reply to Michael Comella (:mcomella) from comment #11)
> (In reply to Josh Dover from comment #8)
> > The Nullable faviconUrl would allow the Favicon service to fallback and determine the URL on its own.
> 
> Also, I'm a bit heistant to use null to indicate state when the method could
> just be overloaded, but that's more subjective.
> 
> To be explicit, I do like the depth flag idea. But, after thinking about it,
> it's a complicated refactor for (probably) little gain. i.e.:
> 
> getSizedFavicon(..., CacheDepth.LOCAL) // vs.
> getSizedFaviconFromLocal(...)
> 
> I'll let ckitching decide if it's worthwhile, but in any case, I like the
> proactiveness. :)

The benefit of using an argument would be if we want to decide the depth on the fly, based off of something like a spotty network connection, we could just pass a different argument at runtime rather than using a an else-if or switch block to decide on a method. In addition, I think having a single method and starting point into the Favicon logic makes it much easier for debugging and new contributors to make changes.

This definitely should be it's own bug though, and can probably be de-prioritized. I will submit a new patch with the URL renames. I chose "Url" after doing some occurrence regex searches through the code, Url is used slightly more often, but there hasn't been an established convention, but I will fix this file for sake of local consistency.
(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8357295 [details] [diff] [review]
> Refactor favicon accessors for naming and usage consistency.
> 
> Review of attachment 8357295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with ckitching's comments. If you have any questions about the
> review comments, please feel free to ask!
> 
> ::: mobile/android/base/favicons/Favicons.java
> @@ +110,5 @@
> >       * page URL to favicon URL, and the favicon URL to in-memory bitmaps.
> >       *
> >       * Returns null otherwise.
> >       */
> > +    public static Bitmap getSizedFaviconForPageFromCache(final String pageURL, int targetSize) {
> 
> "ForPage" - nice catch! This is a good addition!
> 
> @@ +178,5 @@
> >       * @param targetSize The desired size of the returned Favicon.
> >       * @return The cached Favicon, rescaled to be as close as possible to the target size, if any exists.
> >       *         null if no applicable Favicon exists in the cache.
> >       */
> > +    protected static Bitmap getSizedFaviconFromCache(String faviconURL, int targetSize) {
> 
> Why did you change the access level here?

I was going to try consolidating these calls into one public method, but this bug is not the appropriate place to do that. Reversing.

> 
> @@ +196,5 @@
> >       * @param targetSize Target size of the desired Favicon to pass to the cache query
> >       * @param callback Callback to fire with the result.
> >       * @return The job ID of the spawned async task, if any.
> >       */
> > +    protected static int getSizedFaviconForPageFromLocal(final String pageURL, final int targetSize, final OnFaviconLoadedListener callback) {
> 
> Again, why the access level change?
Attachment #8357295 - Attachment is obsolete: true
Attachment #8357368 - Attachment is obsolete: true
Attachment #8357418 - Attachment is obsolete: true
Attachment #8357422 - Attachment is obsolete: true
Attachment #8357425 - Attachment is obsolete: true
Attachment #8357431 - Attachment is obsolete: true
Was having some source control issues, still learning hg. My apologies for the zillion attachments :P
Attachment #8357434 - Flags: review?(michael.l.comella)
Comment on attachment 8357434 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

It seems that something somewhere has gone very wrong with your patch. You've got changes to all sorts of random stuff scattered all over the place :(.
You can see what's going on with `hg qdiff` while sat atop your patch.
You can see unrefreshed changes with `hg status`.

Did you forget to `hg qrefresh` after fixing your patch?

Remember - if everything, everywhere has gone horribly, hopelessly wrong, you can return to sanity (At the expense of all your unqref'd changes) with `hg revert --all && hg purge && hg qpop--all`.

At a guess, I'd say some editor of yours is getting all excited and changing the type of newline on every line you go near with it. Or something. Hop onto #mobile and whinge if you're still stuck.

::: mobile/android/base/BrowserApp.java
@@ +786,5 @@
>              }
>  
>              final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> +            Favicons.getSizedFavicon(url, tab.getFaviconURL(), Integer.MAX_VALUE,
> +                    LoadFaviconTask.FLAG_PERSIST, listener);

Is this actually more readable than the split-across-multiple-lines version? You could touch fewer lines if you didn't change that property of this line.

::: mobile/android/base/db/BrowserProvider.java
@@ +2973,5 @@
>          }
>  
>          // If no URL is provided, insert using the default one.
>          if (TextUtils.isEmpty(faviconUrl) && !TextUtils.isEmpty(pageUrl)) {
> +            values.put(Favicons.URL, org.mozilla.gecko.favicons.Favicons.guessDefaultFaviconUrl(pageUrl));

Why did you touch this?

Related: Why did the original author of this line use the fully qualified class name instead of an import?

::: mobile/android/base/favicons/Favicons.java
@@ +62,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>(NUM_PAGE_URL_MAPPINGS_TO_STORE);

Wat.

@@ +67,3 @@
>  
> +    public static String getFaviconUrlForPageUrlFromCache(String pageUrl) {
> +        return sPageUrlMappings.get(pageUrl);

Why is this being touched?

@@ +73,5 @@
>       * Insert the given pageUrl->faviconUrl mapping into the memory cache of such mappings.
>       * Useful for short-circuiting local database access.
>       */
> +    public static void putFaviconUrlForPageUrlInCache(String pageUrl, String faviconUrl) {
> +        sPageUrlMappings.put(pageUrl, faviconUrl);

Why is this being touched?

@@ +83,5 @@
>       * Returns either NOT_LOADING, or LOADED if the onFaviconLoaded call could
>       * be made on the main thread.
>       * If no listener is provided, NOT_LOADING is returned.
>       */
> +    static int dispatchResult(final String pageUrl, final String faviconUrl, final Bitmap image,

Wat.

@@ +90,5 @@
>              return NOT_LOADING;
>          }
>  
>          if (ThreadUtils.isOnUiThread()) {
> +            listener.onFaviconLoaded(pageUrl, faviconUrl, image);

Further wat.

@@ +98,5 @@
>          // We want to always run the listener on UI thread.
>          ThreadUtils.postToUiThread(new Runnable() {
>              @Override
>              public void run() {
> +                listener.onFaviconLoaded(pageUrl, faviconUrl, image);

Additional further wat.

@@ +149,5 @@
> +
> +        // Failing that, try and get one from the database or internet.
> +        return loadUncachedFavicon(pageUrl, faviconUrl, flags, targetSize, listener);
> +    }
> +

I think your VCS had some kind of mental breakdown.

@@ +239,4 @@
>          if (theTab != null) {
> +            targetUrl = theTab.getFaviconURL();
> +            if (targetUrl != null) {
> +                return targetUrl;

Wat.

@@ +369,5 @@
>          sFaviconsCache = new FaviconCache(FAVICON_CACHE_SIZE_BYTES, res.getDimensionPixelSize(R.dimen.favicon_largest_interesting_size));
>  
>          // Initialize page mappings for each of our special pages.
>          for (String url : AboutPages.getDefaultIconPages()) {
> +            sPageUrlMappings.putWithoutEviction(url, BUILT_IN_FAVICON_URL);

Wat.

@@ +417,5 @@
>          }
>  
>          try {
>              // Fall back to trying "someScheme:someDomain.someExtension/favicon.ico".
> +            URI u = new URI(pageUrl);

Wat.
(In reply to Josh Dover from comment #12)
> The benefit of using an argument would be if we want to decide the depth on
> the fly, based off of something like a spotty network connection, we could
> just pass a different argument at runtime rather than using a an else-if or
> switch block to decide on a method. 

Noted. It might be better wait for a situtation that would benefit from the flag to occur before making the change though.

> In addition, I think having a single
> method and starting point into the Favicon logic makes it much easier for
> debugging and new contributors to make changes.

True. I think consistent method naming (i.e. this bug) will substantially help too. :P
(In reply to Chris Kitching [:ckitching] from comment #19)
> Comment on attachment 8357434 [details] [diff] [review]
> Refactor favicon accessors for naming and usage consistency.
> 
> Review of attachment 8357434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems that something somewhere has gone very wrong with your patch.
> You've got changes to all sorts of random stuff scattered all over the place
> :(.
> You can see what's going on with `hg qdiff` while sat atop your patch.
> You can see unrefreshed changes with `hg status`.
> 
> Did you forget to `hg qrefresh` after fixing your patch?
> 
> Remember - if everything, everywhere has gone horribly, hopelessly wrong,
> you can return to sanity (At the expense of all your unqref'd changes) with
> `hg revert --all && hg purge && hg qpop--all`.
> 
> At a guess, I'd say some editor of yours is getting all excited and changing
> the type of newline on every line you go near with it. Or something. Hop
> onto #mobile and whinge if you're still stuck.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +786,5 @@
> >              }
> >  
> >              final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> > +            Favicons.getSizedFavicon(url, tab.getFaviconURL(), Integer.MAX_VALUE,
> > +                    LoadFaviconTask.FLAG_PERSIST, listener);
> 
> Is this actually more readable than the split-across-multiple-lines version?
> You could touch fewer lines if you didn't change that property of this line.
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +2973,5 @@
> >          }
> >  
> >          // If no URL is provided, insert using the default one.
> >          if (TextUtils.isEmpty(faviconUrl) && !TextUtils.isEmpty(pageUrl)) {
> > +            values.put(Favicons.URL, org.mozilla.gecko.favicons.Favicons.guessDefaultFaviconUrl(pageUrl));
> 
> Why did you touch this?
> 
> Related: Why did the original author of this line use the fully qualified
> class name instead of an import?
> 
> ::: mobile/android/base/favicons/Favicons.java
> @@ +62,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>(NUM_PAGE_URL_MAPPINGS_TO_STORE);
> 
> Wat.
> 
> @@ +67,3 @@
> >  
> > +    public static String getFaviconUrlForPageUrlFromCache(String pageUrl) {
> > +        return sPageUrlMappings.get(pageUrl);
> 
> Why is this being touched?
> 
> @@ +73,5 @@
> >       * Insert the given pageUrl->faviconUrl mapping into the memory cache of such mappings.
> >       * Useful for short-circuiting local database access.
> >       */
> > +    public static void putFaviconUrlForPageUrlInCache(String pageUrl, String faviconUrl) {
> > +        sPageUrlMappings.put(pageUrl, faviconUrl);
> 
> Why is this being touched?
> 
> @@ +83,5 @@
> >       * Returns either NOT_LOADING, or LOADED if the onFaviconLoaded call could
> >       * be made on the main thread.
> >       * If no listener is provided, NOT_LOADING is returned.
> >       */
> > +    static int dispatchResult(final String pageUrl, final String faviconUrl, final Bitmap image,
> 
> Wat.
> 
> @@ +90,5 @@
> >              return NOT_LOADING;
> >          }
> >  
> >          if (ThreadUtils.isOnUiThread()) {
> > +            listener.onFaviconLoaded(pageUrl, faviconUrl, image);
> 
> Further wat.
> 
> @@ +98,5 @@
> >          // We want to always run the listener on UI thread.
> >          ThreadUtils.postToUiThread(new Runnable() {
> >              @Override
> >              public void run() {
> > +                listener.onFaviconLoaded(pageUrl, faviconUrl, image);
> 
> Additional further wat.
> 
> @@ +149,5 @@
> > +
> > +        // Failing that, try and get one from the database or internet.
> > +        return loadUncachedFavicon(pageUrl, faviconUrl, flags, targetSize, listener);
> > +    }
> > +
> 
> I think your VCS had some kind of mental breakdown.
> 
> @@ +239,4 @@
> >          if (theTab != null) {
> > +            targetUrl = theTab.getFaviconURL();
> > +            if (targetUrl != null) {
> > +                return targetUrl;
> 
> Wat.
> 
> @@ +369,5 @@
> >          sFaviconsCache = new FaviconCache(FAVICON_CACHE_SIZE_BYTES, res.getDimensionPixelSize(R.dimen.favicon_largest_interesting_size));
> >  
> >          // Initialize page mappings for each of our special pages.
> >          for (String url : AboutPages.getDefaultIconPages()) {
> > +            sPageUrlMappings.putWithoutEviction(url, BUILT_IN_FAVICON_URL);
> 
> Wat.
> 
> @@ +417,5 @@
> >          }
> >  
> >          try {
> >              // Fall back to trying "someScheme:someDomain.someExtension/favicon.ico".
> > +            URI u = new URI(pageUrl);
> 
> Wat.

Further up in the comments, mcomella suggested making the use of URL / Url in the camelcase symbols consistent within this file as it was all over the place. I opted to use Url to consistent (with the slight majority) of the rest of the project.
Comment on attachment 8357434 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

::: mobile/android/base/BrowserApp.java
@@ +786,5 @@
>              }
>  
>              final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> +            Favicons.getSizedFavicon(url, tab.getFaviconURL(), Integer.MAX_VALUE,
> +                    LoadFaviconTask.FLAG_PERSIST, listener);

Leaning with Chris, though I could go either way.

::: mobile/android/base/db/BrowserProvider.java
@@ +2973,5 @@
>          }
>  
>          // If no URL is provided, insert using the default one.
>          if (TextUtils.isEmpty(faviconUrl) && !TextUtils.isEmpty(pageUrl)) {
> +            values.put(Favicons.URL, org.mozilla.gecko.favicons.Favicons.guessDefaultFaviconUrl(pageUrl));

URL -> Url, for consistency.

Sorry to drive you mad, Josh, but I think we should leave the URL -> Url changes to method names - otherwise we're changing way too many lines (sorry, I didn't realize this was the case before).

::: mobile/android/base/favicons/Favicons.java
@@ +149,5 @@
> +
> +        // Failing that, try and get one from the database or internet.
> +        return loadUncachedFavicon(pageUrl, faviconUrl, flags, targetSize, listener);
> +    }
> +

Agreed that this shouldn't be reordered - it messes up the `hg blame` history.
Attachment #8357434 - Flags: review?(michael.l.comella) → review-
> leave the URL -> Url changes to method names

By this I meant "make the URL -> Url changes only to the method names and their calls".
Attachment #8357434 - Attachment is obsolete: true
Attachment #8357459 - Flags: review?(michael.l.comella)
Fixed to only rename methods, should be all good now :)
Oh. I see. Wasn't expecting you to do all those changes in this bug. Scope creep!

(In reply to Josh Dover from comment #12)
> (In reply to Michael Comella (:mcomella) from comment #11)
> > (In reply to Josh Dover from comment #8)
> > > The Nullable faviconUrl would allow the Favicon service to fallback and determine the URL on its own.
> > 
> > Also, I'm a bit heistant to use null to indicate state when the method could
> > just be overloaded, but that's more subjective.
> > 
> > To be explicit, I do like the depth flag idea. But, after thinking about it,
> > it's a complicated refactor for (probably) little gain. i.e.:
> > 
> > getSizedFavicon(..., CacheDepth.LOCAL) // vs.
> > getSizedFaviconFromLocal(...)
> > 
> > I'll let ckitching decide if it's worthwhile, but in any case, I like the
> > proactiveness. :)
> 
> The benefit of using an argument would be if we want to decide the depth on
> the fly, based off of something like a spotty network connection, we could
> just pass a different argument at runtime rather than using a an else-if or
> switch block to decide on a method.

You'd need exactly the same if-else block to determine if the network is spotty enough for you to use a different flag. You'd only move complexity, not reuce it.

> In addition, I think having a single
> method and starting point into the Favicon logic makes it much easier for
> debugging and new contributors to make changes.

I think that relying on an enum parameter to specify if the network/database are invoked is risky. It seems likely someone will overload the method with one that provides a default value for that parameter, other people will then forget this has been done and proceed to inappropriately use the wrong method (By only using the default one).
It's not going to be immediately clear that getFavicon may or may not use the network. It *is* clear when the method name is more descriptive.

There is the added complication that the cache-query method returns a bitmap right away (Based on what was in the cache), and the others fork a task to the background thread.

Much of this madness exists because of the way we store favicons in the database. The favicons table associates faviconURL with bitmap. The history tables associates pageURL with faviconURL. We always have the pageURL at polling-time, but don't always have the faviconURL, so sometimes have to do magic to get the other one.
There were compelling reasons not for switching (Other than the fact it'd break all existing databases) when I did all the favicons work the other month, but I can't remember what they are. Feel free to start trying it and see what breaks :P .

There is some duplicated logic between getSizedFaviconForPageFromLocal and getFaviconForSize. You could perhaps merge these by exposing the final constructor parameter on LoadFaviconTask. I continue to like having the "fromLocal" explicit in the method name to prevent confusion, but you could at the very least use a private delegator method that gets rid of the shared logic.
Probably. Unclear if it's worth it. Probably should ask RNewman - he's done some work in the area. I don't trust myself to remember all the details of the favicon system at this point.




In general - try and avoid scope-creep. If you find new bugs while solving one bug it's good for reviewer sanity (And to avoid race conditions with non-reviewers noisily having opinions on your patch *ahem*) if you file followups.
Comment on attachment 8357459 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

::: mobile/android/base/favicons/Favicons.java
@@ -64,5 @@
>      // 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>(NUM_PAGE_URL_MAPPINGS_TO_STORE);
>  
> -    public static String getFaviconURLForPageURLFromCache(String pageURL) {

Called elsewhere, see comment below.

https://mxr.mozilla.org/mozilla-central/search?string=getFaviconURLForPageURLF&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

@@ -72,5 @@
>      /**
>       * Insert the given pageUrl->faviconUrl mapping into the memory cache of such mappings.
>       * Useful for short-circuiting local database access.
>       */
> -    public static void putFaviconURLForPageURLInCache(String pageURL, String faviconURL) {

Make sure you change the calls as well - this is used in LoadFaviconTask.java: https://mxr.mozilla.org/mozilla-central/search?string=putFaviconURL&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

If you're using mxr, remember that it only takes input up to a max 29 characters (and I don't think it complains - it just returns nothing).

@@ +156,5 @@
> +     * page URL to favicon URL, and the favicon URL to in-memory bitmaps.
> +     *
> +     * Returns null otherwise.
> +     */
> +    public static Bitmap getSizedFaviconForPageFromCache(final String pageURL, int targetSize) {

Please don't unnecessarily reorder methods - it changes the `hg blame` history.

You can use an hg extension like qcrefresh to more easily choose what gets into a qrefresh to make fixing this a tad easier.

@@ -405,5 @@
>       *
>       * @param pageURL Page URL for which a default Favicon URL is requested
>       * @return The default Favicon URL.
>       */
> -    public static String guessDefaultFaviconURL(String pageURL) {

Also called elsewhere: https://mxr.mozilla.org/mozilla-central/search?string=guessdefaultfaviconurl&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Attachment #8357459 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8357459 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

And because I think Comella missed a spot... :P

::: mobile/android/base/BrowserApp.java
@@ +786,5 @@
>              }
>  
>              final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> +            Favicons.getSizedFavicon(url,
> +                                     tab.getFaviconURL(),

Something is still funky. This second line hasn't changed, so why is it in the diff?
Is your editor changing spaces to tabs, or changing the type of line break?

::: mobile/android/base/favicons/Favicons.java
@@ +221,5 @@
>  
>      public static int getSizedFaviconForPageFromLocal(final String pageURL, final OnFaviconLoadedListener callback) {
>          return getSizedFaviconForPageFromLocal(pageURL, sDefaultFaviconSize, callback);
>      }
> +

Pointless chunk.
Attachment #8357459 - Flags: review- → review?(michael.l.comella)
Comment on attachment 8357459 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

Whoops. For some reason the review flag got maimed there. Last time I ignore a warning about invalid tokens from Bugzilla...
Attachment #8357459 - Flags: review?(michael.l.comella)
(In reply to Chris Kitching [:ckitching] from comment #28)
> Comment on attachment 8357459 [details] [diff] [review]
> Refactor favicon accessors for naming and usage consistency.
> 
> Review of attachment 8357459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> And because I think Comella missed a spot... :P
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +786,5 @@
> >              }
> >  
> >              final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> > +            Favicons.getSizedFavicon(url,
> > +                                     tab.getFaviconURL(),
> 
> Something is still funky. This second line hasn't changed, so why is it in
> the diff?
> Is your editor changing spaces to tabs, or changing the type of line break?
There is a change in the number of spaces to make this in line with the line above it. The method name is not the same length.

> 
> ::: mobile/android/base/favicons/Favicons.java
> @@ +221,5 @@
> >  
> >      public static int getSizedFaviconForPageFromLocal(final String pageURL, final OnFaviconLoadedListener callback) {
> >          return getSizedFaviconForPageFromLocal(pageURL, sDefaultFaviconSize, callback);
> >      }
> > +
> 
> Pointless chunk.
I saw that, turned out there was whitespace there, figured changing the blame here wouldn't affect anything meaningful seeing how there isn't any code?
Attachment #8357459 - Attachment is obsolete: true
Attachment #8357470 - Flags: review?(michael.l.comella)
Comment on attachment 8357470 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

lgtm with the nit.

> > Pointless chunk.
> I saw that, turned out there was whitespace there, figured changing the
> blame here wouldn't affect anything meaningful seeing how there isn't any
> code?

Yep, works (and preferred) in my book.

::: mobile/android/base/db/BrowserProvider.java
@@ -2973,5 @@
>          }
>  
>          // If no URL is provided, insert using the default one.
>          if (TextUtils.isEmpty(faviconUrl) && !TextUtils.isEmpty(pageUrl)) {
> -            values.put(Favicons.URL, org.mozilla.gecko.favicons.Favicons.guessDefaultFaviconURL(pageUrl));

nit: Add the import and call this via Favicons.<method>
Attachment #8357470 - Flags: review?(michael.l.comella) → feedback+
(In reply to Michael Comella (:mcomella) from comment #32)
> Comment on attachment 8357470 [details] [diff] [review]
> Refactor favicon accessors for naming and usage consistency.
> 
> Review of attachment 8357470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm with the nit.
> 
> > > Pointless chunk.
> > I saw that, turned out there was whitespace there, figured changing the
> > blame here wouldn't affect anything meaningful seeing how there isn't any
> > code?
> 
> Yep, works (and preferred) in my book.
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ -2973,5 @@
> >          }
> >  
> >          // If no URL is provided, insert using the default one.
> >          if (TextUtils.isEmpty(faviconUrl) && !TextUtils.isEmpty(pageUrl)) {
> > -            values.put(Favicons.URL, org.mozilla.gecko.favicons.Favicons.guessDefaultFaviconURL(pageUrl));
> 
> nit: Add the import and call this via Favicons.<method>

Ah forgot to mention, there is a conflict with org.mozilla.gecko.db.BrowserContract.Favicons class here, which is used much more frequently within this file. This seems to be the appropriate way to avoid this namespacing issue.
Comment on attachment 8357470 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

> Ah forgot to mention, there is a conflict with org.mozilla.gecko.db.BrowserContract.Favicons class here,
> which is used much more frequently within this file. This seems to be the appropriate way to avoid this
> namespacing issue.

Touche.
Attachment #8357470 - Flags: feedback+ → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a95bf9dca670
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a95bf9dca670
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][fixed-in-fx-team] → [mentor=mcomella][lang=java]
Target Milestone: --- → Firefox 29
Comment on attachment 8357470 [details] [diff] [review]
Refactor favicon accessors for naming and usage consistency.

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

Lovely work; thanks for taking care of this. But…

::: mobile/android/base/favicons/Favicons.java
@@ +64,5 @@
>      // 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>(NUM_PAGE_URL_MAPPINGS_TO_STORE);
>  
> +    public static String getFaviconUrlForPageUrlFromCache(String pageURL) {

Driveby: URL, not Url. We're not savages.

And if you are going to change it, change it everywhere -- like in the argument list on the very same line -- not just in random places!
> Driveby: URL, not Url. We're not savages.

We followed what seemed to be the convention – Url seems to be used more frequently than URL. I probably lean towards URL, but I could go either way. Is it worth filing a followup for?

> And if you are going to change it, change it everywhere -- like in the argument list on the
> very same line -- not just in random places!

I decided it was a bad idea because the change was probably ~100ish lines, and that's a lot of `hg blame` churn for minimal gain so we left it at method names.
(In reply to Michael Comella (:mcomella) from comment #38)
> > Driveby: URL, not Url. We're not savages.
> 
> We followed what seemed to be the convention – Url seems to be used more
> frequently than URL. I probably lean towards URL, but I could go either way.
> Is it worth filing a followup for?

Well, if you and I both have some preference for URL, ckitching apparently did because he wrote it that way, and large chunks of the rest of the codebase do (e.g., Tab uses 'getURL')...

... and all of the local variables in the file use URL...

... then yeah, let's fix it up, rather than continuing to wobble down the line of inconsistency.

I'd like to only see "Url" when we're talking about the Android class with that name.

Rather than a follow-up bug, I'd rubberstamp an automated follow-up patch in this bug. (Or I'll make the change and you can review it, if you choose.)

> I decided it was a bad idea because the change was probably ~100ish lines,
> and that's a lot of `hg blame` churn for minimal gain so we left it at
> method names.

Yeah, I eventually got through all 300 bugmail, and figured out why you made the call that you did :D

Remind me to never take PTO!
Or I can make the patch first. :P
Attachment #8359493 - Flags: review?(rnewman)
Assignee: jdover → michael.l.comella
Assignee: michael.l.comella → jdover
Flags: needinfo?(rnewman)
Sorry, bzexport and other silliness about.
Flags: needinfo?(rnewman)
Attachment #8359493 - Flags: review?(rnewman) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.