Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575

[xlai (Olivia), 2016-07-20 23:06:14]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 23:06:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2016-07-21 16:46:54]

xlai@chromium.org changed reviewers:
+ junov@chromium.org, kinuko@chromium.org, schenney@chromium.org

[xlai (Olivia), 2016-07-21 16:46:55]

kinuko@chromium.org: Please review changes in Source/core/workers/

junov@chromium.org: Please review changes in modules/offscreencanvas2d/ and the
two layout tests

schenney@chromium.org: Please review changes in Source/web/.

Thanks!

[Justin Novosad, 2016-07-21 17:27:39]

What about WebGL? WebGL does not allow cross origin content, but I assume that
it supports this option for blocking toBlob/toDataURL calls.  That should work
in a Worker.

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
(right):

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html:17:
var imageData = context.getImageData(0, 0, 100, 100);
should also test toDataURL and toBlob

[xlai (Olivia), 2016-07-21 17:56:49]

But I checked WebGL in offscreencanvas, it doesn't invoke
OffscreenCanvas::originClean(). I think those API functions that need to invoke
that haven't been implemented yet.

I can move the m_disableReadingFromCanvas flag to OffscreenCanvas, instead of
its 2d context class, so that it will be shared by all contexts in
OffscreenCanvas.

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
(right):

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html:17:
var imageData = context.getImageData(0, 0, 100, 100);
On 2016/07/21 17:27:38, Justin Novosad wrote:
> should also test toDataURL and toBlob

But toDataURL and toBlob have not been implemented in OffscreenCanvas yet.

[Justin Novosad, 2016-07-21 18:06:38]

On 2016/07/21 17:56:49, xlai (Olivia) wrote:
> But I checked WebGL in offscreencanvas, it doesn't invoke
> OffscreenCanvas::originClean(). I think those API functions that need to
invoke
> that haven't been implemented yet.

That is because the notion of originClean does not make sense for WebGL since it
is not allowed to bring cross-origin content into a WebGL context. That said,
the notion of blocking readback does make sense for webGL.  If that flag works
for WebGL, it would not be via origin clean.  If it does not currently work for
webGL, please file a bug so that we can fix that.
> 
> I can move the m_disableReadingFromCanvas flag to OffscreenCanvas, instead of
> its 2d context class, so that it will be shared by all contexts in
> OffscreenCanvas.
> 
>
https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
> (right):
> 
>
https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html:17:
> var imageData = context.getImageData(0, 0, 100, 100);
> On 2016/07/21 17:27:38, Justin Novosad wrote:
> > should also test toDataURL and toBlob
> 
> But toDataURL and toBlob have not been implemented in OffscreenCanvas yet.

[Justin Novosad, 2016-07-21 18:07:46]

lgtm for LayoutTests and modules/offscreencanvas2d

[Justin Novosad, 2016-07-21 18:14:18]

On 2016/07/21 18:07:46, Justin Novosad wrote:
> lgtm for LayoutTests and modules/offscreencanvas2d

Bug title should reflect observable change in behaviour (making the
WebKitDisableReadingFromCanvas flag apply to OffscreenCanvases)

[kinuko, 2016-07-22 09:02:53]

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

[kinuko, 2016-07-22 09:02:54]

(+nhiroki for worker-related changes)

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23: WorkerSettings*
WorkerSettings::from(ExecutionContext* context)
I'm not sure if we should pass this as a supplement of WorkerClients.  It's
handy to pass things from main thread to worker thread, but Settings isn't a
client, and isn't something that's particularly about a specific module. It
needs a little more plumbing but we could probably just pass it directly via
WorkerThreadStartupData?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:47: }
ditto

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:17: class CORE_EXPORT
WorkerSettings : public GarbageCollectedFinalized<WorkerSettings>, public
Supplement<WorkerClients> {
Does this need to be on-heap? (Just asking)

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:43: }
nit: please have empty line before line 43, and make the line 43 as following
for consistency

} // namespace blink

[xlai (Olivia), 2016-07-22 21:53:30]

kinuko@: I replied your comments in-line.

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23: WorkerSettings*
WorkerSettings::from(ExecutionContext* context)
On 2016/07/22 09:02:54, kinuko wrote:
> I'm not sure if we should pass this as a supplement of WorkerClients.  It's
> handy to pass things from main thread to worker thread, but Settings isn't a
> client, and isn't something that's particularly about a specific module. It
> needs a little more plumbing but we could probably just pass it directly via
> WorkerThreadStartupData?

I did think of making WorkerSetting to become a member
in WorkerThreadStartupData but was wondering how the workers
team think about the design. The structure is actually simpler.

Also, if WorkerSetting becomes a member in WorkerThreadStartupData.
How do I make sure that the flag values stored in WorkerSetting
remain unchanged when the StartupData is passed from main
to worker thread? Will just doing release() on std::unique_ptr work?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:47: }
On 2016/07/22 09:02:54, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:17: class CORE_EXPORT
WorkerSettings : public GarbageCollectedFinalized<WorkerSettings>, public
Supplement<WorkerClients> {
On 2016/07/22 09:02:54, kinuko wrote:
> Does this need to be on-heap? (Just asking)

Hmmm, not necessary. But isn't better to be on heap so that we don't worry about
garbage collecting it?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:43: }
On 2016/07/22 09:02:54, kinuko wrote:
> nit: please have empty line before line 43, and make the line 43 as following
> for consistency
> 
> } // namespace blink

