Bug 163292 - Implement displaying context menu asynchronously
Summary: Implement displaying context menu asynchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-11 12:32 PDT by Damian Kaleta
Modified: 2018-01-15 12:45 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch. (31.47 KB, patch)
2016-10-13 16:38 PDT, Damian Kaleta
no flags Details | Formatted Diff | Diff
Patch (30.95 KB, patch)
2016-10-14 11:02 PDT, Damian Kaleta
no flags Details | Formatted Diff | Diff
Patch (32.02 KB, patch)
2016-10-17 12:10 PDT, Damian Kaleta
no flags Details | Formatted Diff | Diff
Patch (32.49 KB, patch)
2016-10-18 16:40 PDT, Damian Kaleta
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.57 MB, application/zip)
2016-10-18 18:02 PDT, Build Bot
no flags Details
Patch (35.19 KB, patch)
2016-10-19 10:24 PDT, Damian Kaleta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>