Bug 60246 – Move burden of checking for BJustify box alignment into RenderStyle

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

[Luke Macpherson]

Move burden of checking for BJustify box alignment into RenderStyle

[Luke Macpherson]

Created attachment 92372 [details]
Patch

[Dimitri Glazkov (Google)]

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

[Luke Macpherson]

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.

[Dimitri Glazkov (Google)]

(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.

[Eric Seidel (no email)]

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?

[Luke Macpherson]

Created attachment 92381 [details]
Patch

[Luke Macpherson]

(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.

[Eric Seidel (no email)]

We should definitely add comments explaining such next to each of these strange ifs.

Also, shouldn't these just be ASSERTs?

[Luke Macpherson]

Created attachment 92514 [details]
Patch

[Eric Seidel (no email)]

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.

[Luke Macpherson]

Created attachment 92533 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 92533 [details]
Patch

Clearing flags on attachment: 92533

Committed r85935: <http://trac.webkit.org/changeset/85935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.