Removing unnecessary PerWorldBindings for DOM API calls that are monitored by Activity Logger.

Removing unnecessary PerWorldBindings for DOM API calls that are monitored by Activity Logger.

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

[haraken, 2014-06-16 14:57:47]

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
File Source/core/frame/Window.idl (left):

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
Source/core/frame/Window.idl:199: [PerWorldBindings, LogActivity=SetterOnly]
attribute EventHandler onwheel;

If we just remove [PerWorldBindings] from DOM attributes that do not have
[LogAllWorlds], it will change behavior, won't it?

Before this CL, those DOM attributes are not logged in the main world. After
this CL, those DOM attributes are logged in the main world.

[pmarch, 2014-06-16 15:11:01]

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
File Source/core/frame/Window.idl (left):

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
Source/core/frame/Window.idl:199: [PerWorldBindings, LogActivity=SetterOnly]
attribute EventHandler onwheel;
Nice catch. You are absolutely right.

For API calls that do not have LogAllWorlds, I need to check if the world is
isolated world. Let me fix this.

On 2014/06/16 14:57:47, haraken wrote:
> 
> If we just remove [PerWorldBindings] from DOM attributes that do not have
> [LogAllWorlds], it will change behavior, won't it?
> 
> Before this CL, those DOM attributes are not logged in the main world. After
> this CL, those DOM attributes are logged in the main world.

[haraken, 2014-06-16 15:16:49]

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
File Source/core/frame/Window.idl (left):

https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
Source/core/frame/Window.idl:199: [PerWorldBindings, LogActivity=SetterOnly]
attribute EventHandler onwheel;
On 2014/06/16 15:11:01, pmarch wrote:
> Nice catch. You are absolutely right.
> 
> For API calls that do not have LogAllWorlds, I need to check if the world is
> isolated world. Let me fix this.
> 
> On 2014/06/16 14:57:47, haraken wrote:
> > 
> > If we just remove [PerWorldBindings] from DOM attributes that do not have
> > [LogAllWorlds], it will change behavior, won't it?
> > 
> > Before this CL, those DOM attributes are not logged in the main world. After
> > this CL, those DOM attributes are logged in the main world.
> 

Probably was this a reason why you've been observing performance regression in
several benchmarks? If we can skip logging in the main world, the overhead of
not having [PerWorldBindings] would be very small.

Either way, I'm fine with landing this CL first and then removing more
[PerWorldBindings] in a follow-up.

[pmarch, 2014-06-16 16:20:24]

On 2014/06/16 15:16:49, haraken wrote:
>
https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
> File Source/core/frame/Window.idl (left):
> 
>
https://codereview.chromium.org/330293002/diff/90001/Source/core/frame/Window...
> Source/core/frame/Window.idl:199: [PerWorldBindings, LogActivity=SetterOnly]
> attribute EventHandler onwheel;
> On 2014/06/16 15:11:01, pmarch wrote:
> > Nice catch. You are absolutely right.
> > 
> > For API calls that do not have LogAllWorlds, I need to check if the world is
> > isolated world. Let me fix this.
> > 
> > On 2014/06/16 14:57:47, haraken wrote:
> > > 
> > > If we just remove [PerWorldBindings] from DOM attributes that do not have
> > > [LogAllWorlds], it will change behavior, won't it?
> > > 
> > > Before this CL, those DOM attributes are not logged in the main world.
After
> > > this CL, those DOM attributes are logged in the main world.
> > 
> 
> Probably was this a reason why you've been observing performance regression in
> several benchmarks? If we can skip logging in the main world, the overhead of
> not having [PerWorldBindings] would be very small.
> 
> Either way, I'm fine with landing this CL first and then removing more
> [PerWorldBindings] in a follow-up.

This should not impact benchmarks as no logging performed in benchmarks.

[pmarch, 2014-06-16 16:21:32]

have another look, I've add a world check when there is no PerWorldBindings and
no LogAllWorlds

[haraken, 2014-06-17 01:22:34]

LGTM.

I'm curious how much this CL decreases the binary size. You can compare the size
of the 'text' section of 'size ./out/Release/chrome'.

https://codereview.chromium.org/330293002/diff/130001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/330293002/diff/130001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:4359: if (contextData &&
contextData->activityLogger() && scriptState->world().isIsolatedWorld()) {

Nit: I'd check scriptState->world().isIsolatedWorld() first, since it's rare
that isIsolatedWorld() returns true.

if (scriptState->world().isIsolatedWorld() && contextData &&
contextData->activityLogger())
  ...;

[pmarch, 2014-06-17 10:00:05]

The size reduction is 92KB

https://codereview.chromium.org/330293002/diff/130001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/330293002/diff/130001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:4359: if (contextData &&
contextData->activityLogger() && scriptState->world().isIsolatedWorld()) {
On 2014/06/17 01:22:34, haraken wrote:
> 
> Nit: I'd check scriptState->world().isIsolatedWorld() first, since it's rare
> that isIsolatedWorld() returns true.
> 
> if (scriptState->world().isIsolatedWorld() && contextData &&
> contextData->activityLogger())
>   ...;

Done.

[pmarch, 2014-06-17 10:04:25]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-17 10:04:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmarch@chromium.org/330293002/150001

[haraken, 2014-06-17 10:14:25]

> The size reduction is 92KB

This looks great!

[pmarch, 2014-06-17 10:24:16]

The CQ bit was unchecked by pmarch@chromium.org

[pmarch, 2014-06-17 10:26:53]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-17 10:27:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmarch@chromium.org/330293002/170001

[commit-bot: I haz the power, 2014-06-17 12:32:04]

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

[commit-bot: I haz the power, 2014-06-17 12:32:05]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[pmarch, 2014-06-17 12:33:32]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-17 12:34:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmarch@chromium.org/330293002/170001

[commit-bot: I haz the power, 2014-06-17 14:19:58]

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

[commit-bot: I haz the power, 2014-06-17 14:19:59]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[pmarch, 2014-06-17 15:53:07]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-17 15:53:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmarch@chromium.org/330293002/170001

[commit-bot: I haz the power, 2014-06-17 17:11:37]

Change committed as 176336