Closed Bug 1473029 Opened Last year Closed Last year

Don't use nsScriptableRegion within drag services

Categories

(Core :: Drag and Drop, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(4 files, 3 obsolete files)

No description provided.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8989437 - Attachment description: Part 2, remove the region arguments from InvokeDragSession and InvokeDragSessionWithImage → Part 4, have nsTreeBodyFrame::GetSelectionRegion use nsIntRegion instead of nsIScriptableRegion
Priority: -- → P3
Blocks: 1478375
Attachment #8989436 - Flags: review?(mstange)
Attachment #8989440 - Flags: review?(mstange)
Attachment #8989438 - Flags: review?(mstange)
Attachment #8989437 - Flags: review?(mstange)
Blocks: 1478388
Attachment #8989436 - Flags: review?(mstange) → review+
Attachment #8989440 - Flags: review?(mstange) → review+
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 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>?
Attachment #8989438 - Attachment is obsolete: true
Attachment #8989438 - Flags: review?(mstange)
Attachment #8997019 - Flags: review?(mstange)
Attachment #8989437 - Attachment is obsolete: true
Attachment #8989437 - Flags: review?(mstange)
Attachment #8997020 - Flags: review?(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".
Attachment #8997020 - Flags: review?(mstange) → review+
Attachment #8997019 - Attachment is obsolete: true
Attachment #8997019 - Flags: review?(mstange)
Attachment #8997488 - Flags: review?(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();
Attachment #8997488 - Flags: review?(mstange) → review+
Pushed by neil@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.