compositor-worker: Restrict the global interface in CompositorWorker.

compositor-worker: Restrict the global interface in CompositorWorker.

A CompositorWorker script is expected to have only a very limited set of API
available to it. So hide the API from compositor workers that are normally
available to other workers (e.g. fileapi, xhr etc.), but is not relevant for
compositor workers.

Notable changes:
. generate_global_constructors.py: In order to allow exposing CompositorProxy to
  CompositorWorker, which lives outside of core into modules, instead of generating
  the list of known global names, use EXPOSED_EXECUTION_CONTEXT_METHOD from
  v8_utilities.

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

[sadrul, 2015-04-22 09:33:19]

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

[sadrul, 2015-04-22 09:33:20]

Hi! Does this look like a reasonable approach for this? The change in
generate_global_constructors.py looks a bit awkward, but there's somewhat
similar code in some other scripts (e.g. EXPOSED_WORKERS in v8_utilities.py), so
perhaps this is acceptable?

Some unwanted interface (e.g. deprecated webkitIDB* stuff, FileError etc.) are
still getting exposed, but I can't tell where they are coming from. Where can I
look to get rid of them too in compositor-workers? Thanks!

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/webexposed/...
File
LayoutTests/webexposed/global-interface-listing-compositor-worker-expected.txt
(right):

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/webexposed/...
LayoutTests/webexposed/global-interface-listing-compositor-worker-expected.txt:8:
CONSOLE WARNING: 'webkitIDBCursor' is deprecated. Please use 'IDBCursor'
instead.
Is there a way for CompositorWorkerGlobalScope to not inherit these deprecated
attributes from WorkerGlobalScope?

[sadrul, 2015-04-22 09:38:53]

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

[sadrul, 2015-04-22 09:38:54]

+kinuko@ specifically for the change in js-test.js

[haraken, 2015-04-22 11:07:28]

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
LayoutTests/resources/js-test.js:816: var shared = (workerType == 'shared' ||
(typeof workerType == 'boolean' && workerType));

What is the 'typeof workerType == 'boolean' && workerType' check for?

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
LayoutTests/resources/js-test.js:820: var worker = shared ? new
SharedWorker(testScriptURL, "Shared Worker") : compositor ? new
CompositorWorker(testScriptURL) : new Worker(testScriptURL);

Can we clean up the code a bit more?

if (...)
  worker = new CompositorWorker();
else if (...)
  worker = new SharedWorker();
else
  worker = new Worker();

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/webexposed/...
File
LayoutTests/webexposed/global-interface-listing-compositor-worker-expected.txt
(right):

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/webexposed/...
LayoutTests/webexposed/global-interface-listing-compositor-worker-expected.txt:8:
CONSOLE WARNING: 'webkitIDBCursor' is deprecated. Please use 'IDBCursor'
instead.
On 2015/04/22 09:33:20, sadrul wrote:
> Is there a way for CompositorWorkerGlobalScope to not inherit these deprecated
> attributes from WorkerGlobalScope?

I'm not sure if there is an easy way to do that. Also I think it would make more
sense to inherit these (deprecated) attributes if other workers still inherit
the attributes. We can deprecate those attributes from all the workers at the
same time.

https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:40: 'CompositorWorker',

Can we avoid adding this here by tweaking ExposureSet in v8_utilities.py?

