Issue 1602343002: compositor-worker: cc->blink mutation plumbing

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 11 months ago (2016-01-19 23:57:48 UTC) #1

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 11 months ago (2016-01-20 00:07:54 UTC) #2

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 11 months ago (2016-01-20 16:24:58 UTC) #3

majidvp

majidvp@chromium.org changed reviewers: + flackr@chromium.org

4 years, 11 months ago (2016-01-20 16:24:58 UTC) #4

flackr

https://codereview.chromium.org/1602343002/diff/20001/third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right): https://codereview.chromium.org/1602343002/diff/20001/third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp#newcode253 third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:253: EXPECT_EQ(String("0.5"), cssValue); It feels strange that you're testing mutating ...

4 years, 11 months ago (2016-01-20 16:50:50 UTC) #6

majidvp

https://codereview.chromium.org/1602343002/diff/20001/third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right): https://codereview.chromium.org/1602343002/diff/20001/third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp#newcode253 third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:253: EXPECT_EQ(String("0.5"), cssValue); On 2016/01/20 16:50:50, flackr wrote: > It ...

4 years, 11 months ago (2016-01-20 19:23:59 UTC) #7

flackr

https://codereview.chromium.org/1602343002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1602343002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1002 third_party/WebKit/Source/core/dom/Element.cpp:1002: TRACE_EVENT0("compositor-worker", "Element::updateFromMutation"); We should have a comment that the ...

4 years, 11 months ago (2016-01-22 14:34:42 UTC) #8

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 11 months ago (2016-01-22 20:40:47 UTC) #9

majidvp

majidvp@chromium.org changed reviewers: + jbroman@chromium.org

4 years, 11 months ago (2016-01-22 21:13:22 UTC) #10

majidvp

On 2016/01/22 14:34:42, flackr wrote: > https://codereview.chromium.org/1602343002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp > File third_party/WebKit/Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/1602343002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1002 > ...

4 years, 11 months ago (2016-01-22 21:16:32 UTC) #11

Ian Vollick

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

4 years, 11 months ago (2016-01-22 21:56:52 UTC) #12

Ian Vollick

A couple of drive-by nits. https://codereview.chromium.org/1602343002/diff/100001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1602343002/diff/100001/content/renderer/render_widget.cc#newcode1004 content/renderer/render_widget.cc:1004: TRACE_EVENT0("compositor-worker", "RenderWidget::ApplyMutations"); Please use ...

4 years, 11 months ago (2016-01-22 21:56:53 UTC) #13

majidvp

https://codereview.chromium.org/1602343002/diff/100001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1602343002/diff/100001/content/renderer/render_widget.cc#newcode1004 content/renderer/render_widget.cc:1004: TRACE_EVENT0("compositor-worker", "RenderWidget::ApplyMutations"); On 2016/01/22 21:56:53, vollick wrote: > Please ...

4 years, 11 months ago (2016-01-22 23:32:06 UTC) #14

jbroman

I've mostly looked at this from the Blink side of things. I assume the reason ...

4 years, 11 months ago (2016-01-23 23:55:10 UTC) #15

majidvp

> I assume the reason this CL doesn't have end-to-end layout tests yet for > ...

4 years, 11 months ago (2016-01-25 20:40:11 UTC) #16

> I assume the reason this CL doesn't have end-to-end layout tests yet for
> mutations being applied and visible to script (assuming I'm understanding the
> unit test correctly) is that more plumbing is required?

That is a correct assumption. This CL does not include plumbing from
blink->cc. Once that is in place we have layout test that verify
the whole pipeline.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:130: #include
"platform/transforms/MatrixTransformOperation.h"
On 2016/01/23 23:55:09, jbroman wrote:
> This include doesn't seem to be used; am I missing the use?

removed.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1000: void
Element::updateFromMutation(const CompositorMutation& mutation)
On 2016/01/23 23:55:09, jbroman wrote:
> Has a core/css/ expert looked at this logic before? If not, I'd suggest having
> one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.

Not yet, +esprehn@ to check. Basically I see two options for updating the style:
1- Update computed style similar to what happens for compositor-driven CSS
animations
2- Update inline style similar to what happens when user script modifies
element.style.{transform,opacity} in a rAF.

These mutations are coming from user script that is directly manipulating
transform/opacity on the proxies element. So it seemed most appropriate that
they get applied to inline style.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.h:546: void updateFromMutation(const
CompositorMutation&);
On 2016/01/23 23:55:09, jbroman wrote:
> Mind also renaming this "updateFromCompositorMutation" or similar? I know it's
> long, but it makes it easier to see in a call site what's up, and it's
> consistent with the methods above it.

