Bug 35953 - REGRESSION: Docked/Undocked state of inspector broken on Windows
Summary: REGRESSION: Docked/Undocked state of inspector broken on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-09 18:49 PST by Brady Eidson
Modified: 2010-03-10 09:20 PST (History)
3 users (show)

See Also:


Attachments
The fix (1.35 KB, patch)
2010-03-09 18:58 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Real fix, with some more proper sharing of the settings key in quesiton (11.67 KB, patch)
2010-03-09 19:34 PST, Brady Eidson
beidson: commit-queue-
Details | Formatted Diff | Diff
Include WebCore.base.exp in the fix. (12.47 KB, patch)
2010-03-09 20:21 PST, Brady Eidson
timothy: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2010-03-09 18:49:11 PST
Docked/Undocked state of inspector horribly broken on Windows.

The Inspector window ends up in an awful hybrid state where it half-thinks it is docked, have doesn't, and draws horribly.

This regressed when I made changes to prevent docking if the host window would be too small in http://trac.webkit.org/changeset/55172 and http://trac.webkit.org/changeset/55107.

When you bring up the web inspector, the following occurs:
In WebCore::InspectorController::scriptObjectReady(), showWindow() is called.  This calls up to InspectorClient::showWindow() which - in both the Windows and Mac case - calls InspectorController::setWindowVisible(true, <shouldattach>), which then later calls back up to InspectorClient::showWindow().

In both the windows and mac case, InspectorClient::showWindow() isn't design to be called twice in this re-entrant manner, nor do I think it should be.

The crux of the problem is that scriptObjectReader() *actually* wants to call setWindowVisible(true, <shouldattach>) directly.  One thing that probably kept us from doing it this way is that the InspectorController didn't know how to make the decision "should I be attached, or not?"

Indeed, both Windows and Mac have their own copies of the "inspectorStartsAttached" settings key.  While the InspectorController in WebCore has it's own copies of many other settings keys, it doesn't know about this one at all.

I've verified locally that having scriptObjectReady() call setWindowVisible() directly fixes this, and I think we need to clean up this mess!
Comment 1 Brady Eidson 2010-03-09 18:58:29 PST
Created attachment 50365 [details]
The fix

This is the fix.  Not putting it up for review yet, because the settings hack is something that isn't sustainable.  Mac and Windows clients each have their own copy of the key that they use in multiple places, for example.

We should strongly consider moving the keys to some sort of interface that could be used in WebCore and by the WebKit clients so it'd be much harder for them to get out of sync.
Comment 2 Brady Eidson 2010-03-09 19:34:27 PST
Created attachment 50367 [details]
Real fix, with some more proper sharing of the settings key in quesiton
Comment 3 Eric Seidel (no email) 2010-03-09 19:37:04 PST
Attachment 50367 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/549005
Comment 4 Brady Eidson 2010-03-09 20:17:15 PST
Dammit, forgot to change the *right* WebCore.exp
Comment 5 Brady Eidson 2010-03-09 20:21:16 PST
Created attachment 50368 [details]
Include WebCore.base.exp in the fix.
Comment 6 Brady Eidson 2010-03-09 20:53:36 PST
Landed in http://trac.webkit.org/changeset/55761
Comment 7 Pavel Feldman 2010-03-09 22:10:13 PST
Brady, it would be great if you could use http://webkit.org/new-inspector-bug when filing inspector-related issues!
Comment 8 Brady Eidson 2010-03-09 22:17:59 PST
(In reply to comment #7)
> Brady, it would be great if you could use http://webkit.org/new-inspector-bug
> when filing inspector-related issues!

I'd be more than happy to, and love the idea.

But the normal workflow for filing "any webkit bug" is to visit bugs.webkit.org.
So:
- How was I supposed to know about this deviation from the normal workflow before now?
- Is it linked from the main WebKit site anywhere?
- It seems to be a feature of http://webkit.org, so why don't I see any mention of it in the WebKitSite directory of the source tree?
- Shouldn't it be linked from bugs.webkit.org when you're filing a new bug?
- If there's any other such convenience templates for other bugs, what are they?
and finally...
- If there's any other such templates... answer all the above questions for them  :)
Comment 9 Pavel Feldman 2010-03-09 22:24:07 PST
Too many questions I can't answer. The only place it is published is our channel. Now that you know about it, would be great if you could use it!
Comment 10 Joseph Pecoraro 2010-03-10 08:33:25 PST
(In reply to comment #8)
> - It seems to be a feature of http://webkit.org, so why don't I see any mention
> of it in the WebKitSite directory of the source tree?

These are redirects set-up on the server, uneditable by the public.  We got this link updated through emails to an admin. The only other ones I'm aware of are linked from:
http://nightly.webkit.org/start/trunk/55516

They are "Forgotten Patches", "Patches awaiting Review", and "Approved Patches", each use the short link style.

I agree its confusing, but its also very nice. I also haven't seen any other templates like it.
Comment 11 Brady Eidson 2010-03-10 09:20:52 PST
(In reply to comment #10)

> I agree its confusing, but its also very nice. I also haven't seen any other
> templates like it.

It wouldn't be confusing if it was described somewhere and easily accessible.

I'm working with Bill on this.