[kinuko, 2015-04-22 14:18:07]

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
LayoutTests/resources/js-test.js:816: var shared = (workerType == 'shared' ||
(typeof workerType == 'boolean' && workerType));
Can we just update all startWorker() for shared worker to take string? (I
guess/hope there're not too many tests for shared worker)

[sadrul, 2015-04-22 22:17:19]

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
LayoutTests/resources/js-test.js:816: var shared = (workerType == 'shared' ||
(typeof workerType == 'boolean' && workerType));
On 2015/04/22 14:18:07, kinuko wrote:
> Can we just update all startWorker() for shared worker to take string? (I
> guess/hope there're not too many tests for shared worker)

Done! (there was certainly way less than I feared)

https://codereview.chromium.org/1101583003/diff/20001/LayoutTests/resources/j...
LayoutTests/resources/js-test.js:820: var worker = shared ? new
SharedWorker(testScriptURL, "Shared Worker") : compositor ? new
CompositorWorker(testScriptURL) : new Worker(testScriptURL);
On 2015/04/22 11:07:28, haraken wrote:
> 
> Can we clean up the code a bit more?
> 
> if (...)
>   worker = new CompositorWorker();
> else if (...)
>   worker = new SharedWorker();
> else
>   worker = new Worker();

Done.

https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:40: 'CompositorWorker',
On 2015/04/22 11:07:28, haraken wrote:
> 
> Can we avoid adding this here by tweaking ExposureSet in v8_utilities.py?

The error I get without the change in this file is from the check below in line
176, with the following backtrace:

Traceback (most recent call last):
  File "../scripts/generate_global_constructors.py", line 195, in <module>
    sys.exit(main())
  File "../scripts/generate_global_constructors.py", line 181, in main
    % list(unknown_global_names))
ValueError: The following global names were used in [Exposed=xxx] but do not
match any [Global] / [PrimaryGlobal] interface: ['CompositorWorker']


I looked at v8_utilities, but it doesn't look like any of the v8_* files get
used from this file, and I couldn't figure out a way of using ExposureSet from
here. Perhaps you could direct me towards some code that I should look at?

[haraken, 2015-04-23 04:32:37]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, jl@opera.com

[haraken, 2015-04-23 04:32:38]

>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> File Source/bindings/scripts/generate_global_constructors.py (right):
> 
>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> Source/bindings/scripts/generate_global_constructors.py:40:
'CompositorWorker',
> On 2015/04/22 11:07:28, haraken wrote:
> > 
> > Can we avoid adding this here by tweaking ExposureSet in v8_utilities.py?
> 
> The error I get without the change in this file is from the check below in
line
> 176, with the following backtrace:
> 
> Traceback (most recent call last):
>   File "../scripts/generate_global_constructors.py", line 195, in <module>
>     sys.exit(main())
>   File "../scripts/generate_global_constructors.py", line 181, in main
>     % list(unknown_global_names))
> ValueError: The following global names were used in [Exposed=xxx] but do not
> match any [Global] / [PrimaryGlobal] interface: ['CompositorWorker']
> 
> 
> I looked at v8_utilities, but it doesn't look like any of the v8_* files get
> used from this file, and I couldn't figure out a way of using ExposureSet from
> here. Perhaps you could direct me towards some code that I should look at?

+bashi-san, Jens?

[sadrul, 2015-04-24 02:19:37]

On 2015/04/23 04:32:38, haraken wrote:
> >
>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> > File Source/bindings/scripts/generate_global_constructors.py (right):
> > 
> >
>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> > Source/bindings/scripts/generate_global_constructors.py:40:
> 'CompositorWorker',
> > On 2015/04/22 11:07:28, haraken wrote:
> > > 
> > > Can we avoid adding this here by tweaking ExposureSet in v8_utilities.py?
> > 
> > The error I get without the change in this file is from the check below in
> line
> > 176, with the following backtrace:
> > 
> > Traceback (most recent call last):
> >   File "../scripts/generate_global_constructors.py", line 195, in <module>
> >     sys.exit(main())
> >   File "../scripts/generate_global_constructors.py", line 181, in main
> >     % list(unknown_global_names))
> > ValueError: The following global names were used in [Exposed=xxx] but do not
> > match any [Global] / [PrimaryGlobal] interface: ['CompositorWorker']
> > 
> > 
> > I looked at v8_utilities, but it doesn't look like any of the v8_* files get
> > used from this file, and I couldn't figure out a way of using ExposureSet
from
> > here. Perhaps you could direct me towards some code that I should look at?
> 
> +bashi-san, Jens?

ping

[bashi, 2015-04-24 02:57:49]

