1190350 - Livemarks display regular bookmark icons on the sidebar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Windows 10 (Fx40 vs Fx39)

Reproducible on:
* Beta 40.0b9 (20150730171029)

Affected platforms:
* Windows 10 Pro x64 (10240)
* Mac OS X 10.10.4
* Ubuntu 14.04 (x64)

Steps to reproduce:
1. Launch Firefox.
2. Subscribe to a few RSS feeds.
3. Enable the Bookmarks Sidebar and check the icons displayed for each of those feeds/livemarks.

Expected result:
Livemarks display their dedicated RSS icons in the sidebar.

Actual result:
A regular bookmark folder icon is displayed for the Livemarks in the sidebar.

Notes:
* This is NOT reproducible on Firefox 39.0 (20150630154324).
* If you're subscribing to an rss with the sidebar opened, the icon will be shown properly for it, but after a restart it will show the regular one instead - see  http://i.imgur.com/KJuLLLi.png.
* I'll follow up with a regression range as soon as possible.

Comment 1

[:Gijs (he/him)]

Did/can you figure out what regressed this?

Comment 2

[Andrei Vaida [:avaida]]

(In reply to :Gijs Kruitbosch from comment #1)
> Did/can you figure out what regressed this?

This is a really old regression, I've managed to track it back to Firefox 23 nightly builds from 2013-05-04, I think that at this point, going further with the bisection wouldn't provide any useful data.

Comment 3

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:avaida] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Did/can you figure out what regressed this?
> 
> This is a really old regression, I've managed to track it back to Firefox 23
> nightly builds from 2013-05-04, I think that at this point, going further
> with the bisection wouldn't provide any useful data.

This doesn't make sense, comment #0 says it doesn't reproduce with Firefox 39. Which is it?

Comment 4

[Andrei Vaida [:avaida]]

(In reply to :Gijs Kruitbosch from comment #3)
> This doesn't make sense, comment #0 says it doesn't reproduce with Firefox
> 39. Which is it?

What I meant to say is that after a more thorough investigation of this issue, it turns it's reproducible all the way back to Firefox 23, including Firefox 39 :). So Comment 2 stands.

Comment 5

[atlanto]

diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -1160,17 +1160,17 @@ PlacesTreeView.prototype = {
                  nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {
           if (this._controller.hasCachedLivemarkInfo(node)) {
             properties += " livemark";
           }
           else {
             PlacesUtils.livemarks.getLivemark({ id: node.itemId })
               .then(aLivemark => {
                 this._controller.cacheLivemarkInfo(node, aLivemark);
-                properties += " livemark";
+                this._cellProperties.set(node, undefined);
                 // The livemark attribute is set as a cell property on the title cell.
                 this._invalidateCellValue(node, this.COLUMN_TYPE_TITLE);
               }, () => undefined);
           }
         }
 
         if (itemId != -1) {
           let queryName = PlacesUIUtils.getLeftPaneQueryNameFromId(itemId);

Comment 6

[:Gijs (he/him)]

(In reply to atlanto from comment #5)
> diff --git a/browser/components/places/content/treeView.js
> b/browser/components/places/content/treeView.js
> --- a/browser/components/places/content/treeView.js
> +++ b/browser/components/places/content/treeView.js
> @@ -1160,17 +1160,17 @@ PlacesTreeView.prototype = {
>                   nodeType ==
> Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {
>            if (this._controller.hasCachedLivemarkInfo(node)) {
>              properties += " livemark";
>            }
>            else {
>              PlacesUtils.livemarks.getLivemark({ id: node.itemId })
>                .then(aLivemark => {
>                  this._controller.cacheLivemarkInfo(node, aLivemark);
> -                properties += " livemark";
> +                this._cellProperties.set(node, undefined);
>                  // The livemark attribute is set as a cell property on the
> title cell.
>                  this._invalidateCellValue(node, this.COLUMN_TYPE_TITLE);
>                }, () => undefined);
>            }
>          }
>  
>          if (itemId != -1) {
>            let queryName = PlacesUIUtils.getLeftPaneQueryNameFromId(itemId);

I don't know enough about places to figure out if this would fix the issue and why (and if the other properties += " livemark" also needs updating ?)

Comment 7

[Marco Bonardo [:mak]]

the proposed change is not exactly right, but it hits the nail on the head.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1

sorry for multiple review requests today, it's trivial stuff though (and I'm done with this for now)

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

stupid copy paste typo.

Comment 10

[Pulsebot]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a7eb74116e1b

Comment 12

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 42.0a1 (Build: 20150803030207)on 
windows 8.1 pro 64-bit with the instructions from comment 0 .

Verified as fixed with Latest Firefox Beta 44.0b7 (Build ID: 20160107144911)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0