Implement empty WorkerInternals interface.

Implement empty WorkerInternals interface.

This adds an (empty) self.internals WorkerInternals interface similar
to the existing window.internals interface. This interface can be used
to expose test only APIs to layout tests that need access to internals
from worker contexts.

BUG=570335
Committed: https://crrev.com/5a8530418d254245a37e39e5f2a22f4e9cb94bee
Cr-Commit-Position: refs/heads/master@{#392628}

[Marijn Kruisselbrink, 2016-04-27 00:45:31]

Description was changed from

==========
WIP try to add a WorkerInternals thingy for layout tests

BUG=
==========

to

==========
WIP try to add a WorkerInternals thingy for layout tests

BUG=
NO_DEPENDENCY_CHECKS=true
==========

[Marijn Kruisselbrink, 2016-04-27 00:58:23]

Description was changed from

==========
WIP try to add a WorkerInternals thingy for layout tests

BUG=
NO_DEPENDENCY_CHECKS=true
==========

to

==========
WIP try to add a WorkerInternals thingy for layout tests

BUG=
==========

[Marijn Kruisselbrink, 2016-04-27 00:58:29]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2016-05-03 18:43:05]

Description was changed from

==========
WIP try to add a WorkerInternals thingy for layout tests

BUG=
==========

to

==========
Implement empty WorkerInternals interface.

This adds an (empty) self.internals WorkerInternals interface similar
to the existing window.internals interface. This interface can be used
to expose test only APIs to layout tests that need access to internals
from worker contexts.

BUG=570335
==========

[Marijn Kruisselbrink, 2016-05-03 18:59:08]

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

[Marijn Kruisselbrink, 2016-05-03 18:59:09]

This implements WorkerInternals fairly similar to how window.internals is
implemented,
by providing hooks for code in content/shell/renderer/layout_test to override to
cause
internals to be injected into worker contents.

Maybe exposing the new worker as a v8::Context like this via Platform is not the
best
approach though. We just don't currently really have any other kind of object to
represent an arbitrary WorkerGlobalScope like we have for documents/windows. I
could
add something that just wraps a v8::Context and use that in Platform and
WebTestingSupport
instead if exposing v8 like this is bad.

[haraken, 2016-05-04 04:03:56]

> Maybe exposing the new worker as a v8::Context like this via Platform is not
the
> best
> approach though. We just don't currently really have any other kind of object
to
> represent an arbitrary WorkerGlobalScope like we have for documents/windows. I
> could
> add something that just wraps a v8::Context and use that in Platform and
> WebTestingSupport
> instead if exposing v8 like this is bad.

Using v8::Context is fine. We're already using v8::Context to let the content
layer understand the worker's context. e.g.,

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:226:
Platform::current()->workerContextCreated(m_workerGlobalScope->scriptController()->context());

Maybe can we reuse m_workerReportingProxy.didInitializeWorkerContext() and avoid
introducing the workerContextCreated API?

[Marijn Kruisselbrink, 2016-05-04 05:11:04]

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:226:
Platform::current()->workerContextCreated(m_workerGlobalScope->scriptController()->context());
On 2016/05/04 at 04:03:56, haraken wrote:
> Maybe can we reuse m_workerReportingProxy.didInitializeWorkerContext() and
avoid introducing the workerContextCreated API?

At some point we still need some API to call out to the content later to have
content shell decide to inject internals. I'm not sure how worker reporting
proxy would help for that.

[haraken, 2016-05-04 05:20:10]

On 2016/05/04 05:11:04, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/workers/WorkerThread.cpp:226:
>
Platform::current()->workerContextCreated(m_workerGlobalScope->scriptController()->context());
> On 2016/05/04 at 04:03:56, haraken wrote:
> > Maybe can we reuse m_workerReportingProxy.didInitializeWorkerContext() and
> avoid introducing the workerContextCreated API?
> 
> At some point we still need some API to call out to the content later to have
> content shell decide to inject internals. I'm not sure how worker reporting
> proxy would help for that.

Makes sense. WebKit/Source/ LGTM.

[Marijn Kruisselbrink, 2016-05-04 16:33:41]

mek@chromium.org changed reviewers:
+ jochen@chromium.org

[Marijn Kruisselbrink, 2016-05-04 16:33:42]

+jochen for WebKit/public and content/ OWNERS

[jochen (gone - plz use gerrit), 2016-05-06 10:42:10]

lgtm with comment

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:58: #include <v8/include/v8.h>
can you forward declare instead?

namespace v8 {
template<class C> Local;
class Context;
}

[Marijn Kruisselbrink, 2016-05-06 16:14:30]

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:58: #include <v8/include/v8.h>
On 2016/05/06 at 10:42:09, jochen wrote:
> can you forward declare instead?
> 
> namespace v8 {
> template<class C> Local;
> class Context;
> }

