Add an optional set_hook argument to defineProperty.

Add an optional set_hook argument to defineProperty.

Allows classes to process a property set before the propertyChanged event is dispatched.


BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74858

[arv (Not doing code reviews), 2011-02-03 21:52:08]

Can you add a tests to cr_test.html?

These tests are not automatically run atm but they are still valuable.

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr.js:177: function getSetter(name, kind,
opt_set_hook) {
opt_setHook

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr.js:186: opt_set_hook.apply(this);
I was thinking that it should pass the new value for convenience.

opt_setHook.call(this, value);

[nduca, 2011-02-04 01:31:32]

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr.js:177: function getSetter(name, kind,
opt_set_hook) {
On 2011/02/03 21:52:08, arv wrote:
> opt_setHook

Done.

http://codereview.chromium.org/6246078/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr.js:186: opt_set_hook.apply(this);
On 2011/02/03 21:52:08, arv wrote:
> I was thinking that it should pass the new value for convenience.
> 
> opt_setHook.call(this, value);

Done.

[arv (Not doing code reviews), 2011-02-04 20:13:10]

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:173: * @param {function():void}
opt_setHook A function to run after the property
This type annotation is no longer correct.

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:225: * @param {function(newVal):void}
opt_setHook A function to run after the
newVal is not a type. You probebly meant

function(*):void

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:244: if(opt_kind ==
PropertyKind.BOOL_ATTR && opt_default)
whitespace after if

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:245: throw Error('opt_default for
BOOl_ATTR properties must be false');
I dissagre. I think it is completely legit to do this

cr.defineProperty(Foo, 'bar', cr.PropertyKind.BOOL_ATTR, function(v) {
  if (v)
    doSomething();
  else
    doSomethingElse();
});

[nduca, 2011-02-08 18:48:52]

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:173: * @param {function():void}
opt_setHook A function to run after the property
On 2011/02/04 20:13:10, arv wrote:
> This type annotation is no longer correct.

Done.

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:225: * @param {function(newVal):void}
opt_setHook A function to run after the
On 2011/02/04 20:13:10, arv wrote:
> newVal is not a type. You probebly meant
> 
> function(*):void
> 

Done.

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:244: if(opt_kind ==
PropertyKind.BOOL_ATTR && opt_default)
On 2011/02/04 20:13:10, arv wrote:
> whitespace after if

Done.

http://codereview.chromium.org/6246078/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:245: throw Error('opt_default for
BOOl_ATTR properties must be false');
I think you misread the check here... This is a bugfix for something existing in
cr.js --- the following code does not work (irrespective of the setHook
addition):
cr.defineProperty(Foo, 'bar', crPropertyKind.BOOL_ATTR, true)
The issue of course is that element doesn't exist at the propertyDefine stage,
so we can't exactly add the attribute onto the element to set up the default.

I figured I'd at least make that case throw an error --- in the current
codebase, it silently fails.

On 2011/02/04 20:13:10, arv wrote:
> I dissagre. I think it is completely legit to do this
> 
> cr.defineProperty(Foo, 'bar', cr.PropertyKind.BOOL_ATTR, function(v) {
>   if (v)
>     doSomething();
>   else
>     doSomethingElse();
> });

[arv (Not doing code reviews), 2011-02-08 19:16:48]

http://codereview.chromium.org/6246078/diff/6001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/6246078/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr.js:244: if (opt_kind ==
PropertyKind.BOOL_ATTR && opt_default)
Same for PropertyKind.ATTR

Actually, I think we should remove the default value. It is only used in one
single place so far
(chrome/trunk/src/chrome/browser/resources/shared/js/cr/ui/menu.js) and since
default values don't work for ATTR nor BOOL_ATTR it makes the setHook strange.

[arv (Not doing code reviews), 2011-02-10 20:18:31]

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:72: set
_selectedIndexChange(selectedIndex,oldSelectedIndex) {
Google style is to use trailing underscore

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:72: set
_selectedIndexChange(selectedIndex,oldSelectedIndex) {
whitespace after ,

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:72: set
_selectedIndexChange(selectedIndex,oldSelectedIndex) {
Shouldn't this be a function to work as set hook?

...also there is no need to put this on the prototype.

Just create a local function inside the "define" scope.

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:148:
Menu.prototype._selectedIndexChange);
selectedIndexChanged

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr_test.html (right):

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:42: assertEquals(undefined,
obj.test);
assertUndefined

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:55: assertEquals(this.test, 2);
wrong order... expected should be first argument

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:175: 
remove this empty line

[nduca, 2011-02-10 20:47:17]

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:72: set
_selectedIndexChange(selectedIndex,oldSelectedIndex) {
Wow, major fail on my part. :)

On 2011/02/10 20:18:31, arv wrote:
> Shouldn't this be a function to work as set hook?
> 
> ...also there is no need to put this on the prototype.
> 
> Just create a local function inside the "define" scope.

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr_test.html (right):

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:42: assertEquals(undefined,
obj.test);
On 2011/02/10 20:18:31, arv wrote:
> assertUndefined

Done.

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:55: assertEquals(this.test, 2);
On 2011/02/10 20:18:31, arv wrote:
> wrong order... expected should be first argument

Done.

http://codereview.chromium.org/6246078/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr_test.html:175: 
On 2011/02/10 20:18:31, arv wrote:
> remove this empty line

Done.

[arv (Not doing code reviews), 2011-02-10 21:30:14]

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:27: this.selectedIndex = -1;
This should probably be done using

{
  ...
  selectedIndex_: -1,
  ...
}

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:135: function(selectedIndex,
oldSelectedIndex) {
This is fine but I think this would be easier to read if you created a named
function and used that instead.

function selectedIndexChanged(selectedIndex, oldSelectedIndex) {
  ...
}

cr.defineProperty(Menu, 'selectedIndex', cr.PropertyKind.JS,
selectedIndexChanged);

[nduca, 2011-02-11 20:40:04]

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:27: this.selectedIndex = -1;
Isn't this worse? You'd be baking in knowledge that cr.js uses propertyName_ for
defining property backings...

On 2011/02/10 21:30:14, arv wrote:
> This should probably be done using
> 
> {
>   ...
>   selectedIndex_: -1,
>   ...
> }

http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:135: function(selectedIndex,
oldSelectedIndex) {
On 2011/02/10 21:30:14, arv wrote:
> This is fine but I think this would be easier to read if you created a named
> function and used that instead.
> 
> function selectedIndexChanged(selectedIndex, oldSelectedIndex) {
>   ...
> }
> 
> cr.defineProperty(Menu, 'selectedIndex', cr.PropertyKind.JS,
> selectedIndexChanged);

Done.

[arv (Not doing code reviews), 2011-02-11 22:39:30]

On Fri, Feb 11, 2011 at 12:40,  <nduca@chromium.org> wrote:
>
>
http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
> File chrome/browser/resources/shared/js/cr/ui/menu.js (right):
>
>
http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
> chrome/browser/resources/shared/js/cr/ui/menu.js:27: this.selectedIndex
> = -1;
> Isn't this worse? You'd be baking in knowledge that cr.js uses
> propertyName_ for defining property backings...

You kind of need to know that anyway to prevent naming conflicts.

> On 2011/02/10 21:30:14, arv wrote:
>>
>> This should probably be done using
>
>> {
>>   ...
>>   selectedIndex_: -1,
>>   ...
>> }
>
>
http://codereview.chromium.org/6246078/diff/16001/chrome/browser/resources/sh...
> chrome/browser/resources/shared/js/cr/ui/menu.js:135:
> function(selectedIndex, oldSelectedIndex) {
> On 2011/02/10 21:30:14, arv wrote:
>>
>> This is fine but I think this would be easier to read if you created a
>
> named
>>
>> function and used that instead.
>
>> function selectedIndexChanged(selectedIndex, oldSelectedIndex) {
>>   ...
>> }
>
>> cr.defineProperty(Menu, 'selectedIndex', cr.PropertyKind.JS,
>> selectedIndexChanged);
>
> Done.
>
> http://codereview.chromium.org/6246078/
>

[arv (Not doing code reviews), 2011-02-11 22:41:38]

http://codereview.chromium.org/6246078/diff/21001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/21001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:131: * The selected menu item.
This JSDoc should go with the defineProperty below

[nduca, 2011-02-12 00:22:47]

http://codereview.chromium.org/6246078/diff/21001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/6246078/diff/21001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/menu.js:131: * The selected menu item.
On 2011/02/11 22:41:38, arv wrote:
> This JSDoc should go with the defineProperty below

Done.

[arv (Not doing code reviews), 2011-02-12 01:34:38]

LGTM