Bug 130062

Summary: Hook up image controls for WebKit1
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebCore Misc.Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, kangil.han, kondapallykalyan, macpherson, menard, sam, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
beidson: review-
patch beidson: review+

Description Tim Horton 2014-03-10 19:14:29 PDT
<rdar://problem/15964809>
Comment 1 Tim Horton 2014-03-10 19:38:02 PDT
Created attachment 226373 [details]
patch
Comment 2 WebKit Commit Bot 2014-03-10 19:40:37 PDT
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.
Comment 3 Tim Horton 2014-03-11 15:35:37 PDT
Created attachment 226436 [details]
patch
Comment 4 Darin Adler 2014-03-11 16:27:19 PDT
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?
Comment 5 Brady Eidson 2014-03-11 16:35:53 PDT
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?
Comment 6 Tim Horton 2014-03-11 16:46:32 PDT
(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.
Comment 7 Tim Horton 2014-03-11 18:52:16 PDT
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.
Comment 8 Tim Horton 2014-03-11 19:18:40 PDT
Created attachment 226465 [details]
patch

Addressing most of the review comments. The bit about copying above still stands.
Comment 9 Brady Eidson 2014-03-11 20:09:57 PDT
Comment on attachment 226465 [details]
patch

I'm fine with this assuming EWS, etc etc!
Comment 10 Tim Horton 2014-03-12 00:52:02 PDT
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.
Comment 11 Tim Horton 2014-03-12 10:57:53 PDT
http://trac.webkit.org/changeset/165479
Comment 12 Brady Eidson 2014-03-12 13:21:09 PDT
(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.
Comment 13 Tim Horton 2014-03-12 16:35:48 PDT
Follow up build fix in http://trac.webkit.org/changeset/165504