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

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has asked  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 Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226436&action=review


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

I think it's important that the replaced image have a URL.

We may discover later that it is or isn't important what that URL is, but we
need something.

I think we should use webkit-fake URL like the pasteboard code uses.

EditorMac.mm and EditorIOS.mm both generate webkit-fake URLs.  I'd like to see
this patch factor that out into a utility method and use it here.

> Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:50
> + at implementation WebSharingServicePickerController {
> +    WebContextMenuClient* _menuClient;
> +    RetainPtr<NSSharingServicePicker> _picker;
> +}

...and...

> Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:61
> +    _menuClient = menuClient;

...I'm concerned about some path to a use-after-free with the menu client. 
Should we explicitly clear out the back pointer?