1473029 - Don't use nsScriptableRegion within drag services

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement, P3)

Comment 1

[Neil Deakin]

Attachment: Part 1 - move the tree drag region handling into nsBaseDragService::InvokeDragSessionWithImage

Comment 2

[Neil Deakin]

Attachment: Part 4, have nsTreeBodyFrame::GetSelectionRegion use nsIntRegion instead of nsIScriptableRegion

Comment 3

[Neil Deakin]

Attachment: Part 3 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

Comment 4

[Neil Deakin]

Attachment: Part 2 - remove the region arguments from InvokeDragSession and InvokeDragSessionWithImage

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8989438 [details] [diff] [review]
Part 3 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

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

::: widget/nsBaseDragService.cpp
@@ +258,5 @@
>    // are in the wrong coord system, so turn off mouse capture.
>    nsIPresShell::ClearMouseCapture(nullptr);
>  
>    nsresult rv = InvokeDragSessionImpl(aTransferableArray,
> +                                      mRegion.IsEmpty() ? nullptr : &mRegion,

Could you make mRegion a Maybe<>, instead of treating empty regions specially? And then you could make this parameter a "const Maybe<CSSIntRegion>& aRegion" and wouldn't need to pass around a raw pointer.

@@ +651,5 @@
>      CSSIntRect dragRect;
>      if (aRegion) {
>        // the region's coordinates are relative to the root frame
> +      IntRect bounds = aRegion->GetBounds();
> +      dragRect.SetRect(bounds.x, bounds.y, bounds.width, bounds.height);

This indicates that the coordinates in the region are CSS int pixels. Could you make the region a CSSIntRegion?

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8989437 [details] [diff] [review]
Part 4, have nsTreeBodyFrame::GetSelectionRegion use nsIntRegion instead of nsIScriptableRegion

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

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +85,5 @@
>    nsresult GetTreeBody(mozilla::dom::Element **aElement);
>    int32_t RowHeight() const;
>    int32_t RowWidth();
>    int32_t GetHorizontalPosition() const;
> +  void GetSelectionRegion(nsIntRegion& aRegion);

Instead of using an outparam, could you make this return a Maybe<CSSIntRegion>?

Comment 7

[Neil Deakin]

Attachment: Part 3.2 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

Comment 8

[Neil Deakin]

Attachment: Part 4.2 - have nsTreeBodyFrame::GetSelectionRegion use nsIntRegion instead of nsIScriptableRegion

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8997019 [details] [diff] [review]
Part 3.2 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

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

Is the mutating aRegion->MoveBy call in PresShell::RenderNode the only reason why the parameters are not const references? I think it would be better to make a copy there, instead of mutating the region that the caller passes in. And then you could change everything to "const Maybe<CSSIntRegion>& aRegion".

Comment 10

[Neil Deakin]

Attachment: Part 3.3 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8997488 [details] [diff] [review]
Part 3.3 - convert drag services to internally use nsIntRegion instead ns nsIScriptableRegion

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

Looks great, thanks!

::: widget/nsBaseDragService.cpp
@@ +638,5 @@
>      CSSIntRect dragRect;
>      if (aRegion) {
>        // the region's coordinates are relative to the root frame
> +      CSSIntRect bounds = aRegion->GetBounds();
> +      dragRect.SetRect(bounds.x, bounds.y, bounds.width, bounds.height);

I think this can become
dragRect = aRegion->GetBounds();

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7b40644b111
move the tree drag region handling into nsBaseDragService::InvokeDragSessionWithImage, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2955b86592b
remove the region arguments from InvokeDragSession and InvokeDragSessionWithImage, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/8727297a0248
convert drag services to internally use CSSIntRegion instead ns nsIScriptableRegion, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/74554b2b65bf
have nsTreeBodyFrame::GetSelectionRegion use nsIntRegion instead of nsIScriptableRegion, r=mstange

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c7b40644b111
https://hg.mozilla.org/mozilla-central/rev/b2955b86592b
https://hg.mozilla.org/mozilla-central/rev/8727297a0248
https://hg.mozilla.org/mozilla-central/rev/74554b2b65bf