Closed Bug 1144571 Opened 9 years ago Closed 9 years ago

First bookmark is the last one (Cut - Paste).

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Version:
35 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Flags:
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: look997, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

patch v1.1
4.93 KB, patch
ttaubert
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

I cut several bookmarks.
Paste the bookmark to another empty folder.


Actual results:

First bookmark is the last one.
The rest is in the normal order.

Before:
Folder1
-bookmark1
-bookmark2
-bookmark3
-bookmark4

Folder2[empty]

After:
Folder1[empty]

Folder2
-bookmark2
-bookmark3
-bookmark4
-bookmark1


Expected results:

First bookmark is the first one.
The rest is in the normal order.

Before:
Folder1
-bookmark1
-bookmark2
-bookmark3
-bookmark4

Folder2[empty]

After:
Folder1[empty]

Folder2
-bookmark1
-bookmark2
-bookmark3
-bookmark4
Component: Untriaged → Bookmarks & History
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b00bdb144e06&tochange=a084c4cfd8a1

Suspect:
6ef192784187	Asaf Romano — Bug 1067954 - Async Places Transactions: Paste functionality. r=mak
Blocks: 1067954
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Places
Ever confirmed: true
Keywords: regression
Product: Firefox → Toolkit
Version: 36 Branch → 35 Branch
Mano could you please verify this is working properly in the new PT code?
Flags: needinfo?(mano)
I think the problem in the legacy code is:

let insertionIndex = ip.index + i;
should be
let insertionIndex = ip.index;

But I'm not sure why this was changed and why it differs from the new code.
We clearly need a test here.
Points: --- → 3
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
I'm taking this regression fix
Assignee: nobody → mak77
Flags: needinfo?(mano)
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Attached patch patch v1 (obsolete) — — Splinter Review 
This was an unintended change, the test will ensure we don't regress it again.
Attachment #8584559 - Flags: review?(ttaubert)
Attached patch patch v1.1 — — Splinter Review 
oops, the timeouts were not expected to be there.
Attachment #8584559 - Attachment is obsolete: true
Attachment #8584559 - Flags: review?(ttaubert)
Attachment #8584560 - Flags: review?(ttaubert)
Attachment #8584560 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/b7a9b5a6983b
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/b7a9b5a6983b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Comment on attachment 8584560 [details] [diff] [review]
patch v1.1

Approval Request Comment
[Feature/regressing bug #]: bug 1067954
[User impact if declined]: pasting multiple bookmarks messes up their order
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: very low risk, restoring code as it was before unintended change
[String/UUID change made/needed]: none
Attachment #8584560 - Flags: approval-mozilla-aurora?
Note, the approval is for FF38, if we merge before the approval, it will be beta, not aurora.
Attachment #8584560 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: