RESOLVED FIXED 113281
REGRESSION (r146518): WebKit2APITests/TestInspector is failing
https://bugs.webkit.org/show_bug.cgi?id=113281
Summary REGRESSION (r146518): WebKit2APITests/TestInspector is failing
Zan Dobersek
Reported 2013-03-26 01:18:33 PDT
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
Attachments
Patch (6.23 KB, patch)
2013-03-26 05:20 PDT, Zan Dobersek
no flags
Patch (8.03 KB, patch)
2013-03-26 05:41 PDT, Zan Dobersek
no flags
Patch (8.87 KB, patch)
2013-03-27 11:27 PDT, Zan Dobersek
no flags
Patch (11.82 KB, patch)
2013-04-02 04:59 PDT, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2013-03-26 05:20:11 PDT
WebKit Review Bot
Comment 2 2013-03-26 05:24:01 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
Zan Dobersek
Comment 3 2013-03-26 05:36:17 PDT
Waiting for the thumbs-up from a GTK reviewer, will look for a WK2 owner afterwards.
Zan Dobersek
Comment 4 2013-03-26 05:41:50 PDT
Martin Robinson
Comment 5 2013-03-26 12:30:53 PDT
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?
Zan Dobersek
Comment 6 2013-03-26 13:24:44 PDT
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.
Zan Dobersek
Comment 7 2013-03-27 11:27:40 PDT
Created attachment 195364 [details] Patch Added the explanation to the ChangeLog.
Martin Robinson
Comment 8 2013-03-27 13:37:23 PDT
Comment on attachment 195364 [details] Patch Okay. Looks good to me. We need an owner to r+ this.
Carlos Garcia Campos
Comment 9 2013-03-28 01:20:10 PDT
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.
Zan Dobersek
Comment 10 2013-03-28 03:51:47 PDT
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.
Carlos Garcia Campos
Comment 11 2013-03-28 05:30:46 PDT
(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.
Zan Dobersek
Comment 12 2013-04-02 04:59:46 PDT
Martin Robinson
Comment 13 2013-04-02 11:51:43 PDT
CCing some owners so they can give the r+.
Zan Dobersek
Comment 14 2013-04-02 12:59:34 PDT
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?
Timothy Hatcher
Comment 15 2013-04-02 13:09:37 PDT
Comment on attachment 196123 [details] Patch Looks good to me.
Darin Adler
Comment 16 2013-04-08 10:29:01 PDT
Comment on attachment 196123 [details] Patch OK.
Zan Dobersek
Comment 17 2013-04-10 00:37:49 PDT
Comment on attachment 196123 [details] Patch Clearing flags on attachment: 196123 Committed r148083: <http://trac.webkit.org/changeset/148083>
Zan Dobersek
Comment 18 2013-04-10 00:37:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.