Summary: | Modernize BoxExtent into RectEdge | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | 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
Alex Christensen
2017-09-05 19:28:03 PDT
Created attachment 319969 [details]
Patch
Created attachment 319981 [details]
Patch
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). 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. (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! RectEdge is like UIRectEdge. Unfortunately NSRectEdge and CGRectEdge are conceptually different. 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! (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 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? I do like the plural better too Created attachment 320203 [details]
Patch
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 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. (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 on attachment 320203 [details] Patch Clearing flags on attachment: 320203 Committed r221866: <http://trac.webkit.org/changeset/221866> All reviewed patches have been landed. Closing bug. |