On 2015/04/24 02:19:37, sadrul wrote:
> On 2015/04/23 04:32:38, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> > > File Source/bindings/scripts/generate_global_constructors.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1101583003/diff/20001/Source/bindings/scripts...
> > > Source/bindings/scripts/generate_global_constructors.py:40:
> > 'CompositorWorker',
> > > On 2015/04/22 11:07:28, haraken wrote:
> > > > 
> > > > Can we avoid adding this here by tweaking ExposureSet in
v8_utilities.py?
> > > 
> > > The error I get without the change in this file is from the check below in
> > line
> > > 176, with the following backtrace:
> > > 
> > > Traceback (most recent call last):
> > >   File "../scripts/generate_global_constructors.py", line 195, in <module>
> > >     sys.exit(main())
> > >   File "../scripts/generate_global_constructors.py", line 181, in main
> > >     % list(unknown_global_names))
> > > ValueError: The following global names were used in [Exposed=xxx] but do
not
> > > match any [Global] / [PrimaryGlobal] interface: ['CompositorWorker']
> > > 
> > > 
> > > I looked at v8_utilities, but it doesn't look like any of the v8_* files
get
> > > used from this file, and I couldn't figure out a way of using ExposureSet
> from
> > > here. Perhaps you could direct me towards some code that I should look at?
> > 
> > +bashi-san, Jens?
> 
> ping

This CL violates core -> modules dependency and that's why you get the error.
When we run generate_global_constructors.py for core/, it doesn't (and
shouldn't) check IDL files under modules/.

If you want to refer CompositorWorker from core, it would mean that
CompositorWorker should be placed in core.

[sadrul, 2015-04-24 03:14:26]

sadrul@chromium.org changed reviewers:
+ esprehn@chromium.org

[sadrul, 2015-04-24 03:33:03]

+esprehn@

> This CL violates core -> modules dependency and that's why you get the error.
> When we run generate_global_constructors.py for core/, it doesn't (and
> shouldn't) check IDL files under modules/.
> 
> If you want to refer CompositorWorker from core, it would mean that
> CompositorWorker should be placed in core.

Right. The desire here is to allow CompositorProxy to be accessible to
CompositorWorker, which is not in core. The options as I see are:
 (1) Allow code in core to expose API to outside of core (the change in
generate_global_constructors.py in this CL)
 (2) Move CompositorProxy into //modules/compositorworker/ out of //core/dom/
 (3) Allow CompositorWorker to list the API it wants exposed to it.
 (4) Move CompositorWorker into //core/workers/ (or
//core/workers/compositorworker/) out of //modules/compositorworker.

This CL provides solution (1). core does allow partial interface to live in
modules, and so this doesn't necessarily seem like something that violates the
core -> module dependency.

Solution (2): CompositorProxy deals pretty closely with Element (and associated
style, layout etc.), and so it'd be better if CompositorProxy lives close-by.

Solution (3): This would be ideal I think, but I am not sure this is currently
supported.

Solution (4): It's cleaner for CompositorWorker to live in its own module (as
does ServiceWorker).

What do you guys think?

Elliott: do you think solution (2) is reasonable, and would be better than the
other proposals?

Kinuko: do you think solution (4) is reasonable, and would be better than the
other proposals?

[sadrul, 2015-04-28 18:02:20]

