Bug 11888

Summary: REGRESSION (r18320): Web Inspector panes broken
Product: WebKit Reporter: Matt Lilek (pewtermoose) <mlilek>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ap, aroben, ddkilzer, mitz, mrowe
Priority: P1    
Version: 420+   
Hardware: Macintosh   
OS: Mac OS X 10.4   
Attachments:
Description Flags
Patch
mrowe: review-
Patch - fix Inspector and Drosera to do the right thing ggaren: review+

Description Matt Lilek (pewtermoose) 2006-12-20 01:46:36 PST
The web inspector panes no longer display.  Seem to have been broken by http://trac.webkit.org/projects/webkit/changeset/18320
Comment 1 David Kilzer (:ddkilzer) 2006-12-20 02:59:26 PST
Whenever the Inspector is brought up or closed, this error is printed to the console:

/absolute/path/to/WebCore/bindings/objc/WebScriptObject.mm:198:[5896]  JavaScript exception:  SYNTAX_ERR: DOM Exception 12

Whenever "Node", "Style", "Metrics" or "Properties" is clicked, this error is printed to the console:

(event handler):SYNTAX_ERR: DOM Exception 12

Using a locally-built debug build of WebKit r18334 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).

Comment 2 Mark Rowe (bdash) 2006-12-20 20:46:59 PST
This happens because several places in the JavaScript for both Drosera and the Web Inspector code does myElement.style.display = null;, which now throws an exception.  A little investigation shows that Firefox doesn't throw an exception on this construct, while IE 7 does.

We expect "myElement.style.display = null" to remove the value of the display property, effectively resetting it to its default value.
Comment 3 Dave Hyatt 2006-12-20 20:54:06 PST
Seems kind of pointless to throw an exception in this case... sounds like we're setting ourselves up for compatibility problems if we throw and Firefox doesn't, since we'll often share the "standards" code path.
Comment 4 Mark Rowe (bdash) 2006-12-20 21:23:21 PST
Created attachment 11943 [details]
Patch
Comment 5 Alexey Proskuryakov 2006-12-20 21:35:42 PST
Comment on attachment 11943 [details]
Patch

Could we wait just a bit more before reverting a behavior that's (a) standard-compliant AND (b) matches WinIE? So far, there were no reports of Web compatibility problems (except for www.apple.com/getamac, where we share IE behavior now).

To correctly remove a property, one should just set it to empty: "myElement.style.display = ''".

I believe this Inspector JS bug needs to be fixed even if we decide to disable exceptions in this case, so r-.
Comment 6 Adam Roben (:aroben) 2006-12-20 21:41:01 PST
Comment on attachment 11943 [details]
Patch

+        Don't throw an exception when setting the value of a SSStyleDeclaration.

Typo: _C_SSStyleDeclaration

+        if (value->isNull() || frame->settings()->shouldUseDashboardBackwardCompatibilityMode()) {

It would be great to have a comment here about why we're allowing setting properties to null.

r=me
Comment 7 Mark Rowe (bdash) 2006-12-20 22:47:00 PST
Comment on attachment 11943 [details]
Patch

I discussed this with Alexey in #webkit, and we decided that it's best to leave the JS bindings as-is unless we come across websites relying on null resetting a property to its initial value.  I'll follow up with a patch that changes the inspector and debugger's erroneous use to the correct method of using removeProperty.
Comment 8 Mark Rowe (bdash) 2006-12-21 00:10:24 PST
Created attachment 11944 [details]
Patch - fix Inspector and Drosera to do the right thing
Comment 9 Dave Hyatt 2006-12-21 00:12:52 PST
I'm pretty nervous about this, but I guess we can wait and see.  In general, the more exceptions you throw, the more likely you are to have really horrible site errors (because of buggy JS that doesn't think any exception is going to happen).
Comment 10 Mark Rowe (bdash) 2006-12-21 00:39:21 PST
Landed in r18374.
Comment 11 Alexey Proskuryakov 2006-12-21 01:14:46 PST
(In reply to comment #7)
> I discussed this with Alexey in #webkit, and we decided that it's best to leave
> the JS bindings as-is unless we come across websites relying on null resetting
> a property to its initial value. 

I guess we convinced each other - I now think that it would be the right thing to support setting the value to null :-). What I didn't consider earlier was that Firefox not only doesn't throw - it actually handles setting the value to null the same way as setting it to "". I thought that FF was just hiding the error, as it does for invalid values.

However, this should be fixed in setProperty(), not in the JS binding code for assignment.