Bug 117277 - [CSS Shapes] Shape methods and member variables should be guarded with the CSS_SHAPES flag
Summary: [CSS Shapes] Shape methods and member variables should be guarded with the CS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
Depends on:
Blocks: 89256 119019
  Show dependency treegraph
 
Reported: 2013-06-05 14:52 PDT by Bear Travis
Modified: 2013-07-25 09:39 PDT (History)
7 users (show)

See Also:


Attachments
Initial Patch (6.25 KB, patch)
2013-06-17 14:45 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Updated patch (12.77 KB, patch)
2013-06-18 12:37 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Modifying one of the .in files (13.25 KB, patch)
2013-06-19 13:01 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Updated (14.54 KB, patch)
2013-07-23 14:04 PDT, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2013-06-05 14:52:27 PDT
See RenderStyle and RareNonInheritedData
Comment 1 Bear Travis 2013-06-17 14:45:26 PDT
Created attachment 204856 [details]
Initial Patch
Comment 2 Build Bot 2013-06-17 18:20:48 PDT
Comment on attachment 204856 [details]
Initial Patch

Attachment 204856 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/884145
Comment 3 Brent Fulgham 2013-06-18 09:32:49 PDT
This patch seems to break the Windows build.  Do we need to touch one of the *.in files to get a full regeneration for this patch to work?
Comment 4 Brent Fulgham 2013-06-18 09:32:49 PDT
This patch seems to break the Windows build.  Do we need to touch one of the *.in files to get a full regeneration for this patch to work?
Comment 5 Bear Travis 2013-06-18 10:38:48 PDT
The cause looks to be a bit more mundane. I've missed a couple segments of code that must also use the compile guard.
Comment 6 Bear Travis 2013-06-18 12:37:08 PDT
Created attachment 204930 [details]
Updated patch
Comment 7 Build Bot 2013-06-18 17:17:05 PDT
Comment on attachment 204930 [details]
Updated patch

Attachment 204930 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/934040
Comment 8 Bear Travis 2013-06-19 13:01:05 PDT
Created attachment 205029 [details]
Modifying one of the .in files
Comment 9 Alexandru Chiculita 2013-07-17 00:45:56 PDT
Comment on attachment 205029 [details]
Modifying one of the .in files

View in context: https://bugs.webkit.org/attachment.cgi?id=205029&action=review

> Source/WebCore/rendering/RenderBlock.cpp:1471
>  static inline bool shapeInfoRequiresRelayout(const RenderBlock* block)
>  {

I think we could fix RenderBlock::updateRegionsAndShapesBeforeChildLayout to not call this function when CSS_SHAPES is disabled. For example, the first lines in RenderBlock::updateRegionsAndShapesBeforeChildLayout are very hard to read.

> Source/WebCore/rendering/RenderBlock.cpp:4457
> +#else
> +        UNUSED_PARAM(offsetMode);

You already have an unused_param for offset mode in that function. Should you remove the other one?

> Source/WebCore/rendering/RenderBlock.cpp:4520
> +        UNUSED_PARAM(offsetMode);

ditto

> Source/WebCore/rendering/style/RenderStyle.cpp:403
>          if (rareNonInheritedData->m_wrapFlow != other->rareNonInheritedData->m_wrapFlow

Is there a patch for adding these lines to the exclusions ifdef?

> Source/WebCore/rendering/style/RenderStyle.cpp:707
> +#if ENABLE(CSS_SHAPES)

The comment before this line seems related to the code inside the ifdef. I would move the comment inside the ifdef.
Comment 10 Bear Travis 2013-07-23 14:04:51 PDT
Created attachment 207348 [details]
Updated

Making requested changes, filed bug 119019 to track exclusions compile guard
Comment 11 Alexandru Chiculita 2013-07-25 00:20:08 PDT
Comment on attachment 207348 [details]
Updated

Thanks! r=me
Comment 12 WebKit Commit Bot 2013-07-25 09:39:27 PDT
Comment on attachment 207348 [details]
Updated

Clearing flags on attachment: 207348

Committed r153330: <http://trac.webkit.org/changeset/153330>
Comment 13 WebKit Commit Bot 2013-07-25 09:39:29 PDT
All reviewed patches have been landed.  Closing bug.