On 2015/04/24 03:33:03, sadrul wrote:
> +esprehn@
> 
> > This CL violates core -> modules dependency and that's why you get the
error.
> > When we run generate_global_constructors.py for core/, it doesn't (and
> > shouldn't) check IDL files under modules/.
> > 
> > If you want to refer CompositorWorker from core, it would mean that
> > CompositorWorker should be placed in core.
> 
> Right. The desire here is to allow CompositorProxy to be accessible to
> CompositorWorker, which is not in core. The options as I see are:
>  (1) Allow code in core to expose API to outside of core (the change in
> generate_global_constructors.py in this CL)
>  (2) Move CompositorProxy into //modules/compositorworker/ out of //core/dom/
>  (3) Allow CompositorWorker to list the API it wants exposed to it.
>  (4) Move CompositorWorker into //core/workers/ (or
> //core/workers/compositorworker/) out of //modules/compositorworker.
> 
> This CL provides solution (1). core does allow partial interface to live in
> modules, and so this doesn't necessarily seem like something that violates the
> core -> module dependency.
> 
> Solution (2): CompositorProxy deals pretty closely with Element (and
associated
> style, layout etc.), and so it'd be better if CompositorProxy lives close-by.
> 
> Solution (3): This would be ideal I think, but I am not sure this is currently
> supported.
> 
> Solution (4): It's cleaner for CompositorWorker to live in its own module (as
> does ServiceWorker).
> 
> What do you guys think?
> 
> Elliott: do you think solution (2) is reasonable, and would be better than the
> other proposals?
> 
> Kinuko: do you think solution (4) is reasonable, and would be better than the
> other proposals?

ping

Some more notes on option 2: Moving CompositorProxy into the module can be
rather tricky. There is a chance that instead of CompositorProxy pushing some
state into Element, Element would poll the state in CompositorProxy (to save
bitfields in element/node rare-data), and to allow for that to happen if needed
in the future, CompositorProxy cannot move into the module.

So the valid options seem to be (1), (3), and (4). Please advise which makes
most sense.

[esprehn, 2015-04-30 05:36:34]

CompositorProxy doesn't feel like a separate module, it's very wrapped up in the
style and Element machinery. In the same way the animations engine doesn't seem
like a separate module.

[sadrul, 2015-04-30 07:52:50]

On 2015/04/30 05:36:34, esprehn wrote:
> CompositorProxy doesn't feel like a separate module, it's very wrapped up in
the
> style and Element machinery. In the same way the animations engine doesn't
seem
> like a separate module.

Thanks Elliott!

So that effectively rules out option (2).

