Handle property definition by {cr|Object}.defineProperty() in compiler pass

Handle property definition by {cr|Object}.defineProperty() in compiler pass

BUG=393873
R=dbeam@chromium.org,tbreisacher@chromium.org
TEST=third_party/closure_compiler/runner/how_to_test_compiler_pass.md

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289535

[Tyler Breisacher (Chromium), 2014-08-12 15:57:30]

lgtm

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:59:
"Invalid cr.PropertyKind passed to cr.defineProperty(): expected ATTR, " +
If you define PropertyKind as an @enum then the typechecker should be able to
catch this for you, I think. But no harm in catching it here too I guess.

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:60:
"BOOL_ATTR or JS, found \"{0}\".");
nit: Put the space and the '+' on the next line:

...ATTR,"
+ " BOOL_ATTR or...

[Tyler Breisacher (Chromium), 2014-08-12 16:32:49]

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:111:
return new Node(Token.STAR);
I was discussing this with dimvar@ a little bit and he pointed out that this
should probably be @type {?} rather than @type {*} otherwise you'll end up
having to add a zillion casts elsewhere.

[Vitaly Pavlenko, 2014-08-12 17:44:18]

I also have fixed source line number information as I got tons of misplaced
errors on real code using Patch Set 1.

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:59:
"Invalid cr.PropertyKind passed to cr.defineProperty(): expected ATTR, " +
On 2014/08/12 15:57:29, Tyler Breisacher (Chromium) wrote:
> If you define PropertyKind as an @enum then the typechecker should be able to
> catch this for you, I think. But no harm in catching it here too I guess.

Acknowledged.

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:60:
"BOOL_ATTR or JS, found \"{0}\".");
On 2014/08/12 15:57:29, Tyler Breisacher (Chromium) wrote:
> nit: Put the space and the '+' on the next line:
> 
> ...ATTR,"
> + " BOOL_ATTR or...

How does one select between two possibilities?

https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:111:
return new Node(Token.STAR);
On 2014/08/12 16:32:49, Tyler Breisacher (Chromium) wrote:
> I was discussing this with dimvar@ a little bit and he pointed out that this
> should probably be @type {?} rather than @type {*} otherwise you'll end up
> having to add a zillion casts elsewhere.

Done.

[Tyler Breisacher (Chromium), 2014-08-12 17:49:28]

>
https://codereview.chromium.org/460163002/diff/1/third_party/closure_compiler...
>
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:60:
> "BOOL_ATTR or JS, found \"{0}\".");
> On 2014/08/12 15:57:29, Tyler Breisacher (Chromium) wrote:
> > nit: Put the space and the '+' on the next line:
> > 
> > ...ATTR,"
> > + " BOOL_ATTR or...
> 
> How does one select between two possibilities?

The google-internal style guide for non-Android projects recommends the '+'
being on the next line. I think the reason is that people found it easy to miss
when it was way over on the right. The Android one doesn't say either way so go
with the Google standard. Plus, it's more common even in Android apparently.

[Vitaly Pavlenko, 2014-08-13 20:28:36]

I have done some more changes in this CL:
- switched to Google-style of string literal joinings
- added comments about cr.PropertyKind type semantics to cr.js definition
- first step in cr.defineProperty() invoked on obj is
  if (typeof obj == 'function')
    obj = obj.prototype;
I copied this behavior to our pass.

Tyler, please, see changes with PS 3 and PS 5.

Dan, please, review.

[Tyler Breisacher (Chromium), 2014-08-13 20:35:43]

https://codereview.chromium.org/460163002/diff/80001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/460163002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr.js:78: * Use only for properties of any type.
s/only //

[Dan Beam, 2014-08-13 20:41:29]

lgtm, but address tbreisacher's comments first

https://codereview.chromium.org/460163002/diff/80001/third_party/closure_comp...
File third_party/closure_compiler/compiler_customization_test.py (right):

https://codereview.chromium.org/460163002/diff/80001/third_party/closure_comp...
third_party/closure_compiler/compiler_customization_test.py:139:
self._runCheckerTestExpectError(source_code=self._CR_DEFINE_DEFINITION + """
is there any reason why you're naming the params?  slightly easier to read?

https://codereview.chromium.org/460163002/diff/80001/third_party/closure_comp...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://codereview.chromium.org/460163002/diff/80001/third_party/closure_comp...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:112:
} else if (propertyKind.matchesQualifiedName("cr.PropertyKind.BOOL_ATTR")) {
nit: no else after if + return, i.e.

  if (propertyKind.matchesQualifiedName("cr.PropertyKind.ATTR")) {
      return IR.string("string");
  }
  if (propertyKind.matchesQualifiedName("cr.PropertyKind.BOOL_ATTR")) {
      return IR.string("boolean");
  }
  if (propertyKind.matchesQualifiedName("cr.PropertyKind.JS")) {
      return new Node(Token.QMARK);
  }
  compiler.report(JSError.make(propertyKind,
CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND,
          propertyKind.getQualifiedName()));
  return null;

[Vitaly Pavlenko, 2014-08-13 21:12:04]

https://chromiumcodereview.appspot.com/460163002/diff/80001/third_party/closu...
File third_party/closure_compiler/compiler_customization_test.py (right):

https://chromiumcodereview.appspot.com/460163002/diff/80001/third_party/closu...
third_party/closure_compiler/compiler_customization_test.py:139:
self._runCheckerTestExpectError(source_code=self._CR_DEFINE_DEFINITION + """
On 2014/08/13 20:41:29, Dan Beam wrote:
> is there any reason why you're naming the params?  slightly easier to read?

You're right. I generally like to name the params in Python, but in this case
one can hardly mismatch them. Deleted the names.

https://chromiumcodereview.appspot.com/460163002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://chromiumcodereview.appspot.com/460163002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:112:
} else if (propertyKind.matchesQualifiedName("cr.PropertyKind.BOOL_ATTR")) {
On 2014/08/13 20:41:29, Dan Beam wrote:
> nit: no else after if + return, i.e.
> 
>   if (propertyKind.matchesQualifiedName("cr.PropertyKind.ATTR")) {
>       return IR.string("string");
>   }
>   if (propertyKind.matchesQualifiedName("cr.PropertyKind.BOOL_ATTR")) {
>       return IR.string("boolean");
>   }
>   if (propertyKind.matchesQualifiedName("cr.PropertyKind.JS")) {
>       return new Node(Token.QMARK);
>   }
>   compiler.report(JSError.make(propertyKind,
> CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND,
>           propertyKind.getQualifiedName()));
>   return null;

Done.

https://chromiumcodereview.appspot.com/460163002/diff/80001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/460163002/diff/80001/ui/webui/resource...
ui/webui/resources/js/cr.js:78: * Use only for properties of any type.
On 2014/08/13 20:35:43, Tyler Breisacher (Chromium) wrote:
> s/only //

Done.

[Vitaly Pavlenko, 2014-08-13 21:32:11]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-08-13 21:34:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalyp@chromium.org/460163002/100001

[Vitaly Pavlenko, 2014-08-13 22:26:40]

The CQ bit was unchecked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-08-13 22:32:34]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:37:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalyp@chromium.org/460163002/120001

[commit-bot: I haz the power, 2014-08-14 12:33:22]

Committed patchset #7 (120001) as 289535