REGRESSION(r55107): Inspector docking is busted.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

REGRESSION(???): Inspector window appears and disappears when running the layout tests on mac

I didn't used to see any windows when running the layout tests. :(

[Eric Seidel (no email)]

I've not narrowed it down to a list of tests yet.  I just know that when I am running the tests (currently using run-chromium-webkit-tests) i see inspector windows appear and disappear.  run-chromium-webkit-tests uses the same DumpRenderTree as run-webkit-tests, so I very much doubt it's related to the test harness.

I didn't see this earlier today, so I think the regression may have started today.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/55107 looks suspect.

[Brady Eidson]

Here's the only thing that could be different:  
If your platform's layouttests show the web inspector for each test, and you used to dock the inspector to the window, but the window is too small to accept the docking, then it will no longer be allowed to dock and will open as it's own window instead.

We don't see this on Mac or Windows, so there must be something peculiar about what Chromium does with the web inspector during layouttests...

[Brady Eidson]

(In reply to comment #1)
> run-chromium-webkit-tests uses the same
> DumpRenderTree as run-webkit-tests, so I very much doubt it's related to the
> test harness.

Not specific to the testing harness, perhaps, but rather chromium's own settings for the layouttests

[Eric Seidel (no email)]

Oh, this has nothing to do with Chromium.   run-chromium-webkit-tests is poorly named (since it is just a python version of run-webkit-tests).  I'm running the normal Mac WebKit DumpRenderTree.

I need to rename run-chromium-webkit-tests.

I just verified that this occurs for me running:

run-webkit-tests inspector

as well.  Again, chromium is not related here.  Sorry for the confusion by mentioning run-chromium-webkit-tests.

[Brady Eidson]

Yup, I can see what eric sees with run-webkit-tests inspector.

I didn't see this because I was away from my computer while running the layouttests, and they still pass.

Since it is actually the intended functional progression intended by the patch, I guess we need to find a way to somehow suppress the inspector window altogether in this case.

[Pavel Feldman]

This is really annoying and should be fixed asap. Our tests are already taking too long, so we can't afford the luxury of opening inspector window.

Can we make sure that defaults (DRT's 800x600 and default inspector ratio) allow docking? That would make DRT happy. If not, we should extend DRT and disable this behavior explicitly.

Wrt to the original change, I am not sure the following scenario would work:

1)
- Open inspector in docked mode
- Maximize the window
- Undock inspector
- Restore original window size
- Dock inspector

2) Furthermore, I see no changes to the frontend and that makes me think that "dock" button will anyways be visible and enabled. What happens when user clicks it?

[Brady Eidson]