Done.

[nhiroki, 2016-07-25 11:14:48]

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23: WorkerSettings*
WorkerSettings::from(ExecutionContext* context)
On 2016/07/22 21:53:30, xlai (Olivia) wrote:
> On 2016/07/22 09:02:54, kinuko wrote:
> > I'm not sure if we should pass this as a supplement of WorkerClients.  It's
> > handy to pass things from main thread to worker thread, but Settings isn't a
> > client, and isn't something that's particularly about a specific module. It
> > needs a little more plumbing but we could probably just pass it directly via
> > WorkerThreadStartupData?
> 
> I did think of making WorkerSetting to become a member
> in WorkerThreadStartupData but was wondering how the workers
> team think about the design. The structure is actually simpler.

I'd prefer to pass it via WorkerThreadStartupData. It'd be more straightforward.

> Also, if WorkerSetting becomes a member in WorkerThreadStartupData.
> How do I make sure that the flag values stored in WorkerSetting
> remain unchanged when the StartupData is passed from main
> to worker thread? Will just doing release() on std::unique_ptr work? 

Could you explain what you're concerned a little more specifically? I'm not sure
who may change the flag values during the period.

In my understanding, WorkerThreadStartupData approach is done as follows:

1) copy flag values from Settings to WorkerSettings on main thread.
2) create WorkerThreadStartupData and make it own the worker settings on main
thread.
3) post the startup data with its ownership from main thread to worker thread
(see WorkerThread::start())
4) WorkerGlobalScope receives the passed settings and owns it on worker thread.

At the step 4, the flag values should not be changed from the step 1.

[xlai (Olivia), 2016-07-25 19:17:38]

As suggested by kinuko@ and nhiroki@, I changed the WorkerSettings so that it
becomes a member in WorkerThreadStartupData. This WorkerSettings copy flag
values
from Settings on main thread when WorkerThreadStartupData is
created. Then inside createWorkerGlobalScope in WorkerThread::initialize, we
take out the WorkerSetting and put it as a member in WorkerGlobalScope.

On the Offscreen side, as suggested by junov@, I put the
m_disableReadingFromCanvas flag in OffscreenCanvas, so that in the future it can
be shared with WebGL context.

[xlai (Olivia), 2016-07-25 21:37:03]

Description was changed from

==========
Add WorkerSettings to WorkerClients to expose certain flag values in
WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========

to

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========

[nhiroki, 2016-07-26 02:30:00]

workers LGTM (you still need owner's stamp)

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:21: void
copyFlagValuesFromSettings(Settings*);
nit: can you add a blank line between line 21 and 22?

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:26: bool
m_disableReadingFromCanvas = false;
How about initializing it on ctor's initializer list and making this 'const'?

[haraken, 2016-07-26 11:39:47]

owner LGTM

[kinuko, 2016-07-26 14:09:20]

Thanks for making the change!  The startup data is getting bigger and a little
messier, but I think for this case just adding the field there makes more sense.
 LGTM

[xlai (Olivia), 2016-07-26 15:02:00]

Patchset #13 (id:230001) has been deleted

[xlai (Olivia), 2016-07-26 15:10:03]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 15:10:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 16:15:39]

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

[commit-bot: I haz the power, 2016-07-26 16:15:40]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2016-07-26 17:27:57]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-07-26 17:27:58]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
kinuko@chromium.org, haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2171563002/#ps250001
(title: "Nits")

[commit-bot: I haz the power, 2016-07-26 17:28:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2016-07-26 17:28:18]

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:21: void
copyFlagValuesFromSettings(Settings*);
On 2016/07/26 02:30:00, nhiroki (slow) wrote:
> nit: can you add a blank line between line 21 and 22?

Done.

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerSettings.h:26: bool
m_disableReadingFromCanvas = false;
On 2016/07/26 02:30:00, nhiroki (slow) wrote:
> How about initializing it on ctor's initializer list and making this 'const'?

I can't make it const. This bool variable needs to be assignable so that
copyFlagValuesFromSettings() can work; otherwise there'll be compile error.
Nevertheless, I extract the default values as a separate function.

[commit-bot: I haz the power, 2016-07-26 17:33:10]

Description was changed from

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========

to

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========

[commit-bot: I haz the power, 2016-07-26 17:33:11]

Committed patchset #13 (id:250001)

[commit-bot: I haz the power, 2016-07-26 17:34:46]

Description was changed from

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========

to

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.

BUG=607575

Committed: https://crrev.com/bcb0806adb80a01830af344ce7d423b1cb864ec3
Cr-Commit-Position: refs/heads/master@{#407842}
==========

[commit-bot: I haz the power, 2016-07-26 17:34:48]

Patchset 13 (id:??) landed as
https://crrev.com/bcb0806adb80a01830af344ce7d423b1cb864ec3
Cr-Commit-Position: refs/heads/master@{#407842}

[xlai (Olivia), 2016-07-26 17:34:56]

Description was changed from

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.

BUG=607575

Committed: https://crrev.com/bcb0806adb80a01830af344ce7d423b1cb864ec3
Cr-Commit-Position: refs/heads/master@{#407842}
==========

to

==========
Add WorkerSettings to expose certain flag values in WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575
==========