Pre-work for adding usage counter for chrome.loadtimes

Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously loadtimes data fields were mutable and persisted modifications that updated the value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to modify the values returned by chrome.loadtimes. Moreover this is a necessary step to get usage counts and move the deprecation further.

BUG=621512
Committed: https://crrev.com/99807eeee0d8426d239376d716cacba2a84cfddf
Cr-Commit-Position: refs/heads/master@{#408226}

[adamk, 2016-07-14 22:09:03]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-07-14 22:09:04]

https://codereview.chromium.org/2149933003/diff/1/chrome/renderer/loadtimes_e...
File chrome/renderer/loadtimes_extension_bindings.cc (right):

https://codereview.chromium.org/2149933003/diff/1/chrome/renderer/loadtimes_e...
chrome/renderer/loadtimes_extension_bindings.cc:112: v8::Local<v8::Object> param
= v8::Local<v8::Object>::Cast(info.Data());
Object here is the wrong type (since you might have a Number or a String), I
think you just want to do:

info.GetReturnValue().Set(info.Data())

to just return it as a Value directly.

[panicker, 2016-07-14 23:17:02]

On 2016/07/14 22:09:04, adamk wrote:
>
https://codereview.chromium.org/2149933003/diff/1/chrome/renderer/loadtimes_e...
> File chrome/renderer/loadtimes_extension_bindings.cc (right):
> 
>
https://codereview.chromium.org/2149933003/diff/1/chrome/renderer/loadtimes_e...
> chrome/renderer/loadtimes_extension_bindings.cc:112: v8::Local<v8::Object>
param
> = v8::Local<v8::Object>::Cast(info.Data());
> Object here is the wrong type (since you might have a Number or a String), I
> think you just want to do:
> 
> info.GetReturnValue().Set(info.Data())
> 
> to just return it as a Value directly.

Done.

[panicker, 2016-07-14 23:23:04]

Description was changed from

==========
Do Not Submit: Test SetAccessor
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to ObjectTemplate and use SetNativeDataProperty to preserve the current
behavior: loadtimes data fields are mutable and can be modified.
This adds a Getter where the Usage counter will be added subsequently.
==========

[panicker, 2016-07-14 23:23:39]

PTAL

[adamk, 2016-07-14 23:48:56]

Shubhie, this looks good, except for the caching issue I talk about below.
Adding a few more V8 people to ask about an API change.

Hi Jochen & Toon,

Shubhie's trying to retrofit an existing web-exposed API with usecounters, using
native accessors to maintain the existing object layout (plain old data
properties). But the easiest way to do this at the moment is with
ObjectTemplate::SetNativeDataProperty, since Object::SetAccessor doesn't do
ReconfigureToDataProperty as would be desired. How would it be to add
Object::SetNativeDataProperty to avoid the weird use of ObjectTemplate here?

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
File chrome/renderer/loadtimes_extension_bindings.cc (right):

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
chrome/renderer/loadtimes_extension_bindings.cc:278:
args.GetReturnValue().Set(load_times_tpl->NewInstance());
So the only thing really bad about this approach is that V8 caches the instance
on the Context. Over time, this could lead to memory bloat, if loadTimes() is
called a lot.

The fact at this "just works" makes me think that perhaps we should just add a
"SetNativeDataProperty" to v8::Object. Adding jochen & verwaest to get their
thoughts on that idea.

[panicker, 2016-07-15 00:32:25]

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
File chrome/renderer/loadtimes_extension_bindings.cc (right):

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
chrome/renderer/loadtimes_extension_bindings.cc:278:
args.GetReturnValue().Set(load_times_tpl->NewInstance());
On 2016/07/14 23:48:56, adamk wrote:
> So the only thing really bad about this approach is that V8 caches the
instance
> on the Context. Over time, this could lead to memory bloat, if loadTimes() is
> called a lot.
> 
> The fact at this "just works" makes me think that perhaps we should just add a
> "SetNativeDataProperty" to v8::Object. Adding jochen & verwaest to get their
> thoughts on that idea.

Since loadtimes is applicable to a specific load, I'd expect it to be unusual to
call it multiple times per load in a script.
+Bryan to comment

[panicker, 2016-07-15 18:15:14]

Description was changed from

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to ObjectTemplate and use SetNativeDataProperty to preserve the current
behavior: loadtimes data fields are mutable and can be modified.
This adds a Getter where the Usage counter will be added subsequently.
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to ObjectTemplate and use SetNativeDataProperty to preserve the current
behavior: loadtimes data fields are mutable and can be modified.
This adds a Getter where the Usage counter will be added subsequently.

BUG=621512
==========

[Bryan McQuade, 2016-07-18 18:33:55]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2016-07-18 18:33:56]

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
File chrome/renderer/loadtimes_extension_bindings.cc (right):

