Bug 130062 – Hook up image controls for WebKit1

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/15964809>

[Tim Horton]

Created attachment 226373 [details]
patch

[WebKit Commit Bot]

Attachment 226373 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.h:59:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.h:59:  The parameter name "view" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.h:45:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.h:43:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/RenderThemeMac.h:229:  The parameter name "paintInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 226436 [details]
patch

[Darin Adler]

Comment on attachment 226436 [details]
patch

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)
> +@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?

[Brady Eidson]

Comment on attachment 226436 [details]
patch

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
> +@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?

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 226436 [details])
> 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.

Agreed, I'll try to figure out what is going on there.

> > 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.

I think so.

> > 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?

Sadly, yes (and I double-checked with Brady).

> > 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"?

Not even sort of.

(In reply to comment #5)
> (From update of attachment 226436 [details])
> 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.

Okay, that sounds like a good plan!

> > Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:50
> > +@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?

A valid point. Sure.

[Tim Horton]

Hmm, I just noticed that Editor::writeImageToPasteboard assumes that CachedImages have a valid ResourceBuffer, which a CachedImage created from an image like I'm doing here does not, so we crash trying to copy a modified image. It's not clear to me if this should change the approach here, or if we should adjust writeImageToPasteboard.

[Tim Horton]

Created attachment 226465 [details]
patch

Addressing most of the review comments. The bit about copying above still stands.

[Brady Eidson]

Comment on attachment 226465 [details]
patch

I'm fine with this assuming EWS, etc etc!

[Tim Horton]

So! I think I'm going to land this, with an added null check to prevent us from crashing when copying. This does mean that copying a replaced image won't work, but we can fix that next (there's so much more to this patch that it will be much easier to work on things like that if it's landed). Sam thinks we should use the standard paste mechanism to replace the image, but I'm somewhat afraid to do that for two reasons:

1. We currently just get back a data blob and have to assume it's an image, so it doesn't buy us much. This will get resolved with API changes, but not yet.
2. Pasting "over" the image will lose things that the page has done to the image (event listeners, inline style, etc.) that aren't persisted in the image itself.

So we can consider that, but it will require some thought.

[Tim Horton]

http://trac.webkit.org/changeset/165479

[Brady Eidson]

(In reply to comment #10)
>Sam thinks we should use the standard paste mechanism to replace the image, but I'm somewhat afraid to do that for two reasons:
...
> 2. Pasting "over" the image will lose things that the page has done to the image (event listeners, inline style, etc.) that aren't persisted in the image itself.

Will also cause DOM mutation, which would generate unexpected events and potentially break javascript that has already cached existing elements, for example.

I think this idea is unworkable.

[Tim Horton]

Follow up build fix in http://trac.webkit.org/changeset/165504