Web Inspector: In Inspector.json PropertyDescriptor.writable should be declared optional

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

[Peter Rybin]

Property PropertyDescriptor.writable in de facto optional. It's also conceptually optional (because only it makes sense for "data" properties, not for "accessor" properties).

Inspector.json should be fixed accordingly.

Note that it formally breaks compatibility with "1.0" and "0.1". Probably they should be patched retroactively too.

[Peter Rybin]

Created attachment 125738 [details]
Patch

[Pavel Feldman]

Comment on attachment 125738 [details]
Patch

Why should it be optional?

[Peter Rybin]

(In reply to comment #2)
> (From update of attachment 125738 [details])
> Why should it be optional?

I'm not sure it must be optional.

But it makes sense, because according to JS specification, "writable" exists only for "data" properties, i.e. there's no "writable" when there is getter or setter.

[Peter Rybin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 125738 [details] [details])
> > Why should it be optional?
> 
> I'm not sure it must be optional.

Another option would be to patch the code to conform with Inspector.json

[Peter Rybin]

Created attachment 125850 [details]
original InspectorTypeBuilder.h

[Peter Rybin]

Created attachment 125851 [details]
original InspectorTypeBuilder.cpp

[Peter Rybin]

Created attachment 125853 [details]
new InspectorTypeBuilder.h

[Peter Rybin]

Created attachment 125854 [details]
new InspectorTypeBuilder.cpp

[Peter Rybin]

attachments 2-5 are added here by mistake.
see correct bug: https://bugs.webkit.org/show_bug.cgi?id=77919

[Peter Rybin]

I think "writable" is ok to be optional.
I looked at the code. It seems to be correct taking that "writable" is optional.

[Peter Rybin]

Created attachment 125965 [details]
Patch

[Peter Rybin]

Additional fix is about "set" and "get" properties. They may be "undefined" in JavaScript descriptor, that gets converted to "null" in JSON format.
"Null" is a problem, because our protocol doesn't have nullable types (only type "any" allows "null" value).

[Pavel Feldman]

Comment on attachment 125965 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptSource.js:236
> +                        if (typeof descriptor.get != "function") {

Should be !==, no need for {}

> Source/WebCore/inspector/InjectedScriptSource.js:239
> +                        if (typeof descriptor.set != "function") {

ditto

[Peter Rybin]

Created attachment 126171 [details]
Patch

[Peter Rybin]

New patch makes sure the output conforms with our protocol (in all cases, including when exception is thrown).

It doesn't lose 'undefined' values of getters and setters, because conceptually accessor property HAS getter and setter, even if both are 'undefined' (this can be checked by getOwnPropertyDescriptor call on such property).

This cases undefined getter/setter showing in UI. This may or may not need fixing.

[WebKit Review Bot]

Comment on attachment 126171 [details]
Patch

Attachment 126171 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11460668

New failing tests:
inspector/runtime/runtime-getProperties.html

[WebKit Review Bot]

Comment on attachment 126171 [details]
Patch

Rejecting attachment 126171 [details] from commit-queue.

New failing tests:
inspector/runtime/runtime-getProperties.html
Full output: http://queues.webkit.org/results/11447199

[Peter Rybin]

Created attachment 126373 [details]
Patch

[Vsevolod Vlasov]

Committed r107567: <http://trac.webkit.org/changeset/107567>

[Vsevolod Vlasov]

Comment on attachment 126373 [details]
Patch

Clearing r?