[webkit-reviews] review granted: [Bug 130062] Hook up image controls for WebKit1 : [Attachment 226436] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 130062: Hook up image controls for WebKit1
https://bugs.webkit.org/show_bug.cgi?id=130062

Attachment 226436: patch
https://bugs.webkit.org/attachment.cgi?id=226436&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226436&action=review


> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:91
> +    if (!document.page())
> +	   return nullptr;

Seems a little strange here.

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:105
> +	   if (Page* page = document().page())
> +	       page->contextMenuController().showImageControlsMenu(event);
> +
> +	   return;

Shouldn’t we be calling event->setDefaultHandled() here (not sure I got the
function name right).

Also, I suggest deleting that blank line.

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.h:37
> +    ~ImageControlsButtonElementMac();

Should say virtual here.

> Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:92
> +    RefPtr<ImageControlsButtonElementMac> button =
ImageControlsButtonElementMac::maybeCreate(document);
> +    controls->appendChild(button.release());

Not sure we need this local variable.

> Source/WebCore/page/ContextMenuController.cpp:140
> +    CachedImage* image = toRenderImage(renderer)->cachedImage();

Compiler probably ends up optimizing this anyway, but I would write:

    toRenderImage(*renderer)->cachedImage()

to be explicit that toRenderImage doesn’t have to bother handling null.

> Source/WebCore/page/ContextMenuController.cpp:1470
> +    CachedResourceHandle<CachedImage> replacedImage = new
CachedImage(newImage.get(), frame->page()->sessionID());

Is this right? we don’t need to call any adopt or anything to do this
correctly?

> Source/WebCore/rendering/RenderThemeMac.mm:1981
> +bool RenderThemeMac::paintImageControlsButton(RenderObject* o, const
PaintInfo& paintInfo, const IntRect& rect)

Do we really need to name the renderer "o"?

> Source/WebCore/rendering/RenderThemeMac.mm:2004
> +    NSServicesRolloverButtonCell *cell = servicesRolloverButtonCell();
> +    return IntSize([cell cellSize]);

Not sure we need the local variable here.

> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.h:33
> +#if ENABLE(IMAGE_CONTROLS)
> + at class WebSharingServicePickerController;
> +#endif

Not sure we really need to put an #if around a forward declaration.

> Source/WebKit2/Shared/ContextMenuContextData.cpp:49
> +    , m_isImageControl(!!context.controlledImage())

Why the !!? Is that really needed?