Note: There are a few cases of duplicates in user autocompletion which are being worked on.

move favicons blobs out of places.sqlite to their own database

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
3 years ago
8 days ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Depends on: 9 bugs, Blocks: 11 bugs, {addon-compat, dev-doc-needed, perf})

unspecified
mozilla55
addon-compat, dev-doc-needed, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, relnote-firefox 55+)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

(Assignee)

Description

3 years ago
Not just cause they take space slowing down any other db access, also cause that may help with future support of larger icons.
We should definitely investigate different storage options for favicons, or even let the cache handle them, like thumbnails.
Following is a Web paage which can be found by Google Search for "sqlite size limit".
> https://www.sqlite.org/limits.html
> The current implementation will only support a string or BLOB length up to 231-1 or 2147483647

Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller than it.
Is there any plan to use favicon larger than 2GB?

From perspective of API, simplest/easiest one is localStorage.
   localStorage is constructed on mozStorage in Firefox, and Table column is "scope, key, value" only, and scope(==URI of site) is  hidden by Firefox,
   and program can update key/value pair only using setItem(key,vlaue)/removeItem(key). (other API is getItem(key), clear() only).
   value is TEXT, so, if JavaScript object, JSON.stringify is needed, and escape, base64 ecoding etc. is needed if binary data(data: ,is bse64).
   If icon data: size is less than size limit of localStorage, localStorage is most easy-to-use one for "per site data".
   This is becuse localStorage is enhancement of Cookie which is "per site data".
Second one is IndexedDB. This is also a well-designed wrapper of mozStorage. Bigest difference from localStorage is "API is asynchrounus".
Third one is mozStorage, and 4-th one is "direct use of SQLite by code himself..

What is size of "future support of larger icons"?
Even if it exceeds "vlaue .size limit of localStorage", following is pretty easy.
   var icondata = base64 data od data: URI. 
   KeyRoot="favicon"; var remain=icondata lenth; var block=65534; var Strt=0; var CNT=0;
   while(0<remain){ 
        CNT++; var Key=KeyRoot+CNT;locaalStorage.setItem(Key,icondata.substr(Start,Math.min(remain,block));
       remain=remain-block; start=start+block;
   }
As far as data size is less than value size limit of SQLite DB column, "writing data to a column of SQLite DB" is one of simplest way of "safest file writing".
     localStorage.setItem8Key,JSON.strigify(Obj)) is far better than "safe file writing of JSON file".
Even if data size is larger than value size limit of SQLite DB column, "split it to multiple pieces" is not so difficult.

Where are you planning to move favicon data?
I think localStorge is best place to hold "per site data" which is irrelevant to history relevant data.
If XUL JavaScript, following can be used.
// Directly access from XUL with any URL
   var ios = Components.classes["@mozilla.org/network/io-service;1"]
             .getService(Components.interfaces.nsIIOService);
   var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
             .getService(Components.interfaces.nsIScriptSecurityManager);
   var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"]
             .getService(Components.interfaces.nsIDOMStorageManager);
get_localStorage = function (Site)
{
   var uri = ios.newURI(Site, "", null);
   var principal = ssm.getCodebasePrincipal(uri);
   var localStorage = dsm.getLocalStorageForPrincipal(principal, "");
   return localStorage;
}
var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value
var localStorage=get_localStorage(URI ) ;
localStorage.setItem("favicon", data of data: uri) ; localStorage.setItem("favicon-save-time", (new Date()).toString() );
(Assignee)

Comment 2

2 years ago
(In reply to WADA from comment #1)
> Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller
> than it.
> Is there any plan to use favicon larger than 2GB?

Nope.

> From perspective of API, simplest/easiest one is localStorage.

localStorage is main-thread only, we want to do any favicon work off the main thread.

> Second one is IndexedDB. This is also a well-designed wrapper of mozStorage.
> Bigest difference from localStorage is "API is asynchrounus".


Yes that is also a possibility, but far more complex than we need.
What we need is still to have all the blobs in a single place, because we have UI views that show many favicons at once, so having blobs separated (like in a file system circle buffer) would be too expensive (opening and managing file handles is expensive).

So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite. IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it.
(Assignee)

Updated

2 years ago
Summary: move favicons blobs out of places.sqlite → move favicons blobs out of places.sqlite to their own database
FYI.
moz_favicons in places.sqlite.
> CREATE TABLE moz_favicons (  id INTEGER PRIMARY KEY, url LONGVARCHAR UNIQUE,
        data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT)
guid is shared with moz_bookmarks?
> CREATE TABLE moz_bookmarks (  id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER,
>     title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT)

Mapping to localStorage.is pretty simple.
         moz_favicons + id + url => scope in localStorage,
         data => set of { key="favicon#"+1 to 2001,value=part#NN of base64 icon data }, or key="favicon",value=base64 icon data if Segments=1
                       + key="Segments",value=2001 + key="Size",value=2GB+800KB, key="BlockSize",value=1MB + key="LastSize",value=800KB
                       + key="guid",value=guid for me 
         mime-type => key=mime-type,value=mime-type of this icon
If guid is shared with moz_bookmarks.
         scope = "favicon://" + "moz_bookmarks" + ".guid" + ":9090"
         key=guid value,value=moz_favicons + id + url (pointer to scope for favicon)
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2)
> > From perspective of API, simplest/easiest one is localStorage.
> localStorage is main-thread only, we want to do any favicon work off the main thread.

What do you call "main-thread"?
Although localStorage is defined for JavaScript in HTML of Web site, there is no limitation  of localStoraage(==webappsstore.sqlite) use in Fx. "localStorage" is one of best Wrapper of mozStorage(==SQLite DB) with pretty simple/easy-to-use *synchrounous* API.
(IndexedDB is asynchrounous like mozStorage)
It's SQLite DB representatiion of var Obj={ A: 1, B:2, C:3, ...}.
    JavaScript Object : var dat = Obj["B"] ;                                   Obj["B"] = 1000;
    localStorage         : var dat = localStorage.getItem("B");        localStorage.setItem("B",1000);
It's a simplest one, and it's suitable to "per site data".
 
I think XUL code in Firefox is also a valid "Web Application". An XUL element can be considered "an Web site which is located in Firefox".
Please note that following is merely emulation of "window.loclStorage object creation in a window context for an URI".
> // Directly access from XUL with any URL
>    var ios = Components.classes["@mozilla.org/network/io-service;1"]
>              .getService(Components.interfaces.nsIIOService);
>    var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>              .getService(Components.interfaces.nsIScriptSecurityManager);
>    var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"]
>              .getService(Components.interfaces.nsIDOMStorageManager);
> get_localStorage = function (Site)
> {
>    var uri = ios.newURI(Site, "", null);
>    var principal = ssm.getCodebasePrincipal(uri);
>    var localStorage = dsm.getLocalStorageForPrincipal(principal, "");
>    return localStorage;
> }
> var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value
> var localStorage=get_localStorage(URI ) ;
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2)
> So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite.
> IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it.

Do you want to split many many BLOB data for big icons to different sqlite file dur to fragmentation in places.sqlire?
If so, I think it's not so effective because SQLite holds a entire table row as a BLOB data.
If collection of many big data, repeated inset/delete produces big unused spaces in sqlite fiile, but "used spce/unused space ratio" is usually doesn't depend on block size. It merely affect on "size of unused space. "places.sqlite + favicons.sqlite + ATTACH" usually doesn't reduce "total unused disk space size in sqlite.file(s)" and "used space/unused space ration"..

Is performance issue found in places.sqlite? Or merely many complaints of "size of places.sqlite file is too big for me!" is annoying?
Biggest volume of BLOB data in places.sqlite "data for favicon"?

