Bug 176437 – Modernize BoxExtent into RectEdge

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Modernize BoxExtent into RectEdge

[Alex Christensen]

Created attachment 319969 [details]
Patch

[Alex Christensen]

Created attachment 319981 [details]
Patch

[Wenson Hsieh]

Comment on attachment 319981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319981&action=review

> Source/WebCore/ChangeLog:8
> +        And give it its own header.

I would include a blurb in the ChangeLog here about why RectEdge is a better name than BoxExtent (from IRC, I learned from Alex that this is to better align with NSRectEdge/UIRectEdge).

[Alex Christensen]

Hmmmm, maybe RectEdge isn't such a good name.  That's more what our *BoxSide enums are.  I'll talk with a few more people about this.

[Wenson Hsieh]

(In reply to Alex Christensen from comment #4)
> Hmmmm, maybe RectEdge isn't such a good name.  That's more what our *BoxSide
> enums are.  I'll talk with a few more people about this.

Good call!

[Alex Christensen]

RectEdge is like UIRectEdge.  Unfortunately NSRectEdge and CGRectEdge are conceptually different.

[Tim Horton]

Comment on attachment 319981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319981&action=review

> Source/WebCore/platform/RectEdge.h:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Too much code to replace the copyright. Inherit it from the original!

[Tim Horton]

(In reply to Alex Christensen from comment #4)
> Hmmmm, maybe RectEdge isn't such a good name.  That's more what our *BoxSide
> enums are.  I'll talk with a few more people about this.

Should see what smfr/zalan think, since it is sort of layouty.

[Simon Fraser (smfr)]

Comment on attachment 319981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319981&action=review

>> Source/WebCore/ChangeLog:8
>> +        And give it its own header.
> 
> I would include a blurb in the ChangeLog here about why RectEdge is a better name than BoxExtent (from IRC, I learned from Alex that this is to better align with NSRectEdge/UIRectEdge).

But RectEdge is a single edge. UIEdgeInsets is the set of 4 edges for a box, so I don't think RectEdge is a good name. Maybe RectEdges?

[Tim Horton]

I do like the plural better too

[Alex Christensen]

Created attachment 320203 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 320203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320203&action=review

> Source/WebCore/platform/RectEdges.h:79
> +    std::array<T, 4> m_sides {{0, 0, 0, 0}};

Any reason not to use Vector?

[Darin Adler]

Comment on attachment 320203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320203&action=review

>> Source/WebCore/platform/RectEdges.h:79
>> +    std::array<T, 4> m_sides {{0, 0, 0, 0}};
> 
> Any reason not to use Vector?

More storage, slower.

If we used a Vector<T, 4> instead of a std::array<T, 4> then we would have additional data members for the vector size, capacity, and pointer to the storage. The size is what makes a Vector resizable, the capacity is what makes it efficient to grow and shrink, and the buffer pointer lets it point either to its own preallocated storage or to the heap. Operations on the Vector will be a bit slower as well, initializing those additional data members, and following the pointer to access the storage.

On a 64-bit system std::array<bool, 4> is 4 bytes and WTF::Vector<bool, 4> is 20 bytes.
On a 64-bit system std::array<float, 4> is 16 bytes and WTF::Vector<bool, 4> is 32 bytes.

[Darin Adler]

(In reply to Darin Adler from comment #13)
> On a 64-bit system std::array<float, 4> is 16 bytes and WTF::Vector<bool, 4>
> is 32 bytes.

Oops, I meant:

On a 64-bit system std::array<float, 4> is 16 bytes and WTF::Vector<float, 4> is 32 bytes.

[WebKit Commit Bot]

Comment on attachment 320203 [details]
Patch

Clearing flags on attachment: 320203

Committed r221866: <http://trac.webkit.org/changeset/221866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693541>