Extend DOMActivityLogger interface for upcoming change

Extend DOMActivityLogger interface for upcoming change

In order to reduce pain (hopefully to zero) for sheriffs and fellow developers
who prefer their trees green rather than red, let's make landing the change
at https://codereview.chromium.org/213783002/ as safe as possible.

Step 1: This CL. Extend DOMActivityLogger with new methods for logGetter,
logSetter, and logMethod, which have the same functionality as the current
log() method.
Step 2: https://codereview.chromium.org/213783002/ - The implementation change
in blink to switch from using log() to using logX.
Step 3: Cleanup -- remove old log method from chrome.

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

[not at google - send to devlin, 2014-04-24 17:35:55]

lgtm

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:61: const
v8::Handle<v8::Value>& new_value,
the only difference is that this doesn't take an old_value? can you just pass in
an empty v8::Handle in that case?

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:66: const
v8::Handle<v8::Value>& old_value,
i find it odd taking the new value after the odd in the parameter list, but
maybe you don't, and I don't feel strongly

[Devlin, 2014-04-24 17:45:30]

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:61: const
v8::Handle<v8::Value>& new_value,
On 2014/04/24 17:35:56, kalman wrote:
> the only difference is that this doesn't take an old_value? can you just pass
in
> an empty v8::Handle in that case?

Haha I did, originally, but blink folks preferred overloading the method. :)
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template...

Since it's a blink interface, I deferred to them.  And it is a little faster,
because all the empty values add up - we'd have to construct an empty V8 value,
and then convert it (which I think constructs an empty base::Value) and then
pass it over IPC, and then create add to the action, etc... gets a bit much,
IMO.

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:66: const
v8::Handle<v8::Value>& old_value,
On 2014/04/24 17:35:56, kalman wrote:
> i find it odd taking the new value after the odd in the parameter list, but
> maybe you don't, and I don't feel strongly

{assuming you meant taking old after new}
Mainly, this is because we always get the new value, making the old value
"optional" -- this ensures that the new value is always at the same point in the
arg list (which is important since we serialize it).

[not at google - send to devlin, 2014-04-24 17:48:37]

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:61: const
v8::Handle<v8::Value>& new_value,
On 2014/04/24 17:45:30, D Cronin wrote:
> On 2014/04/24 17:35:56, kalman wrote:
> > the only difference is that this doesn't take an old_value? can you just
pass
> in
> > an empty v8::Handle in that case?
> 
> Haha I did, originally, but blink folks preferred overloading the method. :)
>
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template...
> 
> Since it's a blink interface, I deferred to them.  And it is a little faster,
> because all the empty values add up - we'd have to construct an empty V8
value,
> and then convert it (which I think constructs an empty base::Value) and then
> pass it over IPC, and then create add to the action, etc... gets a bit much,
> IMO.

you don't need to construct an empty *value* just an empty *handle*.

then the implementation of logSetter would check for handle.IsEmpty().

[Devlin, 2014-04-24 17:57:55]

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/247953008/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.h:61: const
v8::Handle<v8::Value>& new_value,
On 2014/04/24 17:48:38, kalman wrote:
> On 2014/04/24 17:45:30, D Cronin wrote:
> > On 2014/04/24 17:35:56, kalman wrote:
> > > the only difference is that this doesn't take an old_value? can you just
> pass
> > in
> > > an empty v8::Handle in that case?
> > 
> > Haha I did, originally, but blink folks preferred overloading the method. :)
> >
>
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template...
> > 
> > Since it's a blink interface, I deferred to them.  And it is a little
faster,
> > because all the empty values add up - we'd have to construct an empty V8
> value,
> > and then convert it (which I think constructs an empty base::Value) and then
> > pass it over IPC, and then create add to the action, etc... gets a bit much,
> > IMO.
> 
> you don't need to construct an empty *value* just an empty *handle*.
> 
> then the implementation of logSetter would check for handle.IsEmpty().

Ah, right.  Okay, done on chrome side.

[not at google - send to devlin, 2014-04-24 20:38:51]

lgtm

https://codereview.chromium.org/247953008/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.cc (right):

https://codereview.chromium.org/247953008/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.cc:94: logSetter(api_name,
new_value, v8::Handle<v8::Value>(), url, title);
not convinced this method is really useful, callers can just do the
v8::Handle<v8::Value>() construction themselves, but.. sure.

[Devlin, 2014-04-24 21:16:22]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:44:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/247953008/...

[commit-bot: I haz the power, 2014-04-25 08:29:45]

Change committed as 266157