Move DIP translation from ui/aura to ui/compositor

Move DIP translation from ui/aura to ui/compositor.
Layer's coordinate system is now in DIP.
Added support of dynamic density switching.
Removed ENABLE_DIP gyp/macro and added runtime flag "--ui-enable-dip"

BUG=105165, 114666
TEST=enabled monitor test. added new tests to compositor_unittests

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

[oshima, 2012-05-02 20:19:28]

Hi, this is not for full review, but want to get early feedback about the
direction I'm heading.

This CL moves DIP translation (for high DPI display) to layer and it looks to me
that this is better than doing it in aura side. I haven't changed
transform/animation, but I'm planning to do so. I think this makes it easy to
handle dynamic switching. Please let me know what you think about the direction.

[oshima, 2012-05-02 20:22:57]

+vollick for feedback on animation side.

[sky, 2012-05-02 21:15:41]

If I'm reading this right this feels a bit half way there. You're making Layer
know about and apply the scale but it's bounds and everything are still in terms
of pixels.

Making Layers bounds entirely in terms of DIP would be a LOT less error prone.
If we don't do that I guarantee we'll have lots of bugs where people expect the
window's bounds to match the layer's bounds.

[oshima, 2012-05-02 21:29:03]

On 2012/05/02 21:15:41, sky wrote:
> If I'm reading this right this feels a bit half way there. You're making Layer
> know about and apply the scale but it's bounds and everything are still in
terms
> of pixels.
> 
> Making Layers bounds entirely in terms of DIP would be a LOT less error prone.
> If we don't do that I guarantee we'll have lots of bugs where people expect
the
> window's bounds to match the layer's bounds.

I agree, and that's what I want to do eventually. I want to hide pixel form
users as much as
possible and want to keep DIP instead of pixels when switching density, so
that's my direction.

I didn't do it here because that requires changes to transform and animations,
which I was planning to do
in separate CL, but I'm ok to do in one CL if everyone agrees.

[oshima, 2012-05-02 22:06:36]

I was a bit wrong here. If layer uses DIP as default (that is, bounds() return
DIP), then there should be less change we need.
I talked to vollick offline and he is ok with this direction tool.

piman, I'm going to start this now so please let me know if you have
objection or concern.

On 2012/05/02 21:29:03, oshima wrote:
> On 2012/05/02 21:15:41, sky wrote:
> > If I'm reading this right this feels a bit half way there. You're making
Layer
> > know about and apply the scale but it's bounds and everything are still in
> terms
> > of pixels.
> > 
> > Making Layers bounds entirely in terms of DIP would be a LOT less error
prone.
> > If we don't do that I guarantee we'll have lots of bugs where people expect
> the
> > window's bounds to match the layer's bounds.
> 
> I agree, and that's what I want to do eventually. I want to hide pixel form
> users as much as
> possible and want to keep DIP instead of pixels when switching density, so
> that's my direction.
> 
> I didn't do it here because that requires changes to transform and animations,
> which I was planning to do
> in separate CL, but I'm ok to do in one CL if everyone agrees.

[piman, 2012-05-02 22:57:54]

On Wed, May 2, 2012 at 3:06 PM, <oshima@chromium.org> wrote:

> I was a bit wrong here. If layer uses DIP as default (that is, bounds()
> return
> DIP), then there should be less change we need.
> I talked to vollick offline and he is ok with this direction tool.
>
> piman, I'm going to start this now so please let me know if you have
> objection or concern.


As said in person, at a high level, the approach sounds fine. We'll need to
take a look at the perf impact of doing scaling in skia, it's easy for it
to be hidden now.

Antoine


>
>
> On 2012/05/02 21:29:03, oshima wrote:
>
>> On 2012/05/02 21:15:41, sky wrote:
>> > If I'm reading this right this feels a bit half way there. You're making
>>
> Layer
>
>> > know about and apply the scale but it's bounds and everything are still
>> in
>> terms
>> > of pixels.
>> >
>> > Making Layers bounds entirely in terms of DIP would be a LOT less error
>>
> prone.
>
>> > If we don't do that I guarantee we'll have lots of bugs where people
>> expect
>> the
>> > window's bounds to match the layer's bounds.
>>
>
>  I agree, and that's what I want to do eventually. I want to hide pixel
>> form
>> users as much as
>> possible and want to keep DIP instead of pixels when switching density, so
>> that's my direction.
>>
>
>  I didn't do it here because that requires changes to transform and
>> animations,
>> which I was planning to do
>> in separate CL, but I'm ok to do in one CL if everyone agrees.
>>
>
>
>
>
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>

[sky, 2012-05-02 23:16:42]

On Wed, May 2, 2012 at 3:57 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Wed, May 2, 2012 at 3:06 PM, <oshima@chromium.org> wrote:
>>
>> I was a bit wrong here. If layer uses DIP as default (that is, bounds()
>> return
>> DIP), then there should be less change we need.
>> I talked to vollick offline and he is ok with this direction tool.
>>
>> piman, I'm going to start this now so please let me know if you have
>> objection or concern.
>
>
> As said in person, at a high level, the approach sounds fine. We'll need to
> take a look at the perf impact of doing scaling in skia, it's easy for it to
> be hidden now.

Should we do that now? If the performance diff is dramatic might it
force us to consider a different approach?

  -Scott

