REGRESSION (r146518): WebKit2APITests/TestInspector is failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 195061 [details]
Patch

[WebKit Review 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

[Zan Dobersek]

Waiting for the thumbs-up from a GTK reviewer, will look for a WK2 owner afterwards.

[Zan Dobersek]

Created attachment 195065 [details]
Patch

[Martin Robinson]

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]

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]

Created attachment 195364 [details]
Patch

Added the explanation to the ChangeLog.

[Martin Robinson]

Comment on attachment 195364 [details]
Patch

Okay. Looks good to me. We need an owner to r+ this.

[Carlos Garcia Campos]

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

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

Created attachment 196123 [details]
Patch

[Martin Robinson]

CCing some owners so they can give the r+.

[Zan Dobersek]

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 on attachment 196123 [details]
Patch

Looks good to me.

[Darin Adler]

Comment on attachment 196123 [details]
Patch

OK.

[Zan Dobersek]

Comment on attachment 196123 [details]
Patch

Clearing flags on attachment: 196123

Committed r148083: <http://trac.webkit.org/changeset/148083>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.