Move burden of checking for BJustify box alignment into RenderStyle
Created attachment 92372 [details] Patch
Comment on attachment 92372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92372&action=review > Source/WebCore/rendering/style/RenderStyle.h:1029 > + void setBoxAlign(EBoxAlignment a) This guy now wants to be inlined like http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.h&l=727&exact_package=chromium
Last I checked the compiler was smart enough to inline these by default where it had access to the method body (which it always does here since it is in a header file.) I didn't check it in this instance, but have checked it previously for equivalent code on OSX. Moreover, observe that this function (which is not new) did not previously require the inline keyword.
(In reply to comment #3) > Last I checked the compiler was smart enough to inline these by default where it had access to the method body (which it always does here since it is in a header file.) > > I didn't check it in this instance, but have checked it previously for equivalent code on OSX. > > Moreover, observe that this function (which is not new) did not previously require the inline keyword. Yep, it will inline them. But this is somewhat of a preferred style. Granted, newer style, but still.
Comment on attachment 92372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92372&action=review >> Source/WebCore/rendering/style/RenderStyle.h:1029 >> + void setBoxAlign(EBoxAlignment a) > > This guy now wants to be inlined like http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.h&l=727&exact_package=chromium I don't understand why we want this logic in RenderStyle. Why should it be impossible to set BJUSTIFY?
Created attachment 92381 [details] Patch
(In reply to comment #5) > (From update of attachment 92372 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92372&action=review > > >> Source/WebCore/rendering/style/RenderStyle.h:1029 > >> + void setBoxAlign(EBoxAlignment a) > > > > This guy now wants to be inlined like http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.h&l=727&exact_package=chromium > > I don't understand why we want this logic in RenderStyle. Why should it be impossible to set BJUSTIFY? These special cases are arising because one enum is being used as a superset of the possible values for multiple properties. In this instance, the EBoxAlignment enum is used for both the BoxPack and BoxAlignment properties, even though they appear to support different but overlapping sets of values. There are numerous other examples of similar enums in this code. It could be that a better solution is to have two separate enums, though that would require a lot more digging to ensure correctness of the refactoring.
We should definitely add comments explaining such next to each of these strange ifs. Also, shouldn't these just be ASSERTs?
Created attachment 92514 [details] Patch
Comment on attachment 92514 [details] Patch This is strictly better than the current code, but could still be better with use of a comment and possible white-list. Thanks again for taking this on.
Created attachment 92533 [details] Patch
Comment on attachment 92533 [details] Patch Clearing flags on attachment: 92533 Committed r85935: <http://trac.webkit.org/changeset/85935>
All reviewed patches have been landed. Closing bug.