haraken@, kinuko@, jl@, bashi@: Among the remaining options
(https://codereview.chromium.org/1101583003/#msg13), which do you think would be
the best (or the least evil)? Do you have other suggestions I could try? Is
there anyone else who may have opinions/advice on how to proceed?

[Jens Widell, 2015-04-30 10:17:06]

On 2015/04/30 07:52:50, sadrul wrote:
> On 2015/04/30 05:36:34, esprehn wrote:
> > CompositorProxy doesn't feel like a separate module, it's very wrapped up in
> the
> > style and Element machinery. In the same way the animations engine doesn't
> seem
> > like a separate module.
> 
> Thanks Elliott!
> 
> So that effectively rules out option (2).
> 
> haraken@, kinuko@, jl@, bashi@: Among the remaining options
> (https://codereview.chromium.org/1101583003/#msg13), which do you think would
be
> the best (or the least evil)? Do you have other suggestions I could try? Is
> there anyone else who may have opinions/advice on how to proceed?

IMO, just dropping the check would be an acceptable option too. It doesn't add
all that much; hopefully any added feature has at least enough testing to detect
if its interfaces aren't even exposed (due to a misspelled [Exposed=X]).
Alternatively; the full set of environments the IDL compiler actually supports
is in the EXPOSED_EXECUTION_CONTEXT_METHOD global in v8_utilities.py. We could
perhaps use that instead of a computed list of global environments, which, as
seen here, isn't necessarily correct in the first place. Both of those options
would allow some simplification of generate_global_constructors.py and the
related GYP/GN machinery as well, as a bonus.

Your solution (3) has the problem that we might have to modify the IDL relative
the specification just to accommodate for how we arrange the source code into
core/ and modules/, which seems unfortunate and unnecessary to me. Plus added
complexity in the IDL compiler, of course.

[sadrul, 2015-05-01 04:28:38]

On 2015/04/30 10:17:06, Jens Widell wrote:
> On 2015/04/30 07:52:50, sadrul wrote:
> > On 2015/04/30 05:36:34, esprehn wrote:
> > > CompositorProxy doesn't feel like a separate module, it's very wrapped up
in
> > the
> > > style and Element machinery. In the same way the animations engine doesn't
> > seem
> > > like a separate module.
> > 
> > Thanks Elliott!
> > 
> > So that effectively rules out option (2).
> > 
> > haraken@, kinuko@, jl@, bashi@: Among the remaining options
> > (https://codereview.chromium.org/1101583003/#msg13), which do you think
would
> be
> > the best (or the least evil)? Do you have other suggestions I could try? Is
> > there anyone else who may have opinions/advice on how to proceed?
> 
> IMO, just dropping the check would be an acceptable option too. It doesn't add
> all that much; hopefully any added feature has at least enough testing to
detect
> if its interfaces aren't even exposed (due to a misspelled [Exposed=X]).
> Alternatively; the full set of environments the IDL compiler actually supports
> is in the EXPOSED_EXECUTION_CONTEXT_METHOD global in v8_utilities.py. We could
> perhaps use that instead of a computed list of global environments, which, as
> seen here, isn't necessarily correct in the first place.

I have gone ahead and made this change. Thanks!

> Both of those options
> would allow some simplification of generate_global_constructors.py and the
> related GYP/GN machinery as well, as a bonus.

It isn't clear to me what kind of simplification this would allow. If you could
explain, I would be happy to look into them.

> 
> Your solution (3) has the problem that we might have to modify the IDL
relative
> the specification just to accommodate for how we arrange the source code into
> core/ and modules/, which seems unfortunate and unnecessary to me. Plus added
> complexity in the IDL compiler, of course.

Good point. Thanks again for the feedback!

[esprehn, 2015-05-01 04:59:25]

So are we going to get rid of the module? It doesn't make sense to have this
feature split across modules/compositorworker and core, and the feature itself
seems pretty integral to rendering so it seems we should move it into core in a
compositorworker directory. This isn't really dom/ it's an intersection point
between DOM and style.

[kinuko, 2015-05-07 05:16:48]

On 2015/05/01 04:59:25, esprehn wrote:
> So are we going to get rid of the module? It doesn't make sense to have this
> feature split across modules/compositorworker and core, and the feature itself
> seems pretty integral to rendering so it seems we should move it into core in
a
> compositorworker directory. This isn't really dom/ it's an intersection point
> between DOM and style.

In general I prefer having the entire compositor worker stuff in its own module
if possible, given that it's still an unspec'ed new stuff and having a structure
that allows us to write core/ code that depends on compositor worker doesn't
sound like a great idea to me. Is other code in core/ going to heavily depend on
CompositorProxy? In the current code it doesn't seem the case so it looks we
could move the proxy stuff out of core/ too.

[sadrul, 2015-05-07 05:24:22]

On 2015/05/07 05:16:48, kinuko wrote:
> On 2015/05/01 04:59:25, esprehn wrote:
> > So are we going to get rid of the module? It doesn't make sense to have this
> > feature split across modules/compositorworker and core, and the feature
itself
> > seems pretty integral to rendering so it seems we should move it into core
in
> a
> > compositorworker directory. This isn't really dom/ it's an intersection
point
> > between DOM and style.
> 
> In general I prefer having the entire compositor worker stuff in its own
module
> if possible, given that it's still an unspec'ed new stuff and having a
structure
> that allows us to write core/ code that depends on compositor worker doesn't
> sound like a great idea to me. Is other code in core/ going to heavily depend
on
> CompositorProxy?

I have addressed some of the difficulties with moving CompositorProxy out of
core/ in https://codereview.chromium.org/1101583003/#msg14 (and Elliott's
response in https://codereview.chromium.org/1101583003/#msg15 is also relevant)

> In the current code it doesn't seem the case so it looks we
> could move the proxy stuff out of core/ too.

dom/, style/, layout/ all have bits relevant to only compositor proxy.

[kinuko, 2015-05-07 06:49:45]

On 2015/05/07 05:24:22, sadrul wrote:
> On 2015/05/07 05:16:48, kinuko wrote:
> > On 2015/05/01 04:59:25, esprehn wrote:
> > > So are we going to get rid of the module? It doesn't make sense to have
this
> > > feature split across modules/compositorworker and core, and the feature
> itself
> > > seems pretty integral to rendering so it seems we should move it into core
> in
> > a
> > > compositorworker directory. This isn't really dom/ it's an intersection
> point
> > > between DOM and style.
> > 
> > In general I prefer having the entire compositor worker stuff in its own
> module
> > if possible, given that it's still an unspec'ed new stuff and having a
> structure
> > that allows us to write core/ code that depends on compositor worker doesn't
> > sound like a great idea to me. Is other code in core/ going to heavily
depend
> on
> > CompositorProxy?
> 
> I have addressed some of the difficulties with moving CompositorProxy out of
> core/ in https://codereview.chromium.org/1101583003/#msg14 (and Elliott's
> response in https://codereview.chromium.org/1101583003/#msg15 is also
relevant)

I see thanks for the pointers, so you've already excluded 2) as a feasible
solution.

> > In the current code it doesn't seem the case so it looks we
> > could move the proxy stuff out of core/ too.
> 
> dom/, style/, layout/ all have bits relevant to only compositor proxy.

Let me try to think of one more alternative (that could be dumb).  Could
'compositor proxy' be a generic interface that proxies element states to/from
outer compositor module like compositor worker and it's exposed as compositor
proxy in a world where where compositor worker is enabled?  (So the proxy code
could live in core/ while Compsotor* idls are in its own module)

[sadrul, 2015-05-07 07:02:42]

On 2015/05/07 06:49:45, kinuko wrote:
> On 2015/05/07 05:24:22, sadrul wrote:
> > On 2015/05/07 05:16:48, kinuko wrote:
> > > On 2015/05/01 04:59:25, esprehn wrote:
> > > > So are we going to get rid of the module? It doesn't make sense to have
> this
> > > > feature split across modules/compositorworker and core, and the feature
> > itself
> > > > seems pretty integral to rendering so it seems we should move it into
core
> > in
> > > a
> > > > compositorworker directory. This isn't really dom/ it's an intersection
> > point
> > > > between DOM and style.
> > > 
> > > In general I prefer having the entire compositor worker stuff in its own
> > module
> > > if possible, given that it's still an unspec'ed new stuff and having a
> > structure
> > > that allows us to write core/ code that depends on compositor worker
doesn't
> > > sound like a great idea to me. Is other code in core/ going to heavily
> depend
> > on
> > > CompositorProxy?
> > 
> > I have addressed some of the difficulties with moving CompositorProxy out of
> > core/ in https://codereview.chromium.org/1101583003/#msg14 (and Elliott's
> > response in https://codereview.chromium.org/1101583003/#msg15 is also
> relevant)
> 
> I see thanks for the pointers, so you've already excluded 2) as a feasible
> solution.
> 
> > > In the current code it doesn't seem the case so it looks we
> > > could move the proxy stuff out of core/ too.
> > 
> > dom/, style/, layout/ all have bits relevant to only compositor proxy.
> 
> Let me try to think of one more alternative (that could be dumb).  Could
> 'compositor proxy' be a generic interface that proxies element states to/from
> outer compositor module like compositor worker and it's exposed as compositor
> proxy in a world where where compositor worker is enabled?  (So the proxy code
> could live in core/ while Compsotor* idls are in its own module)

I am not sure I understand correctly, but it sounds to me like you are
suggesting essentially splitting up/adding a wrapper to the proxy code so parts
of it could live in the module, while some parts could still live in core/?
Would that really be better than what we have now in this CL?

[kinuko, 2015-05-07 07:33:48]

On 2015/05/07 07:02:42, sadrul wrote:
> On 2015/05/07 06:49:45, kinuko wrote:
> > On 2015/05/07 05:24:22, sadrul wrote:
> > > On 2015/05/07 05:16:48, kinuko wrote:
> > > > On 2015/05/01 04:59:25, esprehn wrote:
> > > > > So are we going to get rid of the module? It doesn't make sense to
have
> > this
> > > > > feature split across modules/compositorworker and core, and the
feature
> > > itself
> > > > > seems pretty integral to rendering so it seems we should move it into
> core
> > > in
> > > > a
> > > > > compositorworker directory. This isn't really dom/ it's an
intersection
> > > point
> > > > > between DOM and style.
> > > > 
> > > > In general I prefer having the entire compositor worker stuff in its own
> > > module
> > > > if possible, given that it's still an unspec'ed new stuff and having a
> > > structure
> > > > that allows us to write core/ code that depends on compositor worker
> doesn't
> > > > sound like a great idea to me. Is other code in core/ going to heavily
> > depend
> > > on
> > > > CompositorProxy?
> > > 
> > > I have addressed some of the difficulties with moving CompositorProxy out
of
> > > core/ in https://codereview.chromium.org/1101583003/#msg14 (and Elliott's
> > > response in https://codereview.chromium.org/1101583003/#msg15 is also
> > relevant)
> > 
> > I see thanks for the pointers, so you've already excluded 2) as a feasible
> > solution.
> > 
> > > > In the current code it doesn't seem the case so it looks we
> > > > could move the proxy stuff out of core/ too.
> > > 
> > > dom/, style/, layout/ all have bits relevant to only compositor proxy.
> > 
> > Let me try to think of one more alternative (that could be dumb).  Could
> > 'compositor proxy' be a generic interface that proxies element states
to/from
> > outer compositor module like compositor worker and it's exposed as
compositor
> > proxy in a world where where compositor worker is enabled?  (So the proxy
code
> > could live in core/ while Compsotor* idls are in its own module)
> 
> I am not sure I understand correctly, but it sounds to me like you are
> suggesting essentially splitting up/adding a wrapper to the proxy code so
parts
> of it could live in the module, while some parts could still live in core/?
> Would that really be better than what we have now in this CL?

