Bug 129339 – Pipe experimental image controls menu up to WebKit2

• Component: Images
• Status: RESOLVED FIXED

[Brady Eidson]

Pipe experimental image controls menu up to WebKit2

Doesn't do anything at the moment, but is an easily reviewable patch separate from the patch that does something with it.

[Brady Eidson]

Created attachment 225187 [details]
Patch v1

[Tim Horton]

Comment on attachment 225187 [details]
Patch v1

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

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

maybe there's a case for flipping the logic here?

{
    if (Page* page = document().page())
        page->contextMenuController().showImageControlsMenu(event);

    return;
}

> Source/WebCore/rendering/HitTestResult.cpp:61
> +    , m_isImageControl(false)

I dunno about all this. Needs someone else's input.

[Brady Eidson]

Created attachment 225211 [details]
Patch v2 - Attempted build fixes (and review feedback)

[Brady Eidson]

Created attachment 225219 [details]
Patch v3 - More build fixes

[Brady Eidson]

With some IRC discussion about HitTestResult, I agree that's the wrong place for it (and that a lot of other out-of-place cruft has been placed there).

New version of the patch coming with a new class to hold "ContextMenu related data"

[Brady Eidson]

Created attachment 225223 [details]
Patch v4 - New "ContextMenuContext" class to encompass this notion of data relevant to a ContextMenu invocation

[Brady Eidson]

Created attachment 225249 [details]
Patch v6 - Build fixes

[Simon Fraser (smfr)]

Comment on attachment 225249 [details]
Patch v6 - Build fixes

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

> Source/WebCore/ChangeLog:8
> +        Handle events for the image control starting the context menu process if appropriate:

"context menu process" confused me

> Source/WebCore/page/ContextMenuContext.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/page/ContextMenuContext.h:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/page/ContextMenuContext.h:39
> +    ContextMenuContext(const HitTestResult&, bool isImageControl = false);

If we end up adding lots of stuff to ContextMenuContext, this isImageControl param doesn't seem like a good start for extensibility.

> Source/WebKit2/Shared/ContextMenuContextData.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2014

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 225249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225249&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Handle events for the image control starting the context menu process if appropriate:
> 
> "context menu process" confused me

Edited for clarity.

> > Source/WebCore/page/ContextMenuContext.h:39
> > +    ContextMenuContext(const HitTestResult&, bool isImageControl = false);
> 
> If we end up adding lots of stuff to ContextMenuContext, this isImageControl param doesn't seem like a good start for extensibility.

Agreed - It will even change in my immediate followup to this patch.

I expect this class will evolve (rapidly) as we move gunk off of HitTestResult that belongs here.

[Brady Eidson]

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

[Eric Carlson]

Plus https://trac.webkit.org/r164723 to fix the release build.

[Darin Adler]

Comment on attachment 225249 [details]
Patch v6 - Build fixes

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

> Source/WebCore/page/ContextMenuContext.cpp:47
> +    ASSERT_UNUSED(isImageControl, true);

This is wrong. It’s the same as:

    ASSERT(true);

as far as the assertion is concerned. I think you should just remove this.

[Brady Eidson]

(In reply to comment #12)
> (From update of attachment 225249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225249&action=review
> 
> > Source/WebCore/page/ContextMenuContext.cpp:47
> > +    ASSERT_UNUSED(isImageControl, true);
> 
> This is wrong. It’s the same as:
> 
>     ASSERT(true);
> 
> as far as the assertion is concerned. I think you should just remove this.

Yah, what I was really looking for was UNUSED_PARAM, but I completely drew a blank.