Bug 101066 - Web Inspector: Fix jscompiler cast syntax
Summary: Web Inspector: Fix jscompiler cast syntax
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-02 09:14 PDT by Andrey Adaikin
Modified: 2014-12-12 13:42 PST (History)
9 users (show)

See Also:


Attachments
Patch (92.96 KB, patch)
2012-11-02 09:15 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Rebased (93.73 KB, patch)
2012-11-05 03:17 PST, Andrey Adaikin
pfeldman: review+
Details | Formatted Diff | Diff
Patch (93.74 KB, patch)
2012-11-05 05:26 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2012-11-02 09:14:33 PDT
Casts should be in the form of "/** @type {TypeName} */ (expr)" instead of "/** @type {TypeName} */ expr".
Patch to follow.
Comment 1 Andrey Adaikin 2012-11-02 09:15:51 PDT
Created attachment 172078 [details]
Patch
Comment 2 Andrey Adaikin 2012-11-05 03:17:22 PST
Created attachment 172305 [details]
Rebased
Comment 3 Pavel Feldman 2012-11-05 03:30:34 PST
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?
Comment 4 Andrey Adaikin 2012-11-05 04:09:36 PST
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.
Comment 5 Pavel Feldman 2012-11-05 04:19:45 PST
> /** @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.
Comment 6 Andrey Adaikin 2012-11-05 04:30:05 PST
(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."
Comment 7 Pavel Feldman 2012-11-05 04:55:20 PST
> 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.
Comment 8 Andrey Adaikin 2012-11-05 05:11:59 PST
(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.
Comment 9 Pavel Feldman 2012-11-05 05:18:28 PST
> 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.
Comment 10 Andrey Adaikin 2012-11-05 05:26:04 PST
Created attachment 172319 [details]
Patch
Comment 11 WebKit Review Bot 2012-11-05 06:15:42 PST
Comment on attachment 172319 [details]
Patch

Clearing flags on attachment: 172319

Committed r133463: <http://trac.webkit.org/changeset/133463>
Comment 12 Brian Burg 2014-12-12 13:40:02 PST
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.