>
> Antoine
>
>>
>>
>>
>> On 2012/05/02 21:29:03, oshima wrote:
>>>
>>> On 2012/05/02 21:15:41, sky wrote:
>>> > If I'm reading this right this feels a bit half way there. You're
>>> > making
>>
>> Layer
>>>
>>> > know about and apply the scale but it's bounds and everything are still
>>> > in
>>> terms
>>> > of pixels.
>>> >
>>> > Making Layers bounds entirely in terms of DIP would be a LOT less error
>>
>> prone.
>>>
>>> > If we don't do that I guarantee we'll have lots of bugs where people
>>> > expect
>>> the
>>> > window's bounds to match the layer's bounds.
>>
>>
>>> I agree, and that's what I want to do eventually. I want to hide pixel
>>> form
>>> users as much as
>>> possible and want to keep DIP instead of pixels when switching density,
>>> so
>>> that's my direction.
>>
>>
>>> I didn't do it here because that requires changes to transform and
>>> animations,
>>> which I was planning to do
>>> in separate CL, but I'm ok to do in one CL if everyone agrees.
>>
>>
>>
>>
>> http://codereview.chromium.org/10221028/
>
>

[oshima, 2012-05-03 17:31:56]

I looked into this yesterday. Turns out that translation operate on web_layer
(which is obvious if you think about it ;), so i don't think we can change
translation/animation to DIP without major change to webkit side.

If we can't change them to DIP, we should keep all of
layer/animation/translation pixel coordinate.

