Bug 163292

Summary: Implement displaying context menu asynchronously
Product: WebKit Reporter: Damian Kaleta <dkaleta>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 10   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213793    
Attachments:
Description Flags
Proposed patch.
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch none

Description Damian Kaleta 2016-10-11 12:32:59 PDT
Currently WebKit doesn't allow any client to display the context menu asynchronously. We should add ability for the client to provide the list of context menu items to WebKit asynchronously.
Comment 1 Damian Kaleta 2016-10-13 16:38:01 PDT
Created attachment 291536 [details]
Proposed patch.
Comment 2 Damian Kaleta 2016-10-14 11:02:15 PDT
Created attachment 291651 [details]
Patch
Comment 3 WebKit Commit Bot 2016-10-14 11:04:44 PDT
Attachment 291651 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebContextMenuListenerProxy.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WebKit2/UIProcess/API/C/WKContextMenuListener.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brady Eidson 2016-10-14 16:33:02 PDT
Comment on attachment 291651 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:857
> +            if (m_client.base.version <= 3 || !m_client.getContextMenuFromProposedMenuAsync)

How about:
m_client.base.version < 4

It reads better, I think, referencing the version where the new bits were added.

> Source/WebKit2/UIProcess/WebContextMenuListenerProxy.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Update copyright for new files

> Source/WebKit2/UIProcess/WebContextMenuListenerProxy.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Update the copyright date for new files

> Source/WebKit2/UIProcess/WebContextMenuListenerProxy.h:27
> +#ifndef WebContextMenuListenerProxy_h
> +#define WebContextMenuListenerProxy_h

#pragma once

> Source/WebKit2/UIProcess/WebContextMenuListenerProxy.h:47
> +    void useProposedContextMenuItems(WKArrayRef items);

The initial call out to the WK client is "get context menu from proposed menu".

So I wouldn't call the returned menu the "proposed menu" as it has assumedly undergone changes.

I'd suggest "useContentMenuItems"

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:470
> +    if (m_contextMenuListener) {
> +        m_contextMenuListener->invalidate();
> +        m_contextMenuListener = nullptr;
> +    }

Very important to invalidate this in the destructor as well.
Comment 5 Damian Kaleta 2016-10-17 12:10:43 PDT
Created attachment 291855 [details]
Patch
Comment 6 Damian Kaleta 2016-10-18 16:40:03 PDT
Created attachment 292002 [details]
Patch
Comment 7 Build Bot 2016-10-18 18:02:13 PDT
Comment on attachment 292002 [details]
Patch

Attachment 292002 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2319358

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-element/security.dataURI.html
Comment 8 Build Bot 2016-10-18 18:02:15 PDT
Created attachment 292015 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Brady Eidson 2016-10-19 08:43:04 PDT
For the iOS build breakage, you'll have to wrap new stuff in #if ENABLE(CONTEXT_MENUS) because they are disabled.
Comment 10 Brady Eidson 2016-10-19 08:48:11 PDT
For the linux build breakage, there's an unimplemented virtual function in the base class:
virtual void showContextMenuWithItems(const Vector<WebContextMenuItemData>& items) = 0;

You'll need to stub it out in WebContextMenuProxyGtk.h and WebContextMenuProxyEfl.h
Comment 11 Damian Kaleta 2016-10-19 10:24:09 PDT
Created attachment 292080 [details]
Patch
Comment 12 WebKit Commit Bot 2016-10-19 12:48:32 PDT
Comment on attachment 292080 [details]
Patch

Clearing flags on attachment: 292080

Committed r207558: <http://trac.webkit.org/changeset/207558>
Comment 13 Michael Catanzaro 2018-01-15 12:44:44 PST
This seems to be fixed, right?
Comment 14 Radar WebKit Bug Importer 2018-01-15 12:45:31 PST
<rdar://problem/36526665>