Last Comment Bug 1423541 - Use Rect's accessors and setters instead of direct member variables
: Use Rect's accessors and setters instead of direct member variables
Status: RESOLVED FIXED
[gfx-noted]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: Unspecified Unspecified
P3 normal (vote)
: mozilla59
Assigned To: Milan Sreckovic [:milan] (needinfo for best results)
:
: Jessie [:jbonisteel] plz needinfo
Mentors:
Depends on: 1428348
Blocks: 1386277 1423919
  Show dependency treegraph
 
Reported: 2017-12-06 04:01 PST by Milan Sreckovic [:milan] (needinfo for best results)
Modified: 2018-01-12 14:07 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Bug 1423541: Use BaseRect access methods instead of member variables in accessible/ (59 bytes, text/x-review-board-request)
2017-12-06 04:15 PST, Milan Sreckovic [:milan] (needinfo for best results)
surkov: review+
Details

Description User image Milan Sreckovic [:milan] (needinfo for best results) 2017-12-06 04:01:52 PST

    
Comment 1 User image Milan Sreckovic [:milan] (needinfo for best results) 2017-12-06 04:15:45 PST Comment hidden (mozreview-request)
Comment 2 User image alexander :surkov (:asurkov) 2017-12-06 07:20:54 PST
Milan, could give some background on the bug please, what benefits are of having accessors methods?
Comment 3 User image Milan Sreckovic [:milan] (needinfo for best results) 2017-12-06 07:36:10 PST
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 User image alexander :surkov (:asurkov) 2017-12-06 09:26:28 PST
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 User image Milan Sreckovic [:milan] (needinfo for best results) 2018-01-10 08:20:18 PST Comment hidden (mozreview-request)
Comment 6 User image Milan Sreckovic [:milan] (needinfo for best results) 2018-01-12 07:42:11 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9c67be2c523a47554cedab957e831946065afe
Comment 7 User image Milan Sreckovic [:milan] (needinfo for best results) 2018-01-12 09:08:40 PST Comment hidden (mozreview-request)
Comment 8 User image Milan Sreckovic [:milan] (needinfo for best results) 2018-01-12 09:09:53 PST
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 User image Pulsebot 2018-01-12 09:18:03 PST
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 User image Natalia Csoregi [:nataliaCs] 2018-01-12 14:07:59 PST
https://hg.mozilla.org/mozilla-central/rev/40161b05e29e

Note You need to log in before you can comment on or make changes to this bug.