[webkit-reviews] review granted: [Bug 240120] Image controls menu button is not appearing for multi-page PDFs : [Attachment 458958] Patch

[bugzilla-daemon at webkit.org]

Megan Gardner <megan_gardner at apple.com> has granted Kate Cheney
<katherine_cheney at apple.com>'s request for review:
Bug 240120: Image controls menu button is not appearing for multi-page PDFs
https://bugs.webkit.org/show_bug.cgi?id=240120

Attachment 458958: Patch

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




--- Comment #17 from Megan Gardner <megan_gardner at apple.com> ---
Comment on attachment 458958
  --> https://bugs.webkit.org/attachment.cgi?id=458958
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:195
> +

nit. I don't think you need this space.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:197
> +

nit. I don't think you need this space.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:198
> +	   bool hasImageMenu = isImageMenuEnabled(*protectedElement);

I feel like hasImgeMenu would be better named isImageMenuEnabled, both the
hasImageMenu and hasControls sound like they should be the same thing, but the
difference really is if they are enable or not.

> Source/WebCore/html/HTMLAttachmentElement.h:101
> +    bool childShouldCreateRenderer(const Node&) const final;

should this be const override?