Done.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorMutation.h (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutation.h:56: struct
CompositorMutations : public WebCompositorMutations {
On 2016/01/23 23:55:09, jbroman wrote:
> I'd kind of like this to explicitly override the destructor:
> 
> ~CompositorMutations() override {}
> 
> if only because I keep going to the base classes to check whether the HashMap
> will be destroyed properly.

Done.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4462: if (!node ||
!toElement(node))
On 2016/01/23 23:55:09, jbroman wrote:
> toElement is not a check; it's a cast (with an assertion that's not run in
> release).
> 
> You want "!node || !node->isElementNode()" if you actually want to check. If
> you're confident this can only ever be an element, then the second part of the
> check isn't necessary.

It is guaranteed to be an an Element. Check removed.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:242:
EXPECT_EQ(String("matrix3d(11px, 12px, 13px, 14px, 21px, 22px, 23px, 24px, 31px,
32px, 33px, 34px, 41px, 42px, 43px, 44px)"), cssValue);
On 2016/01/23 23:55:09, jbroman wrote:
> It shouldn't be necessary to wrap in a WTF::String, here or below. The
necessary
> operator== overload exists. It's shorter and faster. :)

Done.

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/DEPS (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/DEPS:12: "+cc/animation"
On 2016/01/23 23:55:10, jbroman wrote:
> nit: alphabetize?
> 
> (or if my other comment is accepted, this line won't be necessary)

Removed. +cc has already been added by a different CL.
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebWidget.h:152: virtual void applyMutations(const
WebCompositorMutations&) {}
On 2016/01/23 23:55:10, jbroman wrote:
> nit: Would you mind including "compositor" or similar in the method name? It's
> not obvious until you look at the parameters that this has to do with
> compositing (as opposed to mutating some other aspect of a widget).

Done.

> I'm not a public/ owner, but since this CL already requires that
> cc::LayerTreeMutations be used within public/, why not just forward-declare
and
> say "const cc::LayerTreeMutations&"? That class already has the same virtual
> members. (It also avoids having to actually include the cc header, unlike
> subclassing.) Ignore if a public OWNER overrules me. :)

Happy to change if that is desirable. My impression after looking at recent
similar CLs
is that it is still desirable to define a type in Blink  by extending the CC
type [1].

[1]
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...

jbroman

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

4 years, 11 months ago (2016-01-26 15:19:41 UTC) #17

jbroman

On 2016/01/25 at 20:40:11, majidvp wrote: > > I assume the reason this CL doesn't ...

4 years, 11 months ago (2016-01-26 15:19:48 UTC) #18

On 2016/01/25 at 20:40:11, majidvp wrote:
> > I assume the reason this CL doesn't have end-to-end layout tests yet for
> > mutations being applied and visible to script (assuming I'm understanding
the
> > unit test correctly) is that more plumbing is required?
> 
> That is a correct assumption. This CL does not include plumbing from
> blink->cc. Once that is in place we have layout test that verify
> the whole pipeline.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:130: #include
"platform/transforms/MatrixTransformOperation.h"
> On 2016/01/23 23:55:09, jbroman wrote:
> > This include doesn't seem to be used; am I missing the use?
> 
> removed.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:1000: void
Element::updateFromMutation(const CompositorMutation& mutation)
> On 2016/01/23 23:55:09, jbroman wrote:
> > Has a core/css/ expert looked at this logic before? If not, I'd suggest
having
> > one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.
> 
> Not yet, +esprehn@ to check. Basically I see two options for updating the
style:
> 1- Update computed style similar to what happens for compositor-driven CSS
animations
> 2- Update inline style similar to what happens when user script modifies
element.style.{transform,opacity} in a rAF.
> 
> These mutations are coming from user script that is directly manipulating
> transform/opacity on the proxies element. So it seemed most appropriate that
> they get applied to inline style.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.h:546: void
updateFromMutation(const CompositorMutation&);
> On 2016/01/23 23:55:09, jbroman wrote:
> > Mind also renaming this "updateFromCompositorMutation" or similar? I know
it's
> > long, but it makes it easier to see in a call site what's up, and it's
> > consistent with the methods above it.
> 
> Done.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/CompositorMutation.h (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/CompositorMutation.h:56: struct
CompositorMutations : public WebCompositorMutations {
> On 2016/01/23 23:55:09, jbroman wrote:
> > I'd kind of like this to explicitly override the destructor:
> > 
> > ~CompositorMutations() override {}
> > 
> > if only because I keep going to the base classes to check whether the
HashMap
> > will be destroyed properly.
> 
> Done.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebViewImpl.cpp:4462: if (!node ||
!toElement(node))
> On 2016/01/23 23:55:09, jbroman wrote:
> > toElement is not a check; it's a cast (with an assertion that's not run in
> > release).
> > 
> > You want "!node || !node->isElementNode()" if you actually want to check. If
> > you're confident this can only ever be an element, then the second part of
the
> > check isn't necessary.
> 
> It is guaranteed to be an an Element. Check removed.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:242:
EXPECT_EQ(String("matrix3d(11px, 12px, 13px, 14px, 21px, 22px, 23px, 24px, 31px,
32px, 33px, 34px, 41px, 42px, 43px, 44px)"), cssValue);
> On 2016/01/23 23:55:09, jbroman wrote:
> > It shouldn't be necessary to wrap in a WTF::String, here or below. The
necessary
> > operator== overload exists. It's shorter and faster. :)
> 
> Done.
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/DEPS (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/DEPS:12: "+cc/animation"
> On 2016/01/23 23:55:10, jbroman wrote:
> > nit: alphabetize?
> > 
> > (or if my other comment is accepted, this line won't be necessary)
> 
> Removed. +cc has already been added by a different CL.
>
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebWidget.h (right):
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebWidget.h:152: virtual void
applyMutations(const WebCompositorMutations&) {}
> On 2016/01/23 23:55:10, jbroman wrote:
> > nit: Would you mind including "compositor" or similar in the method name?
It's
> > not obvious until you look at the parameters that this has to do with
> > compositing (as opposed to mutating some other aspect of a widget).
> 
> Done.
> 
> > I'm not a public/ owner, but since this CL already requires that
> > cc::LayerTreeMutations be used within public/, why not just forward-declare
and
> > say "const cc::LayerTreeMutations&"? That class already has the same virtual
> > members. (It also avoids having to actually include the cc header, unlike
> > subclassing.) Ignore if a public OWNER overrules me. :)
> 
> Happy to change if that is desirable. My impression after looking at recent
similar CLs
> is that it is still desirable to define a type in Blink  by extending the CC
type [1].
> 
> [1]
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...

Hmm, after talking to esprehn, it sounds like he has a strong view on this. I'll
defer this point to him (and add him as a reviewer).

majidvp

On 2016/01/26 15:19:48, jbroman wrote: > On 2016/01/25 at 20:40:11, majidvp wrote: > > > ...

4 years, 11 months ago (2016-01-26 15:40:53 UTC) #19

On 2016/01/26 15:19:48, jbroman wrote:
> On 2016/01/25 at 20:40:11, majidvp wrote:
> > > I assume the reason this CL doesn't have end-to-end layout tests yet for
> > > mutations being applied and visible to script (assuming I'm understanding
> the
> > > unit test correctly) is that more plumbing is required?
> > 
> > That is a correct assumption. This CL does not include plumbing from
> > blink->cc. Once that is in place we have layout test that verify
> > the whole pipeline.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.cpp:130: #include
> "platform/transforms/MatrixTransformOperation.h"
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > This include doesn't seem to be used; am I missing the use?
> > 
> > removed.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.cpp:1000: void
> Element::updateFromMutation(const CompositorMutation& mutation)
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > Has a core/css/ expert looked at this logic before? If not, I'd suggest
> having
> > > one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.
> > 
> > Not yet, +esprehn@ to check. Basically I see two options for updating the
> style:
> > 1- Update computed style similar to what happens for compositor-driven CSS
> animations
> > 2- Update inline style similar to what happens when user script modifies
> element.style.{transform,opacity} in a rAF.
> > 
> > These mutations are coming from user script that is directly manipulating
> > transform/opacity on the proxies element. So it seemed most appropriate that
> > they get applied to inline style.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/Element.h (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.h:546: void
> updateFromMutation(const CompositorMutation&);
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > Mind also renaming this "updateFromCompositorMutation" or similar? I know
> it's
> > > long, but it makes it easier to see in a call site what's up, and it's
> > > consistent with the methods above it.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/graphics/CompositorMutation.h
(right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/CompositorMutation.h:56: struct
> CompositorMutations : public WebCompositorMutations {
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > I'd kind of like this to explicitly override the destructor:
> > > 
> > > ~CompositorMutations() override {}
> > > 
> > > if only because I keep going to the base classes to check whether the
> HashMap
> > > will be destroyed properly.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/WebViewImpl.cpp:4462: if (!node ||
> !toElement(node))
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > toElement is not a check; it's a cast (with an assertion that's not run in
> > > release).
> > > 
> > > You want "!node || !node->isElementNode()" if you actually want to check.
If
> > > you're confident this can only ever be an element, then the second part of
> the
> > > check isn't necessary.
> > 
> > It is guaranteed to be an an Element. Check removed.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:242:
> EXPECT_EQ(String("matrix3d(11px, 12px, 13px, 14px, 21px, 22px, 23px, 24px,
31px,
> 32px, 33px, 34px, 41px, 42px, 43px, 44px)"), cssValue);
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > It shouldn't be necessary to wrap in a WTF::String, here or below. The
> necessary
> > > operator== overload exists. It's shorter and faster. :)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > File third_party/WebKit/public/platform/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > third_party/WebKit/public/platform/DEPS:12: "+cc/animation"
> > On 2016/01/23 23:55:10, jbroman wrote:
> > > nit: alphabetize?
> > > 
> > > (or if my other comment is accepted, this line won't be necessary)
> > 
> > Removed. +cc has already been added by a different CL.
> >
>
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > File third_party/WebKit/public/web/WebWidget.h (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > third_party/WebKit/public/web/WebWidget.h:152: virtual void
> applyMutations(const WebCompositorMutations&) {}
> > On 2016/01/23 23:55:10, jbroman wrote:
> > > nit: Would you mind including "compositor" or similar in the method name?
> It's
> > > not obvious until you look at the parameters that this has to do with
> > > compositing (as opposed to mutating some other aspect of a widget).
> > 
> > Done.
> > 
> > > I'm not a public/ owner, but since this CL already requires that
> > > cc::LayerTreeMutations be used within public/, why not just
forward-declare
> and
> > > say "const cc::LayerTreeMutations&"? That class already has the same
virtual
> > > members. (It also avoids having to actually include the cc header, unlike
> > > subclassing.) Ignore if a public OWNER overrules me. :)
> > 
> > Happy to change if that is desirable. My impression after looking at recent
> similar CLs
> > is that it is still desirable to define a type in Blink  by extending the CC
> type [1].
> > 
> > [1]
>
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...
> 
> Hmm, after talking to esprehn, it sounds like he has a strong view on this.
I'll
> defer this point to him (and add him as a reviewer).

Happy to address any concerns of esprehn@.

Elliot, another bit of logic in this CL that you may want to take a closer look
at is in |Element::updateFromCompositorMutations|. Here is the previous
comments:

https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:1000: void
Element::updateFromMutation(const CompositorMutation& mutation)
> On 2016/01/23 23:55:09, jbroman wrote:
> > Has a core/css/ expert looked at this logic before? If not, I'd suggest
having
> > one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.
> 
> Not yet, +esprehn@ to check. Basically I see two options for updating the
style:
> 1- Update computed style similar to what happens for compositor-driven CSS
animations
> 2- Update inline style similar to what happens when user script modifies
element.style.{transform,opacity} in a rAF.
> 
> These mutations are coming from user script that is directly manipulating
> transform/opacity on the proxies element. So it seemed most appropriate that
> they get applied to inline style.

Ian Vollick

vollick@chromium.org changed reviewers: + piman@chromium.org

4 years, 11 months ago (2016-01-27 18:55:57 UTC) #20

Ian Vollick

On 2016/01/26 15:40:53, majidvp wrote: > On 2016/01/26 15:19:48, jbroman wrote: > > On 2016/01/25 ...

4 years, 11 months ago (2016-01-27 18:56:29 UTC) #21

On 2016/01/26 15:40:53, majidvp wrote:
> On 2016/01/26 15:19:48, jbroman wrote:
> > On 2016/01/25 at 20:40:11, majidvp wrote:
> > > > I assume the reason this CL doesn't have end-to-end layout tests yet for
> > > > mutations being applied and visible to script (assuming I'm
understanding
> > the
> > > > unit test correctly) is that more plumbing is required?
> > > 
> > > That is a correct assumption. This CL does not include plumbing from
> > > blink->cc. Once that is in place we have layout test that verify
> > > the whole pipeline.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/Element.cpp:130: #include
> > "platform/transforms/MatrixTransformOperation.h"
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > This include doesn't seem to be used; am I missing the use?
> > > 
> > > removed.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/Element.cpp:1000: void
> > Element::updateFromMutation(const CompositorMutation& mutation)
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > Has a core/css/ expert looked at this logic before? If not, I'd suggest
> > having
> > > > one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.
> > > 
> > > Not yet, +esprehn@ to check. Basically I see two options for updating the
> > style:
> > > 1- Update computed style similar to what happens for compositor-driven CSS
> > animations
> > > 2- Update inline style similar to what happens when user script modifies
> > element.style.{transform,opacity} in a rAF.
> > > 
> > > These mutations are coming from user script that is directly manipulating
> > > transform/opacity on the proxies element. So it seemed most appropriate
that
> > > they get applied to inline style.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/dom/Element.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/Element.h:546: void
> > updateFromMutation(const CompositorMutation&);
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > Mind also renaming this "updateFromCompositorMutation" or similar? I
know
> > it's
> > > > long, but it makes it easier to see in a call site what's up, and it's
> > > > consistent with the methods above it.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/graphics/CompositorMutation.h
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/graphics/CompositorMutation.h:56:
struct
> > CompositorMutations : public WebCompositorMutations {
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > I'd kind of like this to explicitly override the destructor:
> > > > 
> > > > ~CompositorMutations() override {}
> > > > 
> > > > if only because I keep going to the base classes to check whether the
> > HashMap
> > > > will be destroyed properly.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/web/WebViewImpl.cpp:4462: if (!node ||
> > !toElement(node))
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > toElement is not a check; it's a cast (with an assertion that's not run
in
> > > > release).
> > > > 
> > > > You want "!node || !node->isElementNode()" if you actually want to
check.
> If
> > > > you're confident this can only ever be an element, then the second part
of
> > the
> > > > check isn't necessary.
> > > 
> > > It is guaranteed to be an an Element. Check removed.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:242:
> > EXPECT_EQ(String("matrix3d(11px, 12px, 13px, 14px, 21px, 22px, 23px, 24px,
> 31px,
> > 32px, 33px, 34px, 41px, 42px, 43px, 44px)"), cssValue);
> > > On 2016/01/23 23:55:09, jbroman wrote:
> > > > It shouldn't be necessary to wrap in a WTF::String, here or below. The
> > necessary
> > > > operator== overload exists. It's shorter and faster. :)
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > > File third_party/WebKit/public/platform/DEPS (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > > third_party/WebKit/public/platform/DEPS:12: "+cc/animation"
> > > On 2016/01/23 23:55:10, jbroman wrote:
> > > > nit: alphabetize?
> > > > 
> > > > (or if my other comment is accepted, this line won't be necessary)
> > > 
> > > Removed. +cc has already been added by a different CL.
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > > File third_party/WebKit/public/web/WebWidget.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/pub...
> > > third_party/WebKit/public/web/WebWidget.h:152: virtual void
> > applyMutations(const WebCompositorMutations&) {}
> > > On 2016/01/23 23:55:10, jbroman wrote:
> > > > nit: Would you mind including "compositor" or similar in the method
name?
> > It's
> > > > not obvious until you look at the parameters that this has to do with
> > > > compositing (as opposed to mutating some other aspect of a widget).
> > > 
> > > Done.
> > > 
> > > > I'm not a public/ owner, but since this CL already requires that
> > > > cc::LayerTreeMutations be used within public/, why not just
> forward-declare
> > and
> > > > say "const cc::LayerTreeMutations&"? That class already has the same
> virtual
> > > > members. (It also avoids having to actually include the cc header,
unlike
> > > > subclassing.) Ignore if a public OWNER overrules me. :)
> > > 
> > > Happy to change if that is desirable. My impression after looking at
recent
> > similar CLs
> > > is that it is still desirable to define a type in Blink  by extending the
CC
> > type [1].
> > > 
> > > [1]
> >
>
https://chromium.googlesource.com/chromium/src/+/0536dbbcab956b879d2bee49f7e4...
> > 
> > Hmm, after talking to esprehn, it sounds like he has a strong view on this.
> I'll
> > defer this point to him (and add him as a reviewer).
> 
> Happy to address any concerns of esprehn@.
> 
> Elliot, another bit of logic in this CL that you may want to take a closer
look
> at is in |Element::updateFromCompositorMutations|. Here is the previous
> comments:
> 
>
https://codereview.chromium.org/1602343002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.cpp:1000: void
> Element::updateFromMutation(const CompositorMutation& mutation)
> > On 2016/01/23 23:55:09, jbroman wrote:
> > > Has a core/css/ expert looked at this logic before? If not, I'd suggest
> having
> > > one (e.g. esprehn, timloh, or mailto:rune@opera.com) double-check it.
> > 
> > Not yet, +esprehn@ to check. Basically I see two options for updating the
> style:
> > 1- Update computed style similar to what happens for compositor-driven CSS
> animations
> > 2- Update inline style similar to what happens when user script modifies
> element.style.{transform,opacity} in a rAF.
> > 
> > These mutations are coming from user script that is directly manipulating
> > transform/opacity on the proxies element. So it seemed most appropriate that
> > they get applied to inline style.

drive-by reviewer addition :) +piman for content/

majidvp

majidvp@chromium.org changed reviewers: + dtrainor@chromium.org

4 years, 11 months ago (2016-01-27 22:10:31 UTC) #22

esprehn

I don't think you want to be messing with inline style, this should just be ...

4 years, 11 months ago (2016-01-27 23:00:32 UTC) #25

esprehn

I don't think you want to be messing with inline style, this should just be ...

4 years, 11 months ago (2016-01-27 23:06:52 UTC) #26

piman

https://codereview.chromium.org/1602343002/diff/160001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1602343002/diff/160001/content/renderer/render_widget.cc#newcode1007 content/renderer/render_widget.cc:1007: static_cast<const blink::WebCompositorMutations&>(mutations)); This bothers me a little, because I ...

4 years, 10 months ago (2016-01-29 01:50:09 UTC) #27

majidvp

On 2016/01/27 23:06:52, esprehn wrote: > I don't think you want to be messing with ...

4 years, 10 months ago (2016-02-16 19:19:09 UTC) #28

esprehn

Future patches will remove all the elementId logic and stuff too? https://codereview.chromium.org/1602343002/diff/180001/third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp File third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp (right): ...

4 years, 10 months ago (2016-02-17 01:32:36 UTC) #29

esprehn

On 2016/02/16 at 19:19:09, majidvp wrote: > On 2016/01/27 23:06:52, esprehn wrote: > > I ...

4 years, 10 months ago (2016-02-17 01:33:55 UTC) #31

On 2016/02/16 at 19:19:09, majidvp wrote:
> On 2016/01/27 23:06:52, esprehn wrote:
> > I don't think you want to be messing with inline style, this should just be
> > an infinite web Animation that keeps applying styles. That actually seems
> > like a simpler implementation too, we wouldn't need all this extra
> > plumbing, the compositor just pushes properties into the Animation and the
> > main thread applies them. Element doesn't need to know about the feature at
> > all then, you just have a different subclass of Animation.
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> esprehn@: After offline discussion with Ian, I think the plan of record is to 
> update the compositor worker pluming to align it more closely with
web-animation 
> machinery. This will happen in multiple stages as it is a major undertaking.
> 
> This patch is mainly concerned with blink side. So, based on your suggestion
> I have updated the logic to use an infinite key frame web animations 
> (duration: 0, fill: forward, composite:replace) for each mutated property
> (at most two per element one for opacity and one for transform).
> These animations are created lazily and only if there is a mutation.
> See |CompositorMutationAnimations.h|
> 
> In this model, the mutation application simply becomes updating key frame
> values for corresponding animation. This still uses the specific plumbing for
> compositor mutations but once the rest of CW becomes aligned with
web-animation
> machinery perhaps that can be removed.
> 
> wdyt?

sgtm, I do think you want to remove all this top down applyAnimations stuff.
Instead you want this to be handled through the WebLayer on the element, and the
update should come through cc/. You shouldn't need this opaque base class you
cast to things. cc/ shouldn't be trying to pretend we don't exist...

majidvp

On 2016/02/17 01:32:36, esprehn wrote: > Future patches will remove all the elementId logic and ...

4 years, 10 months ago (2016-02-18 14:58:09 UTC) #32

majidvp

piman@: On 2016/01/29 01:50:09, piman wrote: > https://codereview.chromium.org/1602343002/diff/160001/content/renderer/render_widget.cc > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/1602343002/diff/160001/content/renderer/render_widget.cc#newcode1007 ...

4 years, 10 months ago (2016-02-18 15:27:47 UTC) #33

piman@:

On 2016/01/29 01:50:09, piman wrote:
>
https://codereview.chromium.org/1602343002/diff/160001/content/renderer/rende...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/1602343002/diff/160001/content/renderer/rende...
> content/renderer/render_widget.cc:1007: static_cast<const
> blink::WebCompositorMutations&>(mutations));
> This bothers me a little, because I have no way of knowing whether the
> static_cast is safe (I guess the code that fills
> BeginMainFrameAndCommitState::mutations is not in this CL).

Your guess is correct.

> Taking a step back, if I understand where this is going, LayerTreeMutations
> doesn't really have anything to do with cc - it's just some placeholder type
so
> that some data can flow backwards (impl->main) through the commit flow, but cc
> never cares about the data itself.

That is exactly right. CC does not need to know the details of
LayerTreeMutations.
It just gets them from compositor worker and plumbs them back to main thread
so that the relevant elements styles are updated properly.

> 
> In a way, it's the pendant of SwapPromise (just works the other way around).
So
> maybe we could introduce a BeginMainFramePromise or something like that, with
a
> virtual DidBeginMainFrame() - or maybe even more simply a
BeginMainFrameCallback
> that could be stashed on the impl thread, and executed on the main thread?
This
> would give us back the type safety, because the callback could encode the
> concrete type in the curried data (or the concrete implementation of
> BeginMainFramePromise would contain the concrete data). It could also be
reused
> for other purpose (e.g. scroll/viewport deltas, maybe input events, etc.).
> WDYT?

I think this can be done. If I understand it correctly, you are suggesting that
instead of creating individual methods to plumb back values (e.g., 
layer tree mutations or scroll deltas) during the beginMainFrame we can just 
inject promises (similar to swap promises but in reverse) that do it instead
once called back.

In this case, as you mentioned, this gives us back the type safety benefit, and
in general the design becomes more symmetrical and extensible and we can
remove existing methods.

I don't have any objection to this but it is orthogonal to this patch and
better done as a separate CL.

Also, we are planning to refactor the CW plumbing to use web animation 
machinery as much as possible which will probably eliminate or replace this
particular path making the suggested update unnecessary for this case.

majidvp

@esprehn: PTAL. All your comments has been addressed. https://codereview.chromium.org/1602343002/diff/180001/third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp File third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp (right): https://codereview.chromium.org/1602343002/diff/180001/third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp#newcode27 third_party/WebKit/Source/core/animation/CompositorMutationAnimations.cpp:27: static ...

4 years, 10 months ago (2016-02-22 17:59:48 UTC) #34

piman

piman@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org

4 years, 10 months ago (2016-02-23 00:46:52 UTC) #35

piman

On 2016/02/18 15:27:47, majidvp wrote: > piman@: > > On 2016/01/29 01:50:09, piman wrote: > ...

4 years, 10 months ago (2016-02-23 00:52:51 UTC) #36

On 2016/02/18 15:27:47, majidvp wrote:
> piman@:
> 
> On 2016/01/29 01:50:09, piman wrote:
> >
>
https://codereview.chromium.org/1602343002/diff/160001/content/renderer/rende...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/1602343002/diff/160001/content/renderer/rende...
> > content/renderer/render_widget.cc:1007: static_cast<const
> > blink::WebCompositorMutations&>(mutations));
> > This bothers me a little, because I have no way of knowing whether the
> > static_cast is safe (I guess the code that fills
> > BeginMainFrameAndCommitState::mutations is not in this CL).
> 
> Your guess is correct.
> 
> > Taking a step back, if I understand where this is going, LayerTreeMutations
> > doesn't really have anything to do with cc - it's just some placeholder type
> so
> > that some data can flow backwards (impl->main) through the commit flow, but
cc
> > never cares about the data itself.
> 
> That is exactly right. CC does not need to know the details of
> LayerTreeMutations.
> It just gets them from compositor worker and plumbs them back to main thread
> so that the relevant elements styles are updated properly.
> 
> > 
> > In a way, it's the pendant of SwapPromise (just works the other way around).
> So
> > maybe we could introduce a BeginMainFramePromise or something like that,
with
> a
> > virtual DidBeginMainFrame() - or maybe even more simply a
> BeginMainFrameCallback
> > that could be stashed on the impl thread, and executed on the main thread?
> This
> > would give us back the type safety, because the callback could encode the
> > concrete type in the curried data (or the concrete implementation of
> > BeginMainFramePromise would contain the concrete data). It could also be
> reused
> > for other purpose (e.g. scroll/viewport deltas, maybe input events, etc.).
> > WDYT?
> 
> I think this can be done. If I understand it correctly, you are suggesting
that
> instead of creating individual methods to plumb back values (e.g., 
> layer tree mutations or scroll deltas) during the beginMainFrame we can just 
> inject promises (similar to swap promises but in reverse) that do it instead
> once called back.
>  
> In this case, as you mentioned, this gives us back the type safety benefit,
and
> in general the design becomes more symmetrical and extensible and we can
> remove existing methods.
> 
> I don't have any objection to this but it is orthogonal to this patch and
> better done as a separate CL.

+enne/danakj for opinion about cc.

I don't really see this as orthogonal, since we're talking about adding a bunch
of methods to cc to marshall this opaque data, that would disappear if/when we
take the other approach. I'd prefer we take the approach that we're committing
to keep long term. If this is it, so be it, but I find the empty
LayerTreeMutations a bit of a code smell - it's really no better than a void*.


> 
> Also, we are planning to refactor the CW plumbing to use web animation 
> machinery as much as possible which will probably eliminate or replace this
> particular path making the suggested update unnecessary for this case.

enne (OOO)

> > > In a way, it's the pendant of SwapPromise (just works the other ...

4 years, 10 months ago (2016-02-24 22:42:02 UTC) #37

> > > In a way, it's the pendant of SwapPromise (just works the other way
around).
> > So
> > > maybe we could introduce a BeginMainFramePromise or something like that,
with
> > a
> > > virtual DidBeginMainFrame() - or maybe even more simply a
> > BeginMainFrameCallback
> > > that could be stashed on the impl thread, and executed on the main thread?
> > This
> > > would give us back the type safety, because the callback could encode the
> > > concrete type in the curried data (or the concrete implementation of
> > > BeginMainFramePromise would contain the concrete data). It could also be
> > reused
> > > for other purpose (e.g. scroll/viewport deltas, maybe input events, etc.).
> > > WDYT?
> > 
> > I think this can be done. If I understand it correctly, you are suggesting
that
> > instead of creating individual methods to plumb back values (e.g., 
> > layer tree mutations or scroll deltas) during the beginMainFrame we can just

> > inject promises (similar to swap promises but in reverse) that do it instead
> > once called back.
> >  
> > In this case, as you mentioned, this gives us back the type safety benefit,
and
> > in general the design becomes more symmetrical and extensible and we can
> > remove existing methods.
> > 
> > I don't have any objection to this but it is orthogonal to this patch and
> > better done as a separate CL.
> 
> +enne/danakj for opinion about cc.
> 
> I don't really see this as orthogonal, since we're talking about adding a
bunch of methods to cc to marshall this opaque data, that would disappear
if/when we take the other approach. I'd prefer we take the approach that we're
committing to keep long term. If this is it, so be it, but I find the empty
LayerTreeMutations a bit of a code smell - it's really no better than a void*.

Agreed that the static cast is a bit weird to be doing in render_widget.  If it
was the same piece of code creating the objects and also receiving and casting
them, I would feel a bit different, but I feel like any "abstraction savings" by
keeping the type opaque in cc are totally lost by it being transparent and
assumed in RenderWidget, which shouldn't know about it either.  It feels like
something I'd see pre-repository merge.

This discussion also does not feel orthogonal to me.  This patch is plumbing and
these approaches are replacement plumbing for the exact same thing.  It'd be
good to just pick the right approach.

I think the more general callback solution seems best to me, for all the reasons
suggested above (type safety, abstraction, hiding implementation details from
cc).

majidvp

On 2016/02/24 22:42:02, enne wrote: > > > > In a way, it's the pendant ...

4 years, 10 months ago (2016-02-25 22:55:07 UTC) #38

On 2016/02/24 22:42:02, enne wrote:
> > > > In a way, it's the pendant of SwapPromise (just works the other way
> around).
> > > So
> > > > maybe we could introduce a BeginMainFramePromise or something like that,
> with
> > > a
> > > > virtual DidBeginMainFrame() - or maybe even more simply a
> > > BeginMainFrameCallback
> > > > that could be stashed on the impl thread, and executed on the main
thread?
> > > This
> > > > would give us back the type safety, because the callback could encode
the
> > > > concrete type in the curried data (or the concrete implementation of
> > > > BeginMainFramePromise would contain the concrete data). It could also be
> > > reused
> > > > for other purpose (e.g. scroll/viewport deltas, maybe input events,
etc.).
> > > > WDYT?
> > > 
> > > I think this can be done. If I understand it correctly, you are suggesting
> that
> > > instead of creating individual methods to plumb back values (e.g., 
> > > layer tree mutations or scroll deltas) during the beginMainFrame we can
just
> 
> > > inject promises (similar to swap promises but in reverse) that do it
instead
> > > once called back.
> > >  
> > > In this case, as you mentioned, this gives us back the type safety
benefit,
> and
> > > in general the design becomes more symmetrical and extensible and we can
> > > remove existing methods.
> > > 
> > > I don't have any objection to this but it is orthogonal to this patch and
> > > better done as a separate CL.
> > 
> > +enne/danakj for opinion about cc.
> > 
> > I don't really see this as orthogonal, since we're talking about adding a
> bunch of methods to cc to marshall this opaque data, that would disappear
> if/when we take the other approach. I'd prefer we take the approach that we're
> committing to keep long term. If this is it, so be it, but I find the empty
> LayerTreeMutations a bit of a code smell - it's really no better than a void*.
> 
> Agreed that the static cast is a bit weird to be doing in render_widget.  If
it
> was the same piece of code creating the objects and also receiving and casting
> them, I would feel a bit different, but I feel like any "abstraction savings"
by
> keeping the type opaque in cc are totally lost by it being transparent and
> assumed in RenderWidget, which shouldn't know about it either.  It feels like
> something I'd see pre-repository merge.
> 
> This discussion also does not feel orthogonal to me.  This patch is plumbing
and
> these approaches are replacement plumbing for the exact same thing.  It'd be
> good to just pick the right approach.
> 
> I think the more general callback solution seems best to me, for all the
reasons
> suggested above (type safety, abstraction, hiding implementation details from
> cc).

piman@, enne@:
Thanks for feedback. So I will switch to use a promise/callback solution instead
of 
the current empty LayerTreeMutations.

While looking at the SwapPromise usage, I noticed that there is an existing path
where these promises go the reverse direction (cc -> main) during the commit[1]
and they get notified OnCommit. At the moment, this is used for some scroll
latency measurements.

I can reuse this path and just add a new virtual method |DidBeginMainFrame| to
SwapPromise which get invoked when we begin the main frame. That means we
don't need to introduce a whole new promise type to the code.

What do you think about that approach?

[1] LayerTreeHostImpl::QueueSwapPromiseForMainThreadScrollUpdate

piman

On Thu, Feb 25, 2016 at 2:55 PM, <majidvp@chromium.org> wrote: > On 2016/02/24 22:42:02, enne ...

4 years, 10 months ago (2016-02-25 23:13:55 UTC) #39

On Thu, Feb 25, 2016 at 2:55 PM, <majidvp@chromium.org> wrote:

> On 2016/02/24 22:42:02, enne wrote:
> > > > > In a way, it's the pendant of SwapPromise (just works the other way
> > around).
> > > > So
> > > > > maybe we could introduce a BeginMainFramePromise or something like
> that,
> > with
> > > > a
> > > > > virtual DidBeginMainFrame() - or maybe even more simply a
> > > > BeginMainFrameCallback
> > > > > that could be stashed on the impl thread, and executed on the main
> thread?
> > > > This
> > > > > would give us back the type safety, because the callback could
> encode
> the
> > > > > concrete type in the curried data (or the concrete implementation
> of
> > > > > BeginMainFramePromise would contain the concrete data). It could
> also be
> > > > reused
> > > > > for other purpose (e.g. scroll/viewport deltas, maybe input events,
> etc.).
> > > > > WDYT?
> > > >
> > > > I think this can be done. If I understand it correctly, you are
> suggesting
> > that
> > > > instead of creating individual methods to plumb back values (e.g.,
> > > > layer tree mutations or scroll deltas) during the beginMainFrame we
> can
> just
> >
> > > > inject promises (similar to swap promises but in reverse) that do it
> instead
> > > > once called back.
> > > >
> > > > In this case, as you mentioned, this gives us back the type safety
> benefit,
> > and
> > > > in general the design becomes more symmetrical and extensible and we
> can
> > > > remove existing methods.
> > > >
> > > > I don't have any objection to this but it is orthogonal to this
> patch and
> > > > better done as a separate CL.
> > >
> > > +enne/danakj for opinion about cc.
> > >
> > > I don't really see this as orthogonal, since we're talking about
> adding a
> > bunch of methods to cc to marshall this opaque data, that would disappear
> > if/when we take the other approach. I'd prefer we take the approach that
> we're
> > committing to keep long term. If this is it, so be it, but I find the
> empty
> > LayerTreeMutations a bit of a code smell - it's really no better than a
> void*.
> >
> > Agreed that the static cast is a bit weird to be doing in render_widget.
> If
> it
> > was the same piece of code creating the objects and also receiving and
> casting
> > them, I would feel a bit different, but I feel like any "abstraction
> savings"
> by
> > keeping the type opaque in cc are totally lost by it being transparent
> and
> > assumed in RenderWidget, which shouldn't know about it either. It feels
> like
> > something I'd see pre-repository merge.
> >
> > This discussion also does not feel orthogonal to me. This patch is
> plumbing
> and
> > these approaches are replacement plumbing for the exact same thing. It'd
> be
> > good to just pick the right approach.
> >
> > I think the more general callback solution seems best to me, for all the
> reasons
> > suggested above (type safety, abstraction, hiding implementation details
> from
> > cc).
>
> piman@, enne@:
> Thanks for feedback. So I will switch to use a promise/callback solution
> instead
> of
> the current empty LayerTreeMutations.
>
> While looking at the SwapPromise usage, I noticed that there is an
> existing path
> where these promises go the reverse direction (cc -> main) during the
> commit[1]
> and they get notified OnCommit. At the moment, this is used for some scroll
> latency measurements.
>
> I can reuse this path and just add a new virtual method
> |DidBeginMainFrame| to
> SwapPromise which get invoked when we begin the main frame. That means we
> don't need to introduce a whole new promise type to the code.
>
> What do you think about that approach?
>

That seems very reasonable to me - the scroll deltas are very much like a
form of tree mutation, so it makes sense to me to do them using the same
mechanism. We just need to make it clear that SwapPromises that are
enqueued from e.g. UpdateLayerTreeHost won't get the DidBeginMainFrame()
callback, but I think that's ok.

Antoine


>
> [1] LayerTreeHostImpl::QueueSwapPromiseForMainThreadScrollUpdate
>
> https://codereview.chromium.org/1602343002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

piman

On Thu, Feb 25, 2016 at 2:55 PM, <majidvp@chromium.org> wrote: > On 2016/02/24 22:42:02, enne ...

4 years, 10 months ago (2016-02-25 23:13:55 UTC) #40

On Thu, Feb 25, 2016 at 2:55 PM, <majidvp@chromium.org> wrote:

> On 2016/02/24 22:42:02, enne wrote:
> > > > > In a way, it's the pendant of SwapPromise (just works the other way
> > around).
> > > > So
> > > > > maybe we could introduce a BeginMainFramePromise or something like
> that,
> > with
> > > > a
> > > > > virtual DidBeginMainFrame() - or maybe even more simply a
> > > > BeginMainFrameCallback
> > > > > that could be stashed on the impl thread, and executed on the main
> thread?
> > > > This
> > > > > would give us back the type safety, because the callback could
> encode
> the
> > > > > concrete type in the curried data (or the concrete implementation
> of
> > > > > BeginMainFramePromise would contain the concrete data). It could
> also be
> > > > reused
> > > > > for other purpose (e.g. scroll/viewport deltas, maybe input events,
> etc.).
> > > > > WDYT?
> > > >
> > > > I think this can be done. If I understand it correctly, you are
> suggesting
> > that
> > > > instead of creating individual methods to plumb back values (e.g.,
> > > > layer tree mutations or scroll deltas) during the beginMainFrame we
> can
> just
> >
> > > > inject promises (similar to swap promises but in reverse) that do it
> instead
> > > > once called back.
> > > >
> > > > In this case, as you mentioned, this gives us back the type safety
> benefit,
> > and
> > > > in general the design becomes more symmetrical and extensible and we
> can
> > > > remove existing methods.
> > > >
> > > > I don't have any objection to this but it is orthogonal to this
> patch and
> > > > better done as a separate CL.
> > >
> > > +enne/danakj for opinion about cc.
> > >
> > > I don't really see this as orthogonal, since we're talking about
> adding a
> > bunch of methods to cc to marshall this opaque data, that would disappear
> > if/when we take the other approach. I'd prefer we take the approach that
> we're
> > committing to keep long term. If this is it, so be it, but I find the
> empty
> > LayerTreeMutations a bit of a code smell - it's really no better than a
> void*.
> >
> > Agreed that the static cast is a bit weird to be doing in render_widget.
> If
> it
> > was the same piece of code creating the objects and also receiving and
> casting
> > them, I would feel a bit different, but I feel like any "abstraction
> savings"
> by
> > keeping the type opaque in cc are totally lost by it being transparent
> and
> > assumed in RenderWidget, which shouldn't know about it either. It feels
> like
> > something I'd see pre-repository merge.
> >
> > This discussion also does not feel orthogonal to me. This patch is
> plumbing
> and
> > these approaches are replacement plumbing for the exact same thing. It'd
> be
> > good to just pick the right approach.
> >
> > I think the more general callback solution seems best to me, for all the
> reasons
> > suggested above (type safety, abstraction, hiding implementation details
> from
> > cc).
>
> piman@, enne@:
> Thanks for feedback. So I will switch to use a promise/callback solution
> instead
> of
> the current empty LayerTreeMutations.
>
> While looking at the SwapPromise usage, I noticed that there is an
> existing path
> where these promises go the reverse direction (cc -> main) during the
> commit[1]
> and they get notified OnCommit. At the moment, this is used for some scroll
> latency measurements.
>
> I can reuse this path and just add a new virtual method
> |DidBeginMainFrame| to
> SwapPromise which get invoked when we begin the main frame. That means we
> don't need to introduce a whole new promise type to the code.
>
> What do you think about that approach?
>

That seems very reasonable to me - the scroll deltas are very much like a
form of tree mutation, so it makes sense to me to do them using the same
mechanism. We just need to make it clear that SwapPromises that are
enqueued from e.g. UpdateLayerTreeHost won't get the DidBeginMainFrame()
callback, but I think that's ok.

Antoine


>
> [1] LayerTreeHostImpl::QueueSwapPromiseForMainThreadScrollUpdate
>
> https://codereview.chromium.org/1602343002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

enne (OOO)

I think the swap promise system is a little bit too specific for the kind ...

4 years, 10 months ago (2016-02-25 23:16:01 UTC) #41

piman

On Thu, Feb 25, 2016 at 3:16 PM, <enne@chromium.org> wrote: > I think the swap ...

4 years, 10 months ago (2016-02-25 23:21:48 UTC) #42

piman

On Thu, Feb 25, 2016 at 3:16 PM, <enne@chromium.org> wrote: > I think the swap ...

4 years, 10 months ago (2016-02-25 23:21:48 UTC) #43

esprehn

Indeed I'd suggest something closer to set_did_scroll_callback, also note that long term blink will hopefully ...

4 years, 10 months ago (2016-02-26 10:06:17 UTC) #44

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 9 months ago (2016-03-01 15:01:13 UTC) #45

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 9 months ago (2016-03-01 15:36:55 UTC) #46

majidvp

On 2016/02/26 10:06:17, esprehn wrote: > Indeed I'd suggest something closer to set_did_scroll_callback, also note ...

4 years, 9 months ago (2016-03-01 15:46:03 UTC) #47

enne (OOO)

https://codereview.chromium.org/1602343002/diff/300001/cc/animation/layer_tree_mutations_callback.h File cc/animation/layer_tree_mutations_callback.h (right): https://codereview.chromium.org/1602343002/diff/300001/cc/animation/layer_tree_mutations_callback.h#newcode13 cc/animation/layer_tree_mutations_callback.h:13: class LayerTreeMutationsCallback { What does this indirection get you ...

4 years, 9 months ago (2016-03-02 19:55:54 UTC) #48

majidvp

https://codereview.chromium.org/1602343002/diff/300001/cc/animation/layer_tree_mutations_callback.h File cc/animation/layer_tree_mutations_callback.h (right): https://codereview.chromium.org/1602343002/diff/300001/cc/animation/layer_tree_mutations_callback.h#newcode13 cc/animation/layer_tree_mutations_callback.h:13: class LayerTreeMutationsCallback { On 2016/03/02 19:55:54, enne wrote: > ...

4 years, 9 months ago (2016-03-02 21:15:10 UTC) #49

esprehn

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

4 years, 9 months ago (2016-03-02 21:21:39 UTC) #50

esprehn

kinuko@ can we use base::Bind in platform? I know it's scary with blink types, but ...

4 years, 9 months ago (2016-03-02 21:21:40 UTC) #51

enne (OOO)

I'm pretty sure this is allowed. esprehn can confirm.

4 years, 9 months ago (2016-03-02 21:22:37 UTC) #52

majidvp

esprehn wrote: > > kinuko@ can we use base::Bind in platform? I know it's scary ...

4 years, 9 months ago (2016-03-07 14:45:12 UTC) #53

kinuko

On 2016/03/07 14:45:12, majidvp wrote: > esprehn wrote: > > > kinuko@ can we use ...

4 years, 9 months ago (2016-03-10 01:57:53 UTC) #54

majidvp

On 2016/03/10 01:57:53, kinuko wrote: > On 2016/03/07 14:45:12, majidvp wrote: > > esprehn wrote: ...

4 years, 9 months ago (2016-03-17 15:48:33 UTC) #55

enne (OOO)

On 2016/03/22 at 13:33:55, majidvp wrote: > enne@: ping Ohmygosh, I'm sorry. I thought I ...

4 years, 9 months ago (2016-03-22 17:26:51 UTC) #57

majidvp

On 2016/03/22 17:26:51, enne wrote: > On 2016/03/22 at 13:33:55, majidvp wrote: > > enne@: ...

4 years, 9 months ago (2016-03-22 17:58:33 UTC) #58

esprehn

Nothing uses the platform/ interface? I don't see any callers. Can we do this in ...

4 years, 9 months ago (2016-03-22 21:16:50 UTC) #59

majidvp

On 2016/03/22 21:16:50, esprehn wrote: > Nothing uses the platform/ interface? I don't see any ...

4 years, 9 months ago (2016-03-22 22:14:40 UTC) #60

majidvp

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 9 months ago (2016-03-23 22:24:27 UTC) #61

Description was changed from

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutationsCallback: callback that is invoked on main thread at begin
  main frame to trigger application of mutations. A layer tree mutator 
  encapsulates its updates in such callbacks.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element 
  and can update it given a mutation.

blink->cc plumbing: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutatorClient: Maintains current set mutations and 
provide a callback that is invoked on main thread at begin
  main frame to trigger application of mutations.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element 
  and can update it given a mutation.

The CompositorMutator and blink->cc plumbing of compositor proxies is in a
followup patch: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

majidvp

esprehn@: PTAL https://codereview.chromium.org/1602343002/diff/340001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1602343002/diff/340001/cc/trees/layer_tree_host_impl.cc#newcode3261 cc/trees/layer_tree_host_impl.cc:3261: auto callbacks = make_scoped_ptr(new BeginFrameCallbackList()); On 2016/03/22 ...

4 years, 9 months ago (2016-03-23 22:24:53 UTC) #62

esprehn

This patch doesn't seem to create a CustomCompositorAnimationManager (or did I miss it?) your linked ...

4 years, 8 months ago (2016-03-31 05:03:10 UTC) #64

esprehn

This patch doesn't seem to create a CustomCompositorAnimationManager (or did I miss it?) your linked ...

4 years, 8 months ago (2016-03-31 05:03:11 UTC) #65

majidvp

> This patch doesn't seem to create a CustomCompositorAnimationManager (or did I miss it?) your ...

4 years, 8 months ago (2016-03-31 19:43:03 UTC) #66

> This patch doesn't seem to create a CustomCompositorAnimationManager (or did I
miss it?) your  linked patch doesn't either. What keeps it alive?

That should be in the follow up patch but that has not been updated with latest
changes yet.
I provide more details below.

>This patch and the linked one are HUGEEEE btw. That's going to make all your
code reviews very slow.

I will chat with flackr@ to see if we can break down the follow up patch in
smaller bits to help with the review.

https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (right):

https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:188: for (auto& callback :
*begin_main_frame_state->begin_frame_callbacks)
On 2016/03/31 05:03:11, esprehn wrote:
> Why here? Why is this before 
> 
> ApplyScrollAndScale, WillBeginMainFrame, BeginMainFrame and AnimateLayers?
> 
> I'm not a cc owner, but if I was designing this I think I'd assume there was a
> callback, and the consumer of cc should do the queuing and ordering of the
> callbacks. This system doesn't give the mutator any control over what's going
on
> here.

We want the mutations to get applied before beginning the main frame so the
new values are available in main frame before scripted animations are
serviced. It could be placed after ApplyScrollAndScale but before
BeginMainFrame.

The original design idea from piman@ was that eventually ApplyScrollAndScale
will become one of these callbacks itself and as you are suggesting proxy main
will not care about the ordering.

https://codereview.chromium.org/1602343002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
(right):

https://codereview.chromium.org/1602343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:33:
return base::Bind(&CompositorMutationsTarget::applyMutations,
On 2016/03/31 05:03:11, esprehn wrote:
> What keeps m_mutationsTarget alive for this callback?

It is kept alive by CompositorMutatorImpl (also owns CompositorMutatorClient)
which has the same lifetime as WebViewImpl. 
Here is the ownership chain: WebViewImpl <>-- CompositorMutatorImpl<>--
CompositorMutationTarget.

|CompositorMutatorImpl| will be in the follow-up patch, but that patch has not
been updated with most recent changes which
includes the introduction of the |CompositorMutationTarget|.

danakj

https://codereview.chromium.org/1602343002/diff/400001/cc/animation/layer_tree_mutator.h File cc/animation/layer_tree_mutator.h (right): https://codereview.chromium.org/1602343002/diff/400001/cc/animation/layer_tree_mutator.h#newcode18 cc/animation/layer_tree_mutator.h:18: using ApplyMutationsCallback = base::Closure; On 2016/03/31 05:03:11, esprehn wrote: ...

4 years, 8 months ago (2016-04-05 17:41:11 UTC) #67

danakj

https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc File cc/trees/proxy_main.cc (right): https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc#newcode188 cc/trees/proxy_main.cc:188: for (auto& callback : *begin_main_frame_state->begin_frame_callbacks) On 2016/03/31 19:43:03, majidvp ...

4 years, 8 months ago (2016-04-05 17:45:21 UTC) #68

majidvp

cc/animation/layer_tree_mutator.h:18: using ApplyMutationsCallback = base::Closure; On 2016/03/31 05:03:11, esprehn wrote: > > why add a ...

4 years, 8 months ago (2016-04-06 23:48:04 UTC) #69

cc/animation/layer_tree_mutator.h:18: using ApplyMutationsCallback =
base::Closure;
On 2016/03/31 05:03:11, esprehn wrote:
> > why add a typedef for this? Seems more clear to just use base::Closure to
me. I
> > guess I defer to Dana. :)

> Ya, just use a Closure IMO.

Done.



On 2016/04/05 at 17:45:21, danakj wrote:
> https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc
> File cc/trees/proxy_main.cc (right):
> 
>
https://codereview.chromium.org/1602343002/diff/400001/cc/trees/proxy_main.cc...
> cc/trees/proxy_main.cc:188: for (auto& callback :
*begin_main_frame_state->begin_frame_callbacks)
> On 2016/03/31 19:43:03, majidvp wrote:
> > On 2016/03/31 05:03:11, esprehn wrote:
> > > Why here? Why is this before 
> > > 
> > > ApplyScrollAndScale, WillBeginMainFrame, BeginMainFrame and AnimateLayers?
> > > 
> > > I'm not a cc owner, but if I was designing this I think I'd assume there
was a
> > > callback, and the consumer of cc should do the queuing and ordering of the
> > > callbacks. This system doesn't give the mutator any control over what's
going
> > on
> > > here.
> > 
> > We want the mutations to get applied before beginning the main frame so the
> > new values are available in main frame before scripted animations are
> > serviced. It could be placed after ApplyScrollAndScale but before
> > BeginMainFrame.
> > 
> > The original design idea from piman@ was that eventually ApplyScrollAndScale
> > will become one of these callbacks itself and as you are suggesting proxy
main
> > will not care about the ordering.
> > 
> > 
> 
> After ApplyScrollAndScale may make sense for now, then whatever callbacks get
registered can use what they do. Later, you'd just have to register callbacks in
the right order if you had dependencies.
> 
> But if it's meant to do things that will influence the main frame then before
BeginMainFrame makes sense. I don't see anything about ordering wrt
WillBeginMainFrame mattering right now.

Done.

majidvp

@esprehn: Did you get a chance to see my responses in https://codereview.chromium.org/1602343002/#msg66 ? Note the ...

4 years, 8 months ago (2016-04-11 17:09:41 UTC) #70

majidvp

On 2016/04/11 17:37:12, esprehn wrote: > lgtm \o/. Thanks to all reviewers for your feedback ...

4 years, 8 months ago (2016-04-11 17:58:18 UTC) #72

majidvp

The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, enne@chromium.org Link to ...

4 years, 8 months ago (2016-04-11 17:58:33 UTC) #74

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 17:59:02 UTC) #75

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:02:33 UTC) #76

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/185317) ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, ...

4 years, 8 months ago (2016-04-11 18:02:37 UTC) #77

majidvp

The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, esprehn@chromium.org, enne@chromium.org Link ...

4 years, 8 months ago (2016-04-11 18:37:40 UTC) #79

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:38:19 UTC) #81

