Fixing wrong extension ID in Activity Log entries.

A prerender process may be shared across two (or more)
extensions when Chrome reaches prerender process limit. In
this case, extensions' main worlds use the same Activity
Logger (which is defined by whoever sets it first); thus
producing incorrect extension ID in AL entries for
extensions.

This fix changes the way we locate Activity Logger for the
main world of an extension. Now, the extension ID is
required to locate the appropriate Activity Logger. Note, it
is only required for the main world not an isolated world.

Note: this CL contains contains stubs that allow
modifications to Chromium. After the Chromium modifications,
the stubs must be removed.

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

[mvrable, 2014-04-29 20:15:32]

Mostly seems okay to me, modulo a few comments.  It's not completely clean but
is a relatively simple fix for the bug.

You'll need a Blink committer to review the patch as well.

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.cpp:48: return it == loggers.end() ? 0 :
it->value.get();
I would have used NULL rather than 0.  However, is it Blink style to use 0 even
for pointer types?

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.h:51: // a main world, an extension ID is
ignored.
Maybe it's worth adding some text in the comment here to explain why this is
needed--that renderer processes can be re-used so there can be multiple world-0
instances (for different extensions) in the same process.

Also, can you end up with collisions among non-zero worldIds?  Why not, or why
isn't it a problem if it happens?

[Nils Barth (inactive), 2014-04-30 00:37:27]

