Closed Bug 1398471 Opened 7 years ago Closed 7 years ago

Impossible to move more than 1 selected bookmark in Library after landing patches from bug #1199729

Categories

(Core :: DOM: Events, defect, P1)

57 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: Virtual, Assigned: nika)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, regression, reproducible)

Attachments

(1 file)

[Tracking Requested - why for this release]: Regression


STR:
1. Move more than 1 selected bookmark to other folder in Library
and see that it doesn't work


Caused by:
bug #1199729

Regression commit message:
Part 7: Add manual web-platform-tests for Drag Data Store status, r=baku

Regression pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=5a69e957b1986e6c2e8ae073dce7c8db5c2f5d9d&full=1
Flags: needinfo?(michael)
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: Impossible to move more than 1 selected bookmark to other folder in Library after landing patches from bug #1199729 → Impossible to move more than 1 selected bookmark in Library after landing patches from bug #1199729
Previously the code looped and yielded between each loop. This caused the
DataTransfer to be cleared before the read completed.

This splits the loop into 2 sections such that we read all important data from
the DataTransfer before it is cleared.
Attachment #8906691 - Flags: review?(amarchesini)
Flags: needinfo?(michael)
Flags: needinfo?(amarchesini)
Assignee: nobody → michael
Status: NEW → ASSIGNED
Comment on attachment 8906691 [details] [diff] [review]
Read data from DataTransfer before yielding

Review of attachment 8906691 [details] [diff] [review]:
-----------------------------------------------------------------

I cannot review this code.
Attachment #8906691 - Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
Comment on attachment 8906691 [details] [diff] [review]
Read data from DataTransfer before yielding

Review of attachment 8906691 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please can you file a followup bug to add a test for this functionality so we don't regress it again?

::: browser/components/places/content/controller.js
@@ +1597,5 @@
>      let duplicable = new Map();
>      duplicable.set(PlacesUtils.TYPE_UNICODE, new Set());
>      duplicable.set(PlacesUtils.TYPE_X_MOZ_URL, new Set());
>  
> +    let dtItems = [];

Can you add a code comment above this along the lines of bug comment 1, maybe "Extract all the dragged data synchronously. If we do it asynchronously, the dragdata will get cleared once we yield to the event loop" or something along those lines. Hopefully that will avoid someone "optimizing" this later and breaking it again.
Attachment #8906691 - Flags: review?(gijskruitbosch+bugs) → review+
Also, what are the odds there are other "multiple drag" operations that were regressed?
(In reply to :Gijs from comment #4)
> Also, what are the odds there are other "multiple drag" operations that were
> regressed?

There's a chance that we could have made other mistakes - I'm actually disabling this change on the DnD side for 57 in bug 1398883 to reduce the risk for 57, and we can look into fixing it up later / making additional changes after then.
Priority: -- → P1
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/972346f3252c
Read data from DataTransfer before yielding, r=gijs
https://hg.mozilla.org/mozilla-central/rev/972346f3252c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-09-14), so I'm marking this bug as VERIFIED. Thank you very much! \o/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.