A known issue in SQLite is VACUUM. "Split to multiple sqlite files(DBs)" may be a good idea for "VACUUM a sqlite file at once". This kind of work can be called "user side DB split by key range or data type for performance".
    Exaample. Split table for bookmarks to multiple sqlite file(DB) by key range(for examle, range of top level domain or hashed value of URL".
                      Split data for the key range to multiple sqlite fille(DB) : sqlite file(DB) for bookmark, and sqlite file(DB) for favicon(Your idea).
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #0)
> future support of larger icons.

"Page Thumnail as *icon* for a page" like one?

Pointer to row of central moz_places. Example of moz_bookmarks
> CREATE TABLE moz_bookmarks (
> id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER,
> title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT)
>   
>   id = "27" , fk = "24" , title = "Forum MozillaZine.jp Firefox"
>   colum named "flk" points id of row in moz_places.

Row in moz_favicons looks pointed only by " favicon_id" of moz_places. A kkind of "Normalization" is done properly.
> CREATE TABLE moz_places (
> id INTEGER PRIMARY KEY,
> url LONGVARCHAR, title LONGVARCHAR, (snip) 
> favicon_id INTEGER, (snip)
> 
>   id = "24" , uri = "http://forums.mozillazine.jp/viewforum.php?f=2"
>   title = "MozillaZine.jp Forum • Forum - Mozilla Firefox"
>   favicon_id = "19"
> 
> CREATE TABLE moz_favicons (
> id INTEGER PRIMARY KEY,
> url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT)
> 
>   id = "19" , uri = "http://forums.mozillazine.jp/favicon.ico" , data = "BLOB (Size: 608)"

If so, I think following is possible and not so hard, and it won't require big change.
  Seprated favicons.sqlite file(SQLite DB) for Table moz_favicons.
  Change definition of favicon_id = "19" of moz_places :
        id of row in Table=moz_favicons of same SQLite DB
     => id of row in Table=moz_favicons of different SQLite DB
     Because ATTACH is supported by SQLite as you say, this is not so hard.
     Consistency of "favicon_id of moz_places" can be kept by simple code in Places and/or FavIcon Manager.
       column of "users" of moz_favicons : Array of id of moz_places who referrs to this favicon.
       When moz_place uses this favico, add himself to "users".
       When moz_place stops using this favico, remove himself from "users". If no user, remove this favicon.
  Full page data, small icon for Tb, mid-size Thumnail for navigation, can be held at out side of important places.sqlite.
  If so, this DB can be used for data like Back&Forward Cache.
  If written to SQLite DB, JavaScrpt Object can be destroyed. It'll reduce memory consumption.

If SQLite DB is split, VACUUM of favicons.sqlite can be executed independently from VACUUM of places.sqlite. 
If SQLite DB is split, unbundling of "site meta data" and "site data" will be achived.
     places.sqlite    : for site meta data such as visitt history
     favicons.sqlite : for site data such as favicon, Thumnail, ...
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Keywords: perf
(Assignee)

Updated

2 years ago
Blocks: 492172
> or even let the cache handle them

Early versions of Mozilla just relied on the cache for favicons, but this meant that your bookmarks would randomly lose and regain their favicons as they expired from the cache / were recached, so that's not a solution I would recommend.
(Assignee)

Updated

a year ago
Blocks: 977163
(Assignee)

Updated

a year ago
Assignee: nobody → mak77
Priority: P2 → P1
(Assignee)

Updated

a year ago
Depends on: 1283083
(Assignee)

Updated

a year ago
Depends on: 1283825
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Blocks: 1337397
(Assignee)

Updated

5 months ago
Blocks: 1337402
(Assignee)

Updated

5 months ago
Blocks: 1337404
(Assignee)

Updated

5 months ago
Blocks: 1337409
(Assignee)

Updated

5 months ago
Depends on: 1337829
(Assignee)

Updated

5 months ago
Blocks: 1337858
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Blocks: 1346554
Status: NEW → ASSIGNED
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

4 months ago
I started posting 4 changesets that are the MVP.
This is not all the work, but it's complex enough to review, that I prefer starting sooner with that.

I plan still at least 2 changesets for this bug:
1. fallback to root icons if direct icon for a page is not found
2. more tests. I marked down parts where there's lack of tests and will write tests while the review proceeds.

In addition I need some additional testing:
3. Talos regressions
4. Migration of my default profile and verification of the results

Depending on the status of bug 1343499 I may decide to add proper ico files support here, or do that in the follow-up bug 1337402. The reason to do that here is mostly a better migration.

Comment 17

4 months ago
mozreview-review
Comment on attachment 8816669 [details]
Bug 977177 - Move favicons to a separate store.

https://reviewboard.mozilla.org/r/97320/#review125516

::: toolkit/components/places/Database.cpp:850
(Diff revision 3)
> -      rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> -          "PRAGMA synchronous = FULL"));
> +    MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA foreign_keys = ON")
> +  );
> -      NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +#ifdef DEBUG
> +  {

I think this could use a comment: you just set the pragma, so briefly explain why you're immediately checking whether it was set.

::: toolkit/components/places/FaviconHelpers.cpp:964
(Diff revision 3)
> +  nsCOMPtr<mozIStorageStatement> stmt;
> +  nsresult rv = mDB->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT id, width, data FROM moz_icons WHERE typeof(width) = 'text'"
> +  ), getter_AddRefs(stmt));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  if (NS_FAILED(rv)) return rv;

Nit: braces + new line as usual for if's