> Can we make sure that defaults (DRT's 800x600 and default inspector ratio)
> allow docking? That would make DRT happy. If not, we should extend DRT and
> disable this behavior explicitly.

Thinking it was very odd that docking wouldn't work with 800px height, I tried this locally.  It's completely busted.  I didn't notice the glaring bug because I happened to be testing with either "really tiny" window heights and 1000+px window heights. I know exactly what I need to do to fix it, and that will again hide the window in DRT.

That said, I have no tree to work out of right now and won't till tomorrow.  I'll pre-emptively rubberstamp someone rolling it out if they truly can't stand it until tomorrow, or I'll get to fixing it tomorrow morning.

That said...

(In reply to comment #7)
> This is really annoying and should be fixed asap. Our tests are already taking
> too long, so we can't afford the luxury of opening inspector window.

I agree the bug is annoying - but it does *NOT* make the inspector tests take longer.  Each test took about 1 second before my change, and each test takes about 1 second after my change.

Opening the web inspector is a painfully, unacceptably slow operation in general.  Doing so in DRT is no exception, even when the window is hidden.

> 2) Furthermore, I see no changes to the frontend and that makes me think that
> "dock" button will anyways be visible and enabled. What happens when user
> clicks it?

You're right, there were no front-end changes in this patch.  I thought I filed a followup, but now that I can't find it by searching from home, I'm sure my browser @ work is sitting on the submit screen reminding me I forgot to choose a component.  ;)

[Brady Eidson]

*** Bug 35293 has been marked as a duplicate of this bug. ***

[Brady Eidson]

Got the math wrong and the inspector won't dock unless the browser window is 1000+px high.

Also, even a failed docking attempt results in the inspector window itself changing into "docked" mode.

*sigh*

I am working on this right now.

[Brady Eidson]

Yah I completely botched this one.  A cleanup will be attached shortly.

In the meantime, I filed https://bugs.webkit.org/show_bug.cgi?id=35306 to cover disabling the Dock button when the attach can't take place.

That one will be easy to fix, but is logically separate from this bug and this one is much more important to get fixed asap.

[Brady Eidson]

Created attachment 49309 [details]
Make this work.

May as well post this while I'm running the layouttests.

Clean this up, including exposing the "can attach?" method to the front end host so the inspector front end can make decisions about this.  (will help resolving https://bugs.webkit.org/show_bug.cgi?id=35306, too)

[Brady Eidson]

Nevermind, didn't know there was an autogen and idl here (I was manually editting a derived source)

Followup coming...

[Brady Eidson]

Created attachment 49313 [details]
Take 2

[Brady Eidson]

Ugggh - causes a layouttest failure...  anyone know how the inspector tests work?

--- /tmp/layout-test-results/inspector/timeline-event-dispatch-expected.txt	2010-02-23 11:40:48.000000000 -0800
+++ /tmp/layout-test-results/inspector/timeline-event-dispatch-actual.txt	2010-02-23 11:40:48.000000000 -0800
@@ -1,12 +1,4 @@
 Tests the Timeline API instrumentation of a DOM Dispatch (mousedown)
 
 Test Mouse Target
-EventDispatch Properties:
-+ startTime : * DEFINED *
-+ data : {
-+- type : mousedown
-+ }
-+ children : * DEFINED *
-+ endTime : * DEFINED *
-+ type : 0

[Timothy Hatcher]

I am not sure why this fails.

[Brady Eidson]

Created attachment 49315 [details]
3rd time's the charm

[Pavel Feldman]

Comment on attachment 49315 [details]
3rd time's the charm

The IDL change looks fine. I would only ask that you check InspectorFrontendHost.canAttach for existence before calling into it. Otherwise Chromium gets doomed. We have not yet upstreamed the frontend interface. I don't follow the general logic though:

> +    return constrainedAttachedWindowHeight(preferredHeight, inspectedPageHeight) <= inspectedPageHeight * maximumAttachedHeightRatio;

If I inline constrainedAttachedWindowHeight, I get:

max(minimumAttachedHeight, min(preferredHeight, inspectedPageHeight * maximumAttachedHeightRatio)) <= inspectedPageHeight * maximumAttachedHeightRatio

this evolves to

max(minimumAttachedHeight, {x | x <= inspectedPageHeight * maximumAttachedHeightRatio}) <= inspectedPageHeight * maximumAttachedHeightRatio

So this thing is only false if minimumAttachedHeight >  inspectedPageHeight * maximumAttachedHeightRatio. This looks like a strange check. What do I miss? r- unless you convince me this is the right check.

[Brady Eidson]

I don't need to convince you, because you're right. ;)

I'm kind of sad I'll be doing the existence check as a temporary fix while Chromium waits to upstream other changes...  seems kind of odd that we might end up littering the code with such checks then forgetting to remove them later.  Have we done this before?

[Brady Eidson]

Created attachment 49317 [details]
With pfeldman's requested change

[Pavel Feldman]

(In reply to comment #19)
> I don't need to convince you, because you're right. ;)
> 
> I'm kind of sad I'll be doing the existence check as a temporary fix while
> Chromium waits to upstream other changes...  seems kind of odd that we might
> end up littering the code with such checks then forgetting to remove them
> later.  Have we done this before?

The way it works these days: when new method is added to
InspectorFrontendHost.idl in WebKit, Chromium's inspector interactive tests
fail. Then I fix Chromium's version of the frontend host and everyone is happy.
It is just that when I am aware of the upcoming change like this I ask that the
call to a new thing is guarded. I typically fix the check same or next day. It
prevents Chromium tests from failing. Chromium's front-end version is living
upstream (WebKit/chromium/), but the tests are not yet being enabled on WebKit
bots. We are working on final bits of migrating to the same implementation of the inspector frontend host, so it'll go away soonish!

[Pavel Feldman]

Comment on attachment 49317 [details]
With pfeldman's requested change

Looks good. Single nit below:

> +    if (constrainedHeight > inspectedPageHeight * maximumAttachedHeightRatio)
>          return;

Same as before, this could be a change against minimumAttachedHeight. You can then use constrainedAttachedWindowHeight in place below as it used to be.
Essentially your change will move from checks against preferredHeight to checks against minimumAttachedHeight, rest of the code won't change.

> +        if (x && InspectorFrontendHost.canAttachWindow && !InspectorFrontendHost.canAttachWindow())
> +            return;

Thanks!

[Brady Eidson]

Yet another problem with my patch, actually, when starting out docked.

Because starting out docked involves such a bizarre series of steps.

I think I just need to forcefully prevent the attach when the button is clicked, and in the original showWindow call.  Trying to do it at any other same leads to things getting inconsistent.

[Brady Eidson]

Created attachment 49328 [details]
Actually feel good about this one *sigh*

[Brady Eidson]

Only different in my last patch from what pfeldman r+'ed is that the actual policy decision of "should I attach?" has been moved out of the native ::attachWindow() and into the dock-button's click handler in the front-end.

[Brady Eidson]

Fixed in http://trac.webkit.org/changeset/55172

[Ain Tohvri]

Great job Brady and guys, works nicely in r55180, thanks!