1423541 - Use Rect's accessors and setters instead of direct member variables

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1423541: Use BaseRect access methods instead of member variables in accessible/

Review commit: https://reviewboard.mozilla.org/r/205876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205876/

Comment 2

[alexander :surkov (:asurkov)]

Milan, could give some background on the bug please, what benefits are of having accessors methods?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Sure - Bug 1386277 - we are trying to switch to a box model, instead of rectangle.  So, we can't just give access to X/Y, as changing those would in the box model also change the other corner.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8934941 [details]
Bug 1423541: Use BaseRect access methods instead of member variables in accessible/

https://reviewboard.mozilla.org/r/205876/#review211498

::: accessible/generic/HyperTextAccessible.cpp:144
(Diff revision 1)
>      // Use the point for the end offset to calculate the width
>      nsPoint frameTextEndPoint;
>      rv = frame->GetPointFromOffset(startContentOffset + frameSubStringLength, &frameTextEndPoint);
>      NS_ENSURE_SUCCESS(rv, nsIntRect());
>  
> -    frameScreenRect.x += std::min(frameTextStartPoint.x, frameTextEndPoint.x);
> +    frameScreenRect.SetRectX(frameScreenRect.X() + std::min(frameTextStartPoint.x, frameTextEndPoint.x),

no SetRectX function on trunk, is it about setting x and width? If so, then name sounds confusing

::: accessible/generic/HyperTextAccessible.cpp:1273
(Diff revision 1)
>      offset1 = 0;
>    }
>  
> -  nsAccUtils::ConvertScreenCoordsTo(&bounds.x, &bounds.y, aCoordType, this);
> +  auto boundsX = bounds.X();
> +  auto boundsY = bounds.Y();
> +  nsAccUtils::ConvertScreenCoordsTo(&boundsX, &boundsY, aCoordType, this);

might be nicer if ConvertScreenCoordsTo worked with Rect

::: accessible/ipc/DocAccessibleParent.cpp:627
(Diff revision 1)
>    nsIntRect rect(CW_USEDEFAULT, CW_USEDEFAULT, 0, 0);
>    if (Compatibility::IsDolphin()) {
>      rect = Bounds();
>      nsIntRect rootRect = rootDocument->Bounds();
> -    rect.x = rootRect.x - rect.x;
> -    rect.y -= rootRect.y;
> +    rect.MoveToX(rootRect.X() - rect.X());
> +    rect.MoveByY(-rootRect.Y());

probably rect.MoveToY(rect.y - rootRect.y) will be a bit more readable

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8934941 [details]
Bug 1423541: Use BaseRect access methods instead of member variables in accessible/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205876/diff/1-2/

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9c67be2c523a47554cedab957e831946065afe

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8934941 [details]
Bug 1423541: Use BaseRect access methods instead of member variables in accessible/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205876/diff/2-3/

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8934941 [details]
Bug 1423541: Use BaseRect access methods instead of member variables in accessible/

https://reviewboard.mozilla.org/r/205876/#review211498

> no SetRectX function on trunk, is it about setting x and width? If so, then name sounds confusing

Good follow up bug.

> might be nicer if ConvertScreenCoordsTo worked with Rect

Good follow up.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40161b05e29e
Use BaseRect access methods instead of member variables in accessible/ r=surkov

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/40161b05e29e