Bugzilla

Assignee

Comment 2

•

10 years ago

This comes from browser.js:7018:

        handleResult: function (results) {
          let bytes = results.getNextRow().getResultByName("favicon");
          favicon = "data:image/png;base64," + btoa(String.fromCharCode.apply(null, bytes));
        },

The favicon isn't actually a PNG, because we now store .ICOs in the DB.

This will cause breakage in potentially two places:

* Services.search, if it tries to decode the icon.
* Upstream in Fennec (Comment 0).

For the latter, this line, and probably the receiver, need to be modified to correctly deserialize into a LoadFaviconResult, not a Bitmap.

Assignee

Updated

•

10 years ago

Assignee: nobody → rnewman

Status: NEW → ASSIGNED

Assignee

Comment 3

•

10 years ago

Attached patch Correctly extract bitmaps from stored icons in search preferences. — Details — Splinter Review

Assignee

Comment 4

•

10 years ago

Comment on attachment 8362302 [details] [diff] [review]
Correctly extract bitmaps from stored icons in search preferences.

This will cause invalid data: URIs to fail quietly, rather than crashing. (Verified.)

My belief is that empty favicons already exist in the DB, and so without refetching, this won't cause icons to be displayed.

But when I blanked my profile and started again, I now see a Microsoft icon.

Note that we pick the largest icon, which looks a little oversized in the list. That's because the other icons are too small. The simple alternative was to take the smallest, but that looks too small. I leave fixing that -- perhaps imposing a desired size -- to a follow-up.

Attachment #8362302 - Flags: review?(mark.finkle)

Assignee

Comment 5

•

10 years ago

Some improvements I'd like to see:

* browser.js queries history_with_favicons to fetch the icon out of the DB to use for a search engine. That's so stupid it hurts. We should be able to get the page favicon URL easily, and use that directly.

* If nothing on the Gecko side needs the page favicon, then computing this bundle, pushing it through Services.search, resulting in a message to Java which ultimately results in a base64 blob being decoded, is the height of inefficiency. We already have the page favicon *in memory and in the cache* on the Java side. Pass the damn tab ID.

* We perhaps need some more entry points into the favicon system for data: URIs.

Assignee

Updated

•

10 years ago

Attachment #8362302 - Flags: review?(chriskitching)

Comment 6

•

10 years ago

Comment on attachment 8362302 [details] [diff] [review]
Correctly extract bitmaps from stored icons in search preferences.

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

>-            mIconBitmap = BitmapUtils.getBitmapFromDataURI(iconURI);
>+            LoadFaviconResult result = FaviconDecoder.decodeDataURI(iconURI);

