Bug 120459

Summary: [GTK] [WK2] TestContextMenu default-menu fails
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: WebKitGTKAssignee: Brian Holt <brian.holt>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, gns, mario, mrobinson, ruthiecftg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117689    
Attachments:
Description Flags
Patch
none
Patch 2 none

Description Brian Holt 2013-08-29 02:18:36 PDT
The unit test for the default menu fails:

ERROR:../../Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:191:GList* ContextMenuTest::checkCurrentItemIsSeparatorAndGetNext(GList*): assertion failed: (webkit_context_menu_item_is_separator(item))
Comment 1 Brian Holt 2013-08-29 07:19:34 PDT
The root cause is that there the test was not updated to reflect that a new tag to download the media item has been been added to the default context menu in https://bugs.webkit.org/show_bug.cgi?id=20599.
Comment 2 Brian Holt 2013-08-29 07:56:59 PDT
Created attachment 209981 [details]
Patch
Comment 3 WebKit Commit Bot 2013-08-29 07:58:12 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 Brian Holt 2013-08-29 07:58:54 PDT
This doesn't fix the timeout under Xvfb, but the unit test now passes under run-gtk-tests --display=":0".
Comment 5 Mario Sanchez Prada 2013-09-02 09:23:14 PDT
Comment on attachment 209981 [details]
Patch

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

The patch looks almost good to me (see comments below), and I can confirm that it works fine since I applied locally to test it :)

Anyway, as you are exposing new items in the public API (the enum), I'd rather have someone else reviewing it.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.cpp:130
> +    case WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK:
> +    case WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK:
> +        return ContextMenuItemTagDownloadMediaToDisk;

I would probably add this before WEBKIT_CONTEXT_MENU_ACTION_CUSTOM case.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.cpp:224
> +    case ContextMenuItemTagDownloadMediaToDisk:
> +        return menuItem->title() == contextMenuItemTagDownloadVideoToDisk() ?
> +            WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK : WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.cpp:322
> +    case WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK:
> +        return contextMenuItemTagDownloadVideoToDisk();
> +    case WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK:
> +        return contextMenuItemTagDownloadAudioToDisk();

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:77
> + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK: Download video to disk.
> + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK: Download audio to disk.

And again! :)
Comment 6 Brian Holt 2013-09-03 02:46:03 PDT
Created attachment 210342 [details]
Patch 2
Comment 7 Martin Robinson 2013-09-03 09:08:49 PDT
Seems reasonable to me. Since this changes API, it needs the approval of another GTK+ reviewer.
Comment 8 Gustavo Noronha (kov) 2013-09-03 09:25:41 PDT
Comment on attachment 210342 [details]
Patch 2

Sure, the API is straightforward, LGTM. Guess I'll just go ahead and set +ses
Comment 9 WebKit Commit Bot 2013-09-03 09:50:03 PDT
Comment on attachment 210342 [details]
Patch 2

Clearing flags on attachment: 210342

Committed r154987: <http://trac.webkit.org/changeset/154987>
Comment 10 WebKit Commit Bot 2013-09-03 09:50:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Carlos Garcia Campos 2013-09-04 23:39:51 PDT
Comment on attachment 210342 [details]
Patch 2

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

I'm sorry to be late here, I have just some comments. I think that a commit that adds new API should not be hidden by a commit message saying that this fixes a unit test, even if in fact it fixes a test. I think this should have renamed as something like "Add download media to disk actions to the context menu actions in WebKit2GTK+" or something similar before landing.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:76
> + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK: Download video to disk.
> + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK: Download audio to disk.

These should have a Since tag, it should be 2.2 if we are going to merge this in the stable branch (I think we should, fwiw) or 2.4 otherwise.

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:312
> +            iter = checkCurrentItemIsStockActionAndGetNext(iter, WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK, Visible | Enabled);

Would it be posible to add a test for download audio too?
Comment 12 Brian Holt 2013-09-05 02:10:46 PDT
(In reply to comment #11)
> (From update of attachment 210342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210342&action=review
> 
> I'm sorry to be late here, I have just some comments. I think that a commit that adds new API should not be hidden by a commit message saying that this fixes a unit test, even if in fact it fixes a test. I think this should have renamed as something like "Add download media to disk actions to the context menu actions in WebKit2GTK+" or something similar before landing.
> 

Good point, I should have thought of that.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:76
> > + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK: Download video to disk.
> > + * @WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK: Download audio to disk.
> 
> These should have a Since tag, it should be 2.2 if we are going to merge this in the stable branch (I think we should, fwiw) or 2.4 otherwise.

Opening a new bug for this: https://bugs.webkit.org/show_bug.cgi?id=120763

> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:312
> > +            iter = checkCurrentItemIsStockActionAndGetNext(iter, WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK, Visible | Enabled);
> 
> Would it be posible to add a test for download audio too?

Will sort it in the new bug.