Web Inspector: empty, non-functional window

[Jeff Johnson]

I'm using a Debug build of WebKit svn r81082 on Mac OS X 10.6.6, Safari 5.0.4.

When I load a web page and open the web inspector, the inspector is just an empty window. There's a search field and buttons, but nothing does anything.

When I open "WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/Resources/inspector/inspector.html" in Safari running the system WebKit, Safari's normal web inspector Console gives this error:

TypeError: Result of expression 'this.classList' [undefined] is not an object.

[Jeff Johnson]

Joe Peck said it's probably the result of https://bugs.webkit.org/show_bug.cgi?id=56096

[Jeff Johnson]

Actually, I can reproduce this bug with svn r80830, so it wasn't broken in 80831.

[Joseph Pecoraro]

Oh, that makes sense if classList was not available in Safari 5.0.4. I thought it was.

[Jeff Johnson]

The cause is actually http://trac.webkit.org/changeset/70622/trunk along with associated build fixes http://trac.webkit.org/changeset/70623/trunk and http://trac.webkit.org/changeset/70633/trunk

To reproduce:
defaults write com.apple.Safari WebKitLocalStorageEnabledPreferenceKey -bool false

The file "WebKit/Source/WebCore/inspector/front-end/Settings.js" needs to check "window.localStorage != null" before using it.

[Pavel Feldman]

> To reproduce:
> defaults write com.apple.Safari WebKitLocalStorageEnabledPreferenceKey -bool false

why would you do this?

[Jeff Johnson]

(In reply to comment #5)
> > To reproduce:
> > defaults write com.apple.Safari WebKitLocalStorageEnabledPreferenceKey -bool false
> 
> why would you do this?

LOL. Why wouldn't I do it? Why do people disable cookies? :-)

[Jeff Johnson]

Created attachment 86759 [details]
Patch

[Pavel Feldman]

Comment on attachment 86759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86759&action=review

Thanks for fixing this. I have to put r- for the poor email in the changelog + nit on the null comparison.

> Source/WebCore/ChangeLog:1
> [phone]  Jeff Johnson  <lapcatsoftware.com>

This should contain your email.

> Source/WebCore/inspector/front-end/Settings.js:117
> +        if (window.localStorage === null)

I'd rather use window.localStorage == null in order to allow undefined value. Have you checked that localStorage is null both for v8 and jsc bindings?

[Jeff Johnson]

(In reply to comment #8)
> (From update of attachment 86759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86759&action=review
> 
> Thanks for fixing this. I have to put r- for the poor email in the changelog + nit on the null comparison.
> 
> > Source/WebCore/ChangeLog:1
> > [phone]  Jeff Johnson  <lapcatsoftware.com>
> 
> This should contain your email.

I actually left the full address out on purpose, because after my last patch, which did contain an email address, I started receiving a ton of spam at that address, whereas I had never received spam at that address before. Is there some way I can leave out the email? I'd rather be anonymous than spammed. ;-)

> > Source/WebCore/inspector/front-end/Settings.js:117
> > +        if (window.localStorage === null)
> 
> I'd rather use window.localStorage == null in order to allow undefined value. Have you checked that localStorage is null both for v8 and jsc bindings?

I don't think there would be a difference between V8 and JSC, because the value comes from DOMWindow::sessionStorage():

https://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp

That said, I'm not opposed to switching operators if you prefer.

I should also note that I've filed this related bug:

https://bugs.webkit.org/show_bug.cgi?id=56703

If the linked bug is implemented, then we'd need to wrap a try around the check as opposed to (or in addition to) checking for null. However, I'd rather fix the web inspector now and possibly revise it later if and when the SECURITY_ERR change is implemented. I don't know how much opposition there would be to throwing a SECURITY_ERR.

[Jeff Johnson]

Typo in the above, I meant DOMWindow::localStorage rather than DOMWindow::sessionStorage.

[Pavel Feldman]

http://www.webkit.org/coding/contributing.html#changelogs claims there should be an email. I'd rather not violate it. I see opendarwin at lapcatsoftware.com in the comments titles, so it is already available to the spammers...

> I don't think there would be a difference between V8 and JSC, because the value comes from DOMWindow::sessionStorage():
> 
> https://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp

It is wrapped in the VM-specific JavaScript bindings, but it looks like both engines behave the same way. However, I suspect that compiling WebKit without ENABLE_DOM_STORAGE would result in window.localStorage being undefined, so == is better.

> 
> https://bugs.webkit.org/show_bug.cgi?id=56703
> 
> If the linked bug is implemented, then we'd need to wrap a try around the check as opposed to (or in addition to) checking for null. However, I'd rather fix the web inspector now and possibly revise it later if and when the SECURITY_ERR change is implemented. I don't know how much opposition there would be to throwing a SECURITY_ERR.

Yes, lets keep them separate.

[Jeff Johnson]

Created attachment 86930 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 86930 [details]
Patch

Clearing flags on attachment: 86930

Committed r82191: <http://trac.webkit.org/changeset/82191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.