Bug 35274 - REGRESSION(r55107): Inspector docking is busted.
: REGRESSION(r55107): Inspector docking is busted.
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-22 17:03 PST by
Modified: 2010-02-24 00:36 PST (History)


Attachments
Make this work. (8.41 KB, patch)
2010-02-23 11:29 PST, Brady Eidson
beidson: review-
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Take 2 (8.95 KB, patch)
2010-02-23 11:42 PST, Brady Eidson
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
3rd time's the charm (10.33 KB, patch)
2010-02-23 11:59 PST, Brady Eidson
pfeldman: review-
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
With pfeldman's requested change (10.71 KB, patch)
2010-02-23 12:53 PST, Brady Eidson
pfeldman: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Actually feel good about this one *sigh* (10.34 KB, patch)
2010-02-23 14:27 PST, Brady Eidson
timothy: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-22 17:03:01 PST
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. :(
------- Comment #1 From 2010-02-22 17:04:24 PST -------
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.
------- Comment #2 From 2010-02-22 17:06:28 PST -------
http://trac.webkit.org/changeset/55107 looks suspect.
------- Comment #3 From 2010-02-22 17:09:39 PST -------
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...
------- Comment #4 From 2010-02-22 17:11:00 PST -------
(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
------- Comment #5 From 2010-02-22 17:13:23 PST -------
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.
------- Comment #6 From 2010-02-22 17:16:10 PST -------
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.
------- Comment #7 From 2010-02-22 23:12:14 PST -------
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?
------- Comment #8 From 2010-02-23 00:52:01 PST -------
> 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.  ;)
------- Comment #9 From 2010-02-23 09:03:34 PST -------
*** Bug 35293 has been marked as a duplicate of this bug. ***
------- Comment #10 From 2010-02-23 09:04:50 PST -------
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.
------- Comment #11 From 2010-02-23 11:25:24 PST -------
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.
------- Comment #12 From 2010-02-23 11:29:41 PST -------
Created an attachment (id=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)
------- Comment #13 From 2010-02-23 11:32:14 PST -------
Nevermind, didn't know there was an autogen and idl here (I was manually editting a derived source)

Followup coming...
------- Comment #14 From 2010-02-23 11:42:14 PST -------
Created an attachment (id=49313) [details]
Take 2
------- Comment #15 From 2010-02-23 11:49:31 PST -------
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
------- Comment #16 From 2010-02-23 11:55:06 PST -------
I am not sure why this fails.
------- Comment #17 From 2010-02-23 11:59:37 PST -------
Created an attachment (id=49315) [details]
3rd time's the charm
------- Comment #18 From 2010-02-23 12:21:06 PST -------
(From update of attachment 49315 [details])
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.
------- Comment #19 From 2010-02-23 12:51:06 PST -------
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?
------- Comment #20 From 2010-02-23 12:53:09 PST -------
Created an attachment (id=49317) [details]
With pfeldman's requested change
------- Comment #21 From 2010-02-23 12:59:29 PST -------
(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!
------- Comment #22 From 2010-02-23 13:09:16 PST -------
(From update of attachment 49317 [details])
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!
------- Comment #23 From 2010-02-23 14:10:29 PST -------
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.
------- Comment #24 From 2010-02-23 14:27:15 PST -------
Created an attachment (id=49328) [details]
Actually feel good about this one *sigh*
------- Comment #25 From 2010-02-23 14:29:44 PST -------
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.
------- Comment #26 From 2010-02-23 14:39:54 PST -------
Fixed in http://trac.webkit.org/changeset/55172
------- Comment #27 From 2010-02-24 00:36:10 PST -------
Great job Brady and guys, works nicely in r55180, thanks!