I wonder if this means that someday we should move the ICO decoder out of Favicon land and into somewhere more image-centric.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>           let bytes = results.getNextRow().getResultByName("favicon");
>-          favicon = "data:image/png;base64," + btoa(String.fromCharCode.apply(null, bytes));
>+          if (bytes && bytes.length) {
>+            favicon = "data:image/x-icon;base64," + btoa(String.fromCharCode.apply(null, bytes));

Are you using "image/x-icon" as a generic image mime or are we explicitly asking for ICO now instead of PNG? "image/x-icon" seems to be the mime type for an ICO image. Some info on that here: http://en.wikipedia.org/wiki/Favicon

Also: Based on this patch and the IRC chatter, I believe we are saving the favicons into the DB in the native format? So ICOs pulled from the web are saved as ICOs. Wouldn't it be better to save them in a less sucky format? Since we extract a single image out of the ICO, wouldn't saving as a PNG be nicer?

Comment 7

•

10 years ago

Comment on attachment 8362302 [details] [diff] [review]
Correctly extract bitmaps from stored icons in search preferences.

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

::: mobile/android/base/favicons/decoders/FaviconDecoder.java
@@ +16,5 @@
>  /**
>   * Class providing static utility methods for decoding favicons.
>   */
>  public class FaviconDecoder {
> +    private static final String LOG_TAG = FaviconDecoder.class.getSimpleName();

Two slightly dull points:

LOGTAG or LOG_TAG? The use of this seems to be somewhat inconsistent across the codebase. We almost certainly don't care.

It's probably not a great plan to call Class.getSimpleName just for this. It makes a tidy log tag, for sure, but that's a sort of expensive JNI call for, essentially, no gain. A string literal is probably a good idea, but nobody really cares.

@@ +118,5 @@
> +            Log.w(LOG_TAG, "Can't decode null data: URI.");
> +            return null;
> +        }
> +
> +        if (!uri.startsWith("data:image/")) {

This isn't sufficient to determine if the data URI you've got is parseable by your code.

From RFC 2397:
>[data URIs] are of the form:
>
>                    data:[<mediatype>][;base64],<data>
>
>   The <mediatype> is an Internet media type specification (with
>   optional parameters.) The appearance of ";base64" means that the data
>   is encoded as base64. Without ";base64", the data (as a sequence of
>   octets) is represented using ASCII encoding for octets inside the
>   range of safe URL characters and using the standard %xx hex encoding
>   of URLs for octets outside that range."

Since your implementation is incapable of processing a non-base64 encoded data URI, you should perhaps check that the URI starts with "data:image/<something>base64,". (Or add support for decoding the other format)

As an added bonus, if you do that, you can omit the second check for the existence of the comma below (As you will be able to safely assume the existence of a comma on the string).

As an additional added bonus, you could import the constant hashmap from the patch on Bug 914027 (which enumerates image MIME-types we can actually process in Android) to check that this is a URI we can safely decode, before we bother going any further with it. This may be computationally advantageous, as it allows us to skip the irritating (and expensive) base64 decode step.

@@ +134,5 @@
> +        try {
> +            String base64 = uri.substring(offset);
> +            byte[] raw = Base64.decode(base64, Base64.DEFAULT);
> +            return decodeFavicon(raw);
> +        } catch (Exception e) {

What exception do you imagine will be thrown here? There are none declared as being throwable from the body of this try, so either the Base64 library is stupidly written (And has an explicit throw for something that it doesn't declare in the method signatures), or you're trying to insure against some generic exception?
Which one?

Can't have a NullPointerException from uri, because you null check it at the start of the function.
Can't have an IndexOutOfBoundsException on the substring call because the result of indexOf will always be either an index into the string or -1. It's not -1, because of the check above the try.
What else? That's it, I believe? So can't this try-catch be safely eliminated?

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ -23,2 @@
>  import org.mozilla.gecko.R;
> -import org.mozilla.gecko.gfx.BitmapUtils;

*whistles innocently*

@@ +137,5 @@
> +            }
> +
> +            Iterator<Bitmap> bitmaps = result.getBitmaps();
> +            if (!bitmaps.hasNext()) {
> +                Log.w(LOGTAG, "No bitmaps in decoded icon.");

This can't happen. You either get a null LoadFaviconResult, or you get at least one entry in the iterator.
In the case of a very broken single-image favicon, that iterator entry may be null - but there will *be* one, so this check is always going to return false.

@@ +144,5 @@
> +
> +            // Find the largest bitmap.
> +            do {
> +                mIconBitmap = bitmaps.next();
> +            } while (bitmaps.hasNext());

This seems to rely on the assumption that the bitmaps in the iterator are in ascending order of size. They're not - no promises are made about the order of the bitmaps in the iterator.

The current implementation of the ICODecoder is such that the bitmaps will end up in the iterator in the order they appear in the header of the ICO file. The ICO spec does not require images in the header to be in any particular order.

Also, given multiple images in an ICO, selecting the "right" one at a particular size isn't really the same as taking the biggest and downscaling it - smaller icons are not always just downscaled editions of the larger ones - so this logic may cause a nonoptimal choice of icon. (This is essentially what a lot of the crap in the favicon cache is all about solving).
A maximally tidy solution would be just to stick the loaded favicon into the favicon cache and then query the cache for an icon of the needed size - it'd then handle the sizing and whatnot for you, always ensuring you get the best one from the ICO you decoded.
Unfortunately, as you pointed out on IRC, we don't have a page URL for this - but this perhaps doesn't matter. You don't *really* need a page URL to use the favicon cache, you just need some string you can use as a cache key. Why not use the search engine's query URL? Then you could add a call to the cache before the attempt to decode the URI. The LRU properties of the cache would cause the icon to get dropped pretty soon after you leave the preferences screen and resume browsing, but it'd give you nice cache behaviour between visits to the settings page, as well as handily recycling the scaling logic.
Might prove nifty. Unclear. The alternative is a sort of irritating duplication of functionality to really find the largest favicon.

Attachment #8362302 - Flags: review?(chriskitching) → review-

Comment 8

•

10 years ago

(In reply to Richard Newman [:rnewman] from comment #5)
> Some improvements I'd like to see:
> 
> * browser.js queries history_with_favicons to fetch the icon out of the DB
> to use for a search engine. That's so stupid it hurts. We should be able to
> get the page favicon URL easily, and use that directly.

To HTTP GET the favicon directly? Or is the next bullet point the better approach?

> * If nothing on the Gecko side needs the page favicon, then computing this
> bundle, pushing it through Services.search, resulting in a message to Java
> which ultimately results in a base64 blob being decoded, is the height of
> inefficiency. We already have the page favicon *in memory and in the cache*
> on the Java side. Pass the damn tab ID.

Remember, this code existed before the favicon cache, so a refactor is worth looking into, but the inefficiency was less high when this originally landed. I assume "Pass the damn tab ID" means passing an ID to Java for the favicon, and return the favicon via JSON/data: URI? That sounds like a reasonable replacement for the SQLite async call. Not sure about using the tab ID though. Maybe the page URL?

Comment 9

•

10 years ago

See also: Bug 913746. This irritated me five months ago, too.

Comment 10

•

10 years ago

> I wonder if this means that someday we should move the ICO decoder out of
> Favicon land and into somewhere more image-centric.

I hope so. It was explicitly designed to be decoupled from the cache.

> Are you using "image/x-icon" as a generic image mime or are we explicitly
> asking for ICO now instead of PNG? "image/x-icon" seems to be the mime type
> for an ICO image. Some info on that here:
> http://en.wikipedia.org/wiki/Favicon

We store arbitrary data -- PNG or ICO -- into the DB. image/x-icon is a more accurate descriptor of these bytes than image/png. I explicitly didn't choose image/vnd.microsoft.ico because they won't always be ICO, and x-icon seems to be consensus for "this is some kind of favicon, probably ICO".

> Also: Based on this patch and the IRC chatter, I believe we are saving the
> favicons into the DB in the native format? So ICOs pulled from the web are
> saved as ICOs. Wouldn't it be better to save them in a less sucky format?
> Since we extract a single image out of the ICO, wouldn't saving as a PNG be
> nicer?

We extract multiple images from the ICO, depending on the requested size (e.g., small for URL bar, big for search engine, bigger for Top Sites) -- ICO is actually a fair approximation to our in-memory cache format, which supports multiple sizes for each icon. I'm actually hoping we move towards increased use of icon containers, rather than e.g., loading our pre-packaged icons out of a zip one size at a time.

We do compact and strip over-sized entries from favicons before we store them, so we're not going to be bloating the DB with giant variants.

Assignee

Comment 11

•

10 years ago

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

> > * browser.js queries history_with_favicons to fetch the icon out of the DB
> > to use for a search engine. That's so stupid it hurts. We should be able to
> > get the page favicon URL easily, and use that directly.
> 
> To HTTP GET the favicon directly?

We don't need to GET it; we already have it (both in the DB and in the cache, as well as in the Tab instance).

What we do here is:

* Ask the page for its URL.
* Ask the DB -- via an expensive view -- to find the favicon URL and thus the favicon data for that page URL.
* Encode that data in base64.
* Send it via JNI to Java.
* Decode the base64 into an in-memory bitmap, just like the one we are already using to display the tab.

We could easily kill the use of the view by querying the favicons table directly, if we got the favicon URL instead of the page URL (disregarding problems around guessed favicon URLs). We could avoid that also by getting the favicon URL from the Tab, or by consulting the cache. But moving all of this out of JS (my second suggestion) makes more sense to me.

> That sounds like a
> reasonable replacement for the SQLite async call. Not sure about using the
> tab ID though. Maybe the page URL?

That works too. The key is the shift in responsibility, from the component that doesn't already have the icon or the icon URL (and thus needs to hit the DB), to the component that needs to do zero work to get it.

Assignee

Comment 12

•

10 years ago

(In reply to Chris Kitching [:ckitching] from comment #7)

> > +        if (!uri.startsWith("data:image/")) {
> 
> This isn't sufficient to determine if the data URI you've got is parseable
> by your code.

True, but correct for the current caller, and I'd rather unbreak Nightly and improve in a follow-up. This is definitely a Sunday night patch.


> > +        try {
> > +            String base64 = uri.substring(offset);
> > +            byte[] raw = Base64.decode(base64, Base64.DEFAULT);
> > +            return decodeFavicon(raw);
> > +        } catch (Exception e) {
> 
> What exception do you imagine will be thrown here?

Runtime exceptions.

How about… OOM? A malformed favicon? A change in the behavior of decodeFavicon, where the callers weren't updated (happens all the time, believe me)?


> This can't happen. You either get a null LoadFaviconResult, or you get at
> least one entry in the iterator.

Belt and braces.


> This seems to rely on the assumption that the bitmaps in the iterator are in
> ascending order of size. They're not - no promises are made about the order
> of the bitmaps in the iterator.

You're right. This worked in my testing, but won't work 100% in the wild.


> Might prove nifty. Unclear. The alternative is a sort of irritating
> duplication of functionality to really find the largest favicon.

Yeah, that's why I didn't bother, and just took the last icon in the set. Seemed significantly easier than rejigging the whole `pushToCacheAndGetResult` stuff from LoadFaviconTask.

Comment 13

•

10 years ago

(In reply to Richard Newman [:rnewman] from comment #12)
> True, but correct for the current caller, and I'd rather unbreak Nightly and
> improve in a follow-up. This is definitely a Sunday night patch.

s/night/morning/ :P

> > What exception do you imagine will be thrown here?
> 
> Runtime exceptions.
> How about… OOM? 

OutOfMemoryError does not extend Exception, it extends Error (So your block won't actually catch an OOM). The idea behind the distinction is that Exceptions are things that can go wrong during the normal operation of a program (Such as bogus user input and whatnot) and Errors are things that are error conditions abnormal in the operation of any program (And usually shouldn't be caught).

You could catch OOMs here by changing Exception to Throwable, but then I'd probably have to kill you. :P

> A malformed favicon?

I appear to have been slightly dense in my earlier comment - the Javadoc for Base64 from Android reports that decode may throw IllegalArgumentException in the case of incorrect padding in the base64. Probably ought to catch that. *whistles innocently*.
FaviconDecoder, however, should not throw an exception in the case of malformed data. If the specification of your procedure is such that it should never throw an exception, then when it starts doing so you've found a bug. While hiding that exception may allow the procedure to continue to function most of the time in its broken state, you're liable to make the bug harder to find (and probably more likely to end up actually shipping it...)
That, or ending up with a tangled mess of patches solving the symptoms not the underlying problem, because nobody *found* the underlying problem as it was hidden away by a tangle of supposedly-redundant exception handlers.

Is it not better to catch the most specific exception possible at all points, so that unexpected ones don't get handled by a handler that was made with something else in mind? (That sort of handling seems like it could hide bugs).

> A change in the behavior of
> decodeFavicon, where the callers weren't updated (happens all the time,
> believe me)?

Then this exception handler will obscure a regression that some hypothetical future changeset creates!

> > This can't happen. You either get a null LoadFaviconResult, or you get at
> > least one entry in the iterator.
> 
> Belt and braces.

But.. How do you determine when to put in ostensibly-redundant checks of this sort, then? Surely the whole point of OOP (and, to a lesser extend, structured programming in general) is to enable you to selectively not think about things in certain contexts? What's the point of having certain pre-and-post-conditions on a procedure if you go ahead and behave as if they don't hold anyway?
Don't such redundant checks hide bugs in the method they're ostensibly-redundantly checking? If the behaviour of the decoder suddenly changes in a way that makes it violate its postconditions then you *really* want to know - not have your problems hidden by this check!

Software design debates on a Sunday. Sorry. But my feeling is that this sort of practice is liable to create significant future problems and, while the notion of entering "technical debt" to fix something quickly is often handy, doing so when the cost of doing it properly is essentially zero seems... questionable.

> > Might prove nifty. Unclear. The alternative is a sort of irritating
> > duplication of functionality to really find the largest favicon.
> 
> Yeah, that's why I didn't bother, and just took the last icon in the set.
> Seemed significantly easier than rejigging the whole
> `pushToCacheAndGetResult` stuff from LoadFaviconTask.

Easier, but less correct ;)

Comment 14

•

10 years ago

(In reply to Richard Newman [:rnewman] from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> 
> > > * browser.js queries history_with_favicons to fetch the icon out of the DB
> > > to use for a search engine. That's so stupid it hurts. We should be able to
> > > get the page favicon URL easily, and use that directly.
> > 
> > To HTTP GET the favicon directly?
> 
> We don't need to GET it; we already have it (both in the DB and in the
> cache, as well as in the Tab instance).
> 
> What we do here is:
> 
> * Ask the page for its URL.
> * Ask the DB -- via an expensive view -- to find the favicon URL and thus
> the favicon data for that page URL.
> * Encode that data in base64.

> * Send it via JNI to Java.

I don't see this step. I see the code use the Services.search XPCOM component to add the engine. Once we have the favicon, we keep if in JS land. I don't know of a better way to get the image to the Search service. We could file a bug to investigate ideas, but in the meantime, we can look to improve the fetching of the favicon.

> We could easily kill the use of the view by querying the favicons table
> directly, if we got the favicon URL instead of the page URL (disregarding
> problems around guessed favicon URLs). We could avoid that also by getting
> the favicon URL from the Tab, or by consulting the cache. But moving all of
> this out of JS (my second suggestion) makes more sense to me.

Unless I am missing something, this needs to involve JS. But +1 to improving the fetch.


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

> > Are you using "image/x-icon" as a generic image mime or are we explicitly
> > asking for ICO now instead of PNG? "image/x-icon" seems to be the mime type
> > for an ICO image. Some info on that here:
> > http://en.wikipedia.org/wiki/Favicon
> 
> We store arbitrary data -- PNG or ICO -- into the DB. image/x-icon is a more
> accurate descriptor of these bytes than image/png. I explicitly didn't
> choose image/vnd.microsoft.ico because they won't always be ICO, and x-icon
> seems to be consensus for "this is some kind of favicon, probably ICO".

I don't really agree with the consenus. That said, it's not really used in the patch so what could go wrong?

> 
> > Also: Based on this patch and the IRC chatter, I believe we are saving the
> > favicons into the DB in the native format? So ICOs pulled from the web are
> > saved as ICOs. Wouldn't it be better to save them in a less sucky format?
> > Since we extract a single image out of the ICO, wouldn't saving as a PNG be
> > nicer?
> 
> We extract multiple images from the ICO, depending on the requested size
> (e.g., small for URL bar, big for search engine, bigger for Top Sites) --
> ICO is actually a fair approximation to our in-memory cache format, which
> supports multiple sizes for each icon. I'm actually hoping we move towards
> increased use of icon containers, rather than e.g., loading our pre-packaged
> icons out of a zip one size at a time.
> 
> We do compact and strip over-sized entries from favicons before we store
> them, so we're not going to be bloating the DB with giant variants.

Saving different image formats almost necessitates the need to save a mime type or file extension or something. What method to we have for sniffing or guessing the file type? This info worries me a little.

Comment 15

•

10 years ago

Comment on attachment 8362302 [details] [diff] [review]
Correctly extract bitmaps from stored icons in search preferences.

I'm OK with this to unbreak Nightly, but I'd like to file some followup patches to move in a better direction. I think these followups include:
* Convert the "SQLite async fetch from DB" to a "JSON async fetch of favicon from Java".
* Think about storing a file type identifier with favicons. Thumbnails are always PNG for now.
* Uncouple the ICO Decoder from the Favicon code. It would be better if something accessible from BitmapUtils (or similar).

Others?

Attachment #8362302 - Flags: review?(mark.finkle) → review+

Assignee

Comment 16

•

10 years ago

(In reply to Chris Kitching [:ckitching] from comment #13)

> Is it not better to catch the most specific exception possible at all
> points, so that unexpected ones don't get handled by a handler that was made
> with something else in mind? (That sort of handling seems like it could hide
> bugs).

Within a module, yes.

...

> But.. How do you determine when to put in ostensibly-redundant checks of
> this sort, then? 

Once you cross a module boundary -- and I regard that as including calls in to the Favicon system and into Android -- I think it's a great idea to program defensively. At best, they catch a problem and turn it into an anticipated, detected error state; at worst they act as local documentation of preconditions.


> Surely the whole point of OOP (and, to a lesser extend,
> structured programming in general) is to enable you to selectively not think
> about things in certain contexts? What's the point of having certain
> pre-and-post-conditions on a procedure if you go ahead and behave as if they
> don't hold anyway?

Oh, absolutely. But most of that applies within the scope of code you already have in your head, or in front of you, or have thoroughly unit-tested.

Once you start gluing other modules together, one should typically start to re-verify those pre- and post-conditions, especially if those other modules aren't well-tested or might change, if you don't own them (*cough* Android) or if the checks are cheap.

You can also look at it this way: by including thorough exception handling and input verification, regardless of whether some other chunk of code miles away might have certain guarantees, you're able to "selectively not think about things" -- always assume that distant code is going to misbehave and lie, rather than constantly referring to its source code to determine what it's really going to do.


> Don't such redundant checks hide bugs in the method they're
> ostensibly-redundantly checking? If the behaviour of the decoder suddenly
> changes in a way that makes it violate its postconditions then you *really*
> want to know - not have your problems hidden by this check!

(a) We'll have tests, (b) we'll get bug reports about favicons not showing. I'd rather get the bug reports than the crash reports.

I acknowledge that that's a personal preference, and others prefer to have shipping code crash rather than behave in a degraded fashion.


> Easier, but less correct ;)

It's our motto.

Assignee

Comment 17

•

10 years ago

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

> > * Send it via JNI to Java.
> 
> I don't see this step. 

At some point we send the list of search engines over the wall to the SearchEnginePreference code:

01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAACv0lEQVR42sWX208TURDG958x8M+It5
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAEJGlDQ1BJQ0MgUHJvZmlsZQAAOBGFVd
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbW
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbW
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAACXBIWXMAAAsTAAALEwEAmpwYAAABZW
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAQAAADZc7J/AAAACXBIWXMAAAsTAAALEwEAmpwYAAADGG
01-19 16:27:16.363 I/FaviconDecoder(18305): Decoding data: URI: data:image/x-icon;base64,AAABAAYAgIAQAAAAAABoKAAAZgAAAEhIEAAAAAAA6A0AAM4oAAAwMBAAAAAAAGgGAAC2NgAAICA


> Saving different image formats almost necessitates the need to save a mime
> type or file extension or something.

Yeah, it'd be nice. But those types are sometimes wrong in the wild anyway.

> What method to we have for sniffing or
> guessing the file type? This info worries me a little.

The FaviconDecoder handles arbitrary content of various types -- if it might be a favicon, it'll do the right thing. That's the main reason why the MIME type stuff is an afterthought.

Comment 18

•

10 years ago

(In reply to Richard Newman [:rnewman] from comment #17)
> (In reply to Mark Finkle (:mfinkle) from comment #14)
> 
> > > * Send it via JNI to Java.
> > 
> > I don't see this step. 
> 
> At some point we send the list of search engines over the wall to the
> SearchEnginePreference code:

Oh my. That is light years from here though. And a different problem.

> > Saving different image formats almost necessitates the need to save a mime
> > type or file extension or something.
> 
> Yeah, it'd be nice. But those types are sometimes wrong in the wild anyway.
> 
> > What method to we have for sniffing or
> > guessing the file type? This info worries me a little.
> 
> The FaviconDecoder handles arbitrary content of various types -- if it might
> be a favicon, it'll do the right thing. That's the main reason why the MIME
> type stuff is an afterthought.

OK, so the "source of truth" mimetype is whatever the FaviconDecoder determines, which is fine. Again, pulling the sniffing out of FaviconDecoder is important. We use those stored images in many other places, and I'd rather not have to keep importing the FaviconDecoder to use the images.

Assignee

Comment 19

•

10 years ago

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

> OK, so the "source of truth" mimetype is whatever the FaviconDecoder
> determines, which is fine. Again, pulling the sniffing out of FaviconDecoder
> is important. We use those stored images in many other places, and I'd
> rather not have to keep importing the FaviconDecoder to use the images.

I don't think that's valid. We should have a single entry point to the favicon system, not be poking around in the DB and having ad-hoc versions of the FaviconDecoder pop up around the codebase. And we always want a Bitmap, which you can't get from the DB without reimplementing everything in FaviconDecoder anyway.

Want a favicon for some kind of URL? Ask Favicons. It does the right caching.

Want to decode favicon bytes? Ask FaviconDecoder. It's aware of all the right types, and is efficient. (But be aware that you probably shouldn't need to do this yourself).

Assignee

Comment 20

•

10 years ago

(In reply to Chris Kitching [:ckitching] from comment #7)

> This seems to rely on the assumption that the bitmaps in the iterator are in
> ascending order of size. They're not - no promises are made about the order
> of the bitmaps in the iterator.

I fixed this temporarily by actually doing the right thing, looking for a big-enough icon. But:

> A maximally tidy solution would be just to stick the loaded favicon into the
> favicon cache...

Two additional reasons to do this:

* We recompute the favicons from the data URI every time we display the list!
* We can't compute the dominant color unless the icon is in the cache.

Comment 21

•

10 years ago

(In reply to Richard Newman [:rnewman] from comment #20)
> * We can't compute the dominant color unless the icon is in the cache.

There's a bug for that.

Updated

•

10 years ago

Blocks: 961600

Assignee

Comment 22

•

10 years ago

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

Hardware: ARM → All

Target Milestone: --- → Firefox 29

Assignee

Comment 23

•

10 years ago

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

> * Uncouple the ICO Decoder from the Favicon code. It would be better if
> something accessible from BitmapUtils (or similar).

There are no source dependencies between FaviconDecoder and the rest of the Favicons code. So: the only coupling is having the package be org.mozilla.gecko.favicons.decoders.

Do you want this moved into a different package, or are you happy with it the way it is?

Flags: needinfo?(mark.finkle)

Carsten Book [:Tomcat]

Comment 24

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/726d0c10a5e3

Status: ASSIGNED → RESOLVED

Closed: 10 years ago

Resolution: --- → FIXED