::: toolkit/components/places/FaviconHelpers.cpp:1017
(Diff revision 3)
> +                                           AcceptedMimeTypes::IMAGES)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // If it's an SVG, there's nothing to optimize or convert.
> +  if (aMimeType.EqualsLiteral(PNG_MIME_TYPE)) {

... this should be SVG_MIME_TYPE, right?
Attachment #8816669 - Flags: review?(adw) → review+

Comment 18

4 months ago
mozreview-review
Comment on attachment 8816670 [details]
Bug 977177 - Update favicons API consumers.

https://reviewboard.mozilla.org/r/97322/#review126032

This is probably one of the biggest patches I've ever reviewed, but I looked it over a couple of times, and I don't see any obvious problems.
Attachment #8816670 - Flags: review?(adw) → review+

Comment 19

4 months ago
mozreview-review
Comment on attachment 8848212 [details]
Bug 977177 - Add size ref fragment to icon protocols.

https://reviewboard.mozilla.org/r/121148/#review126034
Attachment #8848212 - Flags: review?(adw) → review+

Comment 20

4 months ago
mozreview-review
Comment on attachment 8848213 [details]
Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons.

https://reviewboard.mozilla.org/r/121150/#review126036
Attachment #8848213 - Flags: review?(adw) → review+

Comment 21

4 months ago
Nice work Marco (as always)!  I know you said there's more to do, but congratulations on these parts!
(Assignee)

Comment 22

4 months ago
Thanks, I'll shortly start working on the next parts.
Imagelib also got support for ico frames extraction, so I may add that on top before landing the migration.
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1337402
(Assignee)

Updated

4 months ago
Depends on: 1352408
(Assignee)

Comment 24

4 months ago
There's currently a blocking showstopper in bug 1352408, where I'm currently unable to update icons when they change, that basically means after a visit that loads a new icon, the UI won't update. I'm waiting for feedback from imagelib experts to evaluate what to do.
Apart from that, I'm code-complete and just refining the last unit tests and measuring Talos.
(Assignee)

Comment 25

4 months ago
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2fd5b4af803426d9d116d34f50e3f08ea6aa1e
Perfherder (still fetching results):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=80325998821e&newProject=try&newRevision=3a2fd5b4af80&framework=1&showOnlyImportant=0
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8853436 - Flags: review?(jmaher)
(Assignee)

Comment 35

4 months ago
Hi Brindusa, we are "close" to landing this (provided we are missing reviews and a dependency yet, before we can land, but close enough). It would be nice to start figuring out resources and a test plan for favicons functionality in the ui. The scope of the testing is to check that all the ui views showing favicons keep doing that.
Flags: needinfo?(brindusa.tot)

Comment 36

4 months ago
mozreview-review
Comment on attachment 8853436 [details]
Bug 977177 - Add favicons.sqlite to profile related lists.

https://reviewboard.mozilla.org/r/125528/#review128102

thanks for making sure talos was accounted for :)
Attachment #8853436 - Flags: review?(jmaher) → review+
Blocks: 1352459

Updated

4 months ago
Blocks: 1352502
(Assignee)

Comment 37

4 months ago
Talos doesn't show anything interesting apart from a possible improvement to tp5n nonmain_normal_fileio opt (e10s and not)
(Assignee)

Updated

4 months ago
Depends on: 1351163
(Assignee)

Comment 38

4 months ago
Remaining dependencies:

Bug 1351163 is about to change a bunch of code in the moz-anno protocol handler, I'll have to rebase on top of it
Bug 1352408 is confirmed as the reason why the UI icons don't update on visit, the image cache just returns the same payload if not invalidated.
Adding myself to QA Contact to verify this after it will get fixed.
Flags: needinfo?(brindusa.tot)
QA Contact: brindusa.tot
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 49

3 months ago
Latest version of the patches contains unbitrot for bug 1351163 and image cache fix using the patch in bug 
1352408.

Comment 50

3 months ago
mozreview-review
Comment on attachment 8816670 [details]
Bug 977177 - Update favicons API consumers.

https://reviewboard.mozilla.org/r/97322/#review130946

