Animated Rotation

With this CL animated rotations can be triggered by visiting URLs like about:rotate?right. The about handler fires orientation events that the TouchBrowserFrameView is listening for. When notified, the TBFV either rotates itself, or the views desktop (if it is active). The animation code lives in views/animation and knows nothing about sensors.

BUG=5247043
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100148

[sadrul, 2011-06-29 20:35:10]

Quick question before I go into too much details: is this demo expected to work
both when you touch the button in the status-bar, and from the device sensor? If
so, I think it'd be better to split it up into two pieces (1) do the rotation
from the button's callback, and (2) hook into the sensor. The first should be
reasonably small and easily reverted when need be, and can still be useful for
testing. The latter, the sensor stuff, I feel like it's a good time to do it in
a clean way rather than a temporary solution.

http://codereview.chromium.org/7273073/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/7273073/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/power_menu_button.cc:33: } // namespace
two spaces between } and //

http://codereview.chromium.org/7273073/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/power_menu_button.cc:177:
sensors::kScreenOrientationChannel, data);
Since we are upstreaming this change, I think it'd be best to just add a new
temporary button in the status area view :-)
But if we are indeed going to hijack one of the status area buttons, then it's
better to hijack the window-switcher button than this, since we do not fully
support multiple windows at the moment anyway.

http://codereview.chromium.org/7273073/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/power_menu_button.h (right):

http://codereview.chromium.org/7273073/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/power_menu_button.h:9: #include
"content/browser/sensors/provider.h"
-

[Ian Vollick, 2011-06-29 20:53:49]

On 2011/06/29 20:35:10, sadrul wrote:
> Quick question before I go into too much details: is this demo expected to
work
> both when you touch the button in the status-bar, and from the device sensor?
If
> so, I think it'd be better to split it up into two pieces (1) do the rotation
> from the button's callback, and (2) hook into the sensor. The first should be
> reasonably small and easily reverted when need be, and can still be useful for
> testing. The latter, the sensor stuff, I feel like it's a good time to do it
in
> a clean way rather than a temporary solution.
> 

It's supposed to work both when pressing the button, and when visiting
about:rotate?right (or left, bottom, top). In the near future, the button hack
will be dropped and the rotation will be triggered by the sensor. Even after
this switch happens, though, the about:rotate url will still simulate
orientation sensor events so that more people can try the code, and can find out
if they've accidentally broken it.

[cwolfe, 2011-06-29 22:44:47]

Chiming in, since the content/ code is my fault :)

The sensors:: code is pretty messy since:
1) the prototype it came from also passes events through to the renderers
2) the struct-in-string is intended to be replaced by a protocol buffer
3) the "read from chardev" hack is intended to be replaced by an external sensor
daemon
3a) so I didn't bother making the sensor chardev configurable
3b) does not use MessagePumpForIO, as the IIO-provided chardev I tried it with
lacks poll support

It's very much work-in-progress and I'm not certain of some of the design
decisions. I believe plumbing protobuf data from the sensor daemon right through
to the renderers makes sense, but I didn't find other examples of that in the
codebase.

The model is that a Listener opens a connection, and then subscribes to one or
more channels with options (e.g. the update rate). The rotation handler would
subscribe to a coarse "this way is up" channel, which I more-or-less-arbitrarily
called screen orientation.

Chris

On 2011/06/29 20:35:10, sadrul wrote:
> Quick question before I go into too much details: is this demo expected to
work
> both when you touch the button in the status-bar, and from the device sensor?
If
> so, I think it'd be better to split it up into two pieces (1) do the rotation
> from the button's callback, and (2) hook into the sensor. The first should be
> reasonably small and easily reverted when need be, and can still be useful for
> testing. The latter, the sensor stuff, I feel like it's a good time to do it
in
> a clean way rather than a temporary solution.

[Jói, 2011-06-30 16:59:57]

Is this intended for eventual check-in or will it remain an uncommitted patch?

Cheers,
Jói

[sadrul, 2011-06-30 17:49:08]

On 2011/06/30 16:59:57, Jói wrote:
> Is this intended for eventual check-in or will it remain an uncommitted patch?

The plan is to check in most (if not all) of this code, in some form or other
(e.g. some only for touchui etc.)

[Jói, 2011-06-30 19:50:50]