+haraken: WDYT?
(...as you're quite familiar with the fun of main world(s!))

[haraken, 2014-04-30 00:54:20]

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
Source/bindings/v8/V8WindowShell.cpp:215:
m_scriptState->perContextData()->setActivityLogger(

It's mis-designed that ActivityLogger is cached on V8PerContextData.
ActivityLogger is associated with worlds, so it should be cached on
DOMWrapperWorld. Specifically, we want to remove
V8PerContextData::m_activityLogger and add DOMWrapperWorld::m_activityLogger.

Previously we couldn't do that for performance reasons
(DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
performance issue is now gone.

Would you mind making the change before adding more code about ActivityLogger?

[pmarch, 2014-05-07 15:28:09]

On 2014/04/30 00:54:20, haraken wrote:
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> File Source/bindings/v8/V8WindowShell.cpp (right):
> 
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> Source/bindings/v8/V8WindowShell.cpp:215:
> m_scriptState->perContextData()->setActivityLogger(
> 
> It's mis-designed that ActivityLogger is cached on V8PerContextData.
> ActivityLogger is associated with worlds, so it should be cached on
> DOMWrapperWorld. Specifically, we want to remove
> V8PerContextData::m_activityLogger and add DOMWrapperWorld::m_activityLogger.
> 
> Previously we couldn't do that for performance reasons
> (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
> performance issue is now gone.
> 
> Would you mind making the change before adding more code about ActivityLogger?

I will make this change in a separate CL before continue with this one.

[pmarch, 2014-05-07 15:28:23]

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.cpp:48: return it == loggers.end() ? 0 :
it->value.get();
Blink follows C++ style of null pointers, so using 0.

On 2014/04/29 20:15:32, mvrable wrote:
> I would have used NULL rather than 0.  However, is it Blink style to use 0
even
> for pointer types?

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.h:51: // a main world, an extension ID is
ignored.
On 2014/04/29 20:15:32, mvrable wrote:
> Maybe it's worth adding some text in the comment here to explain why this is
> needed--that renderer processes can be re-used so there can be multiple
world-0
> instances (for different extensions) in the same process.
> 
> Also, can you end up with collisions among non-zero worldIds?  Why not, or why
> isn't it a problem if it happens?

Done.

[pmarch, 2014-05-07 15:37:33]

On 2014/05/07 15:28:09, pmarch wrote:
> On 2014/04/30 00:54:20, haraken wrote:
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > File Source/bindings/v8/V8WindowShell.cpp (right):
> > 
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > Source/bindings/v8/V8WindowShell.cpp:215:
> > m_scriptState->perContextData()->setActivityLogger(
> > 
> > It's mis-designed that ActivityLogger is cached on V8PerContextData.
> > ActivityLogger is associated with worlds, so it should be cached on
> > DOMWrapperWorld. Specifically, we want to remove
> > V8PerContextData::m_activityLogger and add
DOMWrapperWorld::m_activityLogger.
> > 
> > Previously we couldn't do that for performance reasons
> > (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
> > performance issue is now gone.
> > 
> > Would you mind making the change before adding more code about
ActivityLogger?
> 
> I will make this change in a separate CL before continue with this one.

I see that this change is already implemented. I will refactor this patch then.

[pmarch, 2014-05-07 18:09:58]

On 2014/05/07 15:37:33, pmarch wrote:
> On 2014/05/07 15:28:09, pmarch wrote:
> > On 2014/04/30 00:54:20, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > File Source/bindings/v8/V8WindowShell.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > Source/bindings/v8/V8WindowShell.cpp:215:
> > > m_scriptState->perContextData()->setActivityLogger(
> > > 
> > > It's mis-designed that ActivityLogger is cached on V8PerContextData.
> > > ActivityLogger is associated with worlds, so it should be cached on
> > > DOMWrapperWorld. Specifically, we want to remove
> > > V8PerContextData::m_activityLogger and add
> DOMWrapperWorld::m_activityLogger.
> > > 
> > > Previously we couldn't do that for performance reasons
> > > (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
> > > performance issue is now gone.
> > > 
> > > Would you mind making the change before adding more code about
> ActivityLogger?
> > 
> > I will make this change in a separate CL before continue with this one.
> 
> I see that this change is already implemented. I will refactor this patch
then.

haraken:

Currently ActivityLogger is in DOMWorldWrapper. This does not work for me and
this is why. When a renderer is reused by multiple extensions, they share the
same DOMWorldWrapper object but have different V8 execution contexts. Thus, we
need to keep ActivityLogger associated with a context. Two possible places for
this are V8PerContextData or a ScriptState.

I am in favor of placing ActivityLogger in ScriptState.

What is your thoughts on this?

[pmarch, 2014-05-07 19:18:12]

Kentaro, Michael

Make another round of reviews.

Thanks

On 2014/05/07 18:09:58, pmarch wrote:
> On 2014/05/07 15:37:33, pmarch wrote:
> > On 2014/05/07 15:28:09, pmarch wrote:
> > > On 2014/04/30 00:54:20, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > File Source/bindings/v8/V8WindowShell.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > Source/bindings/v8/V8WindowShell.cpp:215:
> > > > m_scriptState->perContextData()->setActivityLogger(
> > > > 
> > > > It's mis-designed that ActivityLogger is cached on V8PerContextData.
> > > > ActivityLogger is associated with worlds, so it should be cached on
> > > > DOMWrapperWorld. Specifically, we want to remove
> > > > V8PerContextData::m_activityLogger and add
> > DOMWrapperWorld::m_activityLogger.
> > > > 
> > > > Previously we couldn't do that for performance reasons
> > > > (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
> > > > performance issue is now gone.
> > > > 
> > > > Would you mind making the change before adding more code about
> > ActivityLogger?
> > > 
> > > I will make this change in a separate CL before continue with this one.
> > 
> > I see that this change is already implemented. I will refactor this patch
> then.
> 
> haraken:
> 
> Currently ActivityLogger is in DOMWorldWrapper. This does not work for me and
> this is why. When a renderer is reused by multiple extensions, they share the
> same DOMWorldWrapper object but have different V8 execution contexts. Thus, we
> need to keep ActivityLogger associated with a context. Two possible places for
> this are V8PerContextData or a ScriptState.
> 
> I am in favor of placing ActivityLogger in ScriptState.
> 
> What is your thoughts on this?

[Jeffrey Yasskin, 2014-05-08 00:17:33]

https://codereview.chromium.org/257803006/diff/80001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/80001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.cpp:38: ASSERT(!extensionId.isEmpty());
This creates an intermediate state where any activity logger will crash debug
chromes until you submit the matching patch. Given how random the timing of the
blink roll can be, it seems like it'd be better to bail out some other way until
the patch that removes the overloads without an extension ID.

https://codereview.chromium.org/257803006/diff/80001/public/web/WebDOMActivit...
File public/web/WebDOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/80001/public/web/WebDOMActivit...
public/web/WebDOMActivityLogger.h:52: // extensionID (worldId may be 0
identifying the main world). Extension ID is
Extension ID isn't mentioned in the signature.

[pmarch, 2014-05-08 01:14:04]

https://codereview.chromium.org/257803006/diff/80001/Source/bindings/v8/V8DOM...
File Source/bindings/v8/V8DOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/80001/Source/bindings/v8/V8DOM...
Source/bindings/v8/V8DOMActivityLogger.cpp:38: ASSERT(!extensionId.isEmpty());
On 2014/05/08 00:17:34, Jeffrey Yasskin wrote:
> This creates an intermediate state where any activity logger will crash debug
> chromes until you submit the matching patch. Given how random the timing of
the
> blink roll can be, it seems like it'd be better to bail out some other way
until
> the patch that removes the overloads without an extension ID.

Done.

https://codereview.chromium.org/257803006/diff/80001/public/web/WebDOMActivit...
File public/web/WebDOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/80001/public/web/WebDOMActivit...
public/web/WebDOMActivityLogger.h:52: // extensionID (worldId may be 0
identifying the main world). Extension ID is
On 2014/05/08 00:17:34, Jeffrey Yasskin wrote:
> Extension ID isn't mentioned in the signature.

Done.

[haraken, 2014-05-08 01:24:19]

On 2014/05/07 18:09:58, pmarch wrote:
> On 2014/05/07 15:37:33, pmarch wrote:
> > On 2014/05/07 15:28:09, pmarch wrote:
> > > On 2014/04/30 00:54:20, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > File Source/bindings/v8/V8WindowShell.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > Source/bindings/v8/V8WindowShell.cpp:215:
> > > > m_scriptState->perContextData()->setActivityLogger(
> > > > 
> > > > It's mis-designed that ActivityLogger is cached on V8PerContextData.
> > > > ActivityLogger is associated with worlds, so it should be cached on
> > > > DOMWrapperWorld. Specifically, we want to remove
> > > > V8PerContextData::m_activityLogger and add
> > DOMWrapperWorld::m_activityLogger.
> > > > 
> > > > Previously we couldn't do that for performance reasons
> > > > (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but the
> > > > performance issue is now gone.
> > > > 
> > > > Would you mind making the change before adding more code about
> > ActivityLogger?
> > > 
> > > I will make this change in a separate CL before continue with this one.
> > 
> > I see that this change is already implemented. I will refactor this patch
> then.
> 
> haraken:
> 
> Currently ActivityLogger is in DOMWorldWrapper. This does not work for me and
> this is why. When a renderer is reused by multiple extensions, they share the
> same DOMWorldWrapper object but have different V8 execution contexts. Thus, we
> need to keep ActivityLogger associated with a context. Two possible places for
> this are V8PerContextData or a ScriptState.
> 
> I am in favor of placing ActivityLogger in ScriptState.
> 
> What is your thoughts on this?

In my understanding, ActivityLogger is created per world, so it should be owned
by DOMWrapperWorld. If you need to distinguish contexts when logging, you can
pick up the current context and associate the log with the context.

[pmarch, 2014-05-08 01:58:47]

On 2014/05/08 01:24:19, haraken wrote:
> On 2014/05/07 18:09:58, pmarch wrote:
> > On 2014/05/07 15:37:33, pmarch wrote:
> > > On 2014/05/07 15:28:09, pmarch wrote:
> > > > On 2014/04/30 00:54:20, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > > File Source/bindings/v8/V8WindowShell.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/257803006/diff/60001/Source/bindings/v8/V8Win...
> > > > > Source/bindings/v8/V8WindowShell.cpp:215:
> > > > > m_scriptState->perContextData()->setActivityLogger(
> > > > > 
> > > > > It's mis-designed that ActivityLogger is cached on V8PerContextData.
> > > > > ActivityLogger is associated with worlds, so it should be cached on
> > > > > DOMWrapperWorld. Specifically, we want to remove
> > > > > V8PerContextData::m_activityLogger and add
> > > DOMWrapperWorld::m_activityLogger.
> > > > > 
> > > > > Previously we couldn't do that for performance reasons
> > > > > (DOMWrapperWorld::current(isolate)->activityLogger() was slow), but
the
> > > > > performance issue is now gone.
> > > > > 
> > > > > Would you mind making the change before adding more code about
> > > ActivityLogger?
> > > > 
> > > > I will make this change in a separate CL before continue with this one.
> > > 
> > > I see that this change is already implemented. I will refactor this patch
> > then.
> > 
> > haraken:
> > 
> > Currently ActivityLogger is in DOMWorldWrapper. This does not work for me
and
> > this is why. When a renderer is reused by multiple extensions, they share
the
> > same DOMWorldWrapper object but have different V8 execution contexts. Thus,
we
> > need to keep ActivityLogger associated with a context. Two possible places
for
> > this are V8PerContextData or a ScriptState.
> > 
> > I am in favor of placing ActivityLogger in ScriptState.
> > 
> > What is your thoughts on this?
> 
> In my understanding, ActivityLogger is created per world, so it should be
owned
> by DOMWrapperWorld. If you need to distinguish contexts when logging, you can
> pick up the current context and associate the log with the context.

ActivityLogger should be create per extension. In order to find the correct
ActivityLogger for
a currently running extension, we can use V8PerContextData or ScriptState as
these objects
may match only to a single extension. We cannot use DOMWrapperWorld as it is
shared across
multiple extensions in the scenario described above.

I agree logically, it would be better if DOMWrapperWorld owns ActivityLogger,
but in order to
achieve this, we need to ensure that each extension has it own DOMWrapperWorld
for main world
when extensions running within the same renderer.

[haraken, 2014-05-08 02:38:23]

OK, I understand that the ActivityLogger should be associated with contexts, not
worlds.

Then can we set the ActivityLogger into V8PerContextData (instead of
ScriptState)? Then you can get it via
m_scriptState->perContextData()->activityLogger(). (If that is not OK in terms
of performance, I'm OK with caching it on ScriptState.)

[haraken, 2014-05-08 02:38:31]

https://codereview.chromium.org/257803006/diff/100001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/100001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:220: m_frame->document() ?
m_frame->document()->baseURI() : KURL()));

Is it possible that m_frame->document() is 0? If it's possible, what's an
expected behavior?

[pmarch, 2014-05-08 17:55:47]

https://codereview.chromium.org/257803006/diff/100001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/100001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:220: m_frame->document() ?
m_frame->document()->baseURI() : KURL()));
I do not know if m_frame->document() can be 0 here. I could not force
m_frame->document() to be 0 when tested this patch. I placed this check just in
case as I saw that the code below does check for m_frame->document() == 0.
However, the code below calls updateDocument() which may set m_frame->document()
to 0, if this is the case my check is redundant.

On 2014/05/08 02:38:32, haraken wrote:
> 
> Is it possible that m_frame->document() is 0? If it's possible, what's an
> expected behavior?

[pmarch, 2014-05-08 19:46:57]

On 2014/05/08 02:38:23, haraken wrote:
> OK, I understand that the ActivityLogger should be associated with contexts,
not
> worlds.
> 
> Then can we set the ActivityLogger into V8PerContextData (instead of
> ScriptState)? Then you can get it via
> m_scriptState->perContextData()->activityLogger(). (If that is not OK in terms
> of performance, I'm OK with caching it on ScriptState.)

ActivityLogger placed in ScriptState and V8PerContextData perform the same. I
evaluated on
PerformanceTests/Bindings/get-attribute.html benchmark. Thus, moved
ActivityLogger to V8PerContextData.

[haraken, 2014-05-08 23:47:44]

LGTM with comments.

abarth@: Would you take a look at web/ ?

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
File Source/bindings/v8/V8DOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:56: // A render process may host
multiple extensions when the browser hits a

render => renderer

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:57: // renderer process limit. Thus,
within a single render there may be

render => renderer

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:60: // main world in such cases, we
require an extension ID. Otherwise,

This comment is misleading (e.g., "there may be multiple main worlds",
"extension's main world").

Let's make the following point clear:

- There is only one main world in one renderer process.
- If we reach the renderer process limit, we assign multiple extensions to the
main world of one renderer process. Thus we need an extension ID to distinguish
the extensions in the main world.

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:63: static V8DOMActivityLogger*
activityLogger(int worldId, const String&);

Can we make this a private method?

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:220: m_world->worldId(),
m_frame->document() ? m_frame->document()->baseURI() : KURL()));

Shall we simply give up setting up the activity logger when we cannot get
m_frame->document()? Not setting up the activity logger would be better than
setting up the activity logger on an unexpected context.

https://codereview.chromium.org/257803006/diff/120001/Source/web/WebDOMActivi...
File Source/web/WebDOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/120001/Source/web/WebDOMActivi...
Source/web/WebDOMActivityLogger.cpp:101: // TODO(pmarch): remove the following
two methods after modifying Chrome to

TODO(pmarch) => FIXME

https://codereview.chromium.org/257803006/diff/120001/public/web/WebDOMActivi...
File public/web/WebDOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/120001/public/web/WebDOMActivi...
public/web/WebDOMActivityLogger.h:62: // TODO(pmarch): remove the following two
methods after modifying Chrome to

TODO(pmarch) => FIXME

[pmarch, 2014-05-09 14:53:51]

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
File Source/bindings/v8/V8DOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:56: // A render process may host
multiple extensions when the browser hits a
On 2014/05/08 23:47:45, haraken wrote:
> 
> render => renderer

Done.

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:57: // renderer process limit. Thus,
within a single render there may be
On 2014/05/08 23:47:45, haraken wrote:
> 
> render => renderer

Done.

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:60: // main world in such cases, we
require an extension ID. Otherwise,
On 2014/05/08 23:47:45, haraken wrote:
> 
> This comment is misleading (e.g., "there may be multiple main worlds",
> "extension's main world").
> 
> Let's make the following point clear:
> 
> - There is only one main world in one renderer process.
> - If we reach the renderer process limit, we assign multiple extensions to the
> main world of one renderer process. Thus we need an extension ID to
distinguish
> the extensions in the main world.

Done.

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMActivityLogger.h:63: static V8DOMActivityLogger*
activityLogger(int worldId, const String&);
On 2014/05/08 23:47:45, haraken wrote:
> 
> Can we make this a private method?

No, as it is called from WebDOMActivityLogger.cpp

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:220: m_world->worldId(),
m_frame->document() ? m_frame->document()->baseURI() : KURL()));
On 2014/05/08 23:47:45, haraken wrote:
> 
> Shall we simply give up setting up the activity logger when we cannot get
> m_frame->document()? Not setting up the activity logger would be better than
> setting up the activity logger on an unexpected context.

V8DOMActivityLogger::activityLogger() will return 0 when m_frame->document() ==
0 as there is no ActivityLogger set for an empty KURL(). The same will happen if
m_frame->document()->baseURI() is 0 or any non-extension URL.

However, if you think it will be better, I can wrap this line within 

if (m_frame->document() &&  m_frame->document()->baseURI().isEmpty()) {}

https://codereview.chromium.org/257803006/diff/120001/Source/web/WebDOMActivi...
File Source/web/WebDOMActivityLogger.cpp (right):

https://codereview.chromium.org/257803006/diff/120001/Source/web/WebDOMActivi...
Source/web/WebDOMActivityLogger.cpp:101: // TODO(pmarch): remove the following
two methods after modifying Chrome to
On 2014/05/08 23:47:45, haraken wrote:
> 
> TODO(pmarch) => FIXME

Done.

https://codereview.chromium.org/257803006/diff/120001/public/web/WebDOMActivi...
File public/web/WebDOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/120001/public/web/WebDOMActivi...
public/web/WebDOMActivityLogger.h:62: // TODO(pmarch): remove the following two
methods after modifying Chrome to
On 2014/05/08 23:47:45, haraken wrote:
> 
> TODO(pmarch) => FIXME

Done.

[haraken, 2014-05-09 14:56:06]

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/257803006/diff/120001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:220: m_world->worldId(),
m_frame->document() ? m_frame->document()->baseURI() : KURL()));
On 2014/05/09 14:53:52, pmarch wrote:
> On 2014/05/08 23:47:45, haraken wrote:
> > 
> > Shall we simply give up setting up the activity logger when we cannot get
> > m_frame->document()? Not setting up the activity logger would be better than
> > setting up the activity logger on an unexpected context.
> 
> V8DOMActivityLogger::activityLogger() will return 0 when m_frame->document()
==
> 0 as there is no ActivityLogger set for an empty KURL(). The same will happen
if
> m_frame->document()->baseURI() is 0 or any non-extension URL.
> 
> However, if you think it will be better, I can wrap this line within 
> 
> if (m_frame->document() &&  m_frame->document()->baseURI().isEmpty()) {}

Thanks for the clarification. Then the current CL makes sense to me :)

[pmarch, 2014-05-12 16:12:32]

Hi Adam

Could you take a look at web?

Thanks

[pmarch, 2014-05-21 20:25:50]

ping

[haraken, 2014-05-21 20:26:15]

I think Adam is OOO.

[felt, 2014-05-21 20:29:20]

On 2014/05/21 20:26:15, haraken wrote:
> I think Adam is OOO.

adam is OOO for a while I think, is there someone else doing is reviews?

[pmarch, 2014-05-21 21:01:55]

Hi Adam K,

Could you approve web/* files in this CL since Adam B. is OOO.

Thanks

[adamk, 2014-05-22 06:56:35]

Unfortunately I'm not an API OWNER (so I can't approve changes in public/);
+darin

[pmarch, 2014-05-27 17:53:00]

On 2014/05/22 06:56:35, adamk wrote:
> Unfortunately I'm not an API OWNER (so I can't approve changes in public/);
> +darin

Hi Darin

Could you please look at public/web/*.

Thanks

[pmarch, 2014-05-29 17:02:31]

On 2014/05/27 17:53:00, pmarch wrote:
> On 2014/05/22 06:56:35, adamk wrote:
> > Unfortunately I'm not an API OWNER (so I can't approve changes in public/);
> > +darin
> 
> Hi Darin
> 
> Could you please look at public/web/*.
> 
> Thanks

Hi Jochen,

Can you approve public/web/* changes?

Thanks

[jochen (gone - plz use gerrit), 2014-06-02 07:32:17]

lgtm

https://codereview.chromium.org/257803006/diff/180001/public/web/WebDOMActivi...
File public/web/WebDOMActivityLogger.h (right):

https://codereview.chromium.org/257803006/diff/180001/public/web/WebDOMActivi...
public/web/WebDOMActivityLogger.h:54: BLINK_EXPORT bool hasDOMActivityLogger(int
worldId, const WebString&);
nit, add extensionId to make the signature more readable

[pmarch, 2014-06-02 18:48:17]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-02 18:48:50]

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

[commit-bot: I haz the power, 2014-06-02 20:17:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9907)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10215)

[commit-bot: I haz the power, 2014-06-02 21:11:39]

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

[commit-bot: I haz the power, 2014-06-02 21:11:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[pmarch, 2014-06-02 22:17:58]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:18:40]

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

[commit-bot: I haz the power, 2014-06-02 23:46:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9942)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10244)

[commit-bot: I haz the power, 2014-06-03 00:19:23]

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

[commit-bot: I haz the power, 2014-06-03 00:19:24]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[pmarch, 2014-06-03 13:39:35]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-06-03 13:39:56]

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

[commit-bot: I haz the power, 2014-06-03 14:56:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10070)

[commit-bot: I haz the power, 2014-06-03 15:37:36]

Change committed as 175381