Bug 101066 – Web Inspector: Fix jscompiler cast syntax

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

[Andrey Adaikin]

Casts should be in the form of "/** @type {TypeName} */ (expr)" instead of "/** @type {TypeName} */ expr".
Patch to follow.

[Andrey Adaikin]

Created attachment 172078 [details]
Patch

[Andrey Adaikin]

Created attachment 172305 [details]
Rebased

[Pavel Feldman]

Comment on attachment 172305 [details]
Rebased

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

> Source/WebCore/inspector/front-end/MemoryStatistics.js:412
> +        var anchor = /** @type {Element} */ (this._containerAnchor.nextSibling);

Why did this change?

[Andrey Adaikin]

Comment on attachment 172305 [details]
Rebased

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

>> Source/WebCore/inspector/front-end/MemoryStatistics.js:412
>> +        var anchor = /** @type {Element} */ (this._containerAnchor.nextSibling);
> 
> Why did this change?

/** @type {Element} */ === /** @type {Element|null} */ === /** @type {Element?} */
these are equal notations for jscompiler. we use the first one.

[Pavel Feldman]

> /** @type {Element} */ === /** @type {Element|null} */ === /** @type {Element?} */
> these are equal notations for jscompiler. we use the first one.

Is there a way for js compiler to treat things as non-nullable by default and only allow nullable when declared as "Element?" ? That's the semantics we were following.

[Andrey Adaikin]

(In reply to comment #5)
> > /** @type {Element} */ === /** @type {Element|null} */ === /** @type {Element?} */
> > these are equal notations for jscompiler. we use the first one.
> 
> Is there a way for js compiler to treat things as non-nullable by default and only allow nullable when declared as "Element?" ? That's the semantics we were following.

We also use explicit non-nullable notation in the code: /** @type {!Element} */
Don't know though if it's possible to change the default compiler's behavior.

FYI, an excerpt for the doc https://developers.google.com/closure/compiler/docs/js-for-compiler#null:

"All object types are nullable by default whether or not they are declared with the Nullable operator. An object type is defined as anything except a function, string, number, or boolean. To make an object type non-nullable, use the Non-nullable operator."

[Pavel Feldman]

> We also use explicit non-nullable notation in the code: /** @type {!Element} */
> Don't know though if it's possible to change the default compiler's behavior.

Looks like an oversight to me.

> FYI, an excerpt for the doc https://developers.google.com/closure/compiler/docs/js-for-compiler#null:
> 
> "All object types are nullable by default whether or not they are declared with the Nullable operator. An object type is defined as anything except a function, string, number, or boolean. To make an object type non-nullable, use the Non-nullable operator."

In either case, the one putting Element|null there was adding a bit of information that you are about to lose. Before your change, we could convert {Everything} to {!Everything} and {Everything|null} into {Everything}. After your change, we will not longer be able distinguish between the two. I'd rather revert it.

[Andrey Adaikin]

(In reply to comment #7)
> > We also use explicit non-nullable notation in the code: /** @type {!Element} */
> > Don't know though if it's possible to change the default compiler's behavior.
> 
> Looks like an oversight to me.
> 
> > FYI, an excerpt for the doc https://developers.google.com/closure/compiler/docs/js-for-compiler#null:
> > 
> > "All object types are nullable by default whether or not they are declared with the Nullable operator. An object type is defined as anything except a function, string, number, or boolean. To make an object type non-nullable, use the Non-nullable operator."
> 
> In either case, the one putting Element|null there was adding a bit of information that you are about to lose. Before your change, we could convert {Everything} to {!Everything} and {Everything|null} into {Everything}. After your change, we will not longer be able distinguish between the two. I'd rather revert it.

Seems like this is the only such place in the whole WebKit JS code :)
I searched for "/\*\*\s*@type\s*\{\w+\|null\}" in http://code.google.com/p/chromium/source/search

I also tried: /\*\*\s*@type\s*\{\??\w+(\|null|\?)\}

If you still insist, I will revert it.

[Pavel Feldman]

> Seems like this is the only such place in the whole WebKit JS code :)

What do you mean whole WebKit JS code? Inspector is the only component that is using compiler.

> If you still insist, I will revert it.

Your change is claiming to "Fix jscompiler cast syntax", so I think it should do only that. If you want to fix the nullable ambiguity, it should be done in a separate change.

[Andrey Adaikin]

Created attachment 172319 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172319 [details]
Patch

Clearing flags on attachment: 172319

Committed r133463: <http://trac.webkit.org/changeset/133463>

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.