Moving compositor_bindings from webkit to content

Moving compositor_bindings from webkit to content

- All files from webkit/renderer/compositor_bindings have been moved to content/renderer/compositor_bindings
- Webkit_compositor_support is moved in content_renderer
- Webkit_compositor_bindings_unittests is moved in content_unittests
- A dummy target has been left for webkit_compositor_bindings_unittests so as to not break the build.

TBR=nduca

BUG=265753
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277988

[amogh.bihani, 2014-06-05 13:11:52]

PTAL.

[jam, 2014-06-05 15:24:07]

https://codereview.chromium.org/317163002/diff/1/content/public/common/assert...
File content/public/common/assert_matching_enums.cc (right):

https://codereview.chromium.org/317163002/diff/1/content/public/common/assert...
content/public/common/assert_matching_enums.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
hmm just saw this file, it shouldn't have been in content/public/common. can you
move it to content/common in this cl please?

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
File content/renderer/compositor_bindings/compositor_bindings.gyp (right):

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
content/renderer/compositor_bindings/compositor_bindings.gyp:1: # Copyright (c)
2012 The Chromium Authors. All rights reserved.
this should be a part of content_renderer.gypi

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
File content/renderer/compositor_bindings/compositor_bindings_tests.gyp (right):

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
content/renderer/compositor_bindings/compositor_bindings_tests.gyp:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
ditto for content_tests.gypi

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
File content/renderer/compositor_bindings/content_compositor_bindings_export.h
(right):

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
content/renderer/compositor_bindings/content_compositor_bindings_export.h:2: //
Use of this source code is governed by a BSD-style license that can be
we shouldn't need this file and instead use content_export.h

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
File content/renderer/compositor_bindings/test/run_all_unittests.cc (right):

https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...
content/renderer/compositor_bindings/test/run_all_unittests.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
no need to have a separate unit test target; all unit tests in content run
inside content_unittests

https://codereview.chromium.org/317163002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/DEPS (right):

https://codereview.chromium.org/317163002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/DEPS:2:
"+content/renderer/compositor_bindings",
this is not allowed, content/shell can only include stuff from content/public

it appears that all you need is that TestPlugin needs to instantiate a
blink::WebLayer. So you can add a method to public/test/layouttest_support.h to
return that

[tfarina, 2014-06-06 03:59:41]

I was planning to do this myself someday, but I never get around and always get
busy with other stuff :/

Adrienne should review this in my place.

Moving myself to CC list since you have John, and now Adrienne.

[tfarina, 2014-06-06 04:15:16]

Btw, I think one of the reasons I never get around is that it has an independent
test suite called webkit_compositor_bindings_unittests which we run on the bots.

For example, here,
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.8%20x64...

I didn't look at your changes, but did you consider this? Do you have a plan for
updating the buildbot code during the transition? Like, leaving a dummy target
in Chromium, then removing references to it from buildbot code and finally
removing the dummy target from Chromium.

[amogh.bihani, 2014-06-06 05:03:04]

On 2014/06/06 04:15:16, tfarina wrote:
> Btw, I think one of the reasons I never get around is that it has an
independent
> test suite called webkit_compositor_bindings_unittests which we run on the
bots.

Now, they will run under content_unittests. jam@ told me that we do not need to
have a separate test target for it.
https://codereview.chromium.org/317163002/diff/1/content/renderer/compositor_...

>I didn't look at your changes, but did you consider this? Do you have a plan
for
>updating the buildbot code during the transition? Like, leaving a dummy target
>in Chromium, then removing references to it from buildbot code and finally
>removing the dummy target from Chromium.

Oh! I didn't know about this part. Thanks for the information. I shall do the
change :)

[amogh.bihani, 2014-06-06 13:43:38]

Thanks for the review :)

I have made most of the changes.
Moved the sources from compositor_bindings.gyp to content_renderer.gypi and
compositor_bindings_tests.gypi to content_tests.gypi in PS#2
Moved assert_matching_enums from content/public/common to content/common in PS#3
One change is left, I will make that change in the next PS.

https://codereview.chromium.org/317163002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/DEPS (right):

https://codereview.chromium.org/317163002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/DEPS:1: include_rules = [
I will remove this in the next Patchset

[enne (OOO), 2014-06-06 17:25:13]

https://codereview.chromium.org/317163002/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/317163002/diff/40001/content/content_tests.gy...
content/content_tests.gypi:355: #'content.gyp:content_renderer',
?

[amogh.bihani, 2014-06-06 18:07:33]

