1356440 - Favicons in bookmarks views are not updated on visit

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[atlanto]

Attachment: 20170413 bad

STR:
1. Start nightly with new profile
2. Create new bookmark ("New Bookmark..." on bookmarks sidebar)
3. Click the bookmark to open the page

AR:
The favicon in bookmarks sidebar is still default globe icon.

ER:
The favicon in bookmarks sidebar is updated to the site favicon immediatry.

workaround:
Close bookmarks sidebar and reopen it. (Ctrl+B twice)

Comment 1

[Marco Bonardo [:mak]]

Another plausible regression from bug 977177, to be investigated.

Comment 2

[Marco Bonardo [:mak]]

Fixing menus and the toolbar is probably trivial, looks like it's enough to remove and reset the image attribute.
The treeview is a bit more problematic because it has an internal image cache in nsTreebodyFrame::mImageCache and it doesn't expose a way to remove an entry from it. there is a ClearStyleAndImageCaches() but it's too broad.
I may just try to add a method to nsITreeBoxObject.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

Review commit: https://reviewboard.mozilla.org/r/131634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131634/

Comment 4

[Marco Bonardo [:mak]]

Blake, could you please check I didn't do anything stupid (or missed anything) in webidl?
Neil, please review the tree part.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

https://reviewboard.mozilla.org/r/131634/#review134650

This looks reasonable. I have one question. r=me with it addressed.

::: dom/webidl/TreeBoxObject.webidl:211
(Diff revision 1)
>    void clearStyleAndImageCaches();
> +
> +  /**
> +   * Remove an image source from the image cache to allow its invalidation.
> +   */
> +  void removeImageCacheEntry(long row, TreeColumn? col);

Why is `col` nullable? It looks like it's required so it seems better to make it non-null and throw a better error.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> > +  /**
> > +   * Remove an image source from the image cache to allow its invalidation.
> > +   */
> > +  void removeImageCacheEntry(long row, TreeColumn? col);
> 
> Why is `col` nullable? It looks like it's required so it seems better to
> make it non-null and throw a better error.

Good point, I actually just made it coherent with other APIs here, like isCellCropped, that make it nullable. but actually, isCellCropped will indeed throw if the column argument is null.
I'd be fine throwing, but I'm not sure if I can make the argument non-null considered this is just a 1:1 port of nsITreeBoxObject.idl that takes a simple pointer? It's the first time I touch webidl, let alone a webidl that is 1:1 of an xpidl :/ My suspect is that if I make this non-null the thing won't compile anymore because the implementation is in xpidl. Let me try.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131634/diff/1-2/

Comment 8

[Marco Bonardo [:mak]]

Blake, could you please check i properly addressed your comments? I didn't know I could differentiate webidl and provide a wrapper implementation for it.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I think this does look better. Thanks.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

https://reviewboard.mozilla.org/r/131634/#review134926

