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.
Created attachment 225187 [details] Patch v1
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.
Created attachment 225211 [details] Patch v2 - Attempted build fixes (and review feedback)
Created attachment 225219 [details] Patch v3 - More build fixes
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"
Created attachment 225223 [details] Patch v4 - New "ContextMenuContext" class to encompass this notion of data relevant to a ContextMenu invocation
Created attachment 225249 [details] Patch v6 - Build fixes
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
(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.
http://trac.webkit.org/changeset/164720
Plus https://trac.webkit.org/r164723 to fix the release build.
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.
(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.