https://codereview.chromium.org/317163002/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/317163002/diff/40001/content/content_tests.gy...
content/content_tests.gypi:355: #'content.gyp:content_renderer',
On 2014/06/06 17:25:13, enne wrote:
> ?
Oops!
Earlier it was webkit_compositor_bindings in webkit folder, I renamed it while
moving that gyp file here... commented it as I thought it is not required in
dependency, built it on the local system... but forgot to remove it before
uploading. Sorry.

[amogh.bihani, 2014-06-09 11:29:00]

As suggested by tfarina@, I have added a dummy
webkit_compositor_bindings_unittest in PS#4 so as to keep the waterfall green.
I will modify the buildbots after this merges and then remove the dummy test
suit.

I have instantiated WebLayerImpl in layouttest_support in PS#5 and removed the
incorect DEPS that I had added in PS#1.

PTAL.

[enne (OOO), 2014-06-09 17:08:04]

lgtm

[tfarina, 2014-06-09 18:40:19]

Rietveld has a bug somethere, because it is showing D for
compositor_bindings_tests.gyp, while it is not being deleted in patch set 5.

Do you have access to run trybots?

lgtm with green trybots.

https://codereview.chromium.org/317163002/diff/80001/webkit/renderer/composit...
File webkit/renderer/compositor_bindings/compositor_bindings_tests.gyp (right):

https://codereview.chromium.org/317163002/diff/80001/webkit/renderer/composit...
webkit/renderer/compositor_bindings/compositor_bindings_tests.gyp:6: #
webkit_compositor_bindings_unittests to content_unittests.
Thanks for leaving a dummy target.

[jam, 2014-06-11 00:27:04]

sorry i missed your cl until now.

for a change this big, it would also be helpful to see green try runs when
reviewing.

Thanks for doing this cleanup

https://codereview.chromium.org/317163002/diff/80001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/317163002/diff/80001/content/common/DEPS#newc...
content/common/DEPS:11: "+third_party/WebKit/public/platform/WebAnimation.h",
the contents of this header don't confirm to the big warning two lines above.

i think content/common/assert_matching_enums.cc should be moved to content/child
and that'll solve this problem

https://codereview.chromium.org/317163002/diff/80001/content/public/test/layo...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/317163002/diff/80001/content/public/test/layo...
content/public/test/layouttest_support.h:13: #include
"third_party/WebKit/public/platform/WebLayer.h"
don't add headers if you can forward declare