commit-bot: I haz the power

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 8 months ago (2016-04-11 20:36:14 UTC) #82

Message was sent while issue was closed.

Description was changed from

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutatorClient: Maintains current set mutations and 
provide a callback that is invoked on main thread at begin
  main frame to trigger application of mutations.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element 
  and can update it given a mutation.

The CompositorMutator and blink->cc plumbing of compositor proxies is in a
followup patch: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutatorClient: Maintains current set mutations and 
provide a callback that is invoked on main thread at begin
  main frame to trigger application of mutations.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element 
  and can update it given a mutation.

The CompositorMutator and blink->cc plumbing of compositor proxies is in a
followup patch: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

commit-bot: I haz the power

Description was changed from ========== compositor-worker: cc->blink mutation plumbing Add plubming to CC and blink ...

4 years, 8 months ago (2016-04-11 20:38:19 UTC) #84

Message was sent while issue was closed.

Description was changed from

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutatorClient: Maintains current set mutations and 
provide a callback that is invoked on main thread at begin
  main frame to trigger application of mutations.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element 
  and can update it given a mutation.

The CompositorMutator and blink->cc plumbing of compositor proxies is in a
followup patch: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: cc->blink mutation plumbing

Add plubming to CC and blink to take a set of compositor mutation from LTHI
and apply them to associated elements in blink.

