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))
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.
Created attachment 209981 [details] Patch
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
This doesn't fix the timeout under Xvfb, but the unit test now passes under run-gtk-tests --display=":0".
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! :)
Created attachment 210342 [details] Patch 2
Seems reasonable to me. Since this changes API, it needs the approval of another GTK+ reviewer.
Comment on attachment 210342 [details] Patch 2 Sure, the API is straightforward, LGTM. Guess I'll just go ahead and set +ses
Comment on attachment 210342 [details] Patch 2 Clearing flags on attachment: 210342 Committed r154987: <http://trac.webkit.org/changeset/154987>
All reviewed patches have been landed. Closing bug.
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?
(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.