Bug 48699

Summary: Context menu support for WebKit2
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, commit-queue, ossy, sam, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Part 1 - lay Chrome/ChromeClient groundwork in all the WebKits
commit-queue: commit-queue-
Implement cross platform infrastructure and actual menu implementation for Mac
none
Patch v2 - Fix Qt (and presumably the Windows) builds with the right client method signatures.
none
v3 - Now with more colons!
none
v4 - Updated to this morning
none
v5 - More Qt build fixing
none
v6 - Hopefully the final build fix
none
v7 - with Anders' feedback andersca: review+

Description Brady Eidson 2010-10-29 17:12:41 PDT
Context menu support for WebKit2
Comment 1 Brady Eidson 2010-10-29 17:13:14 PDT
Patches coming shortly, starting with some small WebCore/ChromeClient changes.
Comment 2 Brady Eidson 2010-10-29 17:14:09 PDT
In radar as <rdar://problem/7660547>
Comment 3 Brady Eidson 2010-10-29 17:36:12 PDT
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.
Comment 4 WebKit Commit Bot 2010-10-29 17:53:12 PDT
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
Comment 5 Brady Eidson 2010-10-30 14:14:47 PDT
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.
Comment 6 WebKit Review Bot 2010-10-30 14:16:47 PDT
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.
Comment 7 Early Warning System Bot 2010-10-30 14:33:14 PDT
Attachment 72444 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4745097
Comment 8 Brady Eidson 2010-10-30 15:03:58 PDT
Created attachment 72445 [details]
Patch v2 - Fix Qt (and presumably the Windows) builds with the right client method signatures.
Comment 9 WebKit Review Bot 2010-10-30 15:08:39 PDT
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.
Comment 10 Early Warning System Bot 2010-10-30 15:15:51 PDT
Attachment 72445 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4854111
Comment 11 Brady Eidson 2010-10-30 15:25:49 PDT
Created attachment 72446 [details]
v3 - Now with more colons!
Comment 12 Csaba Osztrogon√°c 2010-10-31 06:01:19 PDT
(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.
Comment 13 Brady Eidson 2010-10-31 09:54:11 PDT
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...
Comment 14 WebKit Review Bot 2010-10-31 09:56:55 PDT
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.
Comment 15 Early Warning System Bot 2010-10-31 10:04:10 PDT
Attachment 72462 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4855105
Comment 16 Brady Eidson 2010-10-31 13:24:21 PDT
Created attachment 72474 [details]
v5 - More Qt build fixing
Comment 17 WebKit Review Bot 2010-10-31 13:27:48 PDT
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.
Comment 18 Early Warning System Bot 2010-10-31 13:36:22 PDT
Attachment 72474 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4797111
Comment 19 Adam Roben (:aroben) 2010-11-01 06:16:31 PDT
(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.
Comment 20 Brady Eidson 2010-11-01 08:37:28 PDT
(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?
Comment 21 Brady Eidson 2010-11-01 08:38:16 PDT
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...  :)
Comment 22 Adam Roben (:aroben) 2010-11-01 09:08:15 PDT
(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!
Comment 23 Brady Eidson 2010-11-01 09:13:11 PDT
(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.
Comment 24 Brady Eidson 2010-11-01 09:16:06 PDT
Created attachment 72514 [details]
v6 - Hopefully the final build fix
Comment 25 WebKit Review Bot 2010-11-01 09:19:36 PDT
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.
Comment 26 Anders Carlsson 2010-11-01 10:40:12 PDT
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?
Comment 27 Brady Eidson 2010-11-01 11:09:38 PDT
(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.
Comment 28 Brady Eidson 2010-11-01 11:15:05 PDT
Created attachment 72524 [details]
v7 - with Anders' feedback
Comment 29 WebKit Review Bot 2010-11-01 11:20:31 PDT
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.
Comment 30 Brady Eidson 2010-11-01 11:34:55 PDT
Landed in r71041
Comment 31 Brady Eidson 2010-11-01 11:56:40 PDT
(In reply to comment #30)
> Landed in r71041

With a Windows build fix in r71042