Not unless I also change the method to pass the v8::Local<> by const-ref rather
than by value.

[Marijn Kruisselbrink, 2016-05-06 16:20:36]

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1915373002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:58: #include <v8/include/v8.h>
On 2016/05/06 at 16:14:30, Marijn Kruisselbrink wrote:
> On 2016/05/06 at 10:42:09, jochen wrote:
> > can you forward declare instead?
> > 
> > namespace v8 {
> > template<class C> Local;
> > class Context;
> > }
> 
> Not unless I also change the method to pass the v8::Local<> by const-ref
rather than by value.

Which actually is probably better (at least not worse) anyway, so done. Sorry
for the noise.

[Marijn Kruisselbrink, 2016-05-06 20:10:28]

mek@chromium.org changed reviewers:
+ kinuko@chromium.org

[Marijn Kruisselbrink, 2016-05-06 20:10:29]

+kinuko

jochen raised the concern that using v8 types in public/platform isn't really
something that's done (currently there's only one other place in public/platform
that exposes a v8 type, which happens to be also one I'm to blame for/should
remove since it isn't really used). So the question came up what the best way to
do is to refer to an arbitrary worker at the blink/content boundary. Am I
missing something, or is there not currently anything to represent an arbitray
worker?

[kinuko, 2016-05-09 11:56:19]

On 2016/05/06 20:10:29, Marijn Kruisselbrink wrote:
> +kinuko
> 
> jochen raised the concern that using v8 types in public/platform isn't really
> something that's done (currently there's only one other place in
public/platform
> that exposes a v8 type, which happens to be also one I'm to blame for/should
> remove since it isn't really used). So the question came up what the best way
to
> do is to refer to an arbitrary worker at the blink/content boundary. Am I
> missing something, or is there not currently anything to represent an arbitray
> worker?

Do you mean if we use v8 types at public layer it should be in public/web rather
than public/platform?  We kinda track workers in content by maintaining worker
threads repository via public/platform but not really have worker representation
that sits across public/web for dedicated / in-process workers.  I think we
could possibly consider adding a simple interface if that's really preferable.

[kinuko, 2016-05-09 12:00:15]

https://codereview.chromium.org/1915373002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1915373002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:626: virtual void
workerContextCreated(const v8::Local<v8::Context>& worker) { }
nit: should this rather be listed in the block commented with 'WebWorker'?  (if
we add this method in Platform)

[jochen (gone - plz use gerrit), 2016-05-09 15:30:24]

a Local<> is just a pointer, so no need to pass it by value.

[Marijn Kruisselbrink, 2016-05-10 00:24:21]

On 2016/05/09 at 11:56:19, kinuko wrote:
> On 2016/05/06 20:10:29, Marijn Kruisselbrink wrote:
> > +kinuko
> > 
> > jochen raised the concern that using v8 types in public/platform isn't
really
> > something that's done (currently there's only one other place in
public/platform
> > that exposes a v8 type, which happens to be also one I'm to blame for/should
> > remove since it isn't really used). So the question came up what the best
way to
> > do is to refer to an arbitrary worker at the blink/content boundary. Am I
> > missing something, or is there not currently anything to represent an
arbitray
> > worker?
> 
> Do you mean if we use v8 types at public layer it should be in public/web
rather than public/platform? 

I'm not sure. I don't know if there is anything inherently wrong with using v8
types in public/platform, but it is curious that is is something that isn't
currently done.

> We kinda track workers in content by maintaining worker threads repository via
public/platform but not really have worker representation that sits across
public/web for dedicated / in-process workers.  I think we could possibly
consider adding a simple interface if that's really preferable.

Yeah, adding a simple (opaque) interface to represent a worker would not be too
hard. I don't know if that's preferable or not though. I'd be happy to leave
this CL as it currently is; I'll let you all decide what is preferable.


On 2016/05/09 at 15:30:24, jochen wrote:
> a Local<> is just a pointer, so no need to pass it by value.

Did you mean "no need to pass it by reference"? I changed it to
pass-by-const-ref to be able to forward declare the v8 types rather than
including the v8 header.

[kinuko, 2016-05-10 00:48:03]

