WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch v2 - Attempted build fixes (and review feedback)
(12.29 KB, patch)
2014-02-25 18:44 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch v3 - More build fixes
(12.47 KB, patch)
2014-02-25 21:09 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Patch v6 - Build fixes
(60.06 KB, patch)
2014-02-26 06:11 PST
,
Brady Eidson
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
http://trac.webkit.org/changeset/164720
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.
Top of Page
Format For Printing
XML
Clone This Bug