Bug 130424 - Make image controls menu work in WK2
Summary: Make image controls menu work in WK2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-18 14:39 PDT by Brady Eidson
Modified: 2014-03-18 16:55 PDT (History)
6 users (show)

See Also:

Attachments
Patch v1 (26.40 KB, patch)
2014-03-18 14:44 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch v2 - Build fixes (29.29 KB, patch)
2014-03-18 15:03 PDT, Brady Eidson 		beidson: commit-queue-
Details | Formatted Diff | Diff
Patch v3 - More build fixes (29.89 KB, patch)
2014-03-18 16:18 PDT, Brady Eidson 		thorton: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-03-18 14:39:21 PDT 
Make image controls menu work in WK2
Comment 1 Brady Eidson 2014-03-18 14:39:47 PDT 
<rdar://problem/16294412>
Comment 2 Brady Eidson 2014-03-18 14:44:06 PDT 
Created attachment 227110 [details]
Patch v1
Comment 3 Brady Eidson 2014-03-18 15:03:31 PDT 
Created attachment 227115 [details]
Patch v2 - Build fixes
Comment 4 Brady Eidson 2014-03-18 16:18:40 PDT 
Created attachment 227127 [details]
Patch v3 - More build fixes
Comment 5 WebKit Commit Bot 2014-03-18 16:21:22 PDT 
Attachment 227127 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:57:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Brady Eidson 2014-03-18 16:25:05 PDT 
(In reply to comment #5)
> Attachment 227127 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:57:  Tab found; better to use spaces  [whitespace/tab] [1]
> Total errors found: 1 in 22 files
> 

Whoops.  Fixed locally.
Comment 7 Tim Horton 2014-03-18 16:43:59 PDT 
Comment on attachment 227127 [details]
Patch v3 - More build fixes

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

> Source/WebKit2/ChangeLog:47
> +        (-[WKSharingServicePickerDelegate WebKit::]):

WebKit:: :|

> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:57
> +void SharedMemory::Handle::clear()

maybe “release” is a better name than “clear”… I don’t know. More evident that you’re losing something.

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:177
> +    // FIXME: What do we do if there are multiple items?

Please remove my silly comment (for now).

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:193
> +- (NSRect)sharingService:(NSSharingService *)sharingService sourceFrameOnScreenForShareItem:(id <NSPasteboardWriting>)item
> +{
> +    notImplemented();
> +    return NSZeroRect;
> +}
> +
> +- (NSImage *)sharingService:(NSSharingService *)sharingService transitionImageForShareItem:(id <NSPasteboardWriting>)item contentRect:(NSRect *)contentRect
> +{
> +    notImplemented();
> +    return nil;
> +}

I think we can elide these.

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:354
> +    NSMenu* menu = m_servicesMenu ? m_servicesMenu.get() : [m_popup menu];

star on the wrong side

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3088
> +    m_contextMenu->replaceControlledImage(bitmap->createImage());

what happens in createImage if bitmapHandle.isNull()?
Comment 8 Brady Eidson 2014-03-18 16:55:26 PDT 
Addressed most feedback in most obvious ways possible.

https://trac.webkit.org/changeset/165856