::: browser/components/places/tests/browser/browser_views_iconsupdate.js:47
(Diff revision 2)
> +  });
> +
> +  // The icon is read asynchronously from the network, we don't have an easy way
> +  // to wait for that.
> +  yield new Promise(resolve => {
> +    setTimeout(resolve, 3000);

Ouch. Can you run this test through try a few dozen times in every platform to make sure this won't become a frequent intermittent?

::: browser/components/places/tests/browser/browser_views_iconsupdate.js:71
(Diff revision 2)
> +  info("Before toolbar: " + toolbarShot1);
> +  info("After toolbar: " + toolbarShot2);

Do you really need to log the data URLs of the images in every run? I'm worried that it will needlessly delay the test and bloat the logs. I assume Assert.notEqual will log the two values if it fails, like isnot() does.

::: testing/modules/TestUtils.jsm:66
(Diff revision 2)
>        }, topic);
>      });
>    },
> +
> +  /**
> +   * Takes a screenshot of an area and returns it as a datauri.

Nit: "data URI"

::: testing/modules/TestUtils.jsm:85
(Diff revision 2)
> +    let ratio = win.devicePixelRatio;
> +    canvas.width = width * ratio;
> +    canvas.height = height * ratio;
> +    ctx.scale(ratio, ratio);
> +    ctx.drawWindow(win, left, top, width, height, "#fff");
> +    let data = canvas.toDataURL("image/png", "");

Why not just use canvas.toDataURL()? PNG is the default and the second argument should actually be a number (for JPEG).

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toDataURL#Parameters

Comment 11

[Marco Bonardo [:mak]]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #10)
> Do you really need to log the data URLs of the images in every run? I'm
> worried that it will needlessly delay the test and bloat the logs. I assume
> Assert.notEqual will log the two values if it fails, like isnot() does.

I'm not sure it will log the complete values, usually they log cropped values and then the result would be pointless. I'll check and see what I can do. Regardless these data uris are not so large

> Why not just use canvas.toDataURL()? PNG is the default and the second
> argument should actually be a number (for JPEG).

I'll check, I honestly just copied this call from devtools assuming it was good ;)

Comment 12

[Marco Bonardo [:mak]]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #10)
> Ouch. Can you run this test through try a few dozen times in every platform
> to make sure this won't become a frequent intermittent?

I retriggered the test more than 15 times on each platform so far, and didn't see any failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66617a11b2bab59efa7875bd32a890f7c39b4944

Comment 13

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Panos Astithas [:past] (please needinfo?) from comment #10)
> > Do you really need to log the data URLs of the images in every run? I'm
> > worried that it will needlessly delay the test and bloat the logs. I assume
> > Assert.notEqual will log the two values if it fails, like isnot() does.

Assert always truncates the values:
http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/testing/modules/Assert.jsm#80

Whether that's expected or not, I have no idea. Since this is only shown on failure I'd expect we should truncate only very long strings (more than 1024 chars), not at 128 chars. There may be a reason though, maybe related to treeherder. Also, bumping up the limit won't be a final solution.
I can probably workaround this by using deepEqual on arrays, or writing my own comparison with Assert.ok(false)

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131634/diff/2-3/

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Panos Astithas [:past] (please needinfo?) from comment #10)
> > Do you really need to log the data URLs of the images in every run? I'm
> > worried that it will needlessly delay the test and bloat the logs. I assume
> > Assert.notEqual will log the two values if it fails, like isnot() does.
> 
> I'm not sure it will log the complete values, usually they log cropped
> values and then the result would be pointless. I'll check and see what I can
> do. Regardless these data uris are not so large

I might have overestimated their length, it's fine if they aren't too long.

> > Why not just use canvas.toDataURL()? PNG is the default and the second
> > argument should actually be a number (for JPEG).
> 
> I'll check, I honestly just copied this call from devtools assuming it was
> good ;)

Ah, I see! The more commonly used responsive design version got this right at least!

Comment 16

[Neil Deakin]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

https://reviewboard.mozilla.org/r/131634/#review135358

::: layout/xul/tree/nsITreeBoxObject.idl:190
(Diff revision 3)
>    /**
>     * Called on a theme switch to flush out the tree's style and image caches.
>     */
>    void clearStyleAndImageCaches();
> +
> +  /**

I'm not an expert in this area but I believe the idl change is only needed if you call from c++.

Or leave it if you want consistency.

::: layout/xul/tree/nsTreeBodyFrame.cpp:4538
(Diff revision 3)
> +nsTreeBodyFrame::RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol)
> +{
> +  nsAutoString imageSrc;
> +  if (NS_SUCCEEDED(mView->GetImageSrc(aRowIndex, aCol, imageSrc))) {
> +    nsTreeImageCacheEntry entry;
> +    if (mImageCache.Get(imageSrc, &entry)) {

This only handles the image supplied by the view, yet nsTreeBodyFrame::GetImage also caches images for the checkboxes/twisties/etc via the style context.

This is probably ok, but we could at least document it in the interface documentation.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

https://reviewboard.mozilla.org/r/131634/#review135358

> I'm not an expert in this area but I believe the idl change is only needed if you call from c++.
> 
> Or leave it if you want consistency.

I think I will leave it, since all the APIs are consistent currently. I don't have good reasons to start forking them.

> This only handles the image supplied by the view, yet nsTreeBodyFrame::GetImage also caches images for the checkboxes/twisties/etc via the style context.
> 
> This is probably ok, but we could at least document it in the interface documentation.

I will document it, the use case I have in mind is indeed only affecting view images, I don't have good use cases for styling invalidation.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131634/diff/3-4/

Comment 19

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 13 changes to 13 files
remote: 
remote: WebIDL file dom/webidl/TreeBoxObject.webidl altered in changeset 3c31d8576b52 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote

Comment 20

[Marco Bonardo [:mak]]

hmm, Blake, you are a dom peer, so what's wrong here?

Comment 21

[Marco Bonardo [:mak]]

Well, I hit some parsing bug in the hook, I'll try a different commit string...

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8859623 [details]
Bug 1356440 - Favicons of bookmarks views don't update on visit. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131634/diff/4-5/

Comment 23

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 13 changes to 13 files
remote: 
remote: WebIDL file dom/webidl/TreeBoxObject.webidl altered in changeset 51c29fd8b1e3 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote

Comment 24

[Marco Bonardo [:mak]]

The problem is the + in "enndeakin+6102" it confuses the regex parsing the reviewers.
I'll file a bug against version-control.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d83d957fc7
Favicons of bookmarks views don't update on visit. r=mrbkap,past,enndeakin

Comment 26

[Marco Bonardo [:mak]]

I gave up with autoland, filed bug 1358608

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/04d83d957fc7

Comment 28

[Hani Yacoub, Desktop QA]

Build ID: 20170511063838
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.