Compositor mutations are applied to the DOM element using a custom key frame
animation.

The following components are added:

- CompositorMutatorClient: Maintains current set mutations and
provide a callback that is invoked on main thread at begin
  main frame to trigger application of mutations.
- CompositorMutationsTarget: Interface of the components that applies the
  mutations.
- CustomCompositorAnimationManager: Takes a set of mutations and updates their
  respective elements.
- CustomCompositorAnimations: Owns custom keyframe animation for the element
  and can update it given a mutation.

The CompositorMutator and blink->cc plumbing of compositor proxies is in a
followup patch: https://codereview.chromium.org/1625473002/

BUG=430155
TESTS=web/tests/CompositorWorkerTest.cpp
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/96452d073371da1e1ab7eb1ef724df0926f76547
Cr-Commit-Position: refs/heads/master@{#386464}
==========

commit-bot: I haz the power

4 years, 8 months ago (2016-04-11 20:38:22 UTC) #85

Message was sent while issue was closed.

Patchset 25 (id:??) landed as
https://crrev.com/96452d073371da1e1ab7eb1ef724df0926f76547
Cr-Commit-Position: refs/heads/master@{#386464}

Issue 1602343002: compositor-worker: cc->blink mutation plumbing (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix DEPS issue and some missing methods #

Patch Set 3 : Fix test and rebase #

Patch Set 4 : #

Patch Set 5 : rebase #

Patch Set 6 : Fix blimp issue #

Patch Set 7 : Address Vollick comments #

Patch Set 8 : Address review feedback from jbroman@ #

Patch Set 9 : rebase #

Patch Set 10 : Use animation machinery instead of updating inline style #

Patch Set 11 : Rebase & address esprehn feedback #

Patch Set 12 : Add missing override #

Patch Set 13 : rebase #

Patch Set 14 : Use callbacks instead of plumbing opaque LayerTreeMutations #

Patch Set 15 : Cleanup #

Patch Set 16 : clean up #

Patch Set 17 : Remove LayerTreeMutationsCallback and use base::Callback directly #

Patch Set 18 : #

Patch Set 19 : Add CompositorMutatorClient and address feedback #

Patch Set 20 : rebase #

Patch Set 21 : Fix export #

Patch Set 22 : #

Patch Set 23 : merge master #

Patch Set 24 : Export LayerTreeMutator #

Patch Set 25 : Merge master + s/scoped_ptr/unique_ptr/ #

Messages