Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449343003:
[Devtools] Cleanup devtools.js and typecast for jsdoc (Closed)
|
Created:
4 years, 1 month ago by allada Modified:
4 years, 1 month ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Cleanup devtools.js and typecast for jsdoc
This patch is for a series of patches in an effort to cleanup our
typing in JSDoc.
R=dgozman,pfeldman,lushnikov
BUG=659722
Committed: https://crrev.com/ad5177579ae54025e3a2280483a722ec72ae95d5
Cr-Commit-Position: refs/heads/master@{#428210}
Patch Set 1 : changes #
Total comments: 14
Patch Set 2 : changes #Patch Set 3 : changes #
Messages
Total messages: 23 (12 generated)
PTL
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:12: Setting: ?{ I feel uneasy about this - we are tricking ourselves, since the compiler does only check that I managed to match the code and this artificial typedef. https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1147: getFloatValue: function() { return /** @type {number} */ (this.__paddingLeft); }, These annotations looks strange: why do we have to repeat paddingLeft 3 times? Looks like not worth it. https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1178: window.FileError = /** @type {!function (new: FileError) : ?} */ ({ Why this?
https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:20: window.WebInspector; this should go to extenrs.js https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1007: * @return {*} does typing something with "*" make sense?
PTaL https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:20: window.WebInspector; On 2016/10/26 22:10:43, lushnikov wrote: > this should go to extenrs.js If it goes in externs it will override all of it... in this case this file is always compiled by itself. https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1007: * @return {*} On 2016/10/26 22:10:43, lushnikov wrote: > does typing something with "*" make sense? We are overriding something that we do not know what can be put here. So in this case yes, * means any data type. "?" means unknown data type... we have many cases where we already use "*". https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1147: getFloatValue: function() { return /** @type {number} */ (this.__paddingLeft); }, On 2016/10/26 22:03:15, dgozman wrote: > These annotations looks strange: why do we have to repeat paddingLeft 3 times? > Looks like not worth it. Done. https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1178: window.FileError = /** @type {!function (new: FileError) : ?} */ ({ On 2016/10/26 22:03:15, dgozman wrote: > Why this? Yes, I was surprised too, but this is how JSDoc has FileError defined.
https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:20: window.WebInspector; What do you mean by "always compiled by itself"? https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1007: * @return {*} AFAIU the * is the default type in closure's type inference system. So why would you want to declare it explicitly?
PTaL https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:12: Setting: ?{ On 2016/10/26 22:03:15, dgozman wrote: > I feel uneasy about this - we are tricking ourselves, since the compiler does > only check that I managed to match the code and this artificial typedef. Closure complains about these references in other places if you run it in super strict mode. The idea is that if these exist they should look like this (like an optional interface). https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:20: window.WebInspector; On 2016/10/27 02:56:21, lushnikov wrote: > What do you mean by "always compiled by itself"? According to dgozman this file may interact with old versions of devtools, so when this file is compiled it is compiled in closure as a standalone file. https://codereview.chromium.org/2449343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:1007: * @return {*} On 2016/10/27 02:56:21, lushnikov wrote: > AFAIU the * is the default type in closure's type inference system. So why would > you want to declare it explicitly? Because my goals it get closure 100% typed.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTaL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Cleanup devtools.js and typecast for jsdoc This patch is for a series of patches in an effort to cleanup our typing in JSDoc. R=dgozman,pfeldman,lushnikov BUG=659722 ========== to ========== [Devtools] Cleanup devtools.js and typecast for jsdoc This patch is for a series of patches in an effort to cleanup our typing in JSDoc. R=dgozman,pfeldman,lushnikov BUG=659722 Committed: https://crrev.com/ad5177579ae54025e3a2280483a722ec72ae95d5 Cr-Commit-Position: refs/heads/master@{#428210} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ad5177579ae54025e3a2280483a722ec72ae95d5 Cr-Commit-Position: refs/heads/master@{#428210} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