I understand that this is a bit error prone with DIP being int. I think this
will
be less issue once DIP becomes float (I'm going to do this in m21).

On 2012/05/02 22:06:36, oshima wrote:
> I was a bit wrong here. If layer uses DIP as default (that is, bounds() return
> DIP), then there should be less change we need.
> I talked to vollick offline and he is ok with this direction tool.
> 
> piman, I'm going to start this now so please let me know if you have
> objection or concern.
> 
> On 2012/05/02 21:29:03, oshima wrote:
> > On 2012/05/02 21:15:41, sky wrote:
> > > If I'm reading this right this feels a bit half way there. You're making
> Layer
> > > know about and apply the scale but it's bounds and everything are still in
> > terms
> > > of pixels.
> > > 
> > > Making Layers bounds entirely in terms of DIP would be a LOT less error
> prone.
> > > If we don't do that I guarantee we'll have lots of bugs where people
expect
> > the
> > > window's bounds to match the layer's bounds.
> > 
> > I agree, and that's what I want to do eventually. I want to hide pixel form
> > users as much as
> > possible and want to keep DIP instead of pixels when switching density, so
> > that's my direction.
> > 
> > I didn't do it here because that requires changes to transform and
animations,
> > which I was planning to do
> > in separate CL, but I'm ok to do in one CL if everyone agrees.

[oshima, 2012-05-03 17:35:16]

On 2012/05/03 17:31:56, oshima wrote:
> I looked into this yesterday. Turns out that translation operate on web_layer
> (which is obvious if you think about it ;), so i don't think we can change
> translation/animation to DIP without major change to webkit side.

I think "major change" was a bit exaggerated. I will talk to piman first and
see how hard/easy it is.

> 
> If we can't change them to DIP, we should keep all of
> layer/animation/translation pixel coordinate.
> 
> I understand that this is a bit error prone with DIP being int. I think this
> will
> be less issue once DIP becomes float (I'm going to do this in m21).
> 
> On 2012/05/02 22:06:36, oshima wrote:
> > I was a bit wrong here. If layer uses DIP as default (that is, bounds()
return
> > DIP), then there should be less change we need.
> > I talked to vollick offline and he is ok with this direction tool.
> > 
> > piman, I'm going to start this now so please let me know if you have
> > objection or concern.
> > 
> > On 2012/05/02 21:29:03, oshima wrote:
> > > On 2012/05/02 21:15:41, sky wrote:
> > > > If I'm reading this right this feels a bit half way there. You're making
> > Layer
> > > > know about and apply the scale but it's bounds and everything are still
in
> > > terms
> > > > of pixels.
> > > > 
> > > > Making Layers bounds entirely in terms of DIP would be a LOT less error
> > prone.
> > > > If we don't do that I guarantee we'll have lots of bugs where people
> expect
> > > the
> > > > window's bounds to match the layer's bounds.
> > > 
> > > I agree, and that's what I want to do eventually. I want to hide pixel
form
> > > users as much as
> > > possible and want to keep DIP instead of pixels when switching density, so
> > > that's my direction.
> > > 
> > > I didn't do it here because that requires changes to transform and
> animations,
> > > which I was planning to do
> > > in separate CL, but I'm ok to do in one CL if everyone agrees.

[oshima, 2012-05-04 18:12:55]

Uploaded new patch. I'm working on tests and will add this to this CL a bit
later, but the rest is ready for review.

Layer's bounds is now in DIP and things looks much better and simple. Many
thanks to piman for helping me on this.

piman -> compositor change
derat -> image_grid
sky -> everything else

[piman, 2012-05-04 18:46:06]

Overall, I think this is a great approach. The UI code doesn't have to care
about DIP (unless it wants to), and the compositor in webkit doesn't need to
care.

The one thing I think we should be careful about, is to not have to walk the
layer tree to make DIP <-> pixels transformations. I think it shouldn't be too
hard.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.cc (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.cc:250: if (root_layer_)
nit: indentation?

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.cc:288:
ret.ConcatTransform(invese_scale_translate_);
nit: you probably want to start with ret=inverse_scale_translate_ and
concatenate the other transforms. It saves a trivial matrix multiply.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.h:252: Transform invese_scale_translate_;
nit: invese->inverse

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/dip_util.cc
File ui/gfx/compositor/dip_util.cc (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/dip_util...
ui/gfx/compositor/dip_util.cc:27: Layer* nonconst = const_cast<Layer*>(layer);
We should add a const version of GetCompositor() instead.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:250: // case under multi monitor environment.
FYI, I think the plan is to have a separate root for each monitor, so the CHECK
above will trigger.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:362: ScheduleDraw();
Let's not force this, except maybe if we explicitly change the scaling factor.
This is called on every layer tree modifications, it'd be better to let the
compositor be smart about what needs to be redrawn or not.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:544: Compositor* compositor = GetCompositor();
Is there a way we could avoid walking the layer tree here? In particular if you
look at UpdateLayerSize, it would be O(d^2) where d is the maximum depth.

The scaling matrix is trivial (so is its inverse), so it feels like we could
just keep the scaling factor in Layer (update maybe in UpdateLayerSize()), and
just do the matrix math here. I don't think there's a lot of benefit in
"caching" the scaling matrix.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:561: web_layer_.setBounds(ConvertSizeToPixel(this,
bounds_.size()));
This, like above, will have to walk the layer tree. If we cache the scaling
factor in Layer, this becomes trivial math.

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:568: gfx::Rect bounds_in_pixel = GetBoundsInPixel();
Same here

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.h#...
ui/gfx/compositor/layer.h:120: gfx::Rect GetBoundsInPixel() const;
Are these called publicly (or at all?)

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.h#...
ui/gfx/compositor/layer.h:311: // before passing to LayerDelegate::OnLayerPaitn.
nit: OnLayerPaitn -> OnLayerPaint

[oshima, 2012-05-04 21:08:05]

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:544: Compositor* compositor = GetCompositor();
On 2012/05/04 18:46:06, piman wrote:
> Is there a way we could avoid walking the layer tree here? In particular if
you
> look at UpdateLayerSize, it would be O(d^2) where d is the maximum depth.
> 
> The scaling matrix is trivial (so is its inverse), so it feels like we could
> just keep the scaling factor in Layer (update maybe in UpdateLayerSize()), and
> just do the matrix math here. I don't think there's a lot of benefit in
> "caching" the scaling matrix.

Given that layer already have compositor_ and I have to notify all layers when
layer is added to the tree to update scales anyway, would you mind if I set this
field for all laers?

[piman, 2012-05-04 21:20:18]

On Fri, May 4, 2012 at 2:08 PM, <oshima@chromium.org> wrote:

>
> http://codereview.chromium.**org/10221028/diff/29009/ui/**
>
gfx/compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
> File ui/gfx/compositor/layer.cc (right):
>
> http://codereview.chromium.**org/10221028/diff/29009/ui/**
>
gfx/compositor/layer.cc#**newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
> ui/gfx/compositor/layer.cc:**544: Compositor* compositor =
> GetCompositor();
> On 2012/05/04 18:46:06, piman wrote:
>
>> Is there a way we could avoid walking the layer tree here? In
>>
> particular if you
>
>> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
>>
> depth.
>
>  The scaling matrix is trivial (so is its inverse), so it feels like we
>>
> could
>
>> just keep the scaling factor in Layer (update maybe in
>>
> UpdateLayerSize()), and
>
>> just do the matrix math here. I don't think there's a lot of benefit
>>
> in
>
>> "caching" the scaling matrix.
>>
>
> Given that layer already have compositor_ and I have to notify all
> layers when layer is added to the tree to update scales anyway, would
> you mind if I set this field for all laers?
>

It used to be done that way, but there were some issues. I don't remember
which. Sky?
One of the things is, I think we should avoid leaving a stale compositor_
there, because layers may outlive the compositor. So you need to add logic
for adding *and* removing layers, as well as reparenting. Updating scale
isn't as problematic.


>
>
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>

[oshima, 2012-05-04 21:42:43]

On 2012/05/04 21:20:18, piman wrote:
> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
> >
>
gfx/compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
> > File ui/gfx/compositor/layer.cc (right):
> >
> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
> >
>
gfx/compositor/layer.cc#**newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
> > ui/gfx/compositor/layer.cc:**544: Compositor* compositor =
> > GetCompositor();
> > On 2012/05/04 18:46:06, piman wrote:
> >
> >> Is there a way we could avoid walking the layer tree here? In
> >>
> > particular if you
> >
> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
> >>
> > depth.
> >
> >  The scaling matrix is trivial (so is its inverse), so it feels like we
> >>
> > could
> >
> >> just keep the scaling factor in Layer (update maybe in
> >>
> > UpdateLayerSize()), and
> >
> >> just do the matrix math here. I don't think there's a lot of benefit
> >>
> > in
> >
> >> "caching" the scaling matrix.
> >>
> >
> > Given that layer already have compositor_ and I have to notify all
> > layers when layer is added to the tree to update scales anyway, would
> > you mind if I set this field for all laers?
> >
> 
> It used to be done that way, but there were some issues. I don't remember
> which. Sky?
> One of the things is, I think we should avoid leaving a stale compositor_
> there, because layers may outlive the compositor. So you need to add logic
> for adding *and* removing layers, as well as reparenting. Updating scale
> isn't as problematic.

Yes, that's what I'm going to do. Scott, do you remeber what was the issue?

> 
> 
> >
> >
>
http://codereview.chromium.**org/10221028/%3Chttp://codereview.chromium.org/1...>
> >

[sky, 2012-05-04 23:23:45]

As discussed in person I don't remember the specifics, but I prefer
looking up the root as we need it.

  -Scott

On Fri, May 4, 2012 at 2:42 PM,  <oshima@chromium.org> wrote:
> On 2012/05/04 21:20:18, piman wrote:
>>
>> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
>
>
>> >
>> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
>> >
>
>
>
gfx/compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
>>
>> > File ui/gfx/compositor/layer.cc (right):
>> >
>> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
>> >
>
>
>
gfx/compositor/layer.cc#**newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
>>
>> > ui/gfx/compositor/layer.cc:**544: Compositor* compositor =
>>
>> > GetCompositor();
>> > On 2012/05/04 18:46:06, piman wrote:
>> >
>> >> Is there a way we could avoid walking the layer tree here? In
>> >>
>> > particular if you
>> >
>> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
>> >>
>> > depth.
>> >
>> >  The scaling matrix is trivial (so is its inverse), so it feels like we
>> >>
>> > could
>> >
>> >> just keep the scaling factor in Layer (update maybe in
>> >>
>> > UpdateLayerSize()), and
>> >
>> >> just do the matrix math here. I don't think there's a lot of benefit
>> >>
>> > in
>> >
>> >> "caching" the scaling matrix.
>> >>
>> >
>> > Given that layer already have compositor_ and I have to notify all
>> > layers when layer is added to the tree to update scales anyway, would
>> > you mind if I set this field for all laers?
>> >
>
>
>> It used to be done that way, but there were some issues. I don't remember
>> which. Sky?
>> One of the things is, I think we should avoid leaving a stale compositor_
>> there, because layers may outlive the compositor. So you need to add logic
>> for adding *and* removing layers, as well as reparenting. Updating scale
>> isn't as problematic.
>
>
> Yes, that's what I'm going to do. Scott, do you remeber what was the issue?
>
>
>
>> >
>> >
>
>
>
http://codereview.chromium.**org/10221028/%3Chttp://codereview.chromium.org/1...>
>>
>> >
>
>
>
>
> http://codereview.chromium.org/10221028/

[oshima, 2012-05-06 02:27:17]

Hi, I uploaded three patches with different approaches.

reduced number of times to lookup to root:
http://codereview.chromium.org/10221028 (this CL)

cache compositor in each layer:  http://codereview.chromium.org/10317036/

cache device scale factor in each layer:
http://codereview.chromium.org/10383025/

(only compositor/layer/root_window are different)

My preference is either 1st or 2nd. I don't like 3rd because it basically does
the same as 2nd but adds extra field and repeats matrix creations (or add two,
identical
matrix to all layers).

Please let me know what you think.

On 2012/05/04 23:23:45, sky wrote:
> As discussed in person I don't remember the specifics, but I prefer
> looking up the root as we need it.
> 
>   -Scott
> 
> On Fri, May 4, 2012 at 2:42 PM,  <mailto:oshima@chromium.org> wrote:
> > On 2012/05/04 21:20:18, piman wrote:
> >>
> >> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
> >
> >
> >> >
> >> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
> >> >
> >
> >
> >
>
gfx/compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
> >>
> >> > File ui/gfx/compositor/layer.cc (right):
> >> >
> >> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
> >> >
> >
> >
> >
>
gfx/compositor/layer.cc#**newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
> >>
> >> > ui/gfx/compositor/layer.cc:**544: Compositor* compositor =
> >>
> >> > GetCompositor();
> >> > On 2012/05/04 18:46:06, piman wrote:
> >> >
> >> >> Is there a way we could avoid walking the layer tree here? In
> >> >>
> >> > particular if you
> >> >
> >> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
> >> >>
> >> > depth.
> >> >
> >> > &nbsp;The scaling matrix is trivial (so is its inverse), so it feels like
we
> >> >>
> >> > could
> >> >
> >> >> just keep the scaling factor in Layer (update maybe in
> >> >>
> >> > UpdateLayerSize()), and
> >> >
> >> >> just do the matrix math here. I don't think there's a lot of benefit
> >> >>
> >> > in
> >> >
> >> >> "caching" the scaling matrix.
> >> >>
> >> >
> >> > Given that layer already have compositor_ and I have to notify all
> >> > layers when layer is added to the tree to update scales anyway, would
> >> > you mind if I set this field for all laers?
> >> >
> >
> >
> >> It used to be done that way, but there were some issues. I don't remember
> >> which. Sky?
> >> One of the things is, I think we should avoid leaving a stale compositor_
> >> there, because layers may outlive the compositor. So you need to add logic
> >> for adding *and* removing layers, as well as reparenting. Updating scale
> >> isn't as problematic.
> >
> >
> > Yes, that's what I'm going to do. Scott, do you remeber what was the issue?
> >
> >
> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.**org/10221028/%253Chttp://codereview.chromium.org...>
> >>
> >> >
> >
> >
> >
> >
> > http://codereview.chromium.org/10221028/

[Daniel Erat, 2012-05-07 19:44:05]

FYI drive-by nits

http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.h:64: SCALE_MONITOR,
nit: keep alphabetized (like the comment at the top says) :-P

http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
File ash/monitor/multi_monitor_manager.cc (right):

http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
ash/monitor/multi_monitor_manager.cc:219: float next =
monitor.device_scale_factor() == 1.0f ? 2.0f : 1.0f;
nit: s/next/factor/

http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h#newc...
ui/aura/root_window.h:107: void SetHostSize(const gfx::Size& size_in_pixel);
i think i might've heard/seen someone mention this already, but "in_pixels" is a
bit better than "in_pixel"

http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.h:114: gfx::Size size_;  // in pixel
nit: s/pixel/pixels/

http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.h:208: // device scale factor to the all translate
operations in the transform.
nit: s/the all/all the/

[piman, 2012-05-07 19:55:38]

On Sat, May 5, 2012 at 7:27 PM, <oshima@chromium.org> wrote:

> Hi, I uploaded three patches with different approaches.
>
> reduced number of times to lookup to root:
>
http://codereview.chromium.**org/10221028<http://codereview.chromium.org/1022...
CL)
>
> cache compositor in each layer:  http://codereview.chromium.**
> org/10317036/ <http://codereview.chromium.org/10317036/>
>
> cache device scale factor in each layer:
>
http://codereview.chromium.**org/10383025/<http://codereview.chromium.org/103...
>
> (only compositor/layer/root_window are different)
>
> My preference is either 1st or 2nd. I don't like 3rd because it basically
> does
> the same as 2nd but adds extra field and repeats matrix creations (or add
> two,
> identical
> matrix to all layers).
>
> Please let me know what you think.


My preference is 3 or if not then 2.
3 is simpler because you don't need to reset anything when removing from
the tree, and so you can write it so that you don't have to recompute all
the transforms all the time when doing tree operations (remove, then add ->
don't change the DIP scale factor -> don't need to recompute).

Antoine


>
>
> On 2012/05/04 23:23:45, sky wrote:
>
>> As discussed in person I don't remember the specifics, but I prefer
>> looking up the root as we need it.
>>
>
>    -Scott
>>
>
>  On Fri, May 4, 2012 at 2:42 PM,  <mailto:oshima@chromium.org> wrote:
>> > On 2012/05/04 21:20:18, piman wrote:
>> >>
>> >> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
>> >
>> >
>> >> >
>> >> > http://codereview.chromium.****org/10221028/diff/29009/ui/**
>> >> >
>> >
>> >
>> >
>>
>
> gfx/compositor/layer.cc<http:/**/codereview.chromium.org/**
>
10221028/diff/29009/ui/gfx/**compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
> >
>
>> >>
>> >> > File ui/gfx/compositor/layer.cc (right):
>> >> >
>> >> > http://codereview.chromium.****org/10221028/diff/29009/ui/**
>> >> >
>> >
>> >
>> >
>>
>
> gfx/compositor/layer.cc#****newcode544<http://codereview.**
>
chromium.org/10221028/diff/**29009/ui/gfx/compositor/layer.**cc#newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
> >
>
>> >>
>> >> > ui/gfx/compositor/layer.cc:****544: Compositor* compositor =
>> >>
>> >> > GetCompositor();
>> >> > On 2012/05/04 18:46:06, piman wrote:
>> >> >
>> >> >> Is there a way we could avoid walking the layer tree here? In
>> >> >>
>> >> > particular if you
>> >> >
>> >> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
>> >> >>
>> >> > depth.
>> >> >
>> >> > &nbsp;The scaling matrix is trivial (so is its inverse), so it feels
>> like
>>
> we
>
>> >> >>
>> >> > could
>> >> >
>> >> >> just keep the scaling factor in Layer (update maybe in
>> >> >>
>> >> > UpdateLayerSize()), and
>> >> >
>> >> >> just do the matrix math here. I don't think there's a lot of benefit
>> >> >>
>> >> > in
>> >> >
>> >> >> "caching" the scaling matrix.
>> >> >>
>> >> >
>> >> > Given that layer already have compositor_ and I have to notify all
>> >> > layers when layer is added to the tree to update scales anyway, would
>> >> > you mind if I set this field for all laers?
>> >> >
>> >
>> >
>> >> It used to be done that way, but there were some issues. I don't
>> remember
>> >> which. Sky?
>> >> One of the things is, I think we should avoid leaving a stale
>> compositor_
>> >> there, because layers may outlive the compositor. So you need to add
>> logic
>> >> for adding *and* removing layers, as well as reparenting. Updating
>> scale
>> >> isn't as problematic.
>> >
>> >
>> > Yes, that's what I'm going to do. Scott, do you remeber what was the
>> issue?
>> >
>> >
>> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/10221028/%253Chttp://coder**
> eview.chromium.org/10221028/ <http://codereview.chromium.org/10221028/>>
>
>> >>
>> >> >
>> >
>> >
>> >
>> >
>> >
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>>
>
>
>
>
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>

[oshima, 2012-05-07 20:01:41]

On Mon, May 7, 2012 at 12:55 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Sat, May 5, 2012 at 7:27 PM, <oshima@chromium.org> wrote:
>
>> Hi, I uploaded three patches with different approaches.
>>
>> reduced number of times to lookup to root:
>>
http://codereview.chromium.**org/10221028<http://codereview.chromium.org/1022...
CL)
>>
>> cache compositor in each layer:  http://codereview.chromium.**
>> org/10317036/ <http://codereview.chromium.org/10317036/>
>>
>> cache device scale factor in each layer:
>>
http://codereview.chromium.**org/10383025/<http://codereview.chromium.org/103...
>>
>> (only compositor/layer/root_window are different)
>>
>> My preference is either 1st or 2nd. I don't like 3rd because it basically
>> does
>> the same as 2nd but adds extra field and repeats matrix creations (or add
>> two,
>> identical
>> matrix to all layers).
>>
>> Please let me know what you think.
>
>
> My preference is 3 or if not then 2.
> 3 is simpler because you don't need to reset anything when removing from
> the tree, and so you can write it so that you don't have to recompute all
> the transforms all the time when doing tree operations (remove, then add ->
> don't change the DIP scale factor -> don't need to recompute).
>

If you like 3 and sky is ok with it, I'm ok with it. Sky?

- oshima



>
> Antoine
>
>
>>
>>
>> On 2012/05/04 23:23:45, sky wrote:
>>
>>> As discussed in person I don't remember the specifics, but I prefer
>>> looking up the root as we need it.
>>>
>>
>>    -Scott
>>>
>>
>>  On Fri, May 4, 2012 at 2:42 PM,  <mailto:oshima@chromium.org> wrote:
>>> > On 2012/05/04 21:20:18, piman wrote:
>>> >>
>>> >> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
>>> >
>>> >
>>> >> >
>>> >> > http://codereview.chromium.****org/10221028/diff/29009/ui/**
>>> >> >
>>> >
>>> >
>>> >
>>>
>>
>> gfx/compositor/layer.cc<http:/**/codereview.chromium.org/**
>>
10221028/diff/29009/ui/gfx/**compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
>> >
>>
>>> >>
>>> >> > File ui/gfx/compositor/layer.cc (right):
>>> >> >
>>> >> > http://codereview.chromium.****org/10221028/diff/29009/ui/**
>>> >> >
>>> >
>>> >
>>> >
>>>
>>
>> gfx/compositor/layer.cc#****newcode544<http://codereview.**
>> chromium.org/10221028/diff/**29009/ui/gfx/compositor/layer.**
>>
cc#newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
>> >
>>
>>> >>
>>> >> > ui/gfx/compositor/layer.cc:****544: Compositor* compositor =
>>> >>
>>> >> > GetCompositor();
>>> >> > On 2012/05/04 18:46:06, piman wrote:
>>> >> >
>>> >> >> Is there a way we could avoid walking the layer tree here? In
>>> >> >>
>>> >> > particular if you
>>> >> >
>>> >> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
>>> >> >>
>>> >> > depth.
>>> >> >
>>> >> > &nbsp;The scaling matrix is trivial (so is its inverse), so it
>>> feels like
>>>
>> we
>>
>>> >> >>
>>> >> > could
>>> >> >
>>> >> >> just keep the scaling factor in Layer (update maybe in
>>> >> >>
>>> >> > UpdateLayerSize()), and
>>> >> >
>>> >> >> just do the matrix math here. I don't think there's a lot of
>>> benefit
>>> >> >>
>>> >> > in
>>> >> >
>>> >> >> "caching" the scaling matrix.
>>> >> >>
>>> >> >
>>> >> > Given that layer already have compositor_ and I have to notify all
>>> >> > layers when layer is added to the tree to update scales anyway,
>>> would
>>> >> > you mind if I set this field for all laers?
>>> >> >
>>> >
>>> >
>>> >> It used to be done that way, but there were some issues. I don't
>>> remember
>>> >> which. Sky?
>>> >> One of the things is, I think we should avoid leaving a stale
>>> compositor_
>>> >> there, because layers may outlive the compositor. So you need to add
>>> logic
>>> >> for adding *and* removing layers, as well as reparenting. Updating
>>> scale
>>> >> isn't as problematic.
>>> >
>>> >
>>> > Yes, that's what I'm going to do. Scott, do you remeber what was the
>>> issue?
>>> >
>>> >
>>> >
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/10221028/%253Chttp://coder**
>> eview.chromium.org/10221028/ <http://codereview.chromium.org/10221028/>>
>>
>>> >>
>>> >> >
>>> >
>>> >
>>> >
>>> >
>>> >
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/10221028/<http://codereview.chromium.org/102...
>>
>
>

[sky, 2012-05-07 21:57:43]

The main area this would effect is paint. In particular paint would
now have to look up the compositor every time. While we don't have
deep trees, this worried Antoine. So, for the record, Oshima is going
with 3.

  -Scott

On Mon, May 7, 2012 at 1:01 PM, oshima <oshima@chromium.org> wrote:
>
>
> On Mon, May 7, 2012 at 12:55 PM, Antoine Labour <piman@chromium.org> wrote:
>>
>>
>>
>> On Sat, May 5, 2012 at 7:27 PM, <oshima@chromium.org> wrote:
>>>
>>> Hi, I uploaded three patches with different approaches.
>>>
>>> reduced number of times to lookup to root:
>>> http://codereview.chromium.org/10221028 (this CL)
>>>
>>> cache compositor in each layer:  http://codereview.chromium.org/10317036/
>>>
>>> cache device scale factor in each layer:
>>> http://codereview.chromium.org/10383025/
>>>
>>> (only compositor/layer/root_window are different)
>>>
>>> My preference is either 1st or 2nd. I don't like 3rd because it basically
>>> does
>>> the same as 2nd but adds extra field and repeats matrix creations (or add
>>> two,
>>> identical
>>> matrix to all layers).
>>>
>>> Please let me know what you think.
>>
>>
>> My preference is 3 or if not then 2.
>> 3 is simpler because you don't need to reset anything when removing from
>> the tree, and so you can write it so that you don't have to recompute all
>> the transforms all the time when doing tree operations (remove, then add ->
>> don't change the DIP scale factor -> don't need to recompute).
>
>
> If you like 3 and sky is ok with it, I'm ok with it. Sky?
>
> - oshima
>
>
>>
>>
>> Antoine
>>
>>>
>>>
>>>
>>> On 2012/05/04 23:23:45, sky wrote:
>>>>
>>>> As discussed in person I don't remember the specifics, but I prefer
>>>> looking up the root as we need it.
>>>
>>>
>>>>   -Scott
>>>
>>>
>>>> On Fri, May 4, 2012 at 2:42 PM,  <mailto:oshima@chromium.org> wrote:
>>>> > On 2012/05/04 21:20:18, piman wrote:
>>>> >>
>>>> >> On Fri, May 4, 2012 at 2:08 PM, <mailto:oshima@chromium.org> wrote:
>>>> >
>>>> >
>>>> >> >
>>>> >> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
>>>> >> >
>>>> >
>>>> >
>>>> >
>>>
>>>
>>>
>>>
gfx/compositor/layer.cc<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc>
>>>>
>>>> >>
>>>> >> > File ui/gfx/compositor/layer.cc (right):
>>>> >> >
>>>> >> > http://codereview.chromium.**org/10221028/diff/29009/ui/**
>>>> >> >
>>>> >
>>>> >
>>>> >
>>>
>>>
>>>
>>>
gfx/compositor/layer.cc#**newcode544<http://codereview.chromium.org/10221028/diff/29009/ui/gfx/compositor/layer.cc#newcode544>
>>>>
>>>> >>
>>>> >> > ui/gfx/compositor/layer.cc:**544: Compositor* compositor =
>>>> >>
>>>> >> > GetCompositor();
>>>> >> > On 2012/05/04 18:46:06, piman wrote:
>>>> >> >
>>>> >> >> Is there a way we could avoid walking the layer tree here? In
>>>> >> >>
>>>> >> > particular if you
>>>> >> >
>>>> >> >> look at UpdateLayerSize, it would be O(d^2) where d is the maximum
>>>> >> >>
>>>> >> > depth.
>>>> >> >
>>>> >> > &nbsp;The scaling matrix is trivial (so is its inverse), so it
>>>> >> > feels like
>>>
>>> we
>>>>
>>>> >> >>
>>>> >> > could
>>>> >> >
>>>> >> >> just keep the scaling factor in Layer (update maybe in
>>>> >> >>
>>>> >> > UpdateLayerSize()), and
>>>> >> >
>>>> >> >> just do the matrix math here. I don't think there's a lot of
>>>> >> >> benefit
>>>> >> >>
>>>> >> > in
>>>> >> >
>>>> >> >> "caching" the scaling matrix.
>>>> >> >>
>>>> >> >
>>>> >> > Given that layer already have compositor_ and I have to notify all
>>>> >> > layers when layer is added to the tree to update scales anyway,
>>>> >> > would
>>>> >> > you mind if I set this field for all laers?
>>>> >> >
>>>> >
>>>> >
>>>> >> It used to be done that way, but there were some issues. I don't
>>>> >> remember
>>>> >> which. Sky?
>>>> >> One of the things is, I think we should avoid leaving a stale
>>>> >> compositor_
>>>> >> there, because layers may outlive the compositor. So you need to add
>>>> >> logic
>>>> >> for adding *and* removing layers, as well as reparenting. Updating
>>>> >> scale
>>>> >> isn't as problematic.
>>>> >
>>>> >
>>>> > Yes, that's what I'm going to do. Scott, do you remeber what was the
>>>> > issue?
>>>> >
>>>> >
>>>> >
>>>> >> >
>>>> >> >
>>>> >
>>>> >
>>>> >
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/10221028/%253Chttp://codereview.chromium.org...>
>>>>
>>>> >>
>>>> >> >
>>>> >
>>>> >
>>>> >
>>>> >
>>>> > http://codereview.chromium.org/10221028/
>>>
>>>
>>>
>>>
>>> http://codereview.chromium.org/10221028/
>>
>>
>

[oshima, 2012-05-08 00:12:04]

Uploaded new patch. PTAL.

[oshima, 2012-05-08 00:17:24]

http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.h:64: SCALE_MONITOR,
On 2012/05/07 19:44:05, Daniel Erat wrote:
> nit: keep alphabetized (like the comment at the top says) :-P

Done.

http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
File ash/monitor/multi_monitor_manager.cc (right):

http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
ash/monitor/multi_monitor_manager.cc:219: float next =
monitor.device_scale_factor() == 1.0f ? 2.0f : 1.0f;
On 2012/05/07 19:44:05, Daniel Erat wrote:
> nit: s/next/factor/

Done.

http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h#newc...
ui/aura/root_window.h:107: void SetHostSize(const gfx::Size& size_in_pixel);
On 2012/05/07 19:44:05, Daniel Erat wrote:
> i think i might've heard/seen someone mention this already, but "in_pixels" is
a
> bit better than "in_pixel"

I used pixel to refer to Pixel Coordinates, but not pixel units, 
and sky was ok with it (we use dip for DIP not dips for example).
That's bing said, I'm no expert on choosing English term, and
I can change if you and sky still think pixels is better. Let me know.

http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.h:208: // device scale factor to the all translate
operations in the transform.
On 2012/05/07 19:44:05, Daniel Erat wrote:
> nit: s/the all/all the/

this is gone now.

[piman, 2012-05-08 00:28:46]

ui/compositor lgtm (with nit)

http://codereview.chromium.org/10221028/diff/28102/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/10221028/diff/28102/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:100: compositor && device_scale_factor_ !=
compositor->device_scale_factor()) {
nit: you may not need the device_scale_factor_ !=
compositor->device_scale_factor() test since it's early'd-out in
OnDeviceScaleFactorChanged

http://codereview.chromium.org/10221028/diff/28102/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:112: if (IsDIPEnabled() && device_scale_factor_ !=
child->device_scale_factor_)
nit: Same here, there's already an early out in OnDeviceScaleFactorChanged

[sky, 2012-05-08 00:41:58]

http://codereview.chromium.org/10221028/diff/20052/ash/monitor/multi_monitor_...
File ash/monitor/multi_monitor_manager.h (right):

http://codereview.chromium.org/10221028/diff/20052/ash/monitor/multi_monitor_...
ash/monitor/multi_monitor_manager.h:40: static void ScaleMonitor();
Should this be named ToggleMonitorScale?

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.cc:235: void
Compositor::WidgetScaleOrSizeChanged(float scale,
How about SetScaleAndSize?

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/dip_util.h
File ui/gfx/compositor/dip_util.h (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/dip_util...
ui/gfx/compositor/dip_util.h:21: class COMPOSITOR_EXPORT ScopedDIPEnablerForTest
{
Add a description. Also, if this is only for testing can you move it into its
own file?

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:56: device_scale_factor_(0.0f) {
I would have expected this to be 1 by default. How come it's 0?

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.h#...
ui/gfx/compositor/layer.h:204: void set_scale_canvas(bool scale_canvas);
I don't see the implementation of this (or using it in your patch). Is it
needed?

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer_un...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer_un...
ui/gfx/compositor/layer_unittest.cc:1026: TEST_F(LayerWithRealCompositorTest,
ScaleUp) {
Can you also add coverage that a scale is set appropriately on the canvas passed
to OnPaintLayer?

[Daniel Erat, 2012-05-08 00:45:07]

On Mon, May 7, 2012 at 5:17 PM,  <oshima@chromium.org> wrote:
>
>
http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
> File ash/accelerators/accelerator_table.h (right):
>
>
http://codereview.chromium.org/10221028/diff/17007/ash/accelerators/accelerat...
> ash/accelerators/accelerator_table.h:64: SCALE_MONITOR,
> On 2012/05/07 19:44:05, Daniel Erat wrote:
>>
>> nit: keep alphabetized (like the comment at the top says) :-P
>
>
> Done.
>
>
>
http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
> File ash/monitor/multi_monitor_manager.cc (right):
>
>
http://codereview.chromium.org/10221028/diff/17007/ash/monitor/multi_monitor_...
> ash/monitor/multi_monitor_manager.cc:219: float next =
> monitor.device_scale_factor() == 1.0f ? 2.0f : 1.0f;
> On 2012/05/07 19:44:05, Daniel Erat wrote:
>>
>> nit: s/next/factor/
>
>
> Done.
>
>
> http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h
> File ui/aura/root_window.h (right):
>
>
http://codereview.chromium.org/10221028/diff/17007/ui/aura/root_window.h#newc...
> ui/aura/root_window.h:107: void SetHostSize(const gfx::Size&
> size_in_pixel);
> On 2012/05/07 19:44:05, Daniel Erat wrote:
>>
>> i think i might've heard/seen someone mention this already, but
>
> "in_pixels" is a
>>
>> bit better than "in_pixel"
>
>
> I used pixel to refer to Pixel Coordinates, but not pixel units,
> and sky was ok with it (we use dip for DIP not dips for example).
> That's bing said, I'm no expert on choosing English term, and
> I can change if you and sky still think pixels is better. Let me know.

_in_pixels or _in_pixel_coords still makes more sense to me, but I
won't argue if others are fine with the singular version.

>
>
http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
> File ui/gfx/compositor/compositor.h (right):
>
>
http://codereview.chromium.org/10221028/diff/17007/ui/gfx/compositor/composit...
> ui/gfx/compositor/compositor.h:208: // device scale factor to the all
> translate operations in the transform.
> On 2012/05/07 19:44:05, Daniel Erat wrote:
>>
>> nit: s/the all/all the/
>
>
> this is gone now.
>
> http://codereview.chromium.org/10221028/

[oshima, 2012-05-08 01:24:16]

http://codereview.chromium.org/10221028/diff/20052/ash/monitor/multi_monitor_...
File ash/monitor/multi_monitor_manager.h (right):

http://codereview.chromium.org/10221028/diff/20052/ash/monitor/multi_monitor_...
ash/monitor/multi_monitor_manager.h:40: static void ScaleMonitor();
On 2012/05/08 00:41:59, sky wrote:
> Should this be named ToggleMonitorScale?

Done.

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/composit...
File ui/gfx/compositor/compositor.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/composit...
ui/gfx/compositor/compositor.cc:235: void
Compositor::WidgetScaleOrSizeChanged(float scale,
On 2012/05/08 00:41:59, sky wrote:
> How about SetScaleAndSize?

Done.

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/dip_util.h
File ui/gfx/compositor/dip_util.h (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/dip_util...
ui/gfx/compositor/dip_util.h:21: class COMPOSITOR_EXPORT ScopedDIPEnablerForTest
{
On 2012/05/08 00:41:59, sky wrote:
> Add a description. Also, if this is only for testing can you move it into its
> own file?

This is necessary only for m20 (because I wanted to put DIP behind a flag).
Since I'm going to remove this after m20 branch, I'll keep it here for now.
(If I can't check it in today, i'll just remove them)

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.h#...
ui/gfx/compositor/layer.h:204: void set_scale_canvas(bool scale_canvas);
On 2012/05/08 00:41:59, sky wrote:
> I don't see the implementation of this (or using it in your patch). Is it
> needed?

This is necessary for RWHV as it will handle scaling itself.
I guess I lost change while I was creating multiple versions of it. done

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer_un...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer_un...
ui/gfx/compositor/layer_unittest.cc:1026: TEST_F(LayerWithRealCompositorTest,
ScaleUp) {
On 2012/05/08 00:41:59, sky wrote:
> Can you also add coverage that a scale is set appropriately on the canvas
passed
> to OnPaintLayer?

Done.

[oshima, 2012-05-08 01:27:25]

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/10221028/diff/20052/ui/gfx/compositor/layer.cc...
ui/gfx/compositor/layer.cc:56: device_scale_factor_(0.0f) {
On 2012/05/08 00:41:59, sky wrote:
> I would have expected this to be 1 by default. How come it's 0?

This was poison value I used while testing to detect the case where scale wasn't
set. Changed to 1.0f.

[sky, 2012-05-08 15:36:15]

LGTM