Closed Bug 636924 Opened 13 years ago Closed 13 years ago

Copied livemarks appear as folders in sidebar


(Firefox :: Bookmarks & History, defect)

Not set



Firefox 5


(Reporter: rnewman, Assigned: asaf)




(2 files, 1 obsolete file)

Follow on from Bug 387138.

Per sleep-deprived mak:

18:43:08 <@mak> nodeAnnotationChanged doesn't invalidate the row for livemark annotations
18:43:14 <@mak> in the treeview


* Open sidebar
* Find a livemark
* Copy
* Paste elsewhere in the tree view
* Observe that the livemark has a folder icon, not a livemark icon.
actually, could also depend on bug 636917.
Mano's going to look at this. For posterity:

08:14 <@dietrich> mak: bug 636924
08:14 <@dietrich> what annotation changed in that str?
08:14 <@mak> livemark/feedURI
08:16 <@mak> or better PlacesUtils.LMANNO_FEEDURI
08:22 <@dietrich> mak: why would that change for a copy/paste operation?
08:24 <@mak> I think we don't set the "livemark" atom on the node... but I didn't follow all the code, looks like it's not listening correctly to annotation changes. if Sync adds a livemark most likely will do the same as copy paste
08:24 <@mak> so, copy paste is anything like "add a livemark"
08:25 <@mak> menu and toolbar views on nodeAnnotationChanged set a "livemark" attribute, treeview should do something similar
08:26 <@mak> (not an attribute but an atom iirc)
Attached patch a patch I couldn't test myself (obsolete) — Splinter Review
For some reason, I cannot reproduce the bug here (Maybe mac trees invalidate too often).
Attachment #516372 - Flags: review?
Comment on attachment 516372 [details] [diff] [review]
a patch I couldn't test myself

wrong patch
Attachment #516372 - Flags: review?
Attachment #516372 - Attachment is obsolete: true
Attachment #516377 - Flags: review?(dietrich)
Comment on attachment 516377 [details] [diff] [review]
a patch I couldn't test myself

the change looks fine, r=me on that. however, we'll not get approval to land without a test for it. can add one? or update an existing test to check for this case?
Attachment #516377 - Flags: review?(dietrich) → review+
Dietrich, I cannot write a test for a bug I couldn't reproduce.  Actually, I can probably do write one - but it would alwyas pass on my mac, seems like.  Will you be able to test the test on a build in which the bug is reprocible?
Go ahead and add the test, and we'll have other people that can reproduce the bug confirm it.
the browser_views_liveupdate and browser_library_views_liveupdate tests could probably be updated to cover this.
(In reply to comment #9)
> the browser_views_liveupdate and browser_library_views_liveupdate tests could
> probably be updated to cover this.

fwiw, I'm looking into these tests update, if you have a separate test it is fine too. (btw, games are closing, so it's possible all of this slips to .next)
hm, I guess if this could rather be a bug in winstripe if Mano cannot reproduce on Mac. But who is setting the atom?
I have updated tests, they are fine for menu and toolbar, in the treeview case I use getCellProperties, but looks like it's not enough to test for the icon.
Attached patch test updatesSplinter Review
fwiw, the updated tests
Comment on attachment 516612 [details] [diff] [review]
test updates

Marco: I don't think it's bug in winstripe. A lot of things could force a redraw of the line at a certain timing.
Attachment #516612 - Flags: review+
Attachment #516377 - Flags: approval2.0?
Comment on attachment 516377 [details] [diff] [review]
a patch I couldn't test myself

Too late to make the RC. We could consider it for a ride-along if we have more RCs, but I'm not sure it's worth the noise at that point. Thanks for jumping in though - let's get it in the first stability update.
Attachment #516377 - Flags: approval2.0? → approval2.0-
Yeah, RC took less than expected (and it's a good thing after all), I'd vote for .next
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Flags: in-testsuite+
Target Milestone: --- → Firefox4.2
Verified on:
Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre
Target Milestone: Firefox5 → Firefox 5
You need to log in before you can comment on or make changes to this bug.