Last Comment Bug 753672 - Dragging and dropping a bookmark in Chevron makes wrong order.
: Dragging and dropping a bookmark in Chevron makes wrong order.
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Marco Bonardo [::mak] (Away 6-20 Aug)
:
Mentors:
Depends on:
Blocks: 731563 734646
  Show dependency treegraph
 
Reported: 2012-05-10 00:56 PDT by Alice0775 White
Modified: 2012-05-31 06:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
json backup if necessary (3.79 KB, application/json)
2012-05-10 01:12 PDT, Alice0775 White
no flags Details
patch v1.0 (1.45 KB, patch)
2012-05-10 16:13 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
no flags Details | Diff | Splinter Review
patch v1.0 (1.60 KB, patch)
2012-05-10 16:14 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
asaf: review+
Details | Diff | Splinter Review

Description Alice0775 White 2012-05-10 00:56:28 PDT
Build Identifier:
http://hg.mozilla.org/releases/mozilla-beta/rev/168f3814b32e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0 ID:20120501201020

Dragging and dropping a bookmark in Chevron makes wrong order.

Bug 731563 and bug 734646 triggered this problem.

Reproducible: Always

Steps to Reproduce:
1. Add bookmarks and shrink window width so that chevron popup contains as follows
aaaaaaaaaaaaaaaaaaaaaaa
bbbbbbbbbbbbbbbbbbbbbbb
ccccccccccccccccccccccc

2. Drag and drop ccccccccccccccccccccccc to between aaaaaaaaaaaaaaaaaaaaaaa and bbbbbbbbbbbbbbbbbbbbbbb

Actual Results:
bbbbbbbbbbbbbbbbbbbbbbb
ccccccccccccccccccccccc
aaaaaaaaaaaaaaaaaaaaaaa

Expand window width, you can see correct order.
aaaaaaaaaaaaaaaaaaaaaaa ccccccccccccccccccccccc bbbbbbbbbbbbbbbbbbbbbbb


Expected Results:  
aaaaaaaaaaaaaaaaaaaaaaa
ccccccccccccccccccccccc
bbbbbbbbbbbbbbbbbbbbbbb
Comment 1 Alice0775 White 2012-05-10 00:57:53 PDT
This problem also exists in Aurora14.0a2 and Nigtly15.0a1.
Comment 2 Alice0775 White 2012-05-10 01:12:25 PDT
Created attachment 622651 [details]
json backup if necessary
Comment 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-10 15:37:40 PDT
The problem is this code:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#1245
that is using wrong indices (with the _startMarker those should be increased by 1), making a patch that uses the right indices is trivial.

Though, the fact is that I don't see why we need that code at all, the chevron view should update quite fine by itself, indeed commenting it out everything works like a charm. I wonder if it's just something inherited from a past where the chevron was not a view. Taking a deeper look at old blame, but I think I'll just remove that code...
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-10 15:59:10 PDT
Funny, I added that code in bug 418671, like 4 years ago.  The reason is updateChevron was synchronous at that time, the toolbar gets the nodeMoved before the chevron, and calls updateChevron, then the menu gets nodeMoved and does the actual nodes exchange, at this point in the old code updateChevron had made its work already, so the visible/hidden nodes status was out of sync.

Now instead, updateChevron is fired on a 100ms timer, this means by the time it runs the menu already got nodeMoved, so the proper nodes are made visible/hidden.
I don't really think we plan to go back to the synchronous behavior, so I'll remove the code and stick a nice comment about what we should do and why we don't need to.
Comment 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-10 16:13:35 PDT
Created attachment 622956 [details] [diff] [review]
patch v1.0

I think this is actually even better than the old approach, the menu view has better knowledge of itself than someone else.
I also think making a test here would be tricky, not impossible but the time to make it proper is not worth it.
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-10 16:14:25 PDT
Created attachment 622957 [details] [diff] [review]
patch v1.0

sorry, attached the queue patch instead of the 8 lines of context ones.
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-30 07:15:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80c83eefc1f
Comment 8 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-30 07:17:29 PDT
I'm not going to ask approval here, even if this would be a safe fix, there is not need to rush it since there is dataloss, just some broken ordering (and it's easy to fix the order in the library or sidebar)
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-05-30 07:17:42 PDT
"since there is NO dataloss"
Comment 10 Ed Morley [:emorley] 2012-05-31 06:16:50 PDT
https://hg.mozilla.org/mozilla-central/rev/d80c83eefc1f

Note You need to log in before you can comment on or make changes to this bug.