I mainly have a high-level question:  Does knowledge of sensors belong under
content/ ?  The content/ directory is intended to contain everything you need to
run a multi-process browser, and it seems sensors perhaps either belong at a
lower level (i.e. the web platform should know about and access them, so they go
in WebKit somewhere) or a higher level (i.e. they're used for Chrome's UI, so
they should be in chrome/, not content/.

What do you think?

Cheers,
Jói

http://codereview.chromium.org/7273073/diff/1/content/browser/sensors/provide...
File content/browser/sensors/provider_impl.cc (right):

http://codereview.chromium.org/7273073/diff/1/content/browser/sensors/provide...
content/browser/sensors/provider_impl.cc:94:
"/sys/bus/iio/devices/device0:event0/tilt_change_en";
This seems device-dependent, or at least POSIX-specific, yet the file is being
compiled on all platforms including Windows.

http://codereview.chromium.org/7273073/diff/1/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7273073/diff/1/views/view.cc#newcode45
views/view.cc:45: //#if !defined(OS_CHROMEOS)
Should these commented-out lines be removed altogether?

http://codereview.chromium.org/7273073/diff/1/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/1/views/view.h#newcode250
views/view.h:250: //TODO(vollick) return a ref-counted lock that releases
painting when destroyed.
+80

[cwolfe, 2011-06-30 20:35:09]

On 2011/06/30 19:50:50, Jói wrote:
> I mainly have a high-level question:  Does knowledge of sensors belong under
> content/ ?  The content/ directory is intended to contain everything you need
to
> run a multi-process browser, and it seems sensors perhaps either belong at a
> lower level (i.e. the web platform should know about and access them, so they
go
> in WebKit somewhere) or a higher level (i.e. they're used for Chrome's UI, so
> they should be in chrome/, not content/.
>
> What do you think?

I am picturing "sensors" as a generalization of passive(ish) devices, like the
existing content/browser/device_orientation and content/browser/geolocation.
jam@ moved those two from chrome/ back in Feb-Mar, so hopefully content/ is the
right place.

The ugly and platform-specific hacks ("/sys/bus/iio/...") are definitely getting
pulled out. My intent is to move such low-level stuff into a loosely-coupled
daemon. The first cut will be platform-specific, but at least the hacks will be
outside the main tree with a clear interface.

Chris

[Jói, 2011-06-30 20:37:24]

Thanks Chris, that sounds sensible.

On Thu, Jun 30, 2011 at 4:35 PM,  <cwolfe@chromium.org> wrote:
> On 2011/06/30 19:50:50, Jói wrote:
>>
>> I mainly have a high-level question:  Does knowledge of sensors belong
>> under
>> content/ ?  The content/ directory is intended to contain everything you
>> need
>
> to
>>
>> run a multi-process browser, and it seems sensors perhaps either belong at
>> a
>> lower level (i.e. the web platform should know about and access them, so
>> they
>
> go
>>
>> in WebKit somewhere) or a higher level (i.e. they're used for Chrome's UI,
>> so
>> they should be in chrome/, not content/.
>
>> What do you think?
>
> I am picturing "sensors" as a generalization of passive(ish) devices, like
> the
> existing content/browser/device_orientation and content/browser/geolocation.
> jam@ moved those two from chrome/ back in Feb-Mar, so hopefully content/ is
> the
> right place.
>
> The ugly and platform-specific hacks ("/sys/bus/iio/...") are definitely
> getting
> pulled out. My intent is to move such low-level stuff into a loosely-coupled
> daemon. The first cut will be platform-specific, but at least the hacks will
> be
> outside the main tree with a clear interface.
>
> Chris
>
> http://codereview.chromium.org/7273073/
>

[Ian Vollick, 2011-07-05 20:37:50]

Still far from submittable, but the power button is no longer hijacked.

On 2011/06/30 20:37:24, Jói wrote:
> Thanks Chris, that sounds sensible.
> 
> On Thu, Jun 30, 2011 at 4:35 PM,  <mailto:cwolfe@chromium.org> wrote:
> > On 2011/06/30 19:50:50, Jói wrote:
> >>
> >> I mainly have a high-level question: &nbsp;Does knowledge of sensors belong
> >> under
> >> content/ ? &nbsp;The content/ directory is intended to contain everything
you
> >> need
> >
> > to
> >>
> >> run a multi-process browser, and it seems sensors perhaps either belong at
> >> a
> >> lower level (i.e. the web platform should know about and access them, so
> >> they
> >
> > go
> >>
> >> in WebKit somewhere) or a higher level (i.e. they're used for Chrome's UI,
> >> so
> >> they should be in chrome/, not content/.
> >
> >> What do you think?
> >
> > I am picturing "sensors" as a generalization of passive(ish) devices, like
> > the
> > existing content/browser/device_orientation and content/browser/geolocation.
> > jam@ moved those two from chrome/ back in Feb-Mar, so hopefully content/ is
> > the
> > right place.
> >
> > The ugly and platform-specific hacks ("/sys/bus/iio/...") are definitely
> > getting
> > pulled out. My intent is to move such low-level stuff into a loosely-coupled
> > daemon. The first cut will be platform-specific, but at least the hacks will
> > be
> > outside the main tree with a clear interface.
> >
> > Chris
> >
> > http://codereview.chromium.org/7273073/
> >

[Ian Vollick, 2011-07-14 16:47:58]

Uses a new, minimal API from: http://codereview.chromium.org/7366011/

[sadrul, 2011-07-15 15:25:48]

Some nits. I didn't look at the content/ code (are there are additional changes
in there on top of cwolfe's CL?)

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:55: class
TouchBrowserFrameRotation : public ui::AnimationDelegate {
This probably should be in the anonymous namespace.

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:67: : root_(root),
4-space indent

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:106: case
sensors::ScreenOrientation::RIGHT: return 90;
indentations!

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:133: case 90:
ditto

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:507: views::Widget*
widget = GetWidget();
Please try this with --views-desktop and see if this needs to change.

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right):

http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.h:16: #include
"ui/base/animation/slide_animation.h"
Forward declaration is generally preferred over including headers in the style
guide, I believe.

http://codereview.chromium.org/7273073/diff/15001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/15001/views/view.h#newcode1420
views/view.h:1420: // Whether this view can be painted
Perhaps leave a note explaining it can be used during animation etc.?

[Ian Vollick, 2011-07-19 21:40:11]

On 2011/07/15 15:25:48, sadrul wrote:
> Some nits. I didn't look at the content/ code (are there are additional
changes
> in there on top of cwolfe's CL?)
> 

Nope -- removed from CL.

>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):
> 
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:55: class
> TouchBrowserFrameRotation : public ui::AnimationDelegate {
> This probably should be in the anonymous namespace.
> 
Moved into views::
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:67: : root_(root),
> 4-space indent
> 
Done.
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:106: case
> sensors::ScreenOrientation::RIGHT: return 90;
> indentations!
> 
Done.
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:133: case 90:
> ditto
> 
Done.
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:507: views::Widget*
> widget = GetWidget();
> Please try this with --views-desktop and see if this needs to change.
> 
Works with --views-desktop
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/15001/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.h:16: #include
> "ui/base/animation/slide_animation.h"
> Forward declaration is generally preferred over including headers in the style
> guide, I believe.
> 

Whoops. Reverted.

> http://codereview.chromium.org/7273073/diff/15001/views/view.h
> File views/view.h (right):
> 
> http://codereview.chromium.org/7273073/diff/15001/views/view.h#newcode1420
> views/view.h:1420: // Whether this view can be painted
> Perhaps leave a note explaining it can be used during animation etc.?

Done.

[cwolfe, 2011-07-21 17:47:01]

LGTM once sadrul has re-landed 7366011.

http://codereview.chromium.org/7273073/diff/30011/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7273073/diff/30011/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:393:
SideToDegrees(change.upward));
ScreenOrientation::FRONT and ::BACK should probably leave the screen at whatever
the current rotation is. Believe this code will snap back to 0 rotation whenever
the sensor reports "face down" or "face up".

Can call this a nit, since the demo won't generate either FRONT or BACK.

[Ian Vollick, 2011-07-21 19:30:44]

On 2011/07/21 17:47:01, cwolfe wrote:
> LGTM once sadrul has re-landed 7366011.
> 
>
http://codereview.chromium.org/7273073/diff/30011/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):
> 
>
http://codereview.chromium.org/7273073/diff/30011/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:393:
> SideToDegrees(change.upward));
> ScreenOrientation::FRONT and ::BACK should probably leave the screen at
whatever
> the current rotation is. Believe this code will snap back to 0 rotation
whenever
> the sensor reports "face down" or "face up".
> 
> Can call this a nit, since the demo won't generate either FRONT or BACK.

Fixed. If we get an unexpected screen orientation, we rotate to the same angle.
Added a check in ScreenRotation to do nothing in this case.

[tfarina, 2011-07-21 21:56:48]

http://codereview.chromium.org/7273073/diff/34001/views/animation/screen_rota...
File views/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/34001/views/animation/screen_rota...
views/animation/screen_rotation.h:27: static void Perform(views::View* root,
float old_degrees, float new_degrees);
views:: is not necessary here and other places since this in the namespace views
already.

[tfarina, 2011-07-21 21:58:31]

http://codereview.chromium.org/7273073/diff/34001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode254
views/view.h:254: void SetPaintingEnabled(bool enabled);
this should have been set_painting_enabled

http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode257
views/view.h:257: bool IsPaintingEnabled() const;
and this just painting_enabled()

And both inlined here.

[Ian Vollick, 2011-07-22 14:21:11]

Got rid of camel case for trivial getters and setters:
  SetPaintingEnabled -> set_painting_enabled
  IsPaintingEnabled -> painting_enabled

Removed views:: namespace qualifications when already in the views namespace. 

On 2011/07/21 21:58:31, tfarina wrote:
> http://codereview.chromium.org/7273073/diff/34001/views/view.h
> File views/view.h (right):
> 
> http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode254
> views/view.h:254: void SetPaintingEnabled(bool enabled);
> this should have been set_painting_enabled
> 
> http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode257
> views/view.h:257: bool IsPaintingEnabled() const;
> and this just painting_enabled()
> 
> And both inlined here.

[Ian Vollick, 2011-07-22 14:32:43]

Sorry for the spam -- set_painting_enabled and painting_enabled are now inlined.

On 2011/07/22 14:21:11, vollick wrote:
> Got rid of camel case for trivial getters and setters:
>   SetPaintingEnabled -> set_painting_enabled
>   IsPaintingEnabled -> painting_enabled
> 
> Removed views:: namespace qualifications when already in the views namespace. 
> 
> On 2011/07/21 21:58:31, tfarina wrote:
> > http://codereview.chromium.org/7273073/diff/34001/views/view.h
> > File views/view.h (right):
> > 
> > http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode254
> > views/view.h:254: void SetPaintingEnabled(bool enabled);
> > this should have been set_painting_enabled
> > 
> > http://codereview.chromium.org/7273073/diff/34001/views/view.h#newcode257
> > views/view.h:257: bool IsPaintingEnabled() const;
> > and this just painting_enabled()
> > 
> > And both inlined here.

[tfarina, 2011-07-22 14:38:17]

http://codereview.chromium.org/7273073/diff/42001/views/animation/screen_rota...
File views/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/42001/views/animation/screen_rota...
views/animation/screen_rotation.h:12: #include "content/common/sensors.h"
I don't think views/ can depend on content/. Also I don't see the usage of these
includes in this header. Can you move them where they are used?

[Ian Vollick, 2011-07-22 14:48:40]

On 2011/07/22 14:38:17, tfarina wrote:
>
http://codereview.chromium.org/7273073/diff/42001/views/animation/screen_rota...
> File views/animation/screen_rotation.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/42001/views/animation/screen_rota...
> views/animation/screen_rotation.h:12: #include "content/common/sensors.h"
> I don't think views/ can depend on content/. Also I don't see the usage of
these
> includes in this header. Can you move them where they are used?

Done.

[cwolfe, 2011-07-22 14:50:41]

On 2011/07/22 14:38:17, tfarina wrote:
> I don't think views/ can depend on content/.

Any suggestions on where to put the sensors definitions (currently in
content/common) such that they can be shared among content/views/etc? I suppose
I could pull them out into third_party, which would be a slightly finer-grained
dependency.

[vollick1, 2011-07-22 15:11:10]

I haven't had any problems with where you've put them. Screen rotation is
now in view/animation so that it can be used in views-specific code
(possibly to rotate the views desktop, for example), but it knows nothing
about sensors or sides -- it deals in degrees only. The code that listens
for orientation changes and triggers the rotation is in the browser and is
allowed to depend on content/. I've run checkdeps.py and it succeeded.

On Fri, Jul 22, 2011 at 10:50 AM, <cwolfe@chromium.org> wrote:

> On 2011/07/22 14:38:17, tfarina wrote:
>
>> I don't think views/ can depend on content/.
>>
>
> Any suggestions on where to put the sensors definitions (currently in
> content/common) such that they can be shared among content/views/etc? I
> suppose
> I could pull them out into third_party, which would be a slightly
> finer-grained
> dependency.
>
>
>
>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>

[Ian Vollick, 2011-07-27 16:37:42]

+sky

[Ian Vollick, 2011-07-27 16:43:16]

Updated subject and description.

[sky, 2011-07-27 22:16:19]

http://codereview.chromium.org/7273073/diff/40014/views/animation/screen_rota...
File views/animation/screen_rotation.cc (right):

http://codereview.chromium.org/7273073/diff/40014/views/animation/screen_rota...
views/animation/screen_rotation.cc:34: : root_(root),
This will crash if the view is deleted before the animation completes.

http://codereview.chromium.org/7273073/diff/40014/views/animation/screen_rota...
views/animation/screen_rotation.cc:50:
root_->SetTransform(interpolated_transform_->Interpolate(t));
Animations should be handled at the layer/texture level and independent of
views. That way they can easily be handled by another thread/process. This is
the point of the LayerPropertySetter class.

http://codereview.chromium.org/7273073/diff/40014/views/animation/screen_rota...
File views/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/40014/views/animation/screen_rota...
views/animation/screen_rotation.h:23: class ScreenRotation : public
ui::AnimationDelegate {
Please add a description

http://codereview.chromium.org/7273073/diff/40014/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/40014/views/view.h#newcode254
views/view.h:254: void set_painting_enabled(bool enabled) { painting_enabled_ =
enabled; }
This makes painting that much more complicated. See if you can implement the
functionality you want without this.

[Ian Vollick, 2011-08-15 21:07:58]

Reimplement screen rotation in terms of layer animations (rather than animating
the view).

[sky, 2011-08-17 16:39:42]

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1517: }
else NOTREACHED?

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1519: host = chrome::kChromeUIBlankHost;
Add a comment as to why you need to reset the host/url

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:382: 
nit: remove this line.

http://codereview.chromium.org/7273073/diff/50001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:393:
to_rotate->EnqueueAnimation(new views::ScreenRotation(to_rotate,
Where is EnqueueAnimation defined?

http://codereview.chromium.org/7273073/diff/50001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation.h (right):

http://codereview.chromium.org/7273073/diff/50001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation.h:63: int duration() { return duration_; }
newline between 62 and 63. Make 63 const.

http://codereview.chromium.org/7273073/diff/50001/views/animation/screen_rota...
File views/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/50001/views/animation/screen_rota...
views/animation/screen_rotation.h:23: class ScreenRotation : public
ui::LayerAnimation {
Have the changes to layeranimation landed?

http://codereview.chromium.org/7273073/diff/50001/views/view.cc
File views/view.cc (left):

http://codereview.chromium.org/7273073/diff/50001/views/view.cc#oldcode437
views/view.cc:437: if (!ShouldPaintToLayer())
How come you're removing this case?

http://codereview.chromium.org/7273073/diff/50001/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7273073/diff/50001/views/view.cc#newcode696
views/view.cc:696: if (parent_) {
no {}

http://codereview.chromium.org/7273073/diff/50001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/50001/views/view.h#newcode263
views/view.h:263: // Sets whether this view is painting.
Is this still needed? If so, what case does it cover?

http://codereview.chromium.org/7273073/diff/50001/views/view.h#newcode472
views/view.h:472: virtual void ScheduleComposite();
Description?

http://codereview.chromium.org/7273073/diff/50001/views/widget/root_view.cc
File views/widget/root_view.cc (right):

http://codereview.chromium.org/7273073/diff/50001/views/widget/root_view.cc#n...
views/widget/root_view.cc:160:
widget_->SchedulePaintInRect(widget_->GetClientAreaScreenBounds());
Why is this asking to paint in terms of screen coordinates?

http://codereview.chromium.org/7273073/diff/50001/views/widget/root_view.cc#n...
views/widget/root_view.cc:171: if (!invalid_rect.IsEmpty()) {
no {}

[Ian Vollick, 2011-08-24 22:30:16]

Hi Scott,

I've reworked the design as per our IM discussion. The TouchBrowserFrameView now
installs a special layer property setter that performs custom transitions upon
calls to SetTransform.

I haven't yet plumbed the view down into the existing property setters because
it seemed nice to me that they could operate with no knowledge of the view (and
hopefully this would encourage people to avoid owning/accessing views in future
subclasses of LayerPropertySetter). Please let me know if you'd prefer me to do
the plumbing.

Lots of the comments no longer apply, but I've tried to address those that do.

Thanks,

Ian

On 2011/08/17 16:39:42, sky wrote:
>
http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
> File chrome/browser/browser_about_handler.cc (right):
> 
>
http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
> chrome/browser/browser_about_handler.cc:1517: }
> else NOTREACHED?
Done.
> 
>
http://codereview.chromium.org/7273073/diff/50001/chrome/browser/browser_abou...
> chrome/browser/browser_about_handler.cc:1519: host =
chrome::kChromeUIBlankHost;
> Add a comment as to why you need to reset the host/url
Done.
> 
>
http://codereview.chromium.org/7273073/diff/50001/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):
> 
> http://codereview.chromium.org/7273073/diff/50001/views/view.h#newcode263
> views/view.h:263: // Sets whether this view is painting.
> Is this still needed? If so, what case does it cover?
My goal was to prevent repainting chrome during an animation. This was causing
me jankiness in the animations.
>

[sky, 2011-08-25 03:00:13]

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
File views/animation/screen_rotation.cc (right):

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:18: const int kDefaultTransitionDurationMs =
500;
500ms is pretty long for an animation, are you sure you want that?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:25: // ScreenRotation private:
private -> public

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:43: animation_.reset();
Why do you need to explicitly stop the animation here? If you do need this,
please document why. Also, what if the animation is running and you've marked
the view painting_enabled(false). Should it be reset?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:47: done_ = true;
DCHECK(!done_) ?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:68: view_->set_painting_enabled(false);
Why do you need to disable painting here? Assuming you need this, do you really
need to check the current value?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:98:
method_factory_.NewRunnableMethod(&ScreenRotation::Finalize),
ick. This seems easy to mess up, but I don't currently have a better idea. Maybe
we need a way to listen for painting to finish on the compositor so that post
processing like this has a cleaner way to complete. Can you add a TODO here to
improve this.

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:110: return;
If you early return here or at 121 should the animation stop?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.cc:114: old_transform_.reset(new
ui::Transform(view_->layer()->transform()));
How come you don't always reset old_transform_?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
File views/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.h:33: };
Add a virtual destructor and make it public. This way it's clear ScreenRotation
doesn't own the listener.

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.h:39: class ScreenRotation : public
ui::AnimationDelegate {
These classes (ScreenRotation and ScreenRotationSetter) should live in touch. If
we really need them outside of touch we can promote them.

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation.h:54: void UpdateTarget(float degrees);
nit: Name this SetTarget.

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
File views/animation/screen_rotation_setter.cc (right):

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation_setter.cc:25: : rotation_(NULL),
don't need rotation_

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation_setter.cc:56: ui::Layer* layer, const gfx::Rect&
bounds) {
nit: each param on its own line.

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation_setter.cc:59: return;
Don't you need to cache and apply the bounds change after the animation
completes?

http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
views/animation/screen_rotation_setter.cc:65: DCHECK(rotation_.get() ==
rotation);
nit: DCHECK_EQ

http://codereview.chromium.org/7273073/diff/58039/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7273073/diff/58039/views/view.cc#newcode697
views/view.cc:697: if (!IsVisible() || !painting_enabled_)
I'm still nervous about having this. If we're going to have it though, seems
like schedulepaint and the like should also check it too.

[Ian Vollick, 2011-08-25 20:34:48]

Things to note in this patch:
 - Screen rotation code has been moved to chrome/browser/ui/touch/animation
 - The dangerous function View::set_painting_enabled is now private and is now
controlled by a PaintLock object. This should make it tougher to make a mistake
and leave painting disabled. 

>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:18: const int kDefaultTransitionDurationMs
=
> 500;
> 500ms is pretty long for an animation, are you sure you want that?
It is a bit too long. I've shortened this to 350ms for 90 degree rotations and
550ms for 180 degree rotations and it feels ok.
> 
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:25: // ScreenRotation private:
> private -> public
> 
Fixed
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:43: animation_.reset();
> Why do you need to explicitly stop the animation here? If you do need this,
> please document why. Also, what if the animation is running and you've marked
> the view painting_enabled(false). Should it be reset?
> 
This was not necessary and is removed. In this CL painting is now
enabled/disabled using a lock object and so painting is automatically disabled
when the screen rotation animation is destroyed (happens after the transition).

>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:47: done_ = true;
> DCHECK(!done_) ?
> 
ScreenRotation does not need to maintain a done flag anymore (now that bounds
setting is handled by the ScreenRotationSetter).

>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:68: view_->set_painting_enabled(false);
> Why do you need to disable painting here? Assuming you need this, do you
really
> need to check the current value?
Disabling painting is controlled with a PaintLock that is created when the
animation is started and is destroyed with the ScreenRotation.
> 
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:98:
> method_factory_.NewRunnableMethod(&ScreenRotation::Finalize),
> ick. This seems easy to mess up, but I don't currently have a better idea.
Maybe
> we need a way to listen for painting to finish on the compositor so that post
> processing like this has a cleaner way to complete. Can you add a TODO here to
> improve this.
> 
It is unfortunate -- I've added a TODO.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:110: return;
> If you early return here or at 121 should the animation stop?
> 
It should. The Start function now aborts if Init fails and there is no
interpolated transform.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.cc:114: old_transform_.reset(new
> ui::Transform(view_->layer()->transform()));
> How come you don't always reset old_transform_?
> 
Fixed. old_transform_ has been demoted to a local value that is always set.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> File views/animation/screen_rotation.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.h:33: };
> Add a virtual destructor and make it public. This way it's clear
ScreenRotation
> doesn't own the listener.
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.h:39: class ScreenRotation : public
> ui::AnimationDelegate {
> These classes (ScreenRotation and ScreenRotationSetter) should live in touch.
If
> we really need them outside of touch we can promote them.
> 
Fixed. I've moved this code to chrome/browser/ui/touch/animation (new folder).
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation.h:54: void UpdateTarget(float degrees);
> nit: Name this SetTarget.
> 
Fixed.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> File views/animation/screen_rotation_setter.cc (right):
> 
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation_setter.cc:25: : rotation_(NULL),
> don't need rotation_
> 
Removed.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation_setter.cc:56: ui::Layer* layer, const
gfx::Rect&
> bounds) {
> nit: each param on its own line.
> 
Fixed.
>
http://codereview.chromium.org/7273073/diff/58039/views/animation/screen_rota...
> views/animation/screen_rotation_setter.cc:59: return;
> Don't you need to cache and apply the bounds change after the animation
> completes?
> 
Fixed.
>  
> http://codereview.chromium.org/7273073/diff/58039/views/view.cc
> File views/view.cc (right):
> 
> http://codereview.chromium.org/7273073/diff/58039/views/view.cc#newcode697
> views/view.cc:697: if (!IsVisible() || !painting_enabled_)
> I'm still nervous about having this. If we're going to have it though, seems
> like schedulepaint and the like should also check it too.
I've updated SchedulePaintInRect (and consequently SchedulePaint) to check this
flag.

[sky, 2011-08-25 21:10:12]

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
File chrome/browser/ui/touch/animation/screen_rotation.cc (right):

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
chrome/browser/ui/touch/animation/screen_rotation.cc:50: // If we post a delayed
task, it will be in the queue after the paint, and
This is going to be so racy because the paint comes from gtk. You have the todo,
which is fine, but be aware this code is very problematic and we definitely need
to find a better solution.

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
File chrome/browser/ui/touch/animation/screen_rotation.h (right):

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
chrome/browser/ui/touch/animation/screen_rotation.h:17: class SlideAnimation;
nit: sort these

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
chrome/browser/ui/touch/animation/screen_rotation.h:34: class
ScreenRotationListenerInterface {
Nuke the interface, eg ScreenRotationListener, or ScreenRotationObserver.

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
chrome/browser/ui/touch/animation/screen_rotation.h:39: };
Add a virtual protected destructor

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right):

http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.h:119:
views::LayerPropertySetter* setter_;
Initialize this. In fact maybe this should just be a boolean indicating whether
you installed the setter and not the actual setter. Maybe you could install it
in the constructor though and not have to explicitly set it at rotate time. Even
better that way.

http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock.h
File views/animation/paint_lock.h (right):

http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
views/animation/paint_lock.h:21: // destroed before the lock.
destroed -> destroyed.

http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
views/animation/paint_lock.h:24: private:
nit: newline between 23 and 24.

http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
views/animation/paint_lock.h:26: };
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/7273073/diff/63002/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7273073/diff/63002/views/view.cc#newcode699
views/view.cc:699: if (!IsVisible() || !painting_enabled_)
You also need to update SchedulePaintInternal.

[Ian Vollick, 2011-08-26 18:47:39]

Noteworthy change:
 - I added a PaintListener class that can be installed on a view if you're
interested in being notified when painting or compositing finishes.

On 2011/08/25 21:10:12, sky wrote:
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> File chrome/browser/ui/touch/animation/screen_rotation.cc (right):
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> chrome/browser/ui/touch/animation/screen_rotation.cc:50: // If we post a
delayed
> task, it will be in the queue after the paint, and
> This is going to be so racy because the paint comes from gtk. You have the
todo,
> which is fine, but be aware this code is very problematic and we definitely
need
> to find a better solution.
I've added the PaintListener class to let me do my post processing when notified
by the view that painting is done.
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> File chrome/browser/ui/touch/animation/screen_rotation.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> chrome/browser/ui/touch/animation/screen_rotation.h:17: class SlideAnimation;
> nit: sort these
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> chrome/browser/ui/touch/animation/screen_rotation.h:34: class
> ScreenRotationListenerInterface {
> Nuke the interface, eg ScreenRotationListener, or ScreenRotationObserver.
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/ani...
> chrome/browser/ui/touch/animation/screen_rotation.h:39: };
> Add a virtual protected destructor
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/fra...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/63002/chrome/browser/ui/touch/fra...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.h:119:
> views::LayerPropertySetter* setter_;
> Initialize this. In fact maybe this should just be a boolean indicating
whether
> you installed the setter and not the actual setter. Maybe you could install it
> in the constructor though and not have to explicitly set it at rotate time.
Even
> better that way.
Fixed. I don't have access to the view I need in the constructor, so I have gone
with the boolean instead.
> 
> http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock.h
> File views/animation/paint_lock.h (right):
> 
>
http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
> views/animation/paint_lock.h:21: // destroed before the lock.
> destroed -> destroyed.
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
> views/animation/paint_lock.h:24: private:
> nit: newline between 23 and 24.
Fixed.
> 
>
http://codereview.chromium.org/7273073/diff/63002/views/animation/paint_lock....
> views/animation/paint_lock.h:26: };
> DISALLOW_COPY_AND_ASSIGN.
Fixed.
> 
> http://codereview.chromium.org/7273073/diff/63002/views/view.cc
> File views/view.cc (right):
> 
> http://codereview.chromium.org/7273073/diff/63002/views/view.cc#newcode699
> views/view.cc:699: if (!IsVisible() || !painting_enabled_)
> You also need to update SchedulePaintInternal.
Fixed.

[sky, 2011-08-26 19:10:24]

http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.cc
File views/paint_listener.cc (right):

http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.cc#new...
views/paint_listener.cc:9: PaintListener::~PaintListener() {
It's ok to inline this in the header.

http://codereview.chromium.org/7273073/diff/71001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/71001/views/view.h#newcode1402
views/view.h:1402: std::set<PaintListener*> paint_listeners_;
Use ObserverList (in base) for this.

I don't like making all views pay the cost of this. How about making it only for
views with layers and moving it to LayerHelper?

[Ben Goodger (Google), 2011-08-26 19:30:00]

Can you provide a succinct description of the design of this feature?

I have a couple of comments below, but I my understanding would be greatly
enhanced by an overview.

http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1496: } else if (host ==
chrome::kChromeUIRotateHost) {
Is this for testing only or is this intended for some purpose in production? If
so I think we are going to need a wider reviewer set.

http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h
File views/paint_listener.h (right):

http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newc...
views/paint_listener.h:17: virtual void OnPainted(View* view) = 0;
From your code, it doesn't appear that the OnPainted notification is actually
used by anyone.

http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newc...
views/paint_listener.h:18: virtual void OnComposited(View* view) = 0;
Looking at what you're using this for, it seems like this could be added to the
WidgetDelegate instead, rather than adding this API overhead to View.

[vollick1, 2011-08-26 20:58:20]

Hi Ben,

I've posted a very high level document here.
https://docs.google.com/a/google.com/document/d/1vgFAGy_TaOYakNsLsW13PB20-FSI...
let me know if you'd like more detail.

There were two reasons for the about handler: to provide a way to trigger
orientation changes until the sensors are working, and so people who aren't
building for touch can verify that rotation has not regressed. I could
certainly #ifdef it out in release builds.

About the PaintListener -- I hope to take sky@'s advice and move this into
Layer. When I do, I will also remove the unused OnPaint method. Does that
sound alright?

Ian

On Fri, Aug 26, 2011 at 3:30 PM, <ben@chromium.org> wrote:

> Can you provide a succinct description of the design of this feature?
>
> I have a couple of comments below, but I my understanding would be greatly
> enhanced by an overview.
>
>
> http://codereview.chromium.**org/7273073/diff/71001/chrome/**
>
browser/browser_about_handler.**cc<http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_about_handler.cc>
> File chrome/browser/browser_about_**handler.cc (right):
>
> http://codereview.chromium.**org/7273073/diff/71001/chrome/**
>
browser/browser_about_handler.**cc#newcode1496<http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_about_handler.cc#newcode1496>
> chrome/browser/browser_about_**handler.cc:1496: } else if (host ==
> chrome::kChromeUIRotateHost) {
> Is this for testing only or is this intended for some purpose in
> production? If so I think we are going to need a wider reviewer set.
>
> http://codereview.chromium.**org/7273073/diff/71001/views/**
>
paint_listener.h<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h>
> File views/paint_listener.h (right):
>
> http://codereview.chromium.**org/7273073/diff/71001/views/**
>
paint_listener.h#newcode17<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newcode17>
> views/paint_listener.h:17: virtual void OnPainted(View* view) = 0;
> From your code, it doesn't appear that the OnPainted notification is
> actually used by anyone.
>
> http://codereview.chromium.**org/7273073/diff/71001/views/**
>
paint_listener.h#newcode18<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newcode18>
> views/paint_listener.h:18: virtual void OnComposited(View* view) = 0;
> Looking at what you're using this for, it seems like this could be added
> to the WidgetDelegate instead, rather than adding this API overhead to
> View.
>
>
>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>

[Ben Goodger (Google), 2011-08-30 17:21:18]

Thanks for the explanation. When I spoke to sky@ he liked my idea of moving
the remaining notification to the WidgetDelegate. This ends up being
implemented by something close to your TouchBrowserFrameView (in this case
the BrowserView, but I imagine there's a touch specialization of that too??)

-Ben

On Fri, Aug 26, 2011 at 1:58 PM, Ian Vollick <vollick@google.com> wrote:

> Hi Ben,
>
> I've posted a very high level document here.
>
https://docs.google.com/a/google.com/document/d/1vgFAGy_TaOYakNsLsW13PB20-FSI...
let me know if you'd like more detail.
>
> There were two reasons for the about handler: to provide a way to trigger
> orientation changes until the sensors are working, and so people who aren't
> building for touch can verify that rotation has not regressed. I could
> certainly #ifdef it out in release builds.
>
> About the PaintListener -- I hope to take sky@'s advice and move this into
> Layer. When I do, I will also remove the unused OnPaint method. Does that
> sound alright?
>
> Ian
>
> On Fri, Aug 26, 2011 at 3:30 PM, <ben@chromium.org> wrote:
>
>> Can you provide a succinct description of the design of this feature?
>>
>> I have a couple of comments below, but I my understanding would be greatly
>> enhanced by an overview.
>>
>>
>> http://codereview.chromium.**org/7273073/diff/71001/chrome/**
>>
browser/browser_about_handler.**cc<http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_about_handler.cc>
>> File chrome/browser/browser_about_**handler.cc (right):
>>
>> http://codereview.chromium.**org/7273073/diff/71001/chrome/**
>>
browser/browser_about_handler.**cc#newcode1496<http://codereview.chromium.org/7273073/diff/71001/chrome/browser/browser_about_handler.cc#newcode1496>
>> chrome/browser/browser_about_**handler.cc:1496: } else if (host ==
>> chrome::kChromeUIRotateHost) {
>> Is this for testing only or is this intended for some purpose in
>> production? If so I think we are going to need a wider reviewer set.
>>
>> http://codereview.chromium.**org/7273073/diff/71001/views/**
>>
paint_listener.h<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h>
>> File views/paint_listener.h (right):
>>
>> http://codereview.chromium.**org/7273073/diff/71001/views/**
>>
paint_listener.h#newcode17<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newcode17>
>> views/paint_listener.h:17: virtual void OnPainted(View* view) = 0;
>> From your code, it doesn't appear that the OnPainted notification is
>> actually used by anyone.
>>
>> http://codereview.chromium.**org/7273073/diff/71001/views/**
>>
paint_listener.h#newcode18<http://codereview.chromium.org/7273073/diff/71001/views/paint_listener.h#newcode18>
>> views/paint_listener.h:18: virtual void OnComposited(View* view) = 0;
>> Looking at what you're using this for, it seems like this could be added
>> to the WidgetDelegate instead, rather than adding this API overhead to
>> View.
>>
>>
>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>
>
>

[Ben Goodger (Google), 2011-08-30 17:40:12]

I think at some point we should investigate the event system in views... I can
see there being the need to register per-widget (or even global) pre- and post-
process filters, much like the accelerator handler. It bothers me that these
events are tied so much to WidgetDelegate, but that is the way it is right now
so I'd rather you plumb through that vs. adding more interfaces.

[vollick1, 2011-08-30 18:57:45]

Hi Ben,

Just to summarize the problem: the new class ScreenRotation needs to be
notified when compositing finishes.

Before I start working on the WidgetDelegate solution to the problem, I was
hoping to clarify what you had in mind. As I understand it, this approach
would involve the following (please correct me if I've got this wrong).
 - Adding a method along the lines of OnComposited to WidgetDelegate
 - Using a subclass of BrowserView in touch (TouchBrowserView, say) that
overrides OnComposited
 - TouchBrowserView would need to be able to accept observers implementing a
new interface that includes a method like NotifyComposited or OnComposited
 - The ScreenRotationSetter would hold onto a pointer to the
TouchBrowserView that it would pass to the ScreenRotations as they were
created.
 - ScreenRotations would add and remove themselves as observers of the
TouchBrowserView when created/destroyed.

Before your last two responses, I'd already started down the road of moving
the compositing notifications out of views and into ui/gfx/compositor/
instead. I've uploaded this change for reference.

Thanks,

Ian

On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:

> I think at some point we should investigate the event system in views... I
> can
> see there being the need to register per-widget (or even global) pre- and
> post-
> process filters, much like the accelerator handler. It bothers me that
> these
> events are tied so much to WidgetDelegate, but that is the way it is right
> now
> so I'd rather you plumb through that vs. adding more interfaces.
>
>
>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>

[Ben Goodger (Google), 2011-08-30 19:02:18]

Naming convention - whatever the method name is in
NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on NativeWidgetDelegate.
Widget implements this method and notifies its delegate.

Do you not already have a BrowserView subclass? I think cros has one. I had
assumed you had one too.

-Ben

On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com> wrote:

> Hi Ben,
>
> Just to summarize the problem: the new class ScreenRotation needs to be
> notified when compositing finishes.
>
> Before I start working on the WidgetDelegate solution to the problem, I was
> hoping to clarify what you had in mind. As I understand it, this approach
> would involve the following (please correct me if I've got this wrong).
>  - Adding a method along the lines of OnComposited to WidgetDelegate
>  - Using a subclass of BrowserView in touch (TouchBrowserView, say) that
> overrides OnComposited
>  - TouchBrowserView would need to be able to accept observers implementing
> a new interface that includes a method like NotifyComposited or OnComposited
>  - The ScreenRotationSetter would hold onto a pointer to the
> TouchBrowserView that it would pass to the ScreenRotations as they were
> created.
>  - ScreenRotations would add and remove themselves as observers of the
> TouchBrowserView when created/destroyed.
>
> Before your last two responses, I'd already started down the road of moving
> the compositing notifications out of views and into ui/gfx/compositor/
> instead. I've uploaded this change for reference.
>
> Thanks,
>
> Ian
>
>
> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>
>> I think at some point we should investigate the event system in views... I
>> can
>> see there being the need to register per-widget (or even global) pre- and
>> post-
>> process filters, much like the accelerator handler. It bothers me that
>> these
>> events are tied so much to WidgetDelegate, but that is the way it is right
>> now
>> so I'd rather you plumb through that vs. adding more interfaces.
>>
>>
>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>
>
>

[vollick1, 2011-08-30 19:39:10]

Yes, I believe that cros has two subclasses, one also named BrowserView and
another named PanelBrowserView. I think that PanelBrowserView is used for
touch. I'd assumed I'd need to create a new class for touch that could
accept listeners to avoid adding cruft to PanelBrowserView (and static cast
from BrowserView to TouchBrowserView when it is passed to the
ScreenRotationListener).

Ian

On Tue, Aug 30, 2011 at 3:02 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Naming convention - whatever the method name is in
> NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on NativeWidgetDelegate.
> Widget implements this method and notifies its delegate.
>
> Do you not already have a BrowserView subclass? I think cros has one. I had
> assumed you had one too.
>
> -Ben
>
>
> On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com> wrote:
>
>> Hi Ben,
>>
>> Just to summarize the problem: the new class ScreenRotation needs to be
>> notified when compositing finishes.
>>
>> Before I start working on the WidgetDelegate solution to the problem, I
>> was hoping to clarify what you had in mind. As I understand it, this
>> approach would involve the following (please correct me if I've got this
>> wrong).
>>  - Adding a method along the lines of OnComposited to WidgetDelegate
>>  - Using a subclass of BrowserView in touch (TouchBrowserView, say) that
>> overrides OnComposited
>>  - TouchBrowserView would need to be able to accept observers implementing
>> a new interface that includes a method like NotifyComposited or OnComposited
>>  - The ScreenRotationSetter would hold onto a pointer to the
>> TouchBrowserView that it would pass to the ScreenRotations as they were
>> created.
>>  - ScreenRotations would add and remove themselves as observers of the
>> TouchBrowserView when created/destroyed.
>>
>> Before your last two responses, I'd already started down the road of
>> moving the compositing notifications out of views and into
>> ui/gfx/compositor/ instead. I've uploaded this change for reference.
>>
>> Thanks,
>>
>> Ian
>>
>>
>> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>>
>>> I think at some point we should investigate the event system in views...
>>> I can
>>> see there being the need to register per-widget (or even global) pre- and
>>> post-
>>> process filters, much like the accelerator handler. It bothers me that
>>> these
>>> events are tied so much to WidgetDelegate, but that is the way it is
>>> right now
>>> so I'd rather you plumb through that vs. adding more interfaces.
>>>
>>>
>>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>>
>>
>>
>

[Ben Goodger (Google), 2011-08-30 19:46:55]

Since the BrowserView is the WidgetDelegate, it is the preferred cruft
collector :-)

-Ben

On Tue, Aug 30, 2011 at 12:39 PM, Ian Vollick <vollick@google.com> wrote:

> Yes, I believe that cros has two subclasses, one also named BrowserView and
> another named PanelBrowserView. I think that PanelBrowserView is used for
> touch. I'd assumed I'd need to create a new class for touch that could
> accept listeners to avoid adding cruft to PanelBrowserView (and static cast
> from BrowserView to TouchBrowserView when it is passed to the
> ScreenRotationListener).
>
> Ian
>
>
> On Tue, Aug 30, 2011 at 3:02 PM, Ben Goodger (Google) <ben@chromium.org>wrote:
>
>> Naming convention - whatever the method name is in
>> NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on NativeWidgetDelegate.
>> Widget implements this method and notifies its delegate.
>>
>> Do you not already have a BrowserView subclass? I think cros has one. I
>> had assumed you had one too.
>>
>> -Ben
>>
>>
>> On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com> wrote:
>>
>>> Hi Ben,
>>>
>>> Just to summarize the problem: the new class ScreenRotation needs to be
>>> notified when compositing finishes.
>>>
>>> Before I start working on the WidgetDelegate solution to the problem, I
>>> was hoping to clarify what you had in mind. As I understand it, this
>>> approach would involve the following (please correct me if I've got this
>>> wrong).
>>>  - Adding a method along the lines of OnComposited to WidgetDelegate
>>>  - Using a subclass of BrowserView in touch (TouchBrowserView, say) that
>>> overrides OnComposited
>>>  - TouchBrowserView would need to be able to accept observers
>>> implementing a new interface that includes a method like NotifyComposited or
>>> OnComposited
>>>  - The ScreenRotationSetter would hold onto a pointer to the
>>> TouchBrowserView that it would pass to the ScreenRotations as they were
>>> created.
>>>  - ScreenRotations would add and remove themselves as observers of the
>>> TouchBrowserView when created/destroyed.
>>>
>>> Before your last two responses, I'd already started down the road of
>>> moving the compositing notifications out of views and into
>>> ui/gfx/compositor/ instead. I've uploaded this change for reference.
>>>
>>> Thanks,
>>>
>>> Ian
>>>
>>>
>>> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>>>
>>>> I think at some point we should investigate the event system in views...
>>>> I can
>>>> see there being the need to register per-widget (or even global) pre-
>>>> and post-
>>>> process filters, much like the accelerator handler. It bothers me that
>>>> these
>>>> events are tied so much to WidgetDelegate, but that is the way it is
>>>> right now
>>>> so I'd rather you plumb through that vs. adding more interfaces.
>>>>
>>>>
>>>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>>>
>>>
>>>
>>
>

[vollick1, 2011-08-31 14:21:04]

Would it be alright if I were to implement the WidgetDelegate change in a
separate changelist (and add a TODO in this CL)? I think that the
WidgetDelegate change will be non-trivial, and this CL has grown quite a bit
already.

Thanks,

Ian

On Tue, Aug 30, 2011 at 3:46 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Since the BrowserView is the WidgetDelegate, it is the preferred cruft
> collector :-)
>
> -Ben
>
>
> On Tue, Aug 30, 2011 at 12:39 PM, Ian Vollick <vollick@google.com> wrote:
>
>> Yes, I believe that cros has two subclasses, one also named BrowserView
>> and another named PanelBrowserView. I think that PanelBrowserView is used
>> for touch. I'd assumed I'd need to create a new class for touch that could
>> accept listeners to avoid adding cruft to PanelBrowserView (and static cast
>> from BrowserView to TouchBrowserView when it is passed to the
>> ScreenRotationListener).
>>
>> Ian
>>
>>
>> On Tue, Aug 30, 2011 at 3:02 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> Naming convention - whatever the method name is in
>>> NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on NativeWidgetDelegate.
>>> Widget implements this method and notifies its delegate.
>>>
>>> Do you not already have a BrowserView subclass? I think cros has one. I
>>> had assumed you had one too.
>>>
>>> -Ben
>>>
>>>
>>> On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com>wrote:
>>>
>>>> Hi Ben,
>>>>
>>>> Just to summarize the problem: the new class ScreenRotation needs to be
>>>> notified when compositing finishes.
>>>>
>>>> Before I start working on the WidgetDelegate solution to the problem, I
>>>> was hoping to clarify what you had in mind. As I understand it, this
>>>> approach would involve the following (please correct me if I've got this
>>>> wrong).
>>>>  - Adding a method along the lines of OnComposited to WidgetDelegate
>>>>  - Using a subclass of BrowserView in touch (TouchBrowserView, say) that
>>>> overrides OnComposited
>>>>  - TouchBrowserView would need to be able to accept observers
>>>> implementing a new interface that includes a method like NotifyComposited
or
>>>> OnComposited
>>>>  - The ScreenRotationSetter would hold onto a pointer to the
>>>> TouchBrowserView that it would pass to the ScreenRotations as they were
>>>> created.
>>>>  - ScreenRotations would add and remove themselves as observers of the
>>>> TouchBrowserView when created/destroyed.
>>>>
>>>> Before your last two responses, I'd already started down the road of
>>>> moving the compositing notifications out of views and into
>>>> ui/gfx/compositor/ instead. I've uploaded this change for reference.
>>>>
>>>> Thanks,
>>>>
>>>> Ian
>>>>
>>>>
>>>> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>>>>
>>>>> I think at some point we should investigate the event system in
>>>>> views... I can
>>>>> see there being the need to register per-widget (or even global) pre-
>>>>> and post-
>>>>> process filters, much like the accelerator handler. It bothers me that
>>>>> these
>>>>> events are tied so much to WidgetDelegate, but that is the way it is
>>>>> right now
>>>>> so I'd rather you plumb through that vs. adding more interfaces.
>>>>>
>>>>>
>>>>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>>>>
>>>>
>>>>
>>>
>>
>

[Ben Goodger (Google), 2011-08-31 17:51:42]

I don't want to add these methods to View, even as an intermediate step.

-Ben

On Wed, Aug 31, 2011 at 7:20 AM, Ian Vollick <vollick@google.com> wrote:

> Would it be alright if I were to implement the WidgetDelegate change in a
> separate changelist (and add a TODO in this CL)? I think that the
> WidgetDelegate change will be non-trivial, and this CL has grown quite a bit
> already.
>
> Thanks,
>
> Ian
>
>
> On Tue, Aug 30, 2011 at 3:46 PM, Ben Goodger (Google) <ben@chromium.org>wrote:
>
>> Since the BrowserView is the WidgetDelegate, it is the preferred cruft
>> collector :-)
>>
>> -Ben
>>
>>
>> On Tue, Aug 30, 2011 at 12:39 PM, Ian Vollick <vollick@google.com> wrote:
>>
>>> Yes, I believe that cros has two subclasses, one also named BrowserView
>>> and another named PanelBrowserView. I think that PanelBrowserView is used
>>> for touch. I'd assumed I'd need to create a new class for touch that could
>>> accept listeners to avoid adding cruft to PanelBrowserView (and static cast
>>> from BrowserView to TouchBrowserView when it is passed to the
>>> ScreenRotationListener).
>>>
>>> Ian
>>>
>>>
>>> On Tue, Aug 30, 2011 at 3:02 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>>
>>>> Naming convention - whatever the method name is in
>>>> NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on
NativeWidgetDelegate.
>>>> Widget implements this method and notifies its delegate.
>>>>
>>>> Do you not already have a BrowserView subclass? I think cros has one. I
>>>> had assumed you had one too.
>>>>
>>>> -Ben
>>>>
>>>>
>>>> On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com>wrote:
>>>>
>>>>> Hi Ben,
>>>>>
>>>>> Just to summarize the problem: the new class ScreenRotation needs to be
>>>>> notified when compositing finishes.
>>>>>
>>>>> Before I start working on the WidgetDelegate solution to the problem, I
>>>>> was hoping to clarify what you had in mind. As I understand it, this
>>>>> approach would involve the following (please correct me if I've got this
>>>>> wrong).
>>>>>  - Adding a method along the lines of OnComposited to WidgetDelegate
>>>>>  - Using a subclass of BrowserView in touch (TouchBrowserView,
>>>>> say) that overrides OnComposited
>>>>>  - TouchBrowserView would need to be able to accept observers
>>>>> implementing a new interface that includes a method like NotifyComposited
or
>>>>> OnComposited
>>>>>  - The ScreenRotationSetter would hold onto a pointer to the
>>>>> TouchBrowserView that it would pass to the ScreenRotations as they were
>>>>> created.
>>>>>  - ScreenRotations would add and remove themselves as observers of the
>>>>> TouchBrowserView when created/destroyed.
>>>>>
>>>>> Before your last two responses, I'd already started down the road of
>>>>> moving the compositing notifications out of views and into
>>>>> ui/gfx/compositor/ instead. I've uploaded this change for reference.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Ian
>>>>>
>>>>>
>>>>> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>>>>>
>>>>>> I think at some point we should investigate the event system in
>>>>>> views... I can
>>>>>> see there being the need to register per-widget (or even global) pre-
>>>>>> and post-
>>>>>> process filters, much like the accelerator handler. It bothers me that
>>>>>> these
>>>>>> events are tied so much to WidgetDelegate, but that is the way it is
>>>>>> right now
>>>>>> so I'd rather you plumb through that vs. adding more interfaces.
>>>>>>
>>>>>>
>>>>>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[vollick1, 2011-08-31 18:14:25]

Yes, I agree. At sky@'s suggestion, I had reverted the interface change to
View. The notification code is now in ui/gfx/compositor (with the Layer
code) and does not impact the View interface.

Ian

On Wed, Aug 31, 2011 at 1:51 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> I don't want to add these methods to View, even as an intermediate step.
>
> -Ben
>
>
> On Wed, Aug 31, 2011 at 7:20 AM, Ian Vollick <vollick@google.com> wrote:
>
>> Would it be alright if I were to implement the WidgetDelegate change in a
>> separate changelist (and add a TODO in this CL)? I think that the
>> WidgetDelegate change will be non-trivial, and this CL has grown quite a bit
>> already.
>>
>> Thanks,
>>
>> Ian
>>
>>
>> On Tue, Aug 30, 2011 at 3:46 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> Since the BrowserView is the WidgetDelegate, it is the preferred cruft
>>> collector :-)
>>>
>>> -Ben
>>>
>>>
>>> On Tue, Aug 30, 2011 at 12:39 PM, Ian Vollick <vollick@google.com>wrote:
>>>
>>>> Yes, I believe that cros has two subclasses, one also named BrowserView
>>>> and another named PanelBrowserView. I think that PanelBrowserView is used
>>>> for touch. I'd assumed I'd need to create a new class for touch that could
>>>> accept listeners to avoid adding cruft to PanelBrowserView (and static cast
>>>> from BrowserView to TouchBrowserView when it is passed to the
>>>> ScreenRotationListener).
>>>>
>>>> Ian
>>>>
>>>>
>>>> On Tue, Aug 30, 2011 at 3:02 PM, Ben Goodger (Google) <ben@chromium.org
>>>> > wrote:
>>>>
>>>>> Naming convention - whatever the method name is in
>>>>> NativeWidgetPlatform::OnFoo -> OnNativeWidgetFoo'ed on
NativeWidgetDelegate.
>>>>> Widget implements this method and notifies its delegate.
>>>>>
>>>>> Do you not already have a BrowserView subclass? I think cros has one. I
>>>>> had assumed you had one too.
>>>>>
>>>>> -Ben
>>>>>
>>>>>
>>>>> On Tue, Aug 30, 2011 at 11:57 AM, Ian Vollick <vollick@google.com>wrote:
>>>>>
>>>>>> Hi Ben,
>>>>>>
>>>>>> Just to summarize the problem: the new class ScreenRotation needs to
>>>>>> be notified when compositing finishes.
>>>>>>
>>>>>> Before I start working on the WidgetDelegate solution to the problem,
>>>>>> I was hoping to clarify what you had in mind. As I understand it, this
>>>>>> approach would involve the following (please correct me if I've got this
>>>>>> wrong).
>>>>>>  - Adding a method along the lines of OnComposited to WidgetDelegate
>>>>>>  - Using a subclass of BrowserView in touch (TouchBrowserView,
>>>>>> say) that overrides OnComposited
>>>>>>  - TouchBrowserView would need to be able to accept observers
>>>>>> implementing a new interface that includes a method like NotifyComposited
or
>>>>>> OnComposited
>>>>>>  - The ScreenRotationSetter would hold onto a pointer to the
>>>>>> TouchBrowserView that it would pass to the ScreenRotations as they were
>>>>>> created.
>>>>>>  - ScreenRotations would add and remove themselves as observers of the
>>>>>> TouchBrowserView when created/destroyed.
>>>>>>
>>>>>> Before your last two responses, I'd already started down the road of
>>>>>> moving the compositing notifications out of views and into
>>>>>> ui/gfx/compositor/ instead. I've uploaded this change for reference.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Ian
>>>>>>
>>>>>>
>>>>>> On Tue, Aug 30, 2011 at 1:40 PM, <ben@chromium.org> wrote:
>>>>>>
>>>>>>> I think at some point we should investigate the event system in
>>>>>>> views... I can
>>>>>>> see there being the need to register per-widget (or even global) pre-
>>>>>>> and post-
>>>>>>> process filters, much like the accelerator handler. It bothers me
>>>>>>> that these
>>>>>>> events are tied so much to WidgetDelegate, but that is the way it is
>>>>>>> right now
>>>>>>> so I'd rather you plumb through that vs. adding more interfaces.
>>>>>>>
>>>>>>>
>>>>>>>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[Ben Goodger (Google), 2011-08-31 20:34:00]

LGTM

http://codereview.chromium.org/7273073/diff/79001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/7273073/diff/79001/views/view.h#newcode1234
views/view.h:1234: void set_painting_enabled(bool enabled) { painting_enabled_ =
enabled; }
It troubles me that we need this.

It seems like the widget should just not dispatch paints into the hierarchy when
a time critical operation is underway. The widget can know this because it owns
(or has a link to) the compositor.

Can you add a TODO() to revisit this.

[sky, 2011-08-31 20:35:09]

This now bulks up every Layer for state that you generally only care about at
the top level.

I believe Ben's suggestion is the following: OnPainted or OnComposited to
WidgetDelegate that is invoked when you need it. TouchBrowserView overrides (it
is a WidgetDelegate) this and forwards it to the PropertySetter.

  -Scott

[Ben Goodger (Google), 2011-08-31 20:38:24]

Sky reminded me that we both thought having the observer list at the
compositor-level (or widget-level) was a better place.

-Ben

On Wed, Aug 31, 2011 at 1:35 PM, <sky@chromium.org> wrote:

> This now bulks up every Layer for state that you generally only care about
> at
> the top level.
>
> I believe Ben's suggestion is the following: OnPainted or OnComposited to
> WidgetDelegate that is invoked when you need it. TouchBrowserView overrides
> (it
> is a WidgetDelegate) this and forwards it to the PropertySetter.
>
>  -Scott
>
>
>
http://codereview.chromium.**org/7273073/<http://codereview.chromium.org/7273...
>

[Ian Vollick, 2011-08-31 22:19:57]

I've moved the observer list into the compositor and added a TODO to revisit
paint locking.

Scott, would the current CL be acceptable as a first cut, and have the
WidgetDelegate change in a separate CL?

On 2011/08/31 20:38:24, Ben Goodger (Google) wrote:
> Sky reminded me that we both thought having the observer list at the
> compositor-level (or widget-level) was a better place.
> 
> -Ben
> 
> On Wed, Aug 31, 2011 at 1:35 PM, <mailto:sky@chromium.org> wrote:
> 
> > This now bulks up every Layer for state that you generally only care about
> > at
> > the top level.
> >
> > I believe Ben's suggestion is the following: OnPainted or OnComposited to
> > WidgetDelegate that is invoked when you need it. TouchBrowserView overrides
> > (it
> > is a WidgetDelegate) this and forwards it to the PropertySetter.
> >
> >  -Scott
> >
> >
> >
>
http://codereview.chromium.**org/7273073/%3Chttp://codereview.chromium.org/72...>
> >

[sky, 2011-08-31 22:44:47]

Yes, LGTM

[commit-bot: I haz the power, 2011-09-06 19:27:46]

Can't process patch for file ui/gfx/compositor/compositor_stub.cc.
A +

[Ian Vollick, 2011-09-08 18:50:09]

On 2011/09/06 19:27:46, I haz the power (commit-bot) wrote:
> Can't process patch for file ui/gfx/compositor/compositor_stub.cc.
> A +

Committed @100148