Add support for calculating the position of the top controls in the cc layer.

Add support for calculating the position of the top controls in the cc layer.

Provides a means for keeping the top controls in sync with the currently
renderered frame, which will allow us to move the top controls around the
screen.

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

[Ted C, 2012-12-12 05:03:16]

@jamesr - Very much in progress, but I wanted to get your thoughts on the
general approach of hooking up the top_controls_manager to the
layer_tree_host_impl.  The debug_layer stuff will all go away once I can
actually get the values to the browser and I'm not 100% set on the logic in
TopControlsManager::updateDrawPositions.

[jamesr, 2012-12-16 03:01:10]

One thing I realized belatedly is that our fixpos logic currently doesn't have
any way to maintain constraints, it only knows how to preserve a fixed offset
between layers when scrolling.  For you, this means we don't have any way to
keep things stuck to the bottom when changing the viewport size - the only
options are to push everything down and clip or to clip the top.  Hopefully
that's OK :)

Am I correct in assuming that m_debugLayer is supposed to be the top bar
controls layer?

Please write new files in chromium style (80col, 2-space indents, etc) and use
base::TimeTicks / base::TimeDelta for animation timing logic.

[Ted C, 2012-12-17 18:16:42]

On 2012/12/16 03:01:10, jamesr wrote:
> One thing I realized belatedly is that our fixpos logic currently doesn't have
> any way to maintain constraints, it only knows how to preserve a fixed offset
> between layers when scrolling.  For you, this means we don't have any way to
> keep things stuck to the bottom when changing the viewport size - the only
> options are to push everything down and clip or to clip the top.  Hopefully
> that's OK :)

Yeah, we came to that conclusion last week as well.  For phase 1, I think it's
fine if they lag behind.

> 
> Am I correct in assuming that m_debugLayer is supposed to be the top bar
> controls layer?

Correct.  But I removed it in this version as I was able to pipe the logic all
the way back to the browser process w/ Alex's patch.

> 
> Please write new files in chromium style (80col, 2-space indents, etc)

Done...I think.

 and use
> base::TimeTicks / base::TimeDelta for animation timing logic.

This one I'm a bit confused about.  I do use TimeTicks in the manager, but at
the animation time I convert to a double.  Should I just push the time down into
the animation and handle the conversions there?  This was somewhat following the
page scale animation example, so I was attempting to keep them somewhat
consistent.  Just want to clarify what I should change before I do something
wrong :-)

[Ted C, 2012-12-18 19:57:44]

On 2012/12/17 18:16:42, Ted C wrote:
> On 2012/12/16 03:01:10, jamesr wrote:
> > One thing I realized belatedly is that our fixpos logic currently doesn't
have
> > any way to maintain constraints, it only knows how to preserve a fixed
offset
> > between layers when scrolling.  For you, this means we don't have any way to
> > keep things stuck to the bottom when changing the viewport size - the only
> > options are to push everything down and clip or to clip the top.  Hopefully
> > that's OK :)
> 
> Yeah, we came to that conclusion last week as well.  For phase 1, I think it's
> fine if they lag behind.
> 
> > 
> > Am I correct in assuming that m_debugLayer is supposed to be the top bar
> > controls layer?
> 
> Correct.  But I removed it in this version as I was able to pipe the logic all
> the way back to the browser process w/ Alex's patch.
> 
> > 
> > Please write new files in chromium style (80col, 2-space indents, etc)
> 
> Done...I think.
> 
>  and use
> > base::TimeTicks / base::TimeDelta for animation timing logic.
> 
> This one I'm a bit confused about.  I do use TimeTicks in the manager, but at
> the animation time I convert to a double.  Should I just push the time down
into
> the animation and handle the conversions there?  This was somewhat following
the
> page scale animation example, so I was attempting to keep them somewhat
> consistent.  Just want to clarify what I should change before I do something
> wrong :-)

I went ahead and convert the whole animation to use TimeTicks and TimeDelta as I
suspect that was what you were looking for.

