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

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: milan, Assigned: milan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

Comment hidden (empty)
Assignee: nobody → milan
Blocks: 1386277
Comment hidden (mozreview-request)
Component: Disability Access APIs → Graphics
Priority: -- → P3
Whiteboard: [gfx-noted]
Milan, could give some background on the bug please, what benefits are of having accessors methods?
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

a year ago
mozreview-review
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
Attachment #8934941 - Flags: review?(surkov.alexander) → review+
Blocks: 1423919
No longer blocks: 1386277
Depends on: 1428348
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
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

10 months ago
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40161b05e29e
Use BaseRect access methods instead of member variables in accessible/ r=surkov

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40161b05e29e
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.