https://codereview.chromium.org/317163002/diff/80001/content/public/test/layo...
content/public/test/layouttest_support.h:115: blink::WebLayer*
InstantiateWebLayer(scoped_refptr<cc::TextureLayer> layer);
nit: add comment

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_external_texture_layer_impl.cc
(right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_external_texture_layer_impl.cc:54:
->SetPremultipliedAlpha(premultiplied_alpha);
the previous formatting is fine, we normally don't begin lines with "->". there
was/is a bug in the clang format plugin (see chromium-dev thread)

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_filter_animation_curve_impl.cc
(right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_filter_animation_curve_impl.cc:32:
static_cast<const content::WebFilterOperationsImpl&>(keyframe.value())
nit: here and below, no "content::" inside content namespace

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_float_animation_curve_impl.cc
(right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_float_animation_curve_impl.cc:24: const
{
don't start a line with const, previous formatting was good. this another bug in
clang format

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_image_layer_impl.cc:32:
->SetFixedBounds(gfx::Size(bitmap.width(), bitmap.height()));
ditto

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_layer_impl.h (right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_layer_impl.h:138: OVERRIDE;
ditto

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
File content/renderer/compositor_bindings/web_transform_animation_curve_impl.cc
(right):

https://codereview.chromium.org/317163002/diff/80001/content/renderer/composi...
content/renderer/compositor_bindings/web_transform_animation_curve_impl.cc:36:
static_cast<const content::WebTransformOperationsImpl&>(keyframe.value())
nit: here and below, no "content::" inside content namespace

[amogh.bihani, 2014-06-11 15:04:21]

Thanks for the review. :)
I have addressed the comments in PS#7.
PS#8 is to resolve the lint errors in all the moved files - "No new line
character at the end of file".
PS#9 is undoes PS#8 because I thought I have done something wrong, PS#10 again
removes that lint error.

PTAL.

[jam, 2014-06-12 15:54:57]

lgtm

https://codereview.chromium.org/317163002/diff/120001/content/public/test/lay...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/317163002/diff/120001/content/public/test/lay...
content/public/test/layouttest_support.h:115: // Instantiates WebLayerImpl for
TestPluggin.
nit: spelling

[amogh.bihani, 2014-06-13 15:20:06]

Oops! Had been testing only with 'rel' bots. All debug bots are going red :(
Getting linking error - undefined reference to "WebCompositorSupportImpl" while
building test_support_content.
web_compositor_support_impl has moved in content_renderer target. It is added in
the dependencies, still I am getting that error. I must be missing something :(

[jam, 2014-06-16 03:23:25]

On 2014/06/13 15:20:06, amogh.bihani wrote:
> Oops! Had been testing only with 'rel' bots. All debug bots are going red :(
> Getting linking error - undefined reference to "WebCompositorSupportImpl"
while
> building test_support_content.
> web_compositor_support_impl has moved in content_renderer target. It is added
in
> the dependencies, still I am getting that error. I must be missing something
:(

Try adding a CONTENT_EXPORT to class WebCompositorSupportImpl.

[amogh.bihani, 2014-06-16 12:36:38]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-16 12:37:44]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/280001

[commit-bot: I haz the power, 2014-06-16 14:59:59]

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

[commit-bot: I haz the power, 2014-06-16 15:00:01]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[amogh.bihani, 2014-06-17 03:07:46]

The CQ bit was checked by amogh.bihani@samsung.com

[amogh.bihani, 2014-06-17 03:08:59]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-06-17 03:20:19]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-17 03:22:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/300001

[commit-bot: I haz the power, 2014-06-17 10:45:15]

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

[commit-bot: I haz the power, 2014-06-17 10:45:16]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[amogh.bihani, 2014-06-17 12:26:29]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-17 12:28:15]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/310001

[amogh.bihani, 2014-06-17 12:29:46]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-06-17 13:45:35]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-17 13:46:04]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/330001

[amogh.bihani, 2014-06-17 13:46:43]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-06-17 13:52:23]

android_dbg_triggered_tests were failing for webkit_compositor_support_unittests
because it was running no tests. This target is left as a dummy to keep
waterfall green. To fix the tests I added a dummy unittest as well. I have also
moved some dependencies in conditiond OS != ios as ios bots were failing.
I have intentionally checked and un-checked the commit bit so as to run the
relevant bots.

Please have a look as compositor_bindings_dummy_unittests.cc

Also for some reasons presubmit is failing stating OWNER's LGTM missing for
webkit/renderer/compositor_bindings although enne@ has already approved it. Is
it a bug or I have to get a review from nduca@ as well?

[jam, 2014-06-18 00:38:13]

On 2014/06/17 13:52:23, amogh.bihani wrote:
> android_dbg_triggered_tests were failing for
webkit_compositor_support_unittests
> because it was running no tests. This target is left as a dummy to keep
> waterfall green. To fix the tests I added a dummy unittest as well. I have
also
> moved some dependencies in conditiond OS != ios as ios bots were failing.
> I have intentionally checked and un-checked the commit bit so as to run the
> relevant bots.
> 
> Please have a look as compositor_bindings_dummy_unittests.cc

I don't see the file. 
> 
> Also for some reasons presubmit is failing stating OWNER's LGTM missing for
> webkit/renderer/compositor_bindings although enne@ has already approved it. Is
> it a bug or I have to get a review from nduca@ as well?

[amogh.bihani, 2014-06-18 03:28:06]

> I don't see the file. 

webkit/renderer/compositor_bindings/compositor_bindings_dummy_unittest.cc

It is in between the deleted files. (Else do a ctrl + F on the page)

[jam, 2014-06-18 04:32:15]

On 2014/06/18 03:28:06, amogh.bihani wrote:
> > I don't see the file. 
> 
> webkit/renderer/compositor_bindings/compositor_bindings_dummy_unittest.cc
> 
> It is in between the deleted files. (Else do a ctrl + F on the page)

ah I didn't see that earlier, no idea why. still lgtm

[jam, 2014-06-18 04:32:17]

On 2014/06/18 03:28:06, amogh.bihani wrote:
> > I don't see the file. 
> 
> webkit/renderer/compositor_bindings/compositor_bindings_dummy_unittest.cc
> 
> It is in between the deleted files. (Else do a ctrl + F on the page)

ah I didn't see that earlier, no idea why. still lgtm

[amogh.bihani, 2014-06-18 04:33:25]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-18 04:35:21]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/330001

[commit-bot: I haz the power, 2014-06-18 07:15:48]

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

[commit-bot: I haz the power, 2014-06-18 07:15:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[amogh.bihani, 2014-06-18 08:24:26]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-18 08:26:47]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/317163002/330001

[commit-bot: I haz the power, 2014-06-18 08:35:47]

Change committed as 277988