The WebKit2APITests/TestInspector unit test is failing after r146518. http://trac.webkit.org/changeset/146518 TEST: ./Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/WebKit2APITests/TestInspector... (pid=28658) /webkit2/WebKitWebInspector/default: ** ERROR:../../Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:169:void testInspectorDefault(InspectorTest*, gconstpointer): assertion failed (events.size() == 2): (1 == 2) FAIL
Created attachment 195061 [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
Waiting for the thumbs-up from a GTK reviewer, will look for a WK2 owner afterwards.
Created attachment 195065 [details] Patch
It's unclear from the patch what the original issue was. Do you mind expanding the ChangeLog a bit or leaving a comment here outlining the problem?
Reconstruction of the WebInspectorProxy opening processing in r146518 caused the change in how the GTK-specific WebInspectorProxy code operates, specifically the 'bring-to-front' signal is not emitted anymore when opening the inspector due to the WebInspectorProxy::bringToFront method now only bringing the inspector window to front if it exists and opening it (and thus being unable to bring it to front) otherwise. Closing of the inspector through the didClose method is now done immediately after sending the WebInspector::Close() message to the WebProcess rather than waiting for the WebProcess to communicate back the order of closing. Due to this the relevant code in the test cases had to be changed to not run the loop but rather just check that the closing was successful.
Created attachment 195364 [details] Patch Added the explanation to the ChangeLog.
Comment on attachment 195364 [details] Patch Okay. Looks good to me. We need an owner to r+ this.
Comment on attachment 195364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195364&action=review > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:154 > - open(); > + m_page->process()->send(Messages::WebInspector::Show(), m_page->pageID()); Do we really need to send the Show message again to the WebProcess? I think that what we want here is calling bringToFront(). Since the inspector is connected m_ignoreFirstBringToFront will be false, and platformBringToFront() or open() will be called depending on whether the inspector is visible or not.
Comment on attachment 195364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195364&action=review >> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:154 >> + m_page->process()->send(Messages::WebInspector::Show(), m_page->pageID()); > > Do we really need to send the Show message again to the WebProcess? I think that what we want here is calling bringToFront(). Since the inspector is connected m_ignoreFirstBringToFront will be false, and platformBringToFront() or open() will be called depending on whether the inspector is visible or not. That's fine by me, but note that this introduces the following changes in behavior: * the 'bring-to-front' signal is fired immediately after webkit_web_inspector_show is called on an already-opened web inspector, * the 'open-window' signal is fired immediately after webkit_web_inspector_show is called on an already-connected web inspector. The current patch already deviates from the current behavior with the 'closed' signal fired immediately upon webkit_web_inspector_close being called (hence no more waiting for the closing signal since it's now called immediately). I don't know if it's smart to introduce these changes to an already stable API, I'll let other people make that decision. But yes, calling bringToFront() here works as well, though it introduces behavior changes in how the current API operates.
(In reply to comment #10) > (From update of attachment 195364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195364&action=review > > >> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:154 > >> + m_page->process()->send(Messages::WebInspector::Show(), m_page->pageID()); > > > > Do we really need to send the Show message again to the WebProcess? I think that what we want here is calling bringToFront(). Since the inspector is connected m_ignoreFirstBringToFront will be false, and platformBringToFront() or open() will be called depending on whether the inspector is visible or not. > > That's fine by me, but note that this introduces the following changes in behavior: > * the 'bring-to-front' signal is fired immediately after webkit_web_inspector_show is called on an already-opened web inspector, > * the 'open-window' signal is fired immediately after webkit_web_inspector_show is called on an already-connected web inspector. There's no problem, nothing in our documentation says this shouldn't happen immediately. > The current patch already deviates from the current behavior with the 'closed' signal fired immediately upon webkit_web_inspector_close being called (hence no more waiting for the closing signal since it's now called immediately). I don't know if it's smart to introduce these changes to an already stable API, I'll let other people make that decision. > > But yes, calling bringToFront() here works as well, though it introduces behavior changes in how the current API operates. Those changes in behaviour doesn't affect the way users use the API.
Created attachment 196123 [details] Patch
CCing some owners so they can give the r+.
Hi Timothy -- this patch forces any connected WebInspectorProxy to call the bringToFront method instead of directly going to opening (which is meaningless if the connected inspector is already open), adjusting slightly the code added in r146518. Does this look OK?
Comment on attachment 196123 [details] Patch Looks good to me.
Comment on attachment 196123 [details] Patch OK.
Comment on attachment 196123 [details] Patch Clearing flags on attachment: 196123 Committed r148083: <http://trac.webkit.org/changeset/148083>
All reviewed patches have been landed. Closing bug.