compositor-worker: Introduce CompositorProxy.

compositor-worker: Introduce CompositorProxy.

Notable changes:

. Make it possible to send a CompositorProxy object via post-message between
  threads.
. Add a test to verify that the proxy objects for the same element get the same
  |elementId|, and different set of attributes set the correct flags on the
  proxies.

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

[sadrul, 2015-03-24 07:22:46]

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

[sadrul, 2015-03-24 07:22:47]

haraken@ Would you please review the changes in Source/bindings/? We want to be
able to send a CompositorProxy from the main-thread to a worker-thread. The
changes in ScriptValueSerializer are related to that.

esprehn@ As you suggested, I have removed the mapping I previously had in
CompositorProxy, and replaced that with core/inspector/InspectorNodeIds.h (which
works both with oilpan (uses hash-map with weak-pointers) and without (using
WeakNodeMap)). Please take a look. (perhaps it would make sense to
rename/relocate InspectorNodeIds to somewhere common so that both inspector and
worker can use it?)

Thanks!

[haraken, 2015-03-24 08:02:14]

bindings/ LGTM.

[esprehn, 2015-03-25 23:21:48]

https://codereview.chromium.org/1024233002/diff/40001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/1024233002/diff/40001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:21: class CompositorProxy : public
GarbageCollectedFinalized<CompositorProxy>, public ScriptWrappable {
final

https://codereview.chromium.org/1024233002/diff/40001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:29: NONE        = 0x0000,
0 << 1,
0 << 2,

is the normal blink pattern.

https://codereview.chromium.org/1024233002/diff/40001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1024233002/diff/40001/Source/core/dom/Element...
Source/core/dom/Element.cpp:180: CompositorProxy::notifyElementGone(this);
You want to use a WeakNodeMap, we don't need to duplicate this.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:18: } allowedAttributes[] = {
Lets declare the struct and the usage separately, we don't generally combine
them like this in blink.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:25: CompositorProxy*
CompositorProxy::create(ExecutionContext* context, Element* element,
Vector<String>& attributeArray, ExceptionState& exceptionState)
OwnPtr, this leaks when not in Oilpan mode.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:37: if (attribute.lower() ==
allowedAttributes[i].name) {
Call .lower() first and then use std::lower_bound to binary search the array.
See RenderQuote, it should be easier than this.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:49: ASSERT(attributeFlags);
This assert should be in the constructor?

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:55: ASSERT(!isMainThread());
Move this assert into the constructor.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:57: uint32_t sanityCheckAttributes =
attributeFlags;
This assert block for sanity checking should be in the constructor, it's okay to
put it into a static function in the file and call it in both constructors.
create methods should have almost no code in them.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:66:
CompositorProxy::CompositorProxy(Element* element, uint32_t attributeFlags)
reference

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:67: :
m_elementId(InspectorNodeIds::idForNode(element))
We should move InspectorNodeIds into code in a first patch.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:21: class CompositorProxy : public
GarbageCollectedFinalized<CompositorProxy>, public ScriptWrappable {
final

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:33: TRANSFORM   = 0x0008,
1 << 1,
1 << 2,

is preferred in blink.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:39: uint32_t attributes() const { return
m_attributes; }
Can this be a bitfield instead?

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:42: CompositorProxy(Element*, uint32_t
attributeFlags);
reference

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:47: uint32_t m_attributes;
const? Attributes doesn't seem to be able to change either.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.idl (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.idl:13: readonly attribute long elementId;
I don't think we should be exposing these ids to the page, they're an
implementation detail.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.idl:14: readonly attribute long attributes;
This is just for hacking? We don't use bit fields like this in web apis.

[esprehn, 2015-03-25 23:22:16]

Sorry you can ignore my comment about non-oilpan mode, I guess we always have it
now so new features can use it.

[sadrul, 2015-03-27 17:23:29]

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:18: } allowedAttributes[] = {
On 2015/03/25 23:21:48, esprehn wrote:
> Lets declare the struct and the usage separately, we don't generally combine
> them like this in blink.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:37: if (attribute.lower() ==
allowedAttributes[i].name) {
On 2015/03/25 23:21:48, esprehn wrote:
> Call .lower() first and then use std::lower_bound to binary search the array.
> See RenderQuote, it should be easier than this.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:49: ASSERT(attributeFlags);
On 2015/03/25 23:21:48, esprehn wrote:
> This assert should be in the constructor?

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:55: ASSERT(!isMainThread());
On 2015/03/25 23:21:48, esprehn wrote:
> Move this assert into the constructor.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:57: uint32_t sanityCheckAttributes =
attributeFlags;
On 2015/03/25 23:21:48, esprehn wrote:
> This assert block for sanity checking should be in the constructor, it's okay
to
> put it into a static function in the file and call it in both constructors.
> create methods should have almost no code in them.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:66:
CompositorProxy::CompositorProxy(Element* element, uint32_t attributeFlags)
On 2015/03/25 23:21:48, esprehn wrote:
> reference

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.cpp:67: :
m_elementId(InspectorNodeIds::idForNode(element))
On 2015/03/25 23:21:48, esprehn wrote:
> We should move InspectorNodeIds into code in a first patch.

Done. (https://codereview.chromium.org/997073004/)

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:21: class CompositorProxy : public
GarbageCollectedFinalized<CompositorProxy>, public ScriptWrappable {
On 2015/03/25 23:21:48, esprehn wrote:
> final

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:33: TRANSFORM   = 0x0008,
On 2015/03/25 23:21:48, esprehn wrote:
> 1 << 1,
> 1 << 2,
> 
> is preferred in blink.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:39: uint32_t attributes() const { return
m_attributes; }
On 2015/03/25 23:21:48, esprehn wrote:
> Can this be a bitfield instead?

I am not entirely sure I understand. Do you mean something like
FontTraitsBitfield
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:42: CompositorProxy(Element*, uint32_t
attributeFlags);
On 2015/03/25 23:21:48, esprehn wrote:
> reference

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:47: uint32_t m_attributes;
On 2015/03/25 23:21:48, esprehn wrote:
> const? Attributes doesn't seem to be able to change either.

Done.

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.idl (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.idl:14: readonly attribute long attributes;
On 2015/03/25 23:21:48, esprehn wrote:
> This is just for hacking? We don't use bit fields like this in web apis.

Both of these are for testing, yes. I have removed both, and updated the test to
use the 'supports()' API call instead.

[sadrul, 2015-03-27 18:14:56]

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
File Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/1024233002/diff/60001/Source/core/dom/Composi...
Source/core/dom/CompositorProxy.h:39: uint32_t attributes() const { return
m_attributes; }
On 2015/03/27 17:23:28, sadrul wrote:
> On 2015/03/25 23:21:48, esprehn wrote:
> > Can this be a bitfield instead?
> 
> I am not entirely sure I understand. Do you mean something like
> FontTraitsBitfield
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I have gone ahead and made this change. PTAL.

[esprehn, 2015-03-27 23:48:42]

The bitfield like this seems kind of worse, maybe we don't do that.

lgtm

https://codereview.chromium.org/1024233002/diff/180001/Source/core/dom/Compos...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1024233002/diff/180001/Source/core/dom/Compos...
Source/core/dom/CompositorProxy.cpp:55:
attributesBitfield.m_attributes.m_opacity = true;
Hmm this is a bit worse I guess since you need all these switch statements. I
think it's fine to switch back to the bitfield like you had before so you don't
need all these switches and conversions. :)

you can just |= them into the other side of the union right?

[sadrul, 2015-03-28 02:12:39]

https://codereview.chromium.org/1024233002/diff/180001/Source/core/dom/Compos...
File Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1024233002/diff/180001/Source/core/dom/Compos...
Source/core/dom/CompositorProxy.cpp:55:
attributesBitfield.m_attributes.m_opacity = true;
On 2015/03/27 23:48:42, esprehn wrote:
> Hmm this is a bit worse I guess since you need all these switch statements. I
> think it's fine to switch back to the bitfield like you had before so you
don't
> need all these switches and conversions. :)
> 

Done.

> you can just |= them into the other side of the union right?

Yep.

[sadrul, 2015-03-28 02:12:45]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-03-28 02:12:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1024233002/#ps200001
(title: "ctrl-z-bitfields")

[commit-bot: I haz the power, 2015-03-28 02:12:52]

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

[commit-bot: I haz the power, 2015-03-28 03:36:18]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192715

[adamk, 2015-05-19 21:38:52]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-05-19 21:38:53]

https://codereview.chromium.org/1024233002/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/SerializationTag.h (right):

https://codereview.chromium.org/1024233002/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/SerializationTag.h:81: CompositorProxyTag = 'C',
Sorry for the month-late comment, but can you please put a comment here matching
the format of the rest of the comments in this file?

[sadrul, 2015-05-23 23:53:43]

https://codereview.chromium.org/1024233002/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/SerializationTag.h (right):

https://codereview.chromium.org/1024233002/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/SerializationTag.h:81: CompositorProxyTag = 'C',
On 2015/05/19 21:38:53, adamk wrote:
> Sorry for the month-late comment, but can you please put a comment here
matching
> the format of the rest of the comments in this file?

Put up for review at: https://codereview.chromium.org/1152943002