[webkit-reviews] review granted: [Bug 234405] Remove unneeded webkit specific CSS attribute for Image Control Menu. : [Attachment 448346] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 234405: Remove unneeded webkit specific CSS attribute for Image Control
Menu.
https://bugs.webkit.org/show_bug.cgi?id=234405

Attachment 448346: Patch

https://bugs.webkit.org/attachment.cgi?id=448346&action=review




--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 448346
  --> https://bugs.webkit.org/attachment.cgi?id=448346
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:748
> +#endif // ENABLE(SERVICE_CONTROLS)

Should omit this comment here.

> Source/WebCore/html/shadow/mac/imageControlsMac.css:42
> -    appearance: -internal-image-controls-button;
> +    appearance: auto;

Should we remove this line entirely instead of changing it to say "auto"?

> Source/WebCore/platform/ThemeTypes.h:114
> +    LargestControlPart = ImageControlsButtonPart
> +#else
> +    LargestControlPart = CapsLockIndicatorPart

Can LargestControlPart be a constant outside the enumeration rather than an
enumeration value?

    constexpr ControlPart largestControlPart = ImageControlsButtonPart;

It can otherwise be the same, still right here in the header next to the
enumeration.

> Source/WebCore/rendering/RenderTheme.cpp:306
> +#endif
>      Ref element = *elementPtr;

I suggest a blank line here.

> Source/WebCore/rendering/RenderTheme.h:370
> +    virtual bool isImageControl(const Element*) const { return false; }

This should take a const Element&, because it dereferences the pointer without
checking for nullptr.

> Source/WebCore/rendering/RenderThemeMac.mm:2321
> +    if (ImageControlsMac::isImageControlsButtonElement(*elementPtr))
> +	   return true;
> +    return false;

Should just be return x, rather than if (x) return true; return false;

> Source/WebCore/rendering/style/RenderStyle.h:1143
> +	   static_assert(LargestControlPart < 1 << APPEARANCE_BIT_WIDTH,
"Control part must fit in storage bits");

I suggest we just put this static_assert outside the functions, alongside them.
Since a static_assert works at runtime it does not need to be inside a
function.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:69
> +#define APPEARANCE_BIT_WIDTH 7

This should be a constexpr integer, not a #define.

    constexpr int appearanceBitWidth = 7;

> Source/WebCore/testing/Internals.cpp:6397
> +#endif // ENABLE(SERVICE_CONTROLS)

This endif doesn’t need a comment, it’s so close to the start of the #If.

> Source/WebCore/testing/Internals.h:1194
> +#endif // ENABLE(SERVICE_CONTROLS)

This endif doesn’t need a comment, it’s so close to the start of the #If.

> LayoutTests/ChangeLog:11
> +	   We need to move this test to be a mac specific one, as it now has
mac specific 
> +	   test harnessing. Also changed the test to be resistent to the async
nature of adding
> +	   the shadow dom elements, and checking the shadow dom directly rather
than the resulting layout,
> +	   which is prone to pixel errors.

What makes this test Mac-specific?

> LayoutTests/fast/images/mac/image-controls-basic.html:32
> +		if (internals.shadowRoot(elem) &&
internals.shadowRoot(elem).getElementById('image-controls') &&
internals.shadowRoot(elem).getElementById('image-controls-button')) 
> +		output += 'PASS: image controls exist in shadowDom';
> +		else
> +			output += 'FAIL: no image controls found in shadowDom';
> +		document.getElementById('log').innerHTML = output;

Broken indentation here.

> LayoutTests/fast/images/mac/image-controls-basic.html:40
> +	checkForShadowDom(elem);

And here.