Bug 154035 - Fix the !(ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)) after r196281
Summary: Fix the !(ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)) after r196281
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 154003
  Show dependency treegraph
 
Reported: 2016-02-09 09:31 PST by Csaba Osztrogonác
Modified: 2016-02-11 10:19 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.41 KB, patch)
2016-02-09 09:34 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch_option1 (2.28 KB, patch)
2016-02-10 09:45 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch_option2 (1.71 KB, patch)
2016-02-11 03:39 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2016-02-09 09:31:10 PST
https://trac.webkit.org/r196281 broke the !(ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)) build:

In file included from ../../Source/WebCore/dom/ComposedTreeIterator.cpp:27:0:
../../Source/WebCore/dom/ComposedTreeIterator.h: In constructor 'WebCore::ComposedTreeIterator::Context::Context(WebCore::ContainerNode&, WebCore::Node&, size_t)':
../../Source/WebCore/dom/ComposedTreeIterator.h:74:15: error: class 'WebCore::ComposedTreeIterator::Context' does not have any field named 'slotNodeIndex'

slotNodeIndex is declared inside ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT) guard,
but it is initialized unconditionally, which is incorrect and caused build failure.
Comment 1 Csaba Osztrogonác 2016-02-09 09:34:48 PST
Created attachment 270936 [details]
Patch
Comment 2 WebKit Commit Bot 2016-02-10 02:28:26 PST
Comment on attachment 270936 [details]
Patch

Clearing flags on attachment: 270936

Committed r196365: <http://trac.webkit.org/changeset/196365>
Comment 3 WebKit Commit Bot 2016-02-10 02:28:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2016-02-10 09:06:14 PST
Comment on attachment 270936 [details]
Patch

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

> Source/WebCore/dom/ComposedTreeIterator.h:81
> +#if ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)
>              , slotNodeIndex(slotNodeIndex)
>          { }
> +#else
> +        {
> +            UNUSED_PARAM(slotNodeIndex);
> +        }
> +#endif

I should have caught you before landing this. I think we should have a style rule that #if should never introduce additional sets of braces unless it’s around an entire function. I also would have moved it outside the ComposedTreeIterator class definition because it starts to crud up the class definition and make it too hard to read. Thus I would have written this:

    Context(ContainerNode& root, Node&, size_t slotNodeIndex = notFound);

...

inline ComposedTreeIterator::Context::Context(ContainerNode& root, Node& node, size_t slotNodeIndex)
    : iterator(root, &node)
#if ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)
    , slotNodeIndex(slotNodeIndex)
#endif
{
#if !(ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT))
    UNUSED_PARAM(slotNodeIndex);
#endif
}
Comment 5 Csaba Osztrogonác 2016-02-10 09:45:11 PST
Reopening to attach new patch.
Comment 6 Csaba Osztrogonác 2016-02-10 09:45:14 PST
Created attachment 270997 [details]
Patch_option1

Style fix based on review comments. Not tested, let the EWS test ut.
Comment 7 WebKit Commit Bot 2016-02-10 09:46:13 PST
Attachment 270997 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ComposedTreeIterator.h:72:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Antti Koivisto 2016-02-10 15:22:06 PST
I think a better fix is to just remove the #if around slotNodeIndex member. It doesn't really hurt to have it everywhere.
Comment 9 Csaba Osztrogonác 2016-02-11 03:39:47 PST
Created attachment 271056 [details]
Patch_option2

It's all the same for me, please decide which one do you prefer.
Comment 10 WebKit Commit Bot 2016-02-11 10:19:03 PST
Comment on attachment 271056 [details]
Patch_option2

Clearing flags on attachment: 271056

Committed r196422: <http://trac.webkit.org/changeset/196422>
Comment 11 WebKit Commit Bot 2016-02-11 10:19:08 PST
All reviewed patches have been landed.  Closing bug.