I think I'm asking if compositor proxy could be considered a generic proxy layer
to the core rendering code or not.  If that's the case the proxy code could live
in core/ and we could give a name in compositor worker context.  I don't think
that necessarily means we'll introduce a wrapper code.  Otherwise we'll be
basically scattering 'compositor worker'-specific code across core code.  If all
of you are happy with the architecture I could be convinced.

[esprehn, 2015-05-07 07:38:13]

Is there a generic primitive that CompositorProxy is built on? If its a
primitive of the platform it should be in core, if its built on primitives
it could be a module and core could have that primitive added.
On May 7, 2015 12:33 AM, <kinuko@chromium.org> wrote:

> On 2015/05/07 07:02:42, sadrul wrote:
>
>> On 2015/05/07 06:49:45, kinuko wrote:
>> > On 2015/05/07 05:24:22, sadrul wrote:
>> > > On 2015/05/07 05:16:48, kinuko wrote:
>> > > > On 2015/05/01 04:59:25, esprehn wrote:
>> > > > > So are we going to get rid of the module? It doesn't make sense to
>>
> have
>
>> > this
>> > > > > feature split across modules/compositorworker and core, and the
>>
> feature
>
>> > > itself
>> > > > > seems pretty integral to rendering so it seems we should move it
>> into
>> core
>> > > in
>> > > > a
>> > > > > compositorworker directory. This isn't really dom/ it's an
>>
> intersection
>
>> > > point
>> > > > > between DOM and style.
>> > > >
>> > > > In general I prefer having the entire compositor worker stuff in
>> its own
>> > > module
>> > > > if possible, given that it's still an unspec'ed new stuff and
>> having a
>> > > structure
>> > > > that allows us to write core/ code that depends on compositor worker
>> doesn't
>> > > > sound like a great idea to me. Is other code in core/ going to
>> heavily
>> > depend
>> > > on
>> > > > CompositorProxy?
>> > >
>> > > I have addressed some of the difficulties with moving CompositorProxy
>> out
>>
> of
>
>> > > core/ in https://codereview.chromium.org/1101583003/#msg14 (and
>> Elliott's
>> > > response in https://codereview.chromium.org/1101583003/#msg15 is also
>> > relevant)
>> >
>> > I see thanks for the pointers, so you've already excluded 2) as a
>> feasible
>> > solution.
>> >
>> > > > In the current code it doesn't seem the case so it looks we
>> > > > could move the proxy stuff out of core/ too.
>> > >
>> > > dom/, style/, layout/ all have bits relevant to only compositor proxy.
>> >
>> > Let me try to think of one more alternative (that could be dumb).  Could
>> > 'compositor proxy' be a generic interface that proxies element states
>>
> to/from
>
>> > outer compositor module like compositor worker and it's exposed as
>>
> compositor
>
>> > proxy in a world where where compositor worker is enabled?  (So the
>> proxy
>>
> code
>
>> > could live in core/ while Compsotor* idls are in its own module)
>>
>
>  I am not sure I understand correctly, but it sounds to me like you are
>> suggesting essentially splitting up/adding a wrapper to the proxy code so
>>
> parts
>
>> of it could live in the module, while some parts could still live in
>> core/?
>> Would that really be better than what we have now in this CL?
>>
>
> I think I'm asking if compositor proxy could be considered a generic proxy
> layer
> to the core rendering code or not.  If that's the case the proxy code
> could live
> in core/ and we could give a name in compositor worker context.  I don't
> think
> that necessarily means we'll introduce a wrapper code.  Otherwise we'll be
> basically scattering 'compositor worker'-specific code across core code.
> If all
> of you are happy with the architecture I could be convinced.
>
>
>
> https://codereview.chromium.org/1101583003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sadrul, 2015-05-07 08:24:52]

