Bug 48699 – Context menu support for WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Context menu support for WebKit2

[Brady Eidson]

Patches coming shortly, starting with some small WebCore/ChromeClient changes.

[Brady Eidson]

In radar as <rdar://problem/7660547>

[Brady Eidson]

Created attachment 72420 [details]
Part 1 - lay Chrome/ChromeClient groundwork in all the WebKits

Just get all this crazy ChromeClient stuff in place before the serious WK2 patch comes.

[WebKit Commit Bot]

Comment on attachment 72420 [details]
Part 1 - lay Chrome/ChromeClient groundwork in all the WebKits

Rejecting patch 72420 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 72420]" exit_code: 1
Last 500 characters of output:
.cgi?id=72420&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=48699&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 72420 from bug 48699.
NOBODY (OOPS!) found in /Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/4790080

[Brady Eidson]

Created attachment 72444 [details]
Implement cross platform infrastructure and actual menu implementation for Mac

Wasn't getting any review traction trying to lay in a small piece fist, may as well put the whole thing up.

[WebKit Review Bot]

Attachment 72444 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 72444 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4745097

[Brady Eidson]

Created attachment 72445 [details]
Patch v2 - Fix Qt (and presumably the Windows) builds with the right client method signatures.

[WebKit Review Bot]

Attachment 72445 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 72445 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4854111

[Brady Eidson]

Created attachment 72446 [details]
v3 - Now with more colons!

[Csaba Osztrogonác]

(In reply to comment #11)
> Created an attachment (id=72446) [details]
> v3 - Now with more colons!

WebKit2/win/WebKit2.vcproj is in conflict with ToT. 
Could you update? EWS bots can test only appliable patches.

[Brady Eidson]

Created attachment 72462 [details]
v4 - Updated to this morning

I updated and there weren't any conflicts with the WebKit2.vcproj, but reattaching the new patch anyways...

[WebKit Review Bot]

Attachment 72462 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 72462 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4855105

[Brady Eidson]

Created attachment 72474 [details]
v5 - More Qt build fixing

[WebKit Review Bot]

Attachment 72474 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 72474 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4797111

[Adam Roben (:aroben)]

(In reply to comment #17)
> Attachment 72474 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> Total errors found: 2 in 52 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

You should fix these style errors.

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #17)
> > Attachment 72474 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> > WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> > WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> > Total errors found: 2 in 52 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> You should fix these style errors.

Do you mean I should file a bug on check-webkit-style, as the style errors are bogus?

[Brady Eidson]

Will work on getting Qt (and presumably Windows as it falls in a similar situation) building this morning, but the fixes will be stubs - someone still can review this...  :)

[Adam Roben (:aroben)]

(In reply to comment #20)
> (In reply to comment #19)
> > You should fix these style errors.
> 
> Do you mean I should file a bug on check-webkit-style, as the style errors are bogus?

I didn't notice they were bogus. You should definitely file a bug!

[Brady Eidson]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > You should fix these style errors.
> > 
> > Do you mean I should file a bug on check-webkit-style, as the style errors are bogus?
> 
> I didn't notice they were bogus. You should definitely file a bug!

Upon reading the official style guidelines, they are not bogus.

But they are an exception to the "no indented code in namespaces" rule that we use liberally throughout the project - forward declarations of other-namespaced types in headers.

I suppose an email to webkit-dev to formalize that rule is in order.

[Brady Eidson]

Created attachment 72514 [details]
v6 - Hopefully the final build fix

[WebKit Review Bot]

Attachment 72514 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 72514 [details]
v6 - Hopefully the final build fix 

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

> WebCore/platform/qt/ContextMenuQt.cpp:82
> +    return result;

Can just return a temporary here:

return Vector<ContextMenuItem>();

> WebCore/platform/win/ContextMenuWin.cpp:164
> +}

Can just return a temporary here:

return Vector<ContextMenuItem>();

> WebKit2/Shared/WebContextMenuItem.cpp:61
> +    ASSERT(type == WebCore::SubmenuType);

If you assert that the type is SubmenuType do you even need to pass it to the constructor?

> WebKit2/Shared/WebContextMenuItem.cpp:64
> +WebContextMenuItem::WebContextMenuItem(WebCore::ContextMenuItem& item, WebCore::ContextMenu* menu)

Can this be a const reference?

> WebKit2/Shared/WebContextMenuItem.h:43
> +struct WebContextMenuItem {

Why is this a struct and not a class?

> WebKit2/UIProcess/WebContextMenuProxy.h:45
> +    }

It's better if you define the destructor in the .cpp file instead; that way the vtable won't be weak and emitted everywhere.

> WebKit2/UIProcess/WebContextMenuProxy.h:53
> +    }

Might define the constructor in the .cpp file too.

> WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:52
> +        target = [[WebMenuTarget alloc] init];

You can just initialize target directly here.

> WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:112
> +        {

{ goes on the same line as the case.

> WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:125
> +        {            

{ goes on the same line as the case.

> WebKit2/WebProcess/WebPage/WebContextMenu.h:35
> +    static PassRefPtr<WebContextMenu> create(WebPage* page) { return adoptRef(new WebContextMenu(page)); }

{ should go on a new line, as should the return statement and the closing brace.

> WebKit2/WebProcess/WebPage/WebPage.cpp:912
> +    m_contextMenu->itemSelected(item);

Could you zero out the m_contextMenu variable here instead of keeping it around for the lifetime of the page?

[Brady Eidson]

(In reply to comment #26)
> (From update of attachment 72514 [details])
> > WebKit2/Shared/WebContextMenuItem.cpp:64
> > +WebContextMenuItem::WebContextMenuItem(WebCore::ContextMenuItem& item, WebCore::ContextMenu* menu)
> 
> Can this be a const reference?

No - WebCore actually modifies the ContextMenuItem when doing the "checkOrEnableIfNeeded() on it.  I'm not happy about this.

> > WebKit2/Shared/WebContextMenuItem.h:43
> > +struct WebContextMenuItem {
> 
> Why is this a struct and not a class?

Fixing.

> 
> > WebKit2/UIProcess/WebContextMenuProxy.h:45
> > +    }
> 
> It's better if you define the destructor in the .cpp file instead; that way the vtable won't be weak and emitted everywhere.
> Might define the constructor in the .cpp file too.

Done.

> > WebKit2/WebProcess/WebPage/WebPage.cpp:912
> > +    m_contextMenu->itemSelected(item);
> 
> Could you zero out the m_contextMenu variable here instead of keeping it around for the lifetime of the page?

Yup.

[Brady Eidson]

Created attachment 72524 [details]
v7 - with Anders' feedback

[WebKit Review Bot]

Attachment 72524 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/WebContextMenuProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Shared/WebContextMenuItem.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Landed in r71041

[Brady Eidson]

(In reply to comment #30)
> Landed in r71041

With a Windows build fix in r71042