I think it should be ready for a proper review (missing all the plumbing, but I
think I'll do that in follow up CLs to avoid this getting too huge).

[Ted C, 2012-12-18 22:14:48]

Turned out to be really straight forward to add the plumbing through the
metadata, so I went ahead and did that.

Added Alex to review that portion.

[aelias_OOO_until_Jul13, 2012-12-19 08:31:18]

I went ahead and reviewed the whole patch.  Looks pretty well-structured in
general, I have mostly nits.  One general nit is to please use Chrome underscore
style for your arguments/local variables and starting caps for your methods in
the new files you're adding.  I realize the policy is confusing in cc/...

https://codereview.chromium.org/11552009/diff/22001/cc/compositor_frame_metad...
File cc/compositor_frame_metadata.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/compositor_frame_metad...
cc/compositor_frame_metadata.h:35: gfx::PointF location_bar_offset;
We've decided on the convention that offsets should be Vector2dF.

https://codereview.chromium.org/11552009/diff/22001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:238: m_topControlsManager =
TopControlsManager::create(this, settings.calculateTopControlsPosition,
settings.topControlsHeight);
I don't think this object should be created at all when
calculateTopControlsPosition is disabled.  Just leave it null and protect all
uses of it with if (m_topControlsManager).  Then you also can get rid of the
"enabled" boolean inside the class.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation.cc
File cc/top_controls_animation.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.cc:56: return scrollOffset <= -top_controls_height_;
It would be simpler to just compare max_duration_ to time here.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation.h
File cc/top_controls_animation.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.h:25: static scoped_ptr<TopControlsAnimation>
create(float startScrollOffset,
New compositor files should use underscore style, e.g. top_controls_height

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.h:48: const float start_scroll_offset_;
Please rename to start_y_offset_.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:38: previous_root_scroll_offset_(-1.f) {
Use 0 here, not -1.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:76: previous_root_scroll_offset_ = -1;
I don't think it's a good idea to have -1 as a magic invalid value.  Why not
just keep it in sync with the layerTreeImpl's value?

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:174: float scrollTotalY;
This may be uninitialized.  Just change it to:
float scrollTotalY = rootScrollLayerTotalScrollY();

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:186: controls_top_offset_ >= -(top_controls_height_ *
0.75f));
Promote 0.75f to a constant at the top of this file.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h
File cc/top_controls_manager.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h...
cc/top_controls_manager.h:33: static const int64 kAutoHideDelayMs = 1500;
Move these values to the .cc file.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h...
cc/top_controls_manager.h:38: float topControlsHeight);
top_controls_height

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager_u...
File cc/top_controls_manager_unittest.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager_u...
cc/top_controls_manager_unittest.cc:70: ASSERT_EQ(0.f, remainingScroll.y());
Use EXPECT_ instead of ASSERT_ all through these tests.  ASSERT causes fatal
errors that prevent the rest of the test from running.

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:863: #if
defined(OS_ANDROID)
I don't think we should have this block here.  Please move this code to
content/browser/android/content_startup_flags.cc

[Ted C, 2012-12-19 21:34:17]

A few follow up questions, but I think I addressed most/all of the issues.

https://codereview.chromium.org/11552009/diff/22001/cc/compositor_frame_metad...
File cc/compositor_frame_metadata.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/compositor_frame_metad...
cc/compositor_frame_metadata.h:35: gfx::PointF location_bar_offset;
On 2012/12/19 08:31:18, aelias wrote:
> We've decided on the convention that offsets should be Vector2dF.

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:238: m_topControlsManager =
TopControlsManager::create(this, settings.calculateTopControlsPosition,
settings.topControlsHeight);
On 2012/12/19 08:31:18, aelias wrote:
> I don't think this object should be created at all when
> calculateTopControlsPosition is disabled.  Just leave it null and protect all
> uses of it with if (m_topControlsManager).  Then you also can get rid of the
> "enabled" boolean inside the class.

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation.cc
File cc/top_controls_animation.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.cc:56: return scrollOffset <= -top_controls_height_;
On 2012/12/19 08:31:18, aelias wrote:
> It would be simpler to just compare max_duration_ to time here.

I'd prefer to keep it comparing against expected height as it will terminate
earlier.  The max_duration is often longer than the animation will run as the
max duration is how long the animation could take if it needed to height or show
the full height, which it is often not doing.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation.h
File cc/top_controls_animation.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.h:25: static scoped_ptr<TopControlsAnimation>
create(float startScrollOffset,
On 2012/12/19 08:31:18, aelias wrote:
> New compositor files should use underscore style, e.g. top_controls_height

Blast...I forgot to convert the parameters to chromium style when I was doing
the massive initial conversion.

Done...everywhere.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_animation...
cc/top_controls_animation.h:48: const float start_scroll_offset_;
On 2012/12/19 08:31:18, aelias wrote:
> Please rename to start_y_offset_.

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:38: previous_root_scroll_offset_(-1.f) {
On 2012/12/19 08:31:18, aelias wrote:
> Use 0 here, not -1.

I need some value to ensure it doesn't update while you are scrolling to prevent
jittery locations of the toolbar.  I updated this to use a separate boolean to
avoid muddling the logic between the two.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:174: float scrollTotalY;
On 2012/12/19 08:31:18, aelias wrote:
> This may be uninitialized.  Just change it to:
> float scrollTotalY = rootScrollLayerTotalScrollY();

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:186: controls_top_offset_ >= -(top_controls_height_ *
0.75f));
On 2012/12/19 08:31:18, aelias wrote:
> Promote 0.75f to a constant at the top of this file.

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h
File cc/top_controls_manager.h (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h...
cc/top_controls_manager.h:33: static const int64 kAutoHideDelayMs = 1500;
On 2012/12/19 08:31:18, aelias wrote:
> Move these values to the .cc file.

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager.h...
cc/top_controls_manager.h:38: float topControlsHeight);
On 2012/12/19 08:31:18, aelias wrote:
> top_controls_height

Done.

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager_u...
File cc/top_controls_manager_unittest.cc (right):

https://codereview.chromium.org/11552009/diff/22001/cc/top_controls_manager_u...
cc/top_controls_manager_unittest.cc:70: ASSERT_EQ(0.f, remainingScroll.y());
On 2012/12/19 08:31:18, aelias wrote:
> Use EXPECT_ instead of ASSERT_ all through these tests.  ASSERT causes fatal
> errors that prevent the rest of the test from running.

Done.

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:863: #if
defined(OS_ANDROID)
On 2012/12/19 08:31:18, aelias wrote:
> I don't think we should have this block here.  Please move this code to
> content/browser/android/content_startup_flags.cc

I only want to enable this in the renderer.  content_startup_flags doesn't seem
to distinguish between the browser and renderer command lines.

[jamesr, 2012-12-19 22:56:29]

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/layer_tree_host...
File cc/layer_tree_host_impl.h (right):

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/layer_tree_host...
cc/layer_tree_host_impl.h:210: virtual LayerTreeImpl* activeTree() OVERRIDE;
document what interface this is overriding, i.e. "// Blahblahblah
implementation."

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/layer_tree_sett...
File cc/layer_tree_settings.h (right):

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/layer_tree_sett...
cc/layer_tree_settings.h:36: size_t topControlsHeight;  // in pixels compensated
for density
in cc, integral pixel offsets are just 'int'

start comments with and uppercase letter and end with a period, or make the
value more apparent from the name. I've no idea what "compensated for density"
is supposed to mean.

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
File cc/top_controls_manager.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
cc/top_controls_manager.cc:22: static const int64 kAutoHideDelayMs = 1500;
I think the floating point constants will generate static initializers, which is
bad. check on that.

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
cc/top_controls_manager.cc:195: weak_ptr_factory_.GetWeakPtr(), trigger_time),
a WeakPtrFactory seems like overkill for this - check out base/timer.h. I think
you want a OneShotTimer. it'll take care of auto-cancellation on destruction

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
File cc/top_controls_manager.h (right):

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
cc/top_controls_manager.h:23: class TopControlsDelegate {
this should have a protected destructor since it wouldn't make any sense for
someone to delete this class via a pointer to one of these.

our naming convention is that this is a "...Client", not Delegate.

can you put this in its own header? then layer_tree_host_impl.h wouldn't have to
#include this file

https://chromiumcodereview.appspot.com/11552009/diff/19002/cc/top_controls_ma...
cc/top_controls_manager.h:64: 
I don't think this comment can be right - we never delete the delegate with this
pointer, nor would it make a lot of sense to do so

[aelias_OOO_until_Jul13, 2012-12-19 23:08:36]

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:863: #if
defined(OS_ANDROID)
You can set it explicitly to false in the browser compositor in our Compositor
class.  That's how ChromeOS deals with browser/renderer setting differences.

[jamesr1, 2012-12-19 23:10:00]

On Wed, Dec 19, 2012 at 3:08 PM, <aelias@chromium.org> wrote:

>
> https://codereview.chromium.**org/11552009/diff/22001/**
>
content/browser/renderer_host/**render_process_host_impl.cc<https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_host/render_process_host_impl.cc>
> File content/browser/renderer_host/**render_process_host_impl.cc (right):
>
> https://codereview.chromium.**org/11552009/diff/22001/**
>
content/browser/renderer_host/**render_process_host_impl.cc#**newcode863<https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_host/render_process_host_impl.cc#newcode863>
> content/browser/renderer_host/**render_process_host_impl.cc:**863: #if
> defined(OS_ANDROID)
> You can set it explicitly to false in the browser compositor in our
> Compositor class.  That's how ChromeOS deals with browser/renderer
> setting differences.
>

However you plumb the command line flags through please pass this into cc::
via LayerTreeSettings and not through command line flags. cc/ really
shouldn't be parsing command line flags unless they are truly universal for
a binary across all processes.

>
>
https://codereview.chromium.**org/11552009/<https://codereview.chromium.org/1...
>
> --
>
>
>

[Ted C, 2012-12-20 00:42:38]

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/22001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:863: #if
defined(OS_ANDROID)
On 2012/12/19 23:08:36, aelias wrote:
> You can set it explicitly to false in the browser compositor in our Compositor
> class.  That's how ChromeOS deals with browser/renderer setting differences.

Done

https://codereview.chromium.org/11552009/diff/19002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11552009/diff/19002/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:210: virtual LayerTreeImpl* activeTree() OVERRIDE;
On 2012/12/19 22:56:29, jamesr wrote:
> document what interface this is overriding, i.e. "// Blahblahblah
> implementation."

Done.

https://codereview.chromium.org/11552009/diff/19002/cc/layer_tree_settings.h
File cc/layer_tree_settings.h (right):

https://codereview.chromium.org/11552009/diff/19002/cc/layer_tree_settings.h#...
cc/layer_tree_settings.h:36: size_t topControlsHeight;  // in pixels compensated
for density
On 2012/12/19 22:56:29, jamesr wrote:
> in cc, integral pixel offsets are just 'int'
> 
> start comments with and uppercase letter and end with a period, or make the
> value more apparent from the name. I've no idea what "compensated for density"
> is supposed to mean.

Done.  Just renamed with the suffix Px, which is at least in android a common
means of specifying true screen pixels.  If that's not common or just simply
implied by sizes here, then I can remove that.

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:195: weak_ptr_factory_.GetWeakPtr(), trigger_time),
On 2012/12/19 22:56:29, jamesr wrote:
> a WeakPtrFactory seems like overkill for this - check out base/timer.h. I
think
> you want a OneShotTimer. it'll take care of auto-cancellation on destruction

Awesome...didn't know about this.  Done

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.h
File cc/top_controls_manager.h (right):

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.h...
cc/top_controls_manager.h:23: class TopControlsDelegate {
On 2012/12/19 22:56:29, jamesr wrote:
> this should have a protected destructor since it wouldn't make any sense for
> someone to delete this class via a pointer to one of these.
> 
> our naming convention is that this is a "...Client", not Delegate.
> 
> can you put this in its own header? then layer_tree_host_impl.h wouldn't have
to
> #include this file

Done.

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.h...
cc/top_controls_manager.h:64: 
On 2012/12/19 22:56:29, jamesr wrote:
> I don't think this comment can be right - we never delete the delegate with
this
> pointer, nor would it make a lot of sense to do so

Yeah...poorly worded.  I meant the delegate is the owner of "this" being the
TopControlsManager.

[Ted C, 2012-12-20 00:54:50]

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:22: static const int64 kAutoHideDelayMs = 1500;
On 2012/12/19 22:56:29, jamesr wrote:
> I think the floating point constants will generate static initializers, which
is
> bad. check on that.

thakis informed me that they do not, but he did point out that the statics are
unnecessary...which I removed.

[Ted C, 2013-01-07 22:19:33]

Ping.  I'd like to get some semblance of this checked in so I can avoid the
continue rebase pain.  I know this is behind a fair bit now, but I'll work on
updating it.

On 2012/12/20 00:54:50, Ted C wrote:
> https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.cc
> File cc/top_controls_manager.cc (right):
> 
>
https://codereview.chromium.org/11552009/diff/19002/cc/top_controls_manager.c...
> cc/top_controls_manager.cc:22: static const int64 kAutoHideDelayMs = 1500;
> On 2012/12/19 22:56:29, jamesr wrote:
> > I think the floating point constants will generate static initializers,
which
> is
> > bad. check on that.
> 
> thakis informed me that they do not, but he did point out that the statics are
> unnecessary...which I removed.

[jamesr, 2013-01-08 02:26:48]

enne - are the scrolling interactions with the active tree here valid?

Left some comments.

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:213: virtual LayerTreeImpl* activeTree() OVERRIDE;  //
TopControlsManagerClient implementation.
put this comment on the previous line

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:250: virtual void setNeedsUpdateDrawProperties()
OVERRIDE;  // TopControlsManagerClient implementation.
put the comment on the previous line and just have one comment and no blank line
between this and the next line

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (right):

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_settings.cc...
cc/layer_tree_settings.cc:46: calculateTopControlsPosition =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableTopControlsPositionCalculation);
I don't understand how this could work for both the browser and render process
embeddings of cc, since they'll have the same command line flags but you don't
want both to have the top controls stuff.  Rather than checking the command line
flags here in cc/switches.cc, you should check whichever flags you care about in
the code embedding cc/ and set the LayerTreeHostSettings there

https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:21: const float kShowHideThreshold = 0.75f;
can you leave a comment here saying where these numbers came from and how they
can be changed? even if the answers are "picked empirically" and "email
tedchoc@chromium.org" having that here will help future generations

[enne (OOO), 2013-01-08 06:23:04]

Whoa there.

Does cc need a second animation system?

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:428: m_topControlsManager->UpdateDrawPositions();
How is this supposed to work? You update the active tree and *then* update impl
transforms on part of the tree via UpdateDrawPositions.  So, now the layer tree
transforms aren't up to date anymore.

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:250: virtual void setNeedsUpdateDrawProperties()
OVERRIDE;  // TopControlsManagerClient implementation.
On 2013/01/08 02:26:48, jamesr wrote:
> put the comment on the previous line and just have one comment and no blank
line
> between this and the next line

You should also group all these functions, like InputHandlerClient does above.

https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:70: child_layer->setImplTransform(transform);
It seems more than a little dangerous that the top controls manager reaching in
and mucking with the layer tree.  This sort of code will break.

Especially with the TODO "Adjust fixed position layers as well.", it seems like
you're trying to work around calcDrawProperties and the layer tree and property
system.  Can what you're trying to do be acheived by adding some LayerImpls to
the tree?

[aelias_OOO_until_Jul13, 2013-01-08 07:29:46]

On 2013/01/08 06:23:04, enne wrote:
> Whoa there.
> 
> Does cc need a second animation system?

I wouldn't describe this as primarily an animation system.  The top bar hides
mainly in sync with scrolling, and since CC is the canonical source of scroll
position it made sense to make the code live here.  There's also
translating/resizing behavior needed on the contents depending on the top bar
position.  The animation comes in in the case where a link is tapped, but
otherwise we want never to unnecessarily animate it (to avoid distraction).

https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.c...
> cc/top_controls_manager.cc:70: child_layer->setImplTransform(transform);
> It seems more than a little dangerous that the top controls manager reaching
in
> and mucking with the layer tree.  This sort of code will break.
> 
> Especially with the TODO "Adjust fixed position layers as well.", it seems
like
> you're trying to work around calcDrawProperties and the layer tree and
property
> system.  Can what you're trying to do be acheived by adding some LayerImpls to
> the tree?

True, but this code will work on the renderer-side CC and WebKit doesn't know
anything about top bars, so it seems even less legit to mutate the
WebKit-originating tree.  Anyway, we should at least structure this more as an
API getting layers of interest rather than walking the tree for
top_controls_manager.cc.

[enne (OOO), 2013-01-08 16:03:55]

On 2013/01/08 07:29:46, aelias wrote:
> On 2013/01/08 06:23:04, enne wrote:
> > Whoa there.
> > 
> > Does cc need a second animation system?
> 
> I wouldn't describe this as primarily an animation system.  The top bar hides
> mainly in sync with scrolling, and since CC is the canonical source of scroll
> position it made sense to make the code live here.  There's also
> translating/resizing behavior needed on the contents depending on the top bar
> position.  The animation comes in in the case where a link is tapped, but
> otherwise we want never to unnecessarily animate it (to avoid distraction).

Perhaps I overstated the case.  :)

Mostly, I look at TopControlsAnimation implementing unchained transform
animations, and that just sounds awfully similar to functionality that already
exists in cc::ActiveAnimation with a TransformAnimationCurve.

[Ted C, 2013-01-08 18:17:32]

My thoughts on the [active_]animation usage versus what I ended up with.  These
are biased by the fact that this was my first dabbling with the cc/ package so
my beliefs could be WAY off.

First off, the animation classes seemed to be designed for using with a
particular layer, but I guess that doesn't necessarily need to be the case. 
Also, none of the current properties apply for my usage case (as I'm doing more
than a simple transform), so it seems odd to me to have to add a new property
here.

I'm animating the position of two "objects" that are dependent on each other and
a simple transform doesn't seem to be appropriate for that.

I based my animation more on the PageScaleAnimation as it's operating at a
similar level in my mind.

Also up for chatting about this in person if that will help.

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
File cc/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
cc/layer_tree_host_impl.cc:428: m_topControlsManager->UpdateDrawPositions();
On 2013/01/08 06:23:05, enne wrote:
> How is this supposed to work? You update the active tree and *then* update
impl
> transforms on part of the tree via UpdateDrawPositions.  So, now the layer
tree
> transforms aren't up to date anymore.

Why wouldn't the transforms be up to date?  The layer tree doesn't seem to hold
onto anything it just updates the containing layers.  This should just be
additive work on top of those previously computed transforms.

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
File cc/layer_tree_host_impl.h (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
cc/layer_tree_host_impl.h:213: virtual LayerTreeImpl* activeTree() OVERRIDE;  //
TopControlsManagerClient implementation.
On 2013/01/08 02:26:48, jamesr wrote:
> put this comment on the previous line

Done.  (moved them all up as suggested below)

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
cc/layer_tree_host_impl.h:250: virtual void setNeedsUpdateDrawProperties()
OVERRIDE;  // TopControlsManagerClient implementation.
On 2013/01/08 06:23:05, enne wrote:
> On 2013/01/08 02:26:48, jamesr wrote:
> > put the comment on the previous line and just have one comment and no blank
> line
> > between this and the next line
> 
> You should also group all these functions, like InputHandlerClient does above.

Done.

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_sett...
File cc/layer_tree_settings.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_sett...
cc/layer_tree_settings.cc:46: calculateTopControlsPosition =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableTopControlsPositionCalculation);
On 2013/01/08 02:26:48, jamesr wrote:
> I don't understand how this could work for both the browser and render process
> embeddings of cc, since they'll have the same command line flags but you don't
> want both to have the top controls stuff.  Rather than checking the command
line
> flags here in cc/switches.cc, you should check whichever flags you care about
in
> the code embedding cc/ and set the LayerTreeHostSettings there

Hmm...so move this check to web_layer_tree_view_impl?  It doesn't look to be
doing anything like this there currently, so I just wanted to confirm that is
the right place.  That is until your change lands and it gets refactored into
render_widget, which I guess it would go there instead.

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/top_controls_ma...
File cc/top_controls_manager.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/top_controls_ma...
cc/top_controls_manager.cc:21: const float kShowHideThreshold = 0.75f;
On 2013/01/08 02:26:48, jamesr wrote:
> can you leave a comment here saying where these numbers came from and how they
> can be changed? even if the answers are "picked empirically" and "email
> tedchoc@chromium.org" having that here will help future generations

Done.

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/top_controls_ma...
cc/top_controls_manager.cc:70: child_layer->setImplTransform(transform);
On 2013/01/08 06:23:05, enne wrote:
> It seems more than a little dangerous that the top controls manager reaching
in
> and mucking with the layer tree.  This sort of code will break.
> 
> Especially with the TODO "Adjust fixed position layers as well.", it seems
like
> you're trying to work around calcDrawProperties and the layer tree and
property
> system.  Can what you're trying to do be acheived by adding some LayerImpls to
> the tree?

For adjusting the fixed position layers, that might not be changes done here but
instead fixing up how we calculate their relative position in calcDrawProperties
when translating/resizing parent layers.

[enne (OOO), 2013-01-08 18:23:50]

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
File cc/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
cc/layer_tree_host_impl.cc:428: m_topControlsManager->UpdateDrawPositions();
On 2013/01/08 18:17:33, Ted C wrote:
> On 2013/01/08 06:23:05, enne wrote:
> > How is this supposed to work? You update the active tree and *then* update
> impl
> > transforms on part of the tree via UpdateDrawPositions.  So, now the layer
> tree
> > transforms aren't up to date anymore.
> 
> Why wouldn't the transforms be up to date?  The layer tree doesn't seem to
hold
> onto anything it just updates the containing layers.  This should just be
> additive work on top of those previously computed transforms.

Because the implTransform on a layer is an input to its drawTransform and
screenSpaceTransform.  If you set it post-calcDrawProperties, you're changing an
input after the output has been calculated.

[danakj, 2013-01-08 18:26:39]

On Tue, Jan 8, 2013 at 1:17 PM, <tedchoc@chromium.org> wrote:

> I'm animating the position of two "objects" that are dependent on each
> other and
> a simple transform doesn't seem to be appropriate for that.
>

Can you expand on why a transform with a translate isn't appropriate for
setting the position of something?

[Ted C, 2013-01-08 18:32:41]

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
File cc/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11552009/diff/30001/cc/layer_tree_host...
cc/layer_tree_host_impl.cc:428: m_topControlsManager->UpdateDrawPositions();
On 2013/01/08 18:23:50, enne wrote:
> On 2013/01/08 18:17:33, Ted C wrote:
> > On 2013/01/08 06:23:05, enne wrote:
> > > How is this supposed to work? You update the active tree and *then* update
> > impl
> > > transforms on part of the tree via UpdateDrawPositions.  So, now the layer
> > tree
> > > transforms aren't up to date anymore.
> > 
> > Why wouldn't the transforms be up to date?  The layer tree doesn't seem to
> hold
> > onto anything it just updates the containing layers.  This should just be
> > additive work on top of those previously computed transforms.
> 
> Because the implTransform on a layer is an input to its drawTransform and
> screenSpaceTransform.  If you set it post-calcDrawProperties, you're changing
an
> input after the output has been calculated.

Ah ha...that certainly wasn't my intention.  When I started this whole patch, I
had my call to this prior to the call to calcDrawProperties, but then it got
factored out into the activeTree()->UpdateDrawProperties() and I guess I merged
it badly.  This certainly can be moved above the above line (and I tested it and
it works fine).

[Ted C, 2013-01-08 23:47:17]

On 2013/01/08 18:26:39, danakj wrote:
> On Tue, Jan 8, 2013 at 1:17 PM, <mailto:tedchoc@chromium.org> wrote:
> 
> > I'm animating the position of two "objects" that are dependent on each
> > other and
> > a simple transform doesn't seem to be appropriate for that.
> >
> 
> Can you expand on why a transform with a translate isn't appropriate for
> setting the position of something?

The positions of the controls and content are dependent on each other, but their
relationship is not always 1-1 (i.e. a translate of Y on the controls does not
always translate the content, but sometimes it does).  They have different
states of whether they can overlap each other or need to be clamped.  This
functionality is driving normally by user scroll, so I need the animation to
mimic scrolling behavior.

Now my animation simply provides a single Y "scroll" offset value that I use to
compute the two positions I need, so I could probably replace my animation with
a transform animation where it provides a translate on just a Y value that
mimics the animation I have.

Would it be something like creating an Animation with a
KeyframedTransformAnimationCurve?  Then replacing my animation with that?  The
AnimationCurve class seems to have all the potential functionality I would need,
while the Animation doesn't seem to offer me much at least as far as I can tell.

Any of that sound reasonable?

[Ted C, 2013-01-09 23:36:01]

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (right):

https://codereview.chromium.org/11552009/diff/30001/cc/layer_tree_settings.cc...
cc/layer_tree_settings.cc:46: calculateTopControlsPosition =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableTopControlsPositionCalculation);
On 2013/01/08 18:17:33, Ted C wrote:
> On 2013/01/08 02:26:48, jamesr wrote:
> > I don't understand how this could work for both the browser and render
process
> > embeddings of cc, since they'll have the same command line flags but you
don't
> > want both to have the top controls stuff.  Rather than checking the command
> line
> > flags here in cc/switches.cc, you should check whichever flags you care
about
> in
> > the code embedding cc/ and set the LayerTreeHostSettings there
> 
> Hmm...so move this check to web_layer_tree_view_impl?  It doesn't look to be
> doing anything like this there currently, so I just wanted to confirm that is
> the right place.  That is until your change lands and it gets refactored into
> render_widget, which I guess it would go there instead.

I just uploaded a patch where I moved it to web_layer_tree_view_impl.  Let me
know if that isn't the right place.

[enne (OOO), 2013-01-10 04:09:20]

On 2013/01/08 23:47:17, Ted C wrote:

> Would it be something like creating an Animation with a
> KeyframedTransformAnimationCurve?  Then replacing my animation with that?  The
> AnimationCurve class seems to have all the potential functionality I would
need,
> while the Animation doesn't seem to offer me much at least as far as I can
tell.

Yeah, that's the kind of thing that I was suggesting.  Maybe your use case isn't
met by the existing animation classes, but if you can use what's already there,
then that'd be great.

[Ted C, 2013-01-10 18:46:43]

On 2013/01/10 04:09:20, enne wrote:
> On 2013/01/08 23:47:17, Ted C wrote:
> 
> > Would it be something like creating an Animation with a
> > KeyframedTransformAnimationCurve?  Then replacing my animation with that? 
The
> > AnimationCurve class seems to have all the potential functionality I would
> need,
> > while the Animation doesn't seem to offer me much at least as far as I can
> tell.
> 
> Yeah, that's the kind of thing that I was suggesting.  Maybe your use case
isn't
> met by the existing animation classes, but if you can use what's already
there,
> then that'd be great.

It was actually pretty straight forward to switch to a
KeyframedFloatAnimationCurve.  I just uploaded a patch that removes the my
custom animation class and uses that instead.  PTAL

[Ian Vollick, 2013-01-10 19:08:51]

On 2013/01/10 18:46:43, Ted C wrote:
> On 2013/01/10 04:09:20, enne wrote:
> > On 2013/01/08 23:47:17, Ted C wrote:
> > 
> > > Would it be something like creating an Animation with a
> > > KeyframedTransformAnimationCurve?  Then replacing my animation with that? 
> The
> > > AnimationCurve class seems to have all the potential functionality I would
> > need,
> > > while the Animation doesn't seem to offer me much at least as far as I can
> > tell.
> > 
> > Yeah, that's the kind of thing that I was suggesting.  Maybe your use case
> isn't
> > met by the existing animation classes, but if you can use what's already
> there,
> > then that'd be great.
> 
> It was actually pretty straight forward to switch to a
> KeyframedFloatAnimationCurve.  I just uploaded a patch that removes the my
> custom animation class and uses that instead.  PTAL

You should be able to create two animation curves, as you've done, and Animation
objects that specify parameters for how they are to be run (the timing function,
etc). These two Animation objects can be scheduled with the two layer animation
controllers.

I'd really like to avoid having more than one bit of code controlling
animations, if possible, so this would be preferable sampling curves with a
"mini animation controller", which is what the top controls manager appears to
be.

[Ted C, 2013-01-10 19:34:14]

On 2013/01/10 19:08:51, vollick wrote:
> On 2013/01/10 18:46:43, Ted C wrote:
> > On 2013/01/10 04:09:20, enne wrote:
> > > On 2013/01/08 23:47:17, Ted C wrote:
> > > 
> > > > Would it be something like creating an Animation with a
> > > > KeyframedTransformAnimationCurve?  Then replacing my animation with
that? 
> > The
> > > > AnimationCurve class seems to have all the potential functionality I
would
> > > need,
> > > > while the Animation doesn't seem to offer me much at least as far as I
can
> > > tell.
> > > 
> > > Yeah, that's the kind of thing that I was suggesting.  Maybe your use case
> > isn't
> > > met by the existing animation classes, but if you can use what's already
> > there,
> > > then that'd be great.
> > 
> > It was actually pretty straight forward to switch to a
> > KeyframedFloatAnimationCurve.  I just uploaded a patch that removes the my
> > custom animation class and uses that instead.  PTAL
> 
> You should be able to create two animation curves, as you've done, and
Animation
> objects that specify parameters for how they are to be run (the timing
function,
> etc). These two Animation objects can be scheduled with the two layer
animation
> controllers.
> 
> I'd really like to avoid having more than one bit of code controlling
> animations, if possible, so this would be preferable sampling curves with a
> "mini animation controller", which is what the top controls manager appears to
> be.

Ok...so I don't really know the cc animation framework...so I'll need to respond
with a "huh?"

I'm don't see where I'm creating two animation curves.  Just one that has two
keyframes (unless the fact that keyframes taking a timing function that is in
fact an animation curve).  I don't have two independent animations happening.  I
have one animation occurring that updates the values of two objects because they
depend on each other.

The only approach I see as possible is to get the animation_registrar from the
layer_tree_host_impl.  Add a LayerAnimationController to it that is owned by the
top controls manager (although it appears these controllers are all associated
with unique ids that I don't currently have).  Then add the top_controls_manager
as a LayerAnimationValueObserver to this controller and add my animations to
that?  Then I would need to wrap the my keyframed animation curve in an
Animation for TargetProperty::Transform w/ some made up id and group id and add
that to the LayerAnimationController.

This seems very heavy handed to me, but oh well.

[Ian Vollick, 2013-01-10 20:34:22]

On 2013/01/10 19:34:14, Ted C wrote:
> On 2013/01/10 19:08:51, vollick wrote:
> > On 2013/01/10 18:46:43, Ted C wrote:
> > > On 2013/01/10 04:09:20, enne wrote:
> > > > On 2013/01/08 23:47:17, Ted C wrote:
> > > > 
> > > > > Would it be something like creating an Animation with a
> > > > > KeyframedTransformAnimationCurve?  Then replacing my animation with
> that? 
> > > The
> > > > > AnimationCurve class seems to have all the potential functionality I
> would
> > > > need,
> > > > > while the Animation doesn't seem to offer me much at least as far as I
> can
> > > > tell.
> > > > 
> > > > Yeah, that's the kind of thing that I was suggesting.  Maybe your use
case
> > > isn't
> > > > met by the existing animation classes, but if you can use what's already
> > > there,
> > > > then that'd be great.
> > > 
> > > It was actually pretty straight forward to switch to a
> > > KeyframedFloatAnimationCurve.  I just uploaded a patch that removes the my
> > > custom animation class and uses that instead.  PTAL
> > 
> > You should be able to create two animation curves, as you've done, and
> Animation
> > objects that specify parameters for how they are to be run (the timing
> function,
> > etc). These two Animation objects can be scheduled with the two layer
> animation
> > controllers.
> > 
> > I'd really like to avoid having more than one bit of code controlling
> > animations, if possible, so this would be preferable sampling curves with a
> > "mini animation controller", which is what the top controls manager appears
to
> > be.
> 
> Ok...so I don't really know the cc animation framework...so I'll need to
respond
> with a "huh?"
> 
> I'm don't see where I'm creating two animation curves.  Just one that has two
> keyframes (unless the fact that keyframes taking a timing function that is in
> fact an animation curve).  I don't have two independent animations happening. 
I
> have one animation occurring that updates the values of two objects because
they
> depend on each other.

I'm sorry, there were a bunch of mistakes in my last reply.

What I'd meant to suggest is that you could create two curves in the same way
that you'd created one here. But as I now understand things, that wouldn't have
worked anyhow, at least not in the way I'd suggested. The reasons:
 1) You're effectively animating implTransform, and you can't currently do that
with the layer animation controller.
 2) The final implTransform is a more complex function of time than can be
expressed as a single cubic bezier (as you said, the value of one guy is
dependant on the value of the other).

That said, I wonder if this would be possible:
 1) Create new subclass(es) of TransformAnimationCurve that are able to figure
out the final implTransform for time t for a given layer. I assume that the
subclass, if initialized correctly, could determine where the 'other guy' would
be during the animation and make decisions based on that.
 2) Add support for a new target property for impl transfom
(Animation::TargetProperty, that is) -- Animation and LayerAnimationController
would at least be affected.

If this were possible, then when the top controls manager started an animation,
it could create and initialize these curves (and Animation's to wrap them) and
install them on the layers that get updated in TCM::UpdateDrawPositions, and the
layers would move as expected.

Do you think this could work?

[Ted C, 2013-01-10 20:55:39]

On 2013/01/10 20:34:22, vollick wrote:
> On 2013/01/10 19:34:14, Ted C wrote:
> > On 2013/01/10 19:08:51, vollick wrote:
> > > On 2013/01/10 18:46:43, Ted C wrote:
> > > > On 2013/01/10 04:09:20, enne wrote:
> > > > > On 2013/01/08 23:47:17, Ted C wrote:
> > > > > 
> > > > > > Would it be something like creating an Animation with a
> > > > > > KeyframedTransformAnimationCurve?  Then replacing my animation with
> > that? 
> > > > The
> > > > > > AnimationCurve class seems to have all the potential functionality I
> > would
> > > > > need,
> > > > > > while the Animation doesn't seem to offer me much at least as far as
I
> > can
> > > > > tell.
> > > > > 
> > > > > Yeah, that's the kind of thing that I was suggesting.  Maybe your use
> case
> > > > isn't
> > > > > met by the existing animation classes, but if you can use what's
already
> > > > there,
> > > > > then that'd be great.
> > > > 
> > > > It was actually pretty straight forward to switch to a
> > > > KeyframedFloatAnimationCurve.  I just uploaded a patch that removes the
my
> > > > custom animation class and uses that instead.  PTAL
> > > 
> > > You should be able to create two animation curves, as you've done, and
> > Animation
> > > objects that specify parameters for how they are to be run (the timing
> > function,
> > > etc). These two Animation objects can be scheduled with the two layer
> > animation
> > > controllers.
> > > 
> > > I'd really like to avoid having more than one bit of code controlling
> > > animations, if possible, so this would be preferable sampling curves with
a
> > > "mini animation controller", which is what the top controls manager
appears
> to
> > > be.
> > 
> > Ok...so I don't really know the cc animation framework...so I'll need to
> respond
> > with a "huh?"
> > 
> > I'm don't see where I'm creating two animation curves.  Just one that has
two
> > keyframes (unless the fact that keyframes taking a timing function that is
in
> > fact an animation curve).  I don't have two independent animations
happening. 
> I
> > have one animation occurring that updates the values of two objects because
> they
> > depend on each other.
> 
> I'm sorry, there were a bunch of mistakes in my last reply.
> 
> What I'd meant to suggest is that you could create two curves in the same way
> that you'd created one here. But as I now understand things, that wouldn't
have
> worked anyhow, at least not in the way I'd suggested. The reasons:
>  1) You're effectively animating implTransform, and you can't currently do
that
> with the layer animation controller.
>  2) The final implTransform is a more complex function of time than can be
> expressed as a single cubic bezier (as you said, the value of one guy is
> dependant on the value of the other).
> 
> That said, I wonder if this would be possible:
>  1) Create new subclass(es) of TransformAnimationCurve that are able to figure
> out the final implTransform for time t for a given layer. I assume that the
> subclass, if initialized correctly, could determine where the 'other guy'
would
> be during the animation and make decisions based on that.
>  2) Add support for a new target property for impl transfom
> (Animation::TargetProperty, that is) -- Animation and LayerAnimationController
> would at least be affected.
> 
> If this were possible, then when the top controls manager started an
animation,
> it could create and initialize these curves (and Animation's to wrap them) and
> install them on the layers that get updated in TCM::UpdateDrawPositions, and
the
> layers would move as expected.
> 
> Do you think this could work?

I'm not entirely sure it would.  The animation updates need to go through the
same path as if it were mimicking a scroll event (the closest corollary I can
think of is WebCompositorInputHandler's handling of fling events).  I'd like to
share the same code without having to try and convert the logic for just the
animations to some animation curve.

I'm curious why the approach I suggested in my previous response wouldn't work. 
If I converted my float animation to a transform animation and made the
TopControlsManager a LayerAnimationValueObserver (and found an appropriate
controller to handle it) then I could keep the logic as it is while moving the
animation handling to a centralized location.  Without having to bloat the
animations classes with something so specific to just one use case.

Also, is any of this appropriate to do in a follow up?  A bunch of people are
trying to help on fullscreen, but this is sadly blocking them all so I'd like to
get some basic functionality checked in to unblock everyone.

[Ian Vollick, 2013-01-11 02:53:05]

On 2013/01/10 20:55:39, Ted C wrote:
> On 2013/01/10 20:34:22, vollick wrote:
> > On 2013/01/10 19:34:14, Ted C wrote:
> > > On 2013/01/10 19:08:51, vollick wrote:
> > > > On 2013/01/10 18:46:43, Ted C wrote:
> > > > > On 2013/01/10 04:09:20, enne wrote:
> > > > > > On 2013/01/08 23:47:17, Ted C wrote:
> > > > > > 
> > > > > > > Would it be something like creating an Animation with a
> > > > > > > KeyframedTransformAnimationCurve?  Then replacing my animation
with
> > > that? 
> > > > > The
> > > > > > > AnimationCurve class seems to have all the potential functionality
I
> > > would
> > > > > > need,
> > > > > > > while the Animation doesn't seem to offer me much at least as far
as
> I
> > > can
> > > > > > tell.
> > > > > > 
> > > > > > Yeah, that's the kind of thing that I was suggesting.  Maybe your
use
> > case
> > > > > isn't
> > > > > > met by the existing animation classes, but if you can use what's
> already
> > > > > there,
> > > > > > then that'd be great.
> > > > > 
> > > > > It was actually pretty straight forward to switch to a
> > > > > KeyframedFloatAnimationCurve.  I just uploaded a patch that removes
the
> my
> > > > > custom animation class and uses that instead.  PTAL
> > > > 
> > > > You should be able to create two animation curves, as you've done, and
> > > Animation
> > > > objects that specify parameters for how they are to be run (the timing
> > > function,
> > > > etc). These two Animation objects can be scheduled with the two layer
> > > animation
> > > > controllers.
> > > > 
> > > > I'd really like to avoid having more than one bit of code controlling
> > > > animations, if possible, so this would be preferable sampling curves
with
> a
> > > > "mini animation controller", which is what the top controls manager
> appears
> > to
> > > > be.
> > > 
> > > Ok...so I don't really know the cc animation framework...so I'll need to
> > respond
> > > with a "huh?"
> > > 
> > > I'm don't see where I'm creating two animation curves.  Just one that has
> two
> > > keyframes (unless the fact that keyframes taking a timing function that is
> in
> > > fact an animation curve).  I don't have two independent animations
> happening. 
> > I
> > > have one animation occurring that updates the values of two objects
because
> > they
> > > depend on each other.
> > 
> > I'm sorry, there were a bunch of mistakes in my last reply.
> > 
> > What I'd meant to suggest is that you could create two curves in the same
way
> > that you'd created one here. But as I now understand things, that wouldn't
> have
> > worked anyhow, at least not in the way I'd suggested. The reasons:
> >  1) You're effectively animating implTransform, and you can't currently do
> that
> > with the layer animation controller.
> >  2) The final implTransform is a more complex function of time than can be
> > expressed as a single cubic bezier (as you said, the value of one guy is
> > dependant on the value of the other).
> > 
> > That said, I wonder if this would be possible:
> >  1) Create new subclass(es) of TransformAnimationCurve that are able to
figure
> > out the final implTransform for time t for a given layer. I assume that the
> > subclass, if initialized correctly, could determine where the 'other guy'
> would
> > be during the animation and make decisions based on that.
> >  2) Add support for a new target property for impl transfom
> > (Animation::TargetProperty, that is) -- Animation and
LayerAnimationController
> > would at least be affected.
> > 
> > If this were possible, then when the top controls manager started an
> animation,
> > it could create and initialize these curves (and Animation's to wrap them)
and
> > install them on the layers that get updated in TCM::UpdateDrawPositions, and
> the
> > layers would move as expected.
> > 
> > Do you think this could work?
> 
> I'm not entirely sure it would.  The animation updates need to go through the
> same path as if it were mimicking a scroll event (the closest corollary I can
> think of is WebCompositorInputHandler's handling of fling events). 

I'm not sure I follow. The result of calling TopControlsManager::Animate
followed by TCM::UpdateDrawPositions looks like the updating of a few impl
transforms. Are there other side effects I'm missing? If not, couldn't the same
thing be accomplished by constructing animations/curves that apply the same
changes to the impl transforms?

> I'd like to
> share the same code without having to try and convert the logic for just the
> animations to some animation curve.

It looks like most of the logic in TCM is animation related, so my hope was that
little would need to be shared and that the logic could almost entirely be moved
to a TopControlsAnimationCurve.

> 
> I'm curious why the approach I suggested in my previous response wouldn't
work. 
> If I converted my float animation to a transform animation and made the
> TopControlsManager a LayerAnimationValueObserver (and found an appropriate
> controller to handle it) then I could keep the logic as it is while moving the
> animation handling to a centralized location.  Without having to bloat the
> animations classes with something so specific to just one use case.

You're right, it would work, but it sounds like it would be essentially the same
as the solution you have here. Rather than doing all the work in Animate, it
would be just be moved to OnTransformAnimated. The intent of OnTransformAnimated
was that it would receive the final animated value, and it would simply set it
(and possibly dirty some state). Using this function as an animation callback
that does arbitrary work is contrary to the aim of the system.

My hope was that you could simply create animations, set them on the layers that
they're to affect, and then forget about them, letting the layer animation code
tick the animations and set the values.

This is not the first time someone's added more code to tick in
LayerTreeHostImpl::updateAnimations, nor is it the first time someone's asked
for (or re-implemented) a custom animation system that operates on the impl
layer tree at each time tick. I'm not against either of these things, really,
but I do worry about recreating the wheel a bunch of time in a bunch of
different ways.

What if we were to generalize what you've done here so that your approach can be
reused in future? I'm imagining that we create a new AnimationController base
class (that has the pure virtual void animate(double, AnimationEventsVector*)
and the AnimationRegistrar-related functions). LayerAnimationController could be
one subclass, TopControlsManager could be another. If using the layer animation
system can't work for your case, I think this would let your code work
essentially unmodified.

> 
> Also, is any of this appropriate to do in a follow up?  A bunch of people are
> trying to help on fullscreen, but this is sadly blocking them all so I'd like
to
> get some basic functionality checked in to unblock everyone.

I'll let James or Enne make this call. (I'm not able to give you OWNERS in any
case).

[Ted C, 2013-01-11 03:35:44]

On 2013/01/11 02:53:05, vollick wrote:
> On 2013/01/10 20:55:39, Ted C wrote:
> > On 2013/01/10 20:34:22, vollick wrote:
> > > On 2013/01/10 19:34:14, Ted C wrote:
> > > > On 2013/01/10 19:08:51, vollick wrote:
> > > > > On 2013/01/10 18:46:43, Ted C wrote:
> > > > > > On 2013/01/10 04:09:20, enne wrote:
> > > > > > > On 2013/01/08 23:47:17, Ted C wrote:
> > > > > > > 
> > > > > > > > Would it be something like creating an Animation with a
> > > > > > > > KeyframedTransformAnimationCurve?  Then replacing my animation
> with
> > > > that? 
> > > > > > The
> > > > > > > > AnimationCurve class seems to have all the potential
functionality
> I
> > > > would
> > > > > > > need,
> > > > > > > > while the Animation doesn't seem to offer me much at least as
far
> as
> > I
> > > > can
> > > > > > > tell.
> > > > > > > 
> > > > > > > Yeah, that's the kind of thing that I was suggesting.  Maybe your
> use
> > > case
> > > > > > isn't
> > > > > > > met by the existing animation classes, but if you can use what's
> > already
> > > > > > there,
> > > > > > > then that'd be great.
> > > > > > 
> > > > > > It was actually pretty straight forward to switch to a
> > > > > > KeyframedFloatAnimationCurve.  I just uploaded a patch that removes
> the
> > my
> > > > > > custom animation class and uses that instead.  PTAL
> > > > > 
> > > > > You should be able to create two animation curves, as you've done, and
> > > > Animation
> > > > > objects that specify parameters for how they are to be run (the timing
> > > > function,
> > > > > etc). These two Animation objects can be scheduled with the two layer
> > > > animation
> > > > > controllers.
> > > > > 
> > > > > I'd really like to avoid having more than one bit of code controlling
> > > > > animations, if possible, so this would be preferable sampling curves
> with
> > a
> > > > > "mini animation controller", which is what the top controls manager
> > appears
> > > to
> > > > > be.
> > > > 
> > > > Ok...so I don't really know the cc animation framework...so I'll need to
> > > respond
> > > > with a "huh?"
> > > > 
> > > > I'm don't see where I'm creating two animation curves.  Just one that
has
> > two
> > > > keyframes (unless the fact that keyframes taking a timing function that
is
> > in
> > > > fact an animation curve).  I don't have two independent animations
> > happening. 
> > > I
> > > > have one animation occurring that updates the values of two objects
> because
> > > they
> > > > depend on each other.
> > > 
> > > I'm sorry, there were a bunch of mistakes in my last reply.
> > > 
> > > What I'd meant to suggest is that you could create two curves in the same
> way
> > > that you'd created one here. But as I now understand things, that wouldn't
> > have
> > > worked anyhow, at least not in the way I'd suggested. The reasons:
> > >  1) You're effectively animating implTransform, and you can't currently do
> > that
> > > with the layer animation controller.
> > >  2) The final implTransform is a more complex function of time than can be
> > > expressed as a single cubic bezier (as you said, the value of one guy is
> > > dependant on the value of the other).
> > > 
> > > That said, I wonder if this would be possible:
> > >  1) Create new subclass(es) of TransformAnimationCurve that are able to
> figure
> > > out the final implTransform for time t for a given layer. I assume that
the
> > > subclass, if initialized correctly, could determine where the 'other guy'
> > would
> > > be during the animation and make decisions based on that.
> > >  2) Add support for a new target property for impl transfom
> > > (Animation::TargetProperty, that is) -- Animation and
> LayerAnimationController
> > > would at least be affected.
> > > 
> > > If this were possible, then when the top controls manager started an
> > animation,
> > > it could create and initialize these curves (and Animation's to wrap them)
> and
> > > install them on the layers that get updated in TCM::UpdateDrawPositions,
and
> > the
> > > layers would move as expected.
> > > 
> > > Do you think this could work?
> > 
> > I'm not entirely sure it would.  The animation updates need to go through
the
> > same path as if it were mimicking a scroll event (the closest corollary I
can
> > think of is WebCompositorInputHandler's handling of fling events). 
> 
> I'm not sure I follow. The result of calling TopControlsManager::Animate
> followed by TCM::UpdateDrawPositions looks like the updating of a few impl
> transforms. Are there other side effects I'm missing? If not, couldn't the
same
> thing be accomplished by constructing animations/curves that apply the same
> changes to the impl transforms?

Animate calls into ScrollInternal, which updates the positions of the top
controls (which is not a layer at all) and the content.  The positions are
dependent on each other and also updated various states depending on where they
are on the page.  Only the impl transforms apply to the content, but the
position of the top controls is the main driver of this class, which is why I
think it's odd to try and force it to behave as a layer in the scope of the
animation.

I guess I see the values calculated in ScrollInternal to be relatively complex
so it would be a pain to figure out animation curves that apply (and they need
to be different depending on where they are on the page etc...).

> 
> > I'd like to
> > share the same code without having to try and convert the logic for just the
> > animations to some animation curve.
> 
> It looks like most of the logic in TCM is animation related, so my hope was
that
> little would need to be shared and that the logic could almost entirely be
moved
> to a TopControlsAnimationCurve.

As I see it, it's more about handling scroll events.  The animations are just
there to supplement scrolls to ensure we're not in a bad ending state when a
scroll is complete.

> 
> > 
> > I'm curious why the approach I suggested in my previous response wouldn't
> work. 
> > If I converted my float animation to a transform animation and made the
> > TopControlsManager a LayerAnimationValueObserver (and found an appropriate
> > controller to handle it) then I could keep the logic as it is while moving
the
> > animation handling to a centralized location.  Without having to bloat the
> > animations classes with something so specific to just one use case.
> 
> You're right, it would work, but it sounds like it would be essentially the
same
> as the solution you have here. Rather than doing all the work in Animate, it
> would be just be moved to OnTransformAnimated. The intent of
OnTransformAnimated
> was that it would receive the final animated value, and it would simply set it
> (and possibly dirty some state). Using this function as an animation callback
> that does arbitrary work is contrary to the aim of the system.
> 
> My hope was that you could simply create animations, set them on the layers
that
> they're to affect, and then forget about them, letting the layer animation
code
> tick the animations and set the values.
> 
> This is not the first time someone's added more code to tick in
> LayerTreeHostImpl::updateAnimations, nor is it the first time someone's asked
> for (or re-implemented) a custom animation system that operates on the impl
> layer tree at each time tick. I'm not against either of these things, really,
> but I do worry about recreating the wheel a bunch of time in a bunch of
> different ways.
> 
> What if we were to generalize what you've done here so that your approach can
be
> reused in future? I'm imagining that we create a new AnimationController base
> class (that has the pure virtual void animate(double, AnimationEventsVector*)
> and the AnimationRegistrar-related functions). LayerAnimationController could
be
> one subclass, TopControlsManager could be another. If using the layer
animation
> system can't work for your case, I think this would let your code work
> essentially unmodified.

This sounds great to me.  And I'm not just saying it because it's less work for
me (yay), but because I do think this doesn't fit the existing layer model all
that well.  Generalizing the animation framework in this way should also play
nicely with refactoring the page scale animation.  Then the animate method in
layer_tree_host_impl would simply be looping over these new Controllers and
having them run a frame of animation.

> 
> > 
> > Also, is any of this appropriate to do in a follow up?  A bunch of people
are
> > trying to help on fullscreen, but this is sadly blocking them all so I'd
like
> to
> > get some basic functionality checked in to unblock everyone.
> 
> I'll let James or Enne make this call. (I'm not able to give you OWNERS in any
> case).

[wjmaclean, 2013-01-11 14:45:24]

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
Why are you using the implTransform for this? It's really not what it's intended
for. You should be making these kind of changes to the layer's draw transform
(layer->setTransform()).

[enne (OOO), 2013-01-11 17:20:29]

If the animation system isn't a good match for what you're trying to do here,
then we can punt on it.  I appreciate you taking the time to investigate it; I
just didn't want to add yet another custom animator without good reason.  I
think I also now have a better picture of what we would need to better support
more general animations within the previously existing system.

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
On 2013/01/11 14:45:24, wjmaclean wrote:
> Why are you using the implTransform for this? It's really not what it's
intended
> for. You should be making these kind of changes to the layer's draw transform
> (layer->setTransform()).

As I mentioned in
https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.c...,
why are you adjusting the layer tree from the top controls manager? This seems
extremely fragile.

[Ted C, 2013-01-11 17:50:54]

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
On 2013/01/11 14:45:24, wjmaclean wrote:
> Why are you using the implTransform for this? It's really not what it's
intended
> for. You should be making these kind of changes to the layer's draw transform
> (layer->setTransform()).

What is the distinction?  I was under the impression that transforms only
applied in the impl thread should use ImplTransforms.  I works with either, but
I couldn't seem to decipher what is the appropriate time to use either.

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
On 2013/01/11 17:20:29, enne wrote:
> On 2013/01/11 14:45:24, wjmaclean wrote:
> > Why are you using the implTransform for this? It's really not what it's
> intended
> > for. You should be making these kind of changes to the layer's draw
transform
> > (layer->setTransform()).
> 
> As I mentioned in
>
https://codereview.chromium.org/11552009/diff/30001/cc/top_controls_manager.c...,
> why are you adjusting the layer tree from the top controls manager? This seems
> extremely fragile.

It needs to move the content/clip layers around somehow though.  This seemed
like the way to best isolate the mutations to just the top controls manager. 
How would you suggest it be done differently?  It certainly could break if the
structure of the impl tree changed, but that is also something we could test
with an integration test (making sure the content actually visibly moves when
exptected).

Previously you mentioned LayerImpls, but I know Alex had reservations about
that.  If I were to insert a new LayerImpl as the only child of the root node
(and reparenting all it's children), then that would give me a layer owned by
this that can easily move around the "world".    But this would get clobbered
with every tree commit and to my understanding would make each commit a much
more expensive operation since the tree structures would be different.

[enne (OOO), 2013-01-11 17:57:01]

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
Why does it need to move the content/clip layers around? What are you trying to
do? What's the goal here?

From your change description, it sounds like you should be treating the tree
properties as read-only in order to sync the top controls to various tree
properties.

[Ted C, 2013-01-11 18:10:23]

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/53002/cc/top_controls_manager.c...
cc/top_controls_manager.cc:71: child_layer->setImplTransform(transform);
On 2013/01/11 17:57:01, enne wrote:
> Why does it need to move the content/clip layers around? What are you trying
to
> do? What's the goal here?

As you scroll down a page, we move the urlbar/top controls off the top of the
screen.  While we are doing this, we need to animate up the content to be
aligned with the moving top controls.

This is all working towards providing fullscreen where the controls move off the
top of the page when you scroll down (and the content fills the viewport) and
you can bring in the controls while in the middle of the page.

> 
> From your change description, it sounds like you should be treating the tree
> properties as read-only in order to sync the top controls to various tree
> properties.

I guess my description is invalid then.  It's to provide position of top
controls back to the browser while modifying the rendering tree to align with
the desired positions of the UI controls.

[enne (OOO), 2013-01-12 00:14:59]

https://codereview.chromium.org/11552009/diff/62001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11552009/diff/62001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:581: m_topControlsContentLayer->setIsDrawable(false);
Can you add a setDebugName("Top Controls Content") or some such here?

Also, did you have a chance to try calling
setIsContainerForFixedPositionLayers(true) and see if that fixed your fixed
position woes too?

https://codereview.chromium.org/11552009/diff/62001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/62001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:66:
client_->activeTree()->FindActiveTreeLayerById(content_layer_id_);
Thanks, this looks way safer to me.

[Ted C, 2013-01-12 01:23:05]

Sadly my fixed position woes rely on me getting an increased root layer clip
size plumbed through, so that will need to wait until my next patch.

https://codereview.chromium.org/11552009/diff/62001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11552009/diff/62001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:581: m_topControlsContentLayer->setIsDrawable(false);
On 2013/01/12 00:15:00, enne wrote:
> Can you add a setDebugName("Top Controls Content") or some such here?
> 
> Also, did you have a chance to try calling
> setIsContainerForFixedPositionLayers(true) and see if that fixed your fixed
> position woes too?

Done.

[aelias_OOO_until_Jul13, 2013-01-14 21:52:50]

lgtm for content/browser/renderer_host except for naming nit:

https://codereview.chromium.org/11552009/diff/69001/cc/compositor_frame_metad...
File cc/compositor_frame_metadata.h (right):

https://codereview.chromium.org/11552009/diff/69001/cc/compositor_frame_metad...
cc/compositor_frame_metadata.h:35: gfx::Vector2dF content_offset;
Could you rename this to location_bar_content_translation?  The current name is
too generic.

[Ted C, 2013-01-14 22:10:42]

+avi - content/port/browser/render_widget_host_view_port.h

+jschuh - (IPC security review) - content/common/cc_messages.h

@enne - I think you're in OWNERS of everything else, so I'll probably defer to
you for the final signoff.

https://codereview.chromium.org/11552009/diff/69001/cc/compositor_frame_metad...
File cc/compositor_frame_metadata.h (right):

https://codereview.chromium.org/11552009/diff/69001/cc/compositor_frame_metad...
cc/compositor_frame_metadata.h:35: gfx::Vector2dF content_offset;
On 2013/01/14 21:52:50, aelias wrote:
> Could you rename this to location_bar_content_translation?  The current name
is
> too generic.

Done.

[Avi (use Gerrit), 2013-01-14 22:50:39]

content/port lgtm

[jschuh, 2013-01-15 15:10:33]

ipc security lgtm.

[enne (OOO), 2013-01-15 17:32:47]

cc lgtm

[danakj, 2013-01-15 18:02:43]

I'm just wondering here..

What prevents us from implementing this outside of cc/ in the android UI code?
AIUI, we provide the scroll offset with each frame, and the UI can manipulate
the UI layer which hosts the web contents.

What else is missing from the CC apis to allow UI to implement things like this
outside of CC?

https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:1: // Copyright 2012 The Chromium Authors. All rights
reserved.
2013 in new files now :)

[Ted C, 2013-01-15 18:13:59]

On 2013/01/15 18:02:43, danakj wrote:
> I'm just wondering here..
> 
> What prevents us from implementing this outside of cc/ in the android UI code?
> AIUI, we provide the scroll offset with each frame, and the UI can manipulate
> the UI layer which hosts the web contents.
> 
> What else is missing from the CC apis to allow UI to implement things like
this
> outside of CC?

This actually consumes scroll from the root layer, so that is the main reason
that makes it difficult to push this out of cc/.

When talking with enne@, we hopefully will be able push all of the layer
manipulation to the browser process once we have the ubercompositor.

So moving as much of this out of cc is definitely something we want to do. 

> 
> https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.cc
> File cc/top_controls_manager.cc (right):
> 
>
https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.c...
> cc/top_controls_manager.cc:1: // Copyright 2012 The Chromium Authors. All
rights
> reserved.
> 2013 in new files now :)

[Ted C, 2013-01-15 19:12:10]

https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11552009/diff/72001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:1: // Copyright 2012 The Chromium Authors. All rights
reserved.
On 2013/01/15 18:02:43, danakj wrote:
> 2013 in new files now :)

Done.

[commit-bot: I haz the power, 2013-01-15 20:08:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11552009/78001

[commit-bot: I haz the power, 2013-01-15 20:08:40]

Failed to apply patch for cc/cc_tests.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/cc_tests.gyp
  Hunk #1 FAILED at 60.
  1 out of 1 hunk FAILED -- saving rejects to file cc/cc_tests.gyp.rej

Patch:       cc/cc_tests.gyp
Index: cc/cc_tests.gyp
diff --git a/cc/cc_tests.gyp b/cc/cc_tests.gyp
index
845b9d2e6c6bc1c6112fcd56cea00ebc0ae5b495..2f22144fd9da5859a37d2c3e9f0210f7d706aeab
100644
--- a/cc/cc_tests.gyp
+++ b/cc/cc_tests.gyp
@@ -60,6 +60,7 @@
       'tile_priority_unittest.cc',
       'tiled_layer_impl_unittest.cc',
       'tiled_layer_unittest.cc',
+      'top_controls_manager_unittest.cc',
       'tree_synchronizer_unittest.cc',
       'timing_function_unittest.cc',
       'test/fake_web_graphics_context_3d_unittest.cc',

[commit-bot: I haz the power, 2013-01-15 21:23:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11552009/83001

[commit-bot: I haz the power, 2013-01-15 21:59:42]

Failed to trigger a try job on android_clang_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-15 21:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11552009/85002

[commit-bot: I haz the power, 2013-01-15 22:26:28]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-15 22:26:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11552009/86003

[commit-bot: I haz the power, 2013-01-15 23:24:16]

Retried try job too often on linux_clang for step(s) compile

[commit-bot: I haz the power, 2013-01-16 00:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11552009/109001

[commit-bot: I haz the power, 2013-01-16 03:58:40]

Change committed as 177081