Favicons in bookmarks views are not updated on visit

VERIFIED FIXED in Firefox 55

Status

()

defect
P1
minor
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: euthanasia_waltz, Assigned: mak)

Tracking

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Reporter

Description

2 years ago
Posted image 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)
Assignee

Comment 1

2 years ago
Another plausible regression from bug 977177, to be investigated.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee

Updated

2 years ago
Summary: Favicon in bookmarks sidebar is not updated immediatry → Favicon in bookmarks views is not updated immediatry
Assignee

Updated

2 years ago
Blocks: 1356525
Assignee

Comment 2

2 years ago
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Summary: Favicon in bookmarks views is not updated immediatry → Favicons in bookmarks views are not updated on visit
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
Blake, could you please check I didn't do anything stupid (or missed anything) in webidl?
Neil, please review the tree part.
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.
Attachment #8859623 - Flags: review?(mrbkap) → review+
Assignee

Comment 6

2 years ago
(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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8859623 - Flags: review?(enndeakin)
Assignee

Comment 8

2 years ago
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.
Flags: needinfo?(mrbkap)
Assignee

Updated

2 years ago
Blocks: 1355377
Yeah, I think this does look better. Thanks.
Flags: needinfo?(mrbkap)
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
Attachment #8859623 - Flags: review?(past) → review+
Assignee

Comment 11

2 years ago
(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 ;)
Assignee

Comment 12

2 years ago
(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
Assignee

Comment 13

2 years ago
(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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8859623 - Flags: review?(enndeakin)
(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

2 years ago
mozreview-review
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.
Attachment #8859623 - Flags: review?(enndeakin) → review+
Assignee

Comment 17

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 19

2 years ago
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
Assignee

Comment 20

2 years ago
hmm, Blake, you are a dom peer, so what's wrong here?
Flags: needinfo?(mrbkap)
Assignee

Comment 21

2 years ago
Well, I hit some parsing bug in the hook, I'll try a different commit string...
Flags: needinfo?(mrbkap)
Comment hidden (mozreview-request)

Comment 23

2 years ago
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
Assignee

Comment 24

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

Comment 25

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d83d957fc7
Favicons of bookmarks views don't update on visit. r=mrbkap,past,enndeakin
Assignee

Comment 26

2 years ago
I gave up with autoland, filed bug 1358608
https://hg.mozilla.org/mozilla-central/rev/04d83d957fc7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 28

2 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.