::: toolkit/components/places/nsAnnoProtocolHandler.cpp:132
(Diff revision 5)
>  
> +    if (!mData.IsEmpty()) {
> +      nsCOMPtr<nsIInputStream> stream;
> +      rv = NS_NewByteInputStream(getter_AddRefs(stream),
> +                                mData.get(), mData.Length(),
> +                                NS_ASSIGNMENT_DEPEND);

I'm not sure using NS_ASSIGNMENT_DEPEND here is safe, since this class instance isn't guaranteed to remain alive until the stream pump is finished. But since you're using a nsCString now rather than a raw byte buffer, you can just use `NS_NewCStringInputStream` instead, which is simpler and guarantees safe refcounting.

::: toolkit/components/places/nsAnnoProtocolHandler.cpp:137
(Diff revision 5)
> +      MOZ_DIAGNOSTIC_ASSERT(mListener);
> +      NS_ENSURE_TRUE(mListener, NS_ERROR_UNEXPECTED);

We should probably move this toward the top of the HandleCompletion method, since also use it further on, and we no longer have the `!mListener` check at the start of the method.
(Assignee)

Comment 51

3 months ago
(In reply to Kris Maglione [:kmag] from comment #50)
> Comment on attachment 8816670 [details]
> Bug 977177 - Update favicons API consumers.
> 
> https://reviewboard.mozilla.org/r/97322/#review130946
> 
> ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:132
> (Diff revision 5)
> >  
> > +    if (!mData.IsEmpty()) {
> > +      nsCOMPtr<nsIInputStream> stream;
> > +      rv = NS_NewByteInputStream(getter_AddRefs(stream),
> > +                                mData.get(), mData.Length(),
> > +                                NS_ASSIGNMENT_DEPEND);
> 
> I'm not sure using NS_ASSIGNMENT_DEPEND here is safe, since this class
> instance isn't guaranteed to remain alive until the stream pump is finished.

Ah ops, the next changeset changes it to COPY!
I just merged the change into the wrong changeset...

> But since you're using a nsCString now rather than a raw byte buffer, you
> can just use `NS_NewCStringInputStream` instead, which is simpler and
> guarantees safe refcounting.

Sounds good.

> ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:137
> (Diff revision 5)
> > +      MOZ_DIAGNOSTIC_ASSERT(mListener);
> > +      NS_ENSURE_TRUE(mListener, NS_ERROR_UNEXPECTED);
> 
> We should probably move this toward the top of the HandleCompletion method,
> since also use it further on, and we no longer have the `!mListener` check
> at the start of the method.

Yep, thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 60

3 months ago
mozreview-review
Comment on attachment 8853434 [details]
bug 977177 - Fallback to the root domain icon.

https://reviewboard.mozilla.org/r/125524/#review131146
Attachment #8853434 - Flags: review?(adw) → review+

Comment 61

3 months ago
mozreview-review
Comment on attachment 8853435 [details]
Bug 977177 - Split ico files into native frames.

https://reviewboard.mozilla.org/r/125526/#review131148
Attachment #8853435 - Flags: review?(adw) → review+

Comment 62

3 months ago
mozreview-review
Comment on attachment 8853436 [details]
Bug 977177 - Add favicons.sqlite to profile related lists.

https://reviewboard.mozilla.org/r/125528/#review131150
Attachment #8853436 - Flags: review?(adw) → review+

Comment 63

3 months ago
mozreview-review
Comment on attachment 8853437 [details]
Bug 977177 - Don't expire root domain icons with history, and don't associate pages to them.

https://reviewboard.mozilla.org/r/125530/#review131152
Attachment #8853437 - Flags: review?(adw) → review+

Comment 64

3 months ago
mozreview-review
Comment on attachment 8853438 [details]
Bug 977177 - Invalidate the page-icon image cache when necessary.

https://reviewboard.mozilla.org/r/125532/#review131154
Attachment #8853438 - Flags: review?(adw) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Blocks: 1355780

Comment 74

3 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/942aa1533e08
Move favicons to a separate store. r=adw
https://hg.mozilla.org/integration/autoland/rev/90d755bcbb92
Update favicons API consumers. r=adw
https://hg.mozilla.org/integration/autoland/rev/4a0770d810dc
Add size ref fragment to icon protocols. r=adw
https://hg.mozilla.org/integration/autoland/rev/60002894a42b
Expire old page to icon relations to avoid serving deprecated icons. r=adw
https://hg.mozilla.org/integration/autoland/rev/62f3fc3cb351
Fallback to the root domain icon. r=adw
https://hg.mozilla.org/integration/autoland/rev/864e72c60156
Split ico files into native frames. r=adw
https://hg.mozilla.org/integration/autoland/rev/5311e5ac3b4b
Add favicons.sqlite to profile related lists. r=adw,jmaher
https://hg.mozilla.org/integration/autoland/rev/42df4b3da073
Don't expire root domain icons with history, and don't associate pages to them. r=adw
https://hg.mozilla.org/integration/autoland/rev/d73a295b3652
Invalidate the page-icon image cache when necessary. r=adw
(Assignee)

Updated

3 months ago
Keywords: addon-compat, dev-doc-needed

Comment 75

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/942aa1533e08
https://hg.mozilla.org/mozilla-central/rev/90d755bcbb92
https://hg.mozilla.org/mozilla-central/rev/4a0770d810dc
https://hg.mozilla.org/mozilla-central/rev/60002894a42b
https://hg.mozilla.org/mozilla-central/rev/62f3fc3cb351
https://hg.mozilla.org/mozilla-central/rev/864e72c60156
https://hg.mozilla.org/mozilla-central/rev/5311e5ac3b4b
https://hg.mozilla.org/mozilla-central/rev/42df4b3da073
https://hg.mozilla.org/mozilla-central/rev/d73a295b3652
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1356079
(Assignee)

Comment 76

3 months ago
As expected, based on the previous Try measurement, this shown a positive improvement to tp5n nonmain_normal_fileio of about 12-13% most likely due to not associating anymore root domain icons to pages.
It's likely we may re-use some of this improvement in the future to store multiple icons for each page in bug 1352459.
(Assignee)

Updated

3 months ago
Blocks: 615602
Great work! Here are the improvements stats:

== Change summary for alert #5970 (as of April 12 2017 14:59 UTC) ==

Improvements:

 14%  tp5n nonmain_normal_fileio windows7-32 pgo e10s     474983808.04 -> 409410164.67
 13%  tp5n nonmain_normal_fileio windows7-32 opt e10s     477240408.25 -> 414585120.08
 13%  tp5n nonmain_normal_fileio windows7-32 opt          470541926.33 -> 409490063.17
 13%  tp5n nonmain_normal_fileio windows7-32 pgo          470547722.25 -> 410897176.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5970
(Assignee)

Updated

3 months ago
Blocks: 1356220
(Assignee)

Updated

3 months ago
Depends on: 1356435
(Assignee)

Updated

3 months ago
Depends on: 1356440
Depends on: 1356525
(Assignee)

Updated

3 months ago
Blocks: 1356531
(Assignee)

Updated

3 months ago
Depends on: 1356567

Updated

3 months ago
Depends on: 1356597
(Assignee)

Comment 78

3 months ago
Release Note Request (optional, but appreciated)
[Why is this notable]: non backwards compatible change
[Affects Firefox for Android]: no
[Suggested wording]: Favicons have been moved to a new data store to allow using higher resolution icons. It's not possible to downgrade a newly created profile to a previous Firefox version.
[Links (documentation, blog post, etc)]: not yet, I may probably write a blog post in the next week.
relnote-firefox: --- → ?

Updated

3 months ago
Depends on: 1356845
No longer depends on: 1356845
(Assignee)

Updated

3 months ago
Depends on: 1357555
(Assignee)

Updated

3 months ago
Blocks: 1358791
Depends on: 1359456
Depends on: 1359487
Depends on: 1360158
Depends on: 1362346

Updated

3 months ago
Depends on: 1362628

Updated

2 months ago
Depends on: 1363620

Updated

2 months ago
Depends on: 1363621

Updated

2 months ago
Depends on: 1363622

Updated

2 months ago
Depends on: 1368442

Updated

2 months ago
Depends on: 1368451

Updated

2 months ago
Depends on: 1368677

Updated

2 months ago
Depends on: 1368683
Depends on: 1371607
Depends on: 1371696

Updated

a month ago
Depends on: 1371702
(Assignee)

Updated

23 days ago
Depends on: 1376149
This is in the release notes as "Breaking profile changes - do not downgrade Firefox and use a profile that has been opened with Firefox 55+"
relnote-firefox: ? → 55+

Comment 80

8 days ago
It seems like there's a minor issue. When I use the new bookmark feature, while I'm editing it a favicon from one of the websites is shown, it only changes to an empty one when I actually add the bookmark.
(Assignee)

Comment 81

8 days ago
that sounds like old bug 946820.

Comment 82

8 days ago
(In reply to Marco Bonardo [::mak] from comment #81)
> that sounds like old bug 946820.

Similar, but to me it only happens while the new bookmark dialog is open. (And I was using the sidebar)
You need to log in before you can comment on or make changes to this bug.