https://codereview.chromium.org/2149933003/diff/20001/chrome/renderer/loadtim...
chrome/renderer/loadtimes_extension_bindings.cc:278:
args.GetReturnValue().Set(load_times_tpl->NewInstance());
On 2016/07/15 at 00:32:25, Shubhie wrote:
> On 2016/07/14 23:48:56, adamk wrote:
> > So the only thing really bad about this approach is that V8 caches the
instance
> > on the Context. Over time, this could lead to memory bloat, if loadTimes()
is
> > called a lot.
> > 
> > The fact at this "just works" makes me think that perhaps we should just add
a
> > "SetNativeDataProperty" to v8::Object. Adding jochen & verwaest to get their
> > thoughts on that idea.
> 
> Since loadtimes is applicable to a specific load, I'd expect it to be unusual
to call it multiple times per load in a script.
> +Bryan to comment

Yes, in code I've looked at, the common pattern is:

      var loadTimes = chrome.loadTimes;
      if (loadTimes) {
        ...
      }

[panicker, 2016-07-19 23:45:20]

PTAL.

Switched back to SetAccessor based on feedback from Adam (feasibility) and Bryan
(okay to make subtly breaking change)

[adamk, 2016-07-20 00:57:22]

lgtm for v8 API usage, if you're OK with the "sets always fail" behavior. At
least no memory leak this way.

https://codereview.chromium.org/2149933003/diff/40001/chrome/renderer/loadtim...
File chrome/renderer/loadtimes_extension_bindings.cc (right):

https://codereview.chromium.org/2149933003/diff/40001/chrome/renderer/loadtim...
chrome/renderer/loadtimes_extension_bindings.cc:181: v8::Local<v8::Object>
load_times = v8::Object::New(isolate);
Nit: looks like indentation got off somewhere here?

[panicker, 2016-07-20 01:05:07]

On 2016/07/20 00:57:22, adamk wrote:
> lgtm for v8 API usage, if you're OK with the "sets always fail" behavior. At
> least no memory leak this way.
> 
>
https://codereview.chromium.org/2149933003/diff/40001/chrome/renderer/loadtim...
> File chrome/renderer/loadtimes_extension_bindings.cc (right):
> 
>
https://codereview.chromium.org/2149933003/diff/40001/chrome/renderer/loadtim...
> chrome/renderer/loadtimes_extension_bindings.cc:181: v8::Local<v8::Object>
> load_times = v8::Object::New(isolate);
> Nit: looks like indentation got off somewhere here?

Oops. Fixed.

[Bryan McQuade, 2016-07-26 23:45:52]

lgtm

[Bryan McQuade, 2016-07-26 23:45:59]

lgtm thanks!

[panicker, 2016-07-27 18:25:58]

Description was changed from

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to ObjectTemplate and use SetNativeDataProperty to preserve the current
behavior: loadtimes data fields are mutable and can be modified.
This adds a Getter where the Usage counter will be added subsequently.

BUG=621512
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- because we are cannot think of scenarios where it makes
sense to modify the values returned by chrome.loadtimes. Moreover this is a
necessary step to get usage counts and move the deprecation further.

BUG=621512
==========

[panicker, 2016-07-27 19:46:04]

The CQ bit was checked by panicker@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 19:46:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 20:17:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 20:17:06]

Dry run: This issue passed the CQ dry run.

[panicker, 2016-07-27 20:20:41]

The CQ bit was checked by panicker@chromium.org

[panicker, 2016-07-27 20:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2149933003/#ps60001
(title: "fix indentation")

[commit-bot: I haz the power, 2016-07-27 20:21:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[panicker, 2016-07-27 20:21:18]

Description was changed from

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- because we are cannot think of scenarios where it makes
sense to modify the values returned by chrome.loadtimes. Moreover this is a
necessary step to get usage counts and move the deprecation further.

BUG=621512
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to
modify the values returned by chrome.loadtimes. Moreover this is a necessary
step to get usage counts and move the deprecation further.

BUG=621512
==========

[commit-bot: I haz the power, 2016-07-27 20:24:44]

Description was changed from

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to
modify the values returned by chrome.loadtimes. Moreover this is a necessary
step to get usage counts and move the deprecation further.

BUG=621512
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to
modify the values returned by chrome.loadtimes. Moreover this is a necessary
step to get usage counts and move the deprecation further.

BUG=621512
==========

[commit-bot: I haz the power, 2016-07-27 20:24:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 20:26:20]

Description was changed from

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to
modify the values returned by chrome.loadtimes. Moreover this is a necessary
step to get usage counts and move the deprecation further.

BUG=621512
==========

to

==========
Pre-req for adding usage counter to chrome.loadtimes:
Switch to SetAccessor, this adds a Getter where the Usage counter will be added
subsequently.

A side effect of SetAccessor is that it subtly changes the  behavior: previously
loadtimes data fields were mutable and persisted modifications that updated the
value programmatically (from javascript).
With this change, such modifications will not be persisted.
We think this is fine -- as we cannot think of scenarios where it makes sense to
modify the values returned by chrome.loadtimes. Moreover this is a necessary
step to get usage counts and move the deprecation further.

BUG=621512

Committed: https://crrev.com/99807eeee0d8426d239376d716cacba2a84cfddf
Cr-Commit-Position: refs/heads/master@{#408226}
==========

[commit-bot: I haz the power, 2016-07-27 20:26:21]

Patchset 4 (id:??) landed as
https://crrev.com/99807eeee0d8426d239376d716cacba2a84cfddf
Cr-Commit-Position: refs/heads/master@{#408226}