Closed Bug 1389152 Opened 2 years ago Closed 2 years ago

Hide nsRect implementation details from WritingModes

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(1 file)

In the context of bug 1386277, we'd want to hide the implementation details of nsRect from WritingModes class.  Right now, we allow direct access to the addresses of BaseRect class members.
Assignee: nobody → milan
Summary: His nsRect implementation details from WritingModes → Hide nsRect implementation details from WritingModes
Comment on attachment 8895943 [details]
Bug 1389152: Change LogicalRect from having nsRect as a member variable, and exposing its member variable addresses, to storing four nscoord values separately and doing some operations by creating temporary rectangles to ensure consistency. Add a method t

https://reviewboard.mozilla.org/r/167214/#review173496

::: layout/generic/WritingModes.h:1926
(Diff revision 1)
>    }
>  
>  #ifdef DEBUG
>    WritingMode mWritingMode;
>  #endif
> -  nsRect      mRect;
> +  nscoord     mIStart;

nit: add comments to the members what these mean. (i.e. duplications of IStart() etc. comments)
Attachment #8895943 - Flags: review?(bas) → review+
Jonathan, I'm not sure that mozreview works with multiple reviewers, especially when post-requested, so this needinfo is just to check if you got this review request.  No big rush.
Flags: needinfo?(jfkthame)
(In reply to Milan Sreckovic [:milan] from comment #3)
> Jonathan, I'm not sure that mozreview works with multiple reviewers,
> especially when post-requested, so this needinfo is just to check if you got
> this review request.  No big rush.

No, I've not had any bugmail about this until now (which seems like a pretty serious deficiency!), so thanks for the heads-up. I'll try to look this over shortly.
Flags: needinfo?(jfkthame)
(In reply to Milan Sreckovic [:milan] from comment #0)
> In the context of bug 1386277, we'd want to hide the implementation details
> of nsRect from WritingModes class.

If we're going to change the representation of BaseRect (and hence nsRect, etc) in that way, I wonder if we should be doing the same for LogicalRect, rather than retaining the origin + size representation here and then having to convert when we want to leverage an implementation from BaseRect (such as Intersect)?
Good question; I opened bug 1390959 to answer that question before we change the BaseRect representation.
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Milan Sreckovic [:milan] from comment #0)
> > In the context of bug 1386277, we'd want to hide the implementation details
> > of nsRect from WritingModes class.
> 
> If we're going to change the representation of BaseRect (and hence nsRect,
> etc) in that way, I wonder if we should be doing the same for LogicalRect,
> rather than retaining the origin + size representation here and then having
> to convert when we want to leverage an implementation from BaseRect (such as
> Intersect)?

Do you consider this blocking to this change, or just blocking to the "start using the box model"?
Comment on attachment 8895943 [details]
Bug 1389152: Change LogicalRect from having nsRect as a member variable, and exposing its member variable addresses, to storing four nscoord values separately and doing some operations by creating temporary rectangles to ensure consistency. Add a method t

https://reviewboard.mozilla.org/r/167214/#review174058

Generally this seems OK, but I do wonder whether there's a perf cost to the added conversions between the LogicalRect and nsRect representations. Maybe with sufficient inlining and optimization, that disappears? If so, fine.

::: layout/generic/WritingModes.h:1719
(Diff revision 2)
> -  void SetEmpty() { mRect.SetEmpty(); }
> +  void SetEmpty() { mISize = mBSize = 0; }
>  
>    bool IsEqualEdges(const LogicalRect aOther) const
>    {
>      CHECK_WRITING_MODE(aOther.GetWritingMode());
> -    return mRect.IsEqualEdges(aOther.mRect);
> +    return nsRect(mIStart, mBStart, mISize, mBSize).IsEqualEdges(nsRect(aOther.mIStart, aOther.mBStart, aOther.mISize, aOther.mBSize));

This looks awfully long - please figure out some way to line-wrap it. Maybe something like

    return nsRect(mIStart, mBStart, mISize, mBSize).
             IsEqualEdges(nsRect(aOther.mIStart, aOther.mBStart,
                                 aOther.mISize, aOther.mBSize));

::: layout/generic/WritingModes.h:1781
(Diff revision 2)
> +    nsRect rect(mIStart, mBStart, mISize, mBSize);
> +    rect.Inflate(aD);
> +    rect.GetRect(&mIStart, &mBStart, &mISize, &mBSize);

Does the compiler adequately optimize this, or would it be better to just do the actual Inflate calculations/assignments directly?

(And the same question for the following methods.)

::: layout/generic/WritingModes.h:1862
(Diff revision 2)
> -    return mRect.IntersectRect(aRect1.mRect, aRect2.mRect);
> +    nsRect rect(aRect1.mIStart, aRect1.mBStart,
> +                aRect1.mISize, aRect1.mBSize);
> +    bool isNotEmpty =
> +      rect.IntersectRect(rect, nsRect(aRect2.mIStart, aRect2.mBStart,
> +                                      aRect2.mISize, aRect2.mBSize));
> +    rect.GetRect(&mIStart, &mBStart, &mISize, &mBSize);
> +    return isNotEmpty;

Again, I wonder how well the compiler will handle this. Superficially, it looks like a lot more work than the current code, which implies a possible perf cost unless aggressively optimized. Have you looked at the generated code at all to see how it compares?
Attachment #8895943 - Flags: review?(jfkthame)
Priority: -- → P3
(In reply to Milan Sreckovic [:milan] from comment #10)
>...
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/167214/diff/2-3/

With the inlining of Inflate/Deflate as suggested in comment 9.
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> ...
> 
> Again, I wonder how well the compiler will handle this. Superficially, it
> looks like a lot more work than the current code, which implies a possible
> perf cost unless aggressively optimized. Have you looked at the generated
> code at all to see how it compares?

These have been inlined now, and the extra work only happens in the debug build where we compute things the old way and compare/assert that the results are the same.  Some unit tests for this as well.
Flags: needinfo?(jfkthame)
OK, sounds fair enough - let's go for it.
Flags: needinfo?(jfkthame)
Comment on attachment 8895943 [details]
Bug 1389152: Change LogicalRect from having nsRect as a member variable, and exposing its member variable addresses, to storing four nscoord values separately and doing some operations by creating temporary rectangles to ensure consistency. Add a method t

https://reviewboard.mozilla.org/r/167214/#review202896
Attachment #8895943 - Flags: review?(jfkthame) → review+
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a52e1d4c300
Change LogicalRect from having nsRect as a member variable, and exposing its member variable addresses, to storing four nscoord values separately and doing some operations by creating temporary rectangles to ensure consistency. Add a method to BaseRect to get OriginAndSize at once. r=bas,jfkthame.schouten
https://hg.mozilla.org/mozilla-central/rev/2a52e1d4c300
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.