Bug 120459 – [GTK] [WK2] TestContextMenu default-menu fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Brian Holt]

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))

[Brian Holt]

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.

[Brian Holt]

Created attachment 209981 [details]
Patch

[WebKit Commit Bot]

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

[Brian Holt]

This doesn't fix the timeout under Xvfb, but the unit test now passes under run-gtk-tests --display=":0".

[Mario Sanchez Prada]

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! :)

[Brian Holt]

Created attachment 210342 [details]
Patch 2

[Martin Robinson]

Seems reasonable to me. Since this changes API, it needs the approval of another GTK+ reviewer.

[Gustavo Noronha (kov)]

Comment on attachment 210342 [details]
Patch 2

Sure, the API is straightforward, LGTM. Guess I'll just go ahead and set +ses

[WebKit Commit Bot]

Comment on attachment 210342 [details]
Patch 2

Clearing flags on attachment: 210342

Committed r154987: <http://trac.webkit.org/changeset/154987>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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?

[Brian Holt]

(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.