RESOLVED FIXED 129339
Pipe experimental image controls menu up to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=129339
Summary Pipe experimental image controls menu up to WebKit2
Brady Eidson
Reported 2014-02-25 14:59:14 PST
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.
Attachments
Patch v1 (12.40 KB, patch)
2014-02-25 15:03 PST, Brady Eidson
no flags
Patch v2 - Attempted build fixes (and review feedback) (12.29 KB, patch)
2014-02-25 18:44 PST, Brady Eidson
no flags
Patch v3 - More build fixes (12.47 KB, patch)
2014-02-25 21:09 PST, Brady Eidson
no flags
Patch v4 - New "ContextMenuContext" class to encompass this notion of data relevant to a ContextMenu invocation (60.02 KB, patch)
2014-02-25 22:54 PST, Brady Eidson
no flags
Patch v6 - Build fixes (60.06 KB, patch)
2014-02-26 06:11 PST, Brady Eidson
simon.fraser: review+
Brady Eidson
Comment 1 2014-02-25 15:03:45 PST
Created attachment 225187 [details] Patch v1
Tim Horton
Comment 2 2014-02-25 16:09:33 PST
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
Comment 3 2014-02-25 18:44:26 PST
Created attachment 225211 [details] Patch v2 - Attempted build fixes (and review feedback)
Brady Eidson
Comment 4 2014-02-25 21:09:29 PST
Created attachment 225219 [details] Patch v3 - More build fixes
Brady Eidson
Comment 5 2014-02-25 22:09:11 PST
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
Comment 6 2014-02-25 22:54:58 PST
Created attachment 225223 [details] Patch v4 - New "ContextMenuContext" class to encompass this notion of data relevant to a ContextMenu invocation
Brady Eidson
Comment 7 2014-02-26 06:11:02 PST
Created attachment 225249 [details] Patch v6 - Build fixes
Simon Fraser (smfr)
Comment 8 2014-02-26 09:46:05 PST
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
Comment 9 2014-02-26 09:55:49 PST
(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
Comment 10 2014-02-26 09:57:37 PST
Eric Carlson
Comment 11 2014-02-26 10:31:52 PST
Plus https://trac.webkit.org/r164723 to fix the release build.
Darin Adler
Comment 12 2014-02-26 13:59:25 PST
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
Comment 13 2014-02-26 14:39:14 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.