731563 - Markers in places popup views are broken and too much bugs-prone, causing broken queries

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/7ce4d9b55863
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120228 Firefox/13.0a1 ID:20120228031102

Smart Bookmarks is broken.
After click reload, Items are doubled.

Reproducible: Always


Steps to Reproduce:
1. Start Firefox with new profile
2. App bottun > Bokmarks > Bookmarks – Recently Bookmarked
   -- remember number of items.
3. Clcik reload button
4. App bottun > Bokmarks > Bookmarks – Recently Bookmarked


Actual Results:
  Items are doubled.

Expected Results:
  should not

Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/2b1a53905350
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120224 Firefox/13.0a1 ID:20120224133104
Fails:
http://hg.mozilla.org/mozilla-central/rev/ce20e9b47e9c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120225 Firefox/13.0a1 ID:20120225031723
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2b1a53905350&tochange=ce20e9b47e9c


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e2ee770cc461
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120224 Firefox/13.0a1 ID:20120224041330
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7925703fbce6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120224 Firefox/13.0a1 ID:20120224044430
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e2ee770cc461&tochange=7925703fbce6

Suspected: Bug 613588

Comment 1

[Alice0775 White]

@jrudkin also reports in mozillazine http://forums.mozillazine.org/viewtopic.php?p=11782043#p11782043

Comment 2

[Marco Bonardo [:mak]]

thanks, I can reproduce

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

oviously what broke is the markers, as usual.
I've seen far too many bogus behaviors with them, so I finally decided to do what I planned from quite some time, rewrite them as hidden elements. This way they just move when adding or removing other content, and we can insertBefore them.

Mano, I still have to test this on Mac, though asking review cause this builds on top of bug 730798 and I don't want to confuse you.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 602086 [details] [diff] [review]
patch v1.0

ehr, indeed this is more a feedback than a review, not ready for review.

Comment 5

[Marco Bonardo [:mak]]

good news, this fixed some of the brokeness I saw in bug 730798, though some popups show scrollarows on mac when there should not be.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

tested on win and mac and passes our b-c tests. I coulnd't ask more.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 602128 [details] [diff] [review]
patch v1.1

a couple bugs still, updating patch

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

includes most of the stuff discussed on irc

Comment 9

[Asaf Romano (gone)]

Comment on attachment 603330 [details] [diff] [review]
patch v1.0

r=mano. nits:

1. Move the markers s/at/to/  the right position.
2. Please referenced the now-filed relevant mac menu bug,
3. It's name is "OS X" ;)

I mean, r=mano. wow.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa498cf8658

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5aa498cf8658

Comment 14

[Marco Bonardo [:mak]]

This may have some effect on add-ons that wrongly use private _startMarker and _endMarker properties of Places popups, or directly use the childNodes count. The change is that instead of being numeric indices, those are now elements of the popup. So adding the keyword.