On 2015/05/07 07:38:13, esprehn wrote:
> Is there a generic primitive that CompositorProxy is built on? If its a
> primitive of the platform it should be in core, if its built on primitives
> it could be a module and core could have that primitive added.

I don't believe there is a generic primitive CompositorProxy is (or could be,
without introducing one just for CP) built on, no
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

From the discussion here, would it be reasonable to say that CompositorProxy
belongs in core/? If so, the unresolved question is whether or not it makes
sense to still have the compositorworker module. The possibilities:
 . Yes: it keeps the code related to the worker and associated thread[s]
separate from core/, while keeping the proxy code inside core/ to allow
cooperating with dom etc. The difficulty then is to expose CompositorProxy (from
core) to CompositorWorker (in module). This is that this CL does.
 . No: it's arguably a bit awkward to have code related to compositor workers
split across both in core/ and modules/. This though probably isn't too
problematic?

I lean towards Yes, although I could be convinced otherwise. If there are no
specific objections to this, perhaps we could proceed with this for now, and
revisit the module vs core decision when the compositor-worker code has matured
a bit more?

[sadrul, 2015-05-20 06:50:15]

haraken@/bashi@/jl@: mind taking a look at the bindings change? (I can split it
into a separate CL if you would prefer?)

[haraken, 2015-05-20 07:17:51]

bindings LGTM

[sadrul, 2015-05-20 07:23:22]

On 2015/05/20 07:17:51, haraken wrote:
> bindings LGTM

Thanks!

kinuko@/esprehn@ Mind reviewing Source/core, Source/modules, and LayoutTests
changes? Thanks!

[kinuko, 2015-05-20 08:30:40]

On 2015/05/20 07:23:22, sadrul wrote:
> kinuko@/esprehn@ Mind reviewing Source/core, Source/modules, and LayoutTests
> changes? Thanks!

lgtm

[sadrul, 2015-05-20 20:00:03]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-20 20:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1101583003/#ps120001
(title: "tot-merge")

[commit-bot: I haz the power, 2015-05-20 20:00:19]

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

[commit-bot: I haz the power, 2015-05-20 20:04:49]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195635