On 2016/05/10 00:24:21, Marijn Kruisselbrink wrote:
> On 2016/05/09 at 11:56:19, kinuko wrote:
> > On 2016/05/06 20:10:29, Marijn Kruisselbrink wrote:
> > > +kinuko
> > > 
> > > jochen raised the concern that using v8 types in public/platform isn't
> really
> > > something that's done (currently there's only one other place in
> public/platform
> > > that exposes a v8 type, which happens to be also one I'm to blame
for/should
> > > remove since it isn't really used). So the question came up what the best
> way to
> > > do is to refer to an arbitrary worker at the blink/content boundary. Am I
> > > missing something, or is there not currently anything to represent an
> arbitray
> > > worker?
> > 
> > Do you mean if we use v8 types at public layer it should be in public/web
> rather than public/platform? 
> 
> I'm not sure. I don't know if there is anything inherently wrong with using v8
> types in public/platform, but it is curious that is is something that isn't
> currently done.
> 
> > We kinda track workers in content by maintaining worker threads repository
via
> public/platform but not really have worker representation that sits across
> public/web for dedicated / in-process workers.  I think we could possibly
> consider adding a simple interface if that's really preferable.
> 
> Yeah, adding a simple (opaque) interface to represent a worker would not be
too
> hard. I don't know if that's preferable or not though. I'd be happy to leave
> this CL as it currently is; I'll let you all decide what is preferable.

Got it- I think I'm fine with landing this CL as is too (i.e. lgtm) as there
doesn't seem much need to have the interface other than this one for now. We
could revisit this later if it starts to look more preferable.

> On 2016/05/09 at 15:30:24, jochen wrote:
> > a Local<> is just a pointer, so no need to pass it by value.
> 
> Did you mean "no need to pass it by reference"? I changed it to
> pass-by-const-ref to be able to forward declare the v8 types rather than
> including the v8 header.

[jochen (gone - plz use gerrit), 2016-05-10 11:56:45]

On 2016/05/10 at 00:48:03, kinuko wrote:
> On 2016/05/10 00:24:21, Marijn Kruisselbrink wrote:
> > On 2016/05/09 at 11:56:19, kinuko wrote:
> > > On 2016/05/06 20:10:29, Marijn Kruisselbrink wrote:
> > > > +kinuko
> > > > 
> > > > jochen raised the concern that using v8 types in public/platform isn't
> > really
> > > > something that's done (currently there's only one other place in
> > public/platform
> > > > that exposes a v8 type, which happens to be also one I'm to blame
for/should
> > > > remove since it isn't really used). So the question came up what the
best
> > way to
> > > > do is to refer to an arbitrary worker at the blink/content boundary. Am
I
> > > > missing something, or is there not currently anything to represent an
> > arbitray
> > > > worker?
> > > 
> > > Do you mean if we use v8 types at public layer it should be in public/web
> > rather than public/platform? 
> > 
> > I'm not sure. I don't know if there is anything inherently wrong with using
v8
> > types in public/platform, but it is curious that is is something that isn't
> > currently done.
> > 
> > > We kinda track workers in content by maintaining worker threads repository
via
> > public/platform but not really have worker representation that sits across
> > public/web for dedicated / in-process workers.  I think we could possibly
> > consider adding a simple interface if that's really preferable.
> > 
> > Yeah, adding a simple (opaque) interface to represent a worker would not be
too
> > hard. I don't know if that's preferable or not though. I'd be happy to leave
> > this CL as it currently is; I'll let you all decide what is preferable.
> 
> Got it- I think I'm fine with landing this CL as is too (i.e. lgtm) as there
doesn't seem much need to have the interface other than this one for now. We
could revisit this later if it starts to look more preferable.
> 
> > On 2016/05/09 at 15:30:24, jochen wrote:
> > > a Local<> is just a pointer, so no need to pass it by value.
> > 
> > Did you mean "no need to pass it by reference"? I changed it to
> > pass-by-const-ref to be able to forward declare the v8 types rather than
> > including the v8 header.

yeah, i guess not include v8.h trumps, so lgtm

[Marijn Kruisselbrink, 2016-05-10 15:53:00]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-05-10 15:53:01]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1915373002/#ps200001
(title: "nit")

[commit-bot: I haz the power, 2016-05-10 15:53:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915373002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915373002/200001

[commit-bot: I haz the power, 2016-05-10 17:01:51]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-05-10 17:03:18]

Description was changed from

==========
Implement empty WorkerInternals interface.

This adds an (empty) self.internals WorkerInternals interface similar
to the existing window.internals interface. This interface can be used
to expose test only APIs to layout tests that need access to internals
from worker contexts.

BUG=570335
==========

to

==========
Implement empty WorkerInternals interface.

This adds an (empty) self.internals WorkerInternals interface similar
to the existing window.internals interface. This interface can be used
to expose test only APIs to layout tests that need access to internals
from worker contexts.

BUG=570335

Committed: https://crrev.com/5a8530418d254245a37e39e5f2a22f4e9cb94bee
Cr-Commit-Position: refs/heads/master@{#392628}
==========

[commit-bot: I haz the power, 2016-05-10 17:03:20]

Patchset 10 (id:??) landed as
https://crrev.com/5a8530418d254245a37e39e5f2a22f4e9cb94bee
Cr-Commit-Position: refs/heads/master@{#392628}