Bug 60246 - Move burden of checking for BJustify box alignment into RenderStyle
Summary: Move burden of checking for BJustify box alignment into RenderStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-04 21:47 PDT by Luke Macpherson
Modified: 2011-05-06 01:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.85 KB, patch)
2011-05-04 21:51 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2011-05-04 23:28 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (2.81 KB, patch)
2011-05-05 17:52 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2011-05-05 20:14 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-05-04 21:47:10 PDT
Move burden of checking for BJustify box alignment into RenderStyle
Comment 1 Luke Macpherson 2011-05-04 21:51:01 PDT
Created attachment 92372 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-05-04 21:54:57 PDT
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
Comment 3 Luke Macpherson 2011-05-04 22:02:48 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2011-05-04 22:10:22 PDT
(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 5 Eric Seidel (no email) 2011-05-04 23:15:11 PDT
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?
Comment 6 Luke Macpherson 2011-05-04 23:28:46 PDT
Created attachment 92381 [details]
Patch
Comment 7 Luke Macpherson 2011-05-04 23:35:58 PDT
(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.
Comment 8 Eric Seidel (no email) 2011-05-04 23:44:54 PDT
We should definitely add comments explaining such next to each of these strange ifs.

Also, shouldn't these just be ASSERTs?
Comment 9 Luke Macpherson 2011-05-05 17:52:19 PDT
Created attachment 92514 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-05-05 17:55:58 PDT
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.
Comment 11 Luke Macpherson 2011-05-05 20:14:37 PDT
Created attachment 92533 [details]
Patch
Comment 12 WebKit Commit Bot 2011-05-06 01:17:39 PDT
Comment on attachment 92533 [details]
Patch

Clearing flags on attachment: 92533

Committed r85935: <http://trac.webkit.org/changeset/85935>
Comment 13 WebKit Commit Bot 2011-05-06 01:17:45 PDT
All reviewed patches have been landed.  Closing bug.