Bug 176437

Summary: Modernize BoxExtent into RectEdge
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alex Christensen 2017-09-05 19:28:03 PDT
Modernize BoxExtent into RectEdge
Comment 1 Alex Christensen 2017-09-05 19:29:44 PDT
Created attachment 319969 [details]
Patch
Comment 2 Alex Christensen 2017-09-05 21:39:24 PDT
Created attachment 319981 [details]
Patch
Comment 3 Wenson Hsieh 2017-09-06 08:20:57 PDT
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).
Comment 4 Alex Christensen 2017-09-06 08:43:39 PDT
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.
Comment 5 Wenson Hsieh 2017-09-06 08:52:00 PDT
(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!
Comment 6 Alex Christensen 2017-09-06 13:14:58 PDT
RectEdge is like UIRectEdge.  Unfortunately NSRectEdge and CGRectEdge are conceptually different.
Comment 7 Tim Horton 2017-09-06 13:18:00 PDT
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!
Comment 8 Tim Horton 2017-09-06 13:18:54 PDT
(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.
Comment 9 Simon Fraser (smfr) 2017-09-06 14:23:48 PDT
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?
Comment 10 Tim Horton 2017-09-06 14:34:21 PDT
I do like the plural better too
Comment 11 Alex Christensen 2017-09-07 16:26:44 PDT
Created attachment 320203 [details]
Patch
Comment 12 Simon Fraser (smfr) 2017-09-07 17:23:22 PDT
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?
Comment 13 Darin Adler 2017-09-10 17:33:31 PDT
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.
Comment 14 Darin Adler 2017-09-10 17:34:09 PDT
(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.
Comment 15 WebKit Commit Bot 2017-09-11 10:57:59 PDT
Comment on attachment 320203 [details]
Patch

Clearing flags on attachment: 320203

Committed r221866: <http://trac.webkit.org/changeset/221866>
Comment 16 WebKit Commit Bot 2017-09-11 10:58:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-09-27 12:35:18 PDT
<rdar://problem/34693541>