Bug 35953 – REGRESSION: Docked/Undocked state of inspector broken on Windows

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

[Brady Eidson]

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!

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 50367 [details]
Real fix, with some more proper sharing of the settings key in quesiton

[Eric Seidel (no email)]

Attachment 50367 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/549005

[Brady Eidson]

Dammit, forgot to change the *right* WebCore.exp

[Brady Eidson]

Created attachment 50368 [details]
Include WebCore.base.exp in the fix.

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/55761

[Pavel Feldman]

Brady, it would be great if you could use http://webkit.org/new-inspector-bug when filing inspector-related issues!

[Brady Eidson]

(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  :)

[Pavel Feldman]

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!

[Joseph Pecoraro]

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

[Brady Eidson]

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