Bug 154035 – Fix the !(ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)) after r196281

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 270936 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 270936 [details]
Patch

Clearing flags on attachment: 270936

Committed r196365: <http://trac.webkit.org/changeset/196365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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
}

[Csaba Osztrogonác]

Reopening to attach new patch.

[Csaba Osztrogonác]

Created attachment 270997 [details]
Patch_option1

Style fix based on review comments. Not tested, let the EWS test ut.

[WebKit Commit Bot]

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.

[Antti Koivisto]

I think a better fix is to just remove the #if around slotNodeIndex member. It doesn't really hurt to have it everywhere.

[Csaba Osztrogonác]

Created attachment 271056 [details]
Patch_option2

It's all the same for me, please decide which one do you prefer.

[WebKit Commit Bot]

Comment on attachment 271056 [details]
Patch_option2

Clearing flags on attachment: 271056

Committed r196422: <http://trac.webkit.org/changeset/196422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.