Collect use counter for v8BreakIterator

Collect use counter for v8BreakIterator

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183757

[Erik Corry Chromium.org, 2014-10-14 10:55:56]

erikcorry@chromium.org changed reviewers:
+ marja@chromium.org

[Erik Corry Chromium.org, 2014-10-14 10:59:05]

Now that https://codereview.chromium.org/619913002/ has rolled into Chromium we
can complete the support on the Blink side.  PTAL.

[marja, 2014-10-14 11:01:36]

lgtm

[haraken, 2014-10-14 11:01:41]

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

[haraken, 2014-10-14 11:01:41]

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:60:
UseCounter::count(currentExecutionContext(isolate), UseCounter::BreakIterator);

Are you sure that you want to use currentExecutionContext, not
callingExecutionContext? We normally use callingExecutionContext for
UseCounter::count.

(To be honest, I don't think the difference matters in most cases.)

[Erik Corry Chromium.org, 2014-10-14 11:21:00]

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:60:
UseCounter::count(currentExecutionContext(isolate), UseCounter::BreakIterator);
On 2014/10/14 11:01:41, haraken wrote:
> 
> Are you sure that you want to use currentExecutionContext, not
> callingExecutionContext? We normally use callingExecutionContext for
> UseCounter::count.
> 
> (To be honest, I don't think the difference matters in most cases.)

I'm doing the same as the kUseAsm counter does.  I'm not sure what the
difference is?

[marja, 2014-10-14 11:23:20]

(I don't know where the difference would matter either)

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:60:
UseCounter::count(currentExecutionContext(isolate), UseCounter::BreakIterator);
On 2014/10/14 11:21:00, Erik Corry Chromium.org wrote:
> On 2014/10/14 11:01:41, haraken wrote:
> > 
> > Are you sure that you want to use currentExecutionContext, not
> > callingExecutionContext? We normally use callingExecutionContext for
> > UseCounter::count.
> > 
> > (To be honest, I don't think the difference matters in most cases.)
> 
> I'm doing the same as the kUseAsm counter does.  I'm not sure what the
> difference is?

haraken@: ultimately both get associated to the FrameHost (we use the Context to
get the Document and get the FrameHost from it); is it possible that they'd get
associated to different FrameHosts?

[haraken, 2014-10-14 11:29:43]

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:60:
UseCounter::count(currentExecutionContext(isolate), UseCounter::BreakIterator);
On 2014/10/14 11:23:20, marja wrote:
> On 2014/10/14 11:21:00, Erik Corry http://Chromium.org wrote:
> > On 2014/10/14 11:01:41, haraken wrote:
> > > 
> > > Are you sure that you want to use currentExecutionContext, not
> > > callingExecutionContext? We normally use callingExecutionContext for
> > > UseCounter::count.
> > > 
> > > (To be honest, I don't think the difference matters in most cases.)
> > 
> > I'm doing the same as the kUseAsm counter does.  I'm not sure what the
> > difference is?
> 
> haraken@: ultimately both get associated to the FrameHost (we use the Context
to
> get the Document and get the FrameHost from it); is it possible that they'd
get
> associated to different FrameHosts?

If the current execution context is a document of an iframe and the calling
execution context is a document of the iframe's parent frame, it's possible that
the FrameHosts differ. The question is for which document we want to count
UseCounters. We normally count UseCounters for the parent frame, and thus we
normally use callingExecutionContext().

So I think these two end up with different results, but I'm not sure if the
difference matters. If you don't have a strong reason to use
currentExecutionContext(), I'd prefer using callingExecutionContext() in both
kUseAsm and kBreakIterator for consistency.

[haraken, 2014-10-14 11:31:06]

On 2014/10/14 11:29:43, haraken wrote:
>
https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
> File Source/bindings/core/v8/V8PerIsolateData.cpp (right):
> 
>
https://codereview.chromium.org/639703003/diff/1/Source/bindings/core/v8/V8Pe...
> Source/bindings/core/v8/V8PerIsolateData.cpp:60:
> UseCounter::count(currentExecutionContext(isolate),
UseCounter::BreakIterator);
> On 2014/10/14 11:23:20, marja wrote:
> > On 2014/10/14 11:21:00, Erik Corry http://Chromium.org wrote:
> > > On 2014/10/14 11:01:41, haraken wrote:
> > > > 
> > > > Are you sure that you want to use currentExecutionContext, not
> > > > callingExecutionContext? We normally use callingExecutionContext for
> > > > UseCounter::count.
> > > > 
> > > > (To be honest, I don't think the difference matters in most cases.)
> > > 
> > > I'm doing the same as the kUseAsm counter does.  I'm not sure what the
> > > difference is?
> > 
> > haraken@: ultimately both get associated to the FrameHost (we use the
Context
> to
> > get the Document and get the FrameHost from it); is it possible that they'd
> get
> > associated to different FrameHosts?
> 
> If the current execution context is a document of an iframe and the calling
> execution context is a document of the iframe's parent frame, it's possible
that
> the FrameHosts differ. The question is for which document we want to count
> UseCounters. We normally count UseCounters for the parent frame, and thus we
> normally use callingExecutionContext().
> 
> So I think these two end up with different results, but I'm not sure if the
> difference matters. If you don't have a strong reason to use
> currentExecutionContext(), I'd prefer using callingExecutionContext() in both
> kUseAsm and kBreakIterator for consistency.

V8PerIsolateData.cpp is the only place we're using currentExecutionContext() :)

[Erik Corry Chromium.org, 2014-10-15 15:40:52]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2014-10-15 15:41:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/639703003/50001

[commit-bot: I haz the power, 2014-10-15 16:48:06]

Committed patchset #2 (id:50001) as 183757

[haraken, 2014-10-15 23:28:04]

LGTM