Implements core logic for Pixel Canvas

Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slide
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2877483003
Cr-Commit-Position: refs/heads/master@{#491587}
Committed: https://chromium.googlesource.com/chromium/src/+/55c9f6d21086e418dac45a58e5f51a3a5ef7bc1e

[malaykeshav, 2017-05-10 19:29:20]

Description was changed from

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
==========

to

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[malaykeshav, 2017-05-10 19:29:40]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:31:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 19:53:52]

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

[commit-bot: I haz the power, 2017-05-10 19:53:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-05-10 20:52:28]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 20:53:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 22:28:31]

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

[commit-bot: I haz the power, 2017-05-10 22:28:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-05-10 22:29:00]

The CQ bit was unchecked by malaykeshav@chromium.org

[malaykeshav, 2017-05-10 22:29:57]

Patchset #2 (id:20001) has been deleted

[malaykeshav, 2017-05-10 22:30:01]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[malaykeshav, 2017-05-10 22:31:06]

Description was changed from

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that occur when this mode is enabled.
This is to avoid one massive CL and breaking it into smaller chunks.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-05-10 22:31:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-05-10 22:32:23]

Description was changed from

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL and break it down into smaller chunks
that can be traced back in case of error.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[malaykeshav, 2017-05-11 01:10:57]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 01:11:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 01:21:21]

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

[commit-bot: I haz the power, 2017-05-11 01:21:22]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[malaykeshav, 2017-05-11 08:26:01]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 08:26:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 11:33:01]

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

[commit-bot: I haz the power, 2017-05-11 11:33:02]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-05-11 18:08:49]

malaykeshav@chromium.org changed reviewers:
+ oshima@chromium.org

[malaykeshav, 2017-05-11 18:08:50]

PTAL

[malaykeshav, 2017-05-11 18:22:02]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 18:22:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 20:24:33]

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

[commit-bot: I haz the power, 2017-05-11 20:24:34]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-05-12 09:14:31]

On 2017/05/11 at 18:08:50, malaykeshav wrote:
> PTAL

Ping

[malaykeshav, 2017-06-13 21:11:50]

Patchset #7 (id:140001) has been deleted

[malaykeshav, 2017-06-13 21:13:41]

Description was changed from

==========
Implements core logic for scalable views

This patch implements the core logic for scalable views while hiding it
behind a compositor switch(Disabled by default).

Scalable views enables all canvas draw commands to be recorded at pixel
size, thus eliminating a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL and break it down into smaller chunks
that can be traced back in case of error.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[malaykeshav, 2017-06-13 22:02:39]

Patchset #7 (id:160001) has been deleted

[malaykeshav, 2017-06-13 22:02:51]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:03:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 22:25:17]

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

[commit-bot: I haz the power, 2017-06-13 22:25:18]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-06-13 22:42:25]

Patchset #7 (id:180001) has been deleted

[malaykeshav, 2017-06-13 22:42:32]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:43:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 23:11:17]

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

[commit-bot: I haz the power, 2017-06-13 23:11:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-06-14 00:44:52]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 00:45:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 02:14:29]

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

[commit-bot: I haz the power, 2017-06-14 02:14:31]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-06-15 22:59:13]

https://codereview.chromium.org/2877483003/diff/220001/cc/paint/display_item_...
File cc/paint/display_item_list.h (right):

https://codereview.chromium.org/2877483003/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.h:177: // the corner points. Use compositor switch
--enable-pixel-canvas-recording to
remove the reference to command line

https://codereview.chromium.org/2877483003/diff/220001/cc/raster/raster_source.h
File cc/raster/raster_source.h (right):

https://codereview.chromium.org/2877483003/diff/220001/cc/raster/raster_sourc...
cc/raster/raster_source.h:91: // this raster source, as well as the solid color
value.
update the comment

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/layer.cc...
ui/compositor/layer.cc:924: switches::kEnablePixelCanvasRecording));
can you initialize once instead of querying everytime?

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:91: return (float)pixel_bounds_.width() /
(float)bounds_.width();
static_cast<float>
same for the other places.

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.h:40: // |invalidation|.
update the comment

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.h:99: gfx::Rect ScaleToEffectivePixelBounds(const
gfx::Rect& bounds) const;
Effective, may not be a good name in this context. 
 How about ScaleToCorneredPixelBounds ?

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:23: const gfx::Size& effective_recording_size,
do you need this change?

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:33: const gfx::Size& pixel_recording_size,
do you need this change?

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:194: void Scale(float x_scale, float y_scale);
Can you land this fix separately?

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc#newco...
ui/views/view.cc:891:
SkFloatToScalar(parent_context.effective_scale_factor_y())));
shouldn't this match the scale type requested?

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc#newco...
ui/views/view.cc:1989: gfx::Vector2d offset_from_parent =
context.pixel_bounds().OffsetFromOrigin();
Don't you have to mirror this for RTL?

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:127: }
The pixel bounds should always be the scale to fit. Scaling logic should affect
only painting logic, not the bounds calculation (otherwise it'll affect
children's bounds).

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
ui/compositor/paint_context.h:99: gfx::Rect ScaleToEffectivePixelBounds(const
gfx::Rect& bounds) const;
looks like this isn't used? (sorry if I missed it)

[oshima, 2017-06-16 00:15:03]

https://codereview.chromium.org/2877483003/diff/240001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:387: ui::PaintRecorder recorder(context,
context.ScaleToEffectivePixelSize(size()),
shouldn't this be context.pixel_size()? and if so, can we eliminate the
parameter?

[malaykeshav, 2017-06-16 18:57:08]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 18:57:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 19:02:01]

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

[commit-bot: I haz the power, 2017-06-16 19:02:02]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[malaykeshav, 2017-06-16 19:07:38]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[malaykeshav, 2017-06-16 19:07:44]

Patchset #10 (id:260001) has been deleted

[commit-bot: I haz the power, 2017-06-16 19:07:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 20:23:00]

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

[commit-bot: I haz the power, 2017-06-16 20:23:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-06-16 20:41:58]

PTAL

https://codereview.chromium.org/2877483003/diff/220001/cc/paint/display_item_...
File cc/paint/display_item_list.h (right):

https://codereview.chromium.org/2877483003/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.h:177: // the corner points. Use compositor switch
--enable-pixel-canvas-recording to
On 2017/06/15 at 22:59:12, oshima wrote:
> remove the reference to command line

Done

https://codereview.chromium.org/2877483003/diff/220001/cc/raster/raster_source.h
File cc/raster/raster_source.h (right):

https://codereview.chromium.org/2877483003/diff/220001/cc/raster/raster_sourc...
cc/raster/raster_source.h:91: // this raster source, as well as the solid color
value.
On 2017/06/15 at 22:59:12, oshima wrote:
> update the comment

Done

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/layer.cc...
ui/compositor/layer.cc:924: switches::kEnablePixelCanvasRecording));
On 2017/06/15 at 22:59:12, oshima wrote:
> can you initialize once instead of querying everytime?

Done

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:91: return (float)pixel_bounds_.width() /
(float)bounds_.width();
On 2017/06/15 at 22:59:12, oshima wrote:
> static_cast<float>
> same for the other places.

Done

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.h:40: // |invalidation|.
On 2017/06/15 at 22:59:12, oshima wrote:
> update the comment

Done

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_co...
ui/compositor/paint_context.h:99: gfx::Rect ScaleToEffectivePixelBounds(const
gfx::Rect& bounds) const;
On 2017/06/15 at 22:59:12, oshima wrote:
> Effective, may not be a good name in this context. 
>  How about ScaleToCorneredPixelBounds ?

Done

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:23: const gfx::Size& effective_recording_size,
On 2017/06/15 at 22:59:12, oshima wrote:
> do you need this change?

Added it to differentiate if from the other constructor in which the recording
size is always in DIPs while in this constructor the recording size should be in
pixels if Pixel canvas is enabled.

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:33: const gfx::Size& pixel_recording_size,
On 2017/06/15 at 22:59:13, oshima wrote:
> do you need this change?

Updating the comment instead.

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:194: void Scale(float x_scale, float y_scale);
On 2017/06/15 at 22:59:13, oshima wrote:
> Can you land this fix separately?

This is required only for Pixel canvas to scale with fractional values. I think
it should go along with this change.

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc#newco...
ui/views/view.cc:891:
SkFloatToScalar(parent_context.effective_scale_factor_y())));
On 2017/06/15 at 22:59:13, oshima wrote:
> shouldn't this match the scale type requested?

effective_scale_factor_x() and effective_scale_factor_y() handle this in their
impl.

if (scale_type_ == SCALE_TO_SCALE_FACTOR)
  return device_scale_factor_;

https://codereview.chromium.org/2877483003/diff/220001/ui/views/view.cc#newco...
ui/views/view.cc:1989: gfx::Vector2d offset_from_parent =
context.pixel_bounds().OffsetFromOrigin();
On 2017/06/15 at 22:59:13, oshima wrote:
> Don't you have to mirror this for RTL?

context.pixel_bounds() is already mirrored since we use GetPaintContextBounds()
which calls GetMirroredBounds().

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:127: }
On 2017/06/15 at 22:59:13, oshima wrote:
> The pixel bounds should always be the scale to fit. Scaling logic should
affect only painting logic, not the bounds calculation (otherwise it'll affect
children's bounds).

Done

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/compositor/paint_co...
ui/compositor/paint_context.h:99: gfx::Rect ScaleToEffectivePixelBounds(const
gfx::Rect& bounds) const;
On 2017/06/15 at 22:59:13, oshima wrote:
> looks like this isn't used? (sorry if I missed it)

It isnt used in the current change. 
Done

https://codereview.chromium.org/2877483003/diff/240001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2877483003/diff/240001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:387: ui::PaintRecorder recorder(context,
context.ScaleToEffectivePixelSize(size()),
On 2017/06/16 at 00:15:03, oshima wrote:
> shouldn't this be context.pixel_size()? and if so, can we eliminate the
parameter?

I think its not always the case. But in this case it may be true.

[malaykeshav, 2017-06-16 22:29:02]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 22:29:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2017-06-17 00:55:48]

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:194: void Scale(float x_scale, float y_scale);
On 2017/06/16 20:41:58, malaykeshav wrote:
> On 2017/06/15 at 22:59:13, oshima wrote:
> > Can you land this fix separately?
> 
> This is required only for Pixel canvas to scale with fractional values. I
think
> it should go along with this change.

I mean this isn't just a bug that we need to fix anyway?

https://codereview.chromium.org/2877483003/diff/300001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/300001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:59: scale_type_(scale_type) {
I wasn't clear, sorry. Scale type shouldn't be a part of paint context. This
type should be applied only to painting (thus, clipping) logic. 

the scale type should be used inside View::Paint to clip & paint accordingly.

ping me or drop in when you have a chance.

[commit-bot: I haz the power, 2017-06-17 01:07:02]

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

[commit-bot: I haz the power, 2017-06-17 01:07:03]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-06-17 01:45:27]

On 2017/06/17 at 00:55:48, oshima wrote:
> https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h
> File ui/gfx/canvas.h (right):
> 
>
https://codereview.chromium.org/2877483003/diff/220001/ui/gfx/canvas.h#newcod...
> ui/gfx/canvas.h:194: void Scale(float x_scale, float y_scale);
> On 2017/06/16 20:41:58, malaykeshav wrote:
> > On 2017/06/15 at 22:59:13, oshima wrote:
> > > Can you land this fix separately?
> > 
> > This is required only for Pixel canvas to scale with fractional values. I
think
> > it should go along with this change.
> 
> I mean this isn't just a bug that we need to fix anyway?
> 
>
https://codereview.chromium.org/2877483003/diff/300001/ui/compositor/paint_co...
> File ui/compositor/paint_context.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/300001/ui/compositor/paint_co...
> ui/compositor/paint_context.cc:59: scale_type_(scale_type) {
> I wasn't clear, sorry. Scale type shouldn't be a part of paint context. This
type should be applied only to painting (thus, clipping) logic. 
> 
> the scale type should be used inside View::Paint to clip & paint accordingly.
> 
> ping me or drop in when you have a chance.

Done

[malaykeshav, 2017-06-17 01:45:47]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 01:45:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-17 03:14:19]

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

[commit-bot: I haz the power, 2017-06-17 03:14:20]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-06-17 16:05:53]

https://codereview.chromium.org/2877483003/diff/300001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/300001/ui/views/view.h#newcod...
ui/views/view.h:1051: virtual int GetContextScaleType() const;
can you make this protected?

[malaykeshav, 2017-06-20 18:03:22]

On 2017/06/17 at 16:05:53, oshima wrote:
> https://codereview.chromium.org/2877483003/diff/300001/ui/views/view.h
> File ui/views/view.h (right):
> 
>
https://codereview.chromium.org/2877483003/diff/300001/ui/views/view.h#newcod...
> ui/views/view.h:1051: virtual int GetContextScaleType() const;
> can you make this protected?

Done

[malaykeshav, 2017-06-20 18:03:28]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 18:03:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-06-20 18:04:42]

Patchset #13 (id:340001) has been deleted

[malaykeshav, 2017-06-20 18:04:47]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 18:05:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 21:19:12]

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

[commit-bot: I haz the power, 2017-06-20 21:19:14]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-06-22 15:24:12]

https://codereview.chromium.org/2877483003/diff/360001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:32: // |recording_size| should be in pixels if
pixel canvas is enabled.
I think it's better to hide this unless it's necessary. An argument that changes
meaning depending on the mode is also confusing and error prone, at least until
transition is complete.

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:892: context.pixel_bounds().OffsetFromOrigin().y());
can we move the clipping function to context as well? We should try to hide API
that may return pixel or dip as it's confusing.

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:904: ui::PaintRecorder recorder(context, context.pixel_size(),
&paint_cache_);
same here. I'm still not convinced that extra argument, which can be derived
from the argument already passed, is useful unless it has usage.

[malaykeshav, 2017-06-22 20:26:02]

Patchset #14 (id:380001) has been deleted

[malaykeshav, 2017-06-22 20:26:07]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 20:26:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-06-22 20:29:03]

Patchset #14 (id:400001) has been deleted

[malaykeshav, 2017-06-22 20:29:09]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 20:29:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-06-22 21:02:28]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[malaykeshav, 2017-06-22 21:02:37]

Patchset #14 (id:420001) has been deleted

[malaykeshav, 2017-06-22 21:02:43]

Patchset #14 (id:440001) has been deleted

[malaykeshav, 2017-06-22 21:02:53]

PTAL

https://codereview.chromium.org/2877483003/diff/360001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:32: // |recording_size| should be in pixels if
pixel canvas is enabled.
On 2017/06/22 at 15:24:11, oshima wrote:
> I think it's better to hide this unless it's necessary. An argument that
changes meaning depending on the mode is also confusing and error prone, at
least until transition is complete.

Done

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:892: context.pixel_bounds().OffsetFromOrigin().y());
On 2017/06/22 at 15:24:11, oshima wrote:
> can we move the clipping function to context as well? We should try to hide
API that may return pixel or dip as it's confusing.

Done

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:904: ui::PaintRecorder recorder(context, context.pixel_size(),
&paint_cache_);
On 2017/06/22 at 15:24:11, oshima wrote:
> same here. I'm still not convinced that extra argument, which can be derived
from the argument already passed, is useful unless it has usage.

A lot of files are calling the PaintRecorder's second constructor so adding a
TODO for now and we can change this in subsequent CLs.

[commit-bot: I haz the power, 2017-06-22 21:03:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2017-06-23 18:42:32]

It's better to avoid exposing "pixelxx" outside of ui/compositor.

Can you try to hide pixel_bounds and pixel_size()?

(SetupTransformRecorderForPainting doesn't seem to rely in Views other than
layer() check, for example).

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:904: ui::PaintRecorder recorder(context, context.pixel_size(),
&paint_cache_);
On 2017/06/22 21:02:53, malaykeshav wrote:
> On 2017/06/22 at 15:24:11, oshima wrote:
> > same here. I'm still not convinced that extra argument, which can be derived
> from the argument already passed, is useful unless it has usage.
> 
> A lot of files are calling the PaintRecorder's second constructor so adding a
> TODO for now and we can change this in subsequent CLs.

You can keep the second ctr. 

There are only 3~4 places that are using this ctr, and they are already in this
CL.

[oshima, 2017-06-23 18:49:36]

by the way, we should get feedback from owners by now.
(sky@ and danakj@)

[malaykeshav, 2017-06-23 18:53:21]

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:904: ui::PaintRecorder recorder(context, context.pixel_size(),
&paint_cache_);
On 2017/06/23 at 18:42:32, oshima wrote:
> On 2017/06/22 21:02:53, malaykeshav wrote:
> > On 2017/06/22 at 15:24:11, oshima wrote:
> > > same here. I'm still not convinced that extra argument, which can be
derived
> > from the argument already passed, is useful unless it has usage.
> > 
> > A lot of files are calling the PaintRecorder's second constructor so adding
a
> > TODO for now and we can change this in subsequent CLs.
> 
> You can keep the second ctr. 
> 
> There are only 3~4 places that are using this ctr, and they are already in
this CL.

The second constructor needs this constructor to have the size argument.

[malaykeshav, 2017-06-23 18:54:07]

malaykeshav@chromium.org changed reviewers:
+ danakj@chromium.org, sky@chromium.org

[malaykeshav, 2017-06-23 18:54:08]

+sky and +danakj 
PTAL

[malaykeshav, 2017-06-23 18:54:34]

Description was changed from

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slides
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[malaykeshav, 2017-06-23 18:55:00]

Description was changed from

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slides
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slide
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sky, 2017-06-23 23:40:24]

I don't see any new assertions in views_unittests. Can you please add test
coverage?

Also, my understanding is this effects the transform of views. Does event
delivery need to be updated?

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.h:29: enum ScaleType {
enum class.

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.cc#newco...
ui/views/view.cc:1971: // add the offset from the parent into the total offset
from the paint root.
Update comment. I'm not clear on why this is GetLocalBounds() can you elaborate
on that?

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.h#newcod...
ui/views/view.h:1275: virtual int GetContextScaleType() const;
Why is this virtual? Why would it need to be overriden? And why is the return
type int instead of ScaleType?

[malaykeshav, 2017-06-24 00:30:03]

I couldn't come up with a reasonable new unittest for views. Majority of the
changes are in PaintContext, for which I have added thorough testing. Ideally I
could run all the view unittest twice, once with experiment enabled, and again
with it disabled. If required, I can add that. The assertions would all be the
same.


This does not make any changes to the view transform so event propagation should
be as is.


(Did not respond to one of the comments to remind me on Monday to fix it.)

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.cc#newco...
ui/views/view.cc:1971: // add the offset from the parent into the total offset
from the paint root.
On 2017/06/23 at 23:40:23, sky wrote:
> Update comment. I'm not clear on why this is GetLocalBounds() can you
elaborate on that?

In case of a layer, the context is the root context; so offset has not meaning.
GetLocalBounds() returns no offset, and only the size.

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/views/view.h#newcod...
ui/views/view.h:1275: virtual int GetContextScaleType() const;
On 2017/06/23 at 23:40:23, sky wrote:
> Why is this virtual? Why would it need to be overriden? And why is the return
type int instead of ScaleType?

The ScaleType can vary depending on whether we are scaling to fit/snap (default)
or 
if we are simply scaling with no snapping.

With scaling by fractional scale factors, the effective scale factor is
different 
from the original scale factor due to rounding. 

 - In Scale to Fit/Snap, all the draw commands are scaled via the effective
scale 
   factor.
 - But this leads to some undesired scaling in terms of images or vector icons, 
   that require consistent scaling along x and y axis. (effective scale along x 
   may not be equal to effective scale along y). For such cases we use Scale To 
   Scale Factor. Which scales all draw commands with device_scale_factor_. But
the 
   canvas is still bounded with the scale to fit bounds (For any child snapping
and
   FillCanvas() like commands).

By default this function returns SCALE_TO_FIT. In case a view wants to
SCALE_TO_SCALE_FACTOR, it can
override accordingly and return the correct enum.
This can also be made a member variable along with a setter.

I was using int as the return type to prevent including paint_context.h files in
all the header files that were overriding this function.

[malaykeshav, 2017-06-24 00:49:36]

> This does not make any changes to the view transform so event propagation
should be as is.

There can actually be an accuracy error of upto dsf pixels in some cases due to
the snap.

[malaykeshav, 2017-06-24 01:02:36]

> Does event delivery need to be updated?

It should not be a big issue as the error would be of around 1px  in few cases.
But if required, this 
can also be fixed by applying the same scaling logic to event propagation.
(Scaling using the corner
points)

[sky, 2017-06-26 15:40:38]

Do any changes need to be done to code that draws at 1x by resetting the
scale?

On Fri, Jun 23, 2017 at 6:02 PM, <malaykeshav@chromium.org> wrote:

> > Does event delivery need to be updated?
>
> It should not be a big issue as the error would be of around 1px in few
> cases.
> But if required, this
> can also be fixed by applying the same scaling logic to event propagation.
> (Scaling using the corner
> points)
>
>
> https://codereview.chromium.org/2877483003/
>

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

[malaykeshav, 2017-06-26 16:53:41]

On 2017/06/26 at 15:40:38, sky wrote:
> Do any changes need to be done to code that draws at 1x by resetting the
> scale?

Are you referring to UndoDeviceScaleFactor()?
https://cs.chromium.org/chromium/src/ui/gfx/canvas.h?q=UndoDeviceScaleFactor&...

[sky, 2017-06-26 20:50:46]

Yes.

On Mon, Jun 26, 2017 at 9:53 AM, <malaykeshav@chromium.org> wrote:

> On 2017/06/26 at 15:40:38, sky wrote:
> > Do any changes need to be done to code that draws at 1x by resetting the
> > scale?
>
> Are you referring to UndoDeviceScaleFactor()?
> https://cs.chromium.org/chromium/src/ui/gfx/canvas.h?
> q=UndoDeviceScaleFactor&l=156
>
> https://codereview.chromium.org/2877483003/
>

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

[malaykeshav, 2017-06-26 21:01:15]

On 2017/06/26 at 20:50:46, sky wrote:
> Yes.
> 
> On Mon, Jun 26, 2017 at 9:53 AM, <malaykeshav@chromium.org> wrote:
> 
> > On 2017/06/26 at 15:40:38, sky wrote:
> > > Do any changes need to be done to code that draws at 1x by resetting the
> > > scale?

No change is needed. They still work as is. But with Pixel Canvas we can get
more accurate results than UndoDeviceScaleFactor().We would be able to pass in
the size in pixels when required.

Something like |OnPixelPaint(canvas, pixel_size)| which will be called instead
of |OnPaint(canvas)| for views that wish to paint at pixel dimensions. And
enabled by view->set_pixel_paint(true);

[sky, 2017-06-26 22:15:03]

I would prefer not to introduce another paint function. I think that's just
going to make things more complex.

  -Scott

On Mon, Jun 26, 2017 at 2:01 PM, <malaykeshav@chromium.org> wrote:

> On 2017/06/26 at 20:50:46, sky wrote:
> > Yes.
> >
> > On Mon, Jun 26, 2017 at 9:53 AM, <malaykeshav@chromium.org&gt; wrote:
> >
> > > On 2017/06/26 at 15:40:38, sky wrote:
> > > > Do any changes need to be done to code that draws at 1x by resetting
> the
> > > > scale?
>
> No change is needed. They still work as is. But with Pixel Canvas we can
> get
> more accurate results than UndoDeviceScaleFactor().We would be able to
> pass in
> the size in pixels when required.
>
> Something like |OnPixelPaint(canvas, pixel_size)| which will be called
> instead
> of |OnPaint(canvas)| for views that wish to paint at pixel dimensions. And
> enabled by view->set_pixel_paint(true);
>
>
> https://codereview.chromium.org/2877483003/
>

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

[malaykeshav, 2017-06-26 23:12:02]

On 2017/06/26 at 22:15:03, sky wrote:
> I would prefer not to introduce another paint function. I think that's just
> going to make things more complex.
> 
>   -Scott

The primary reason to add another paint function is to pass down the pixel_size
of the view during paint. Which 
would always be required when doing Pixel painting instead of DIP. Without the
correct device_scale_factor or the 
accurate pixel size, OnPaint(canvas) cannot paint in pixels.

I would be more inclined towards having another paint function (or overloaded or
even with a default argument)
  - OnPixelPaint(canvas, pixel_size) or 
    OR
  - OnPaint(canvas, pixel_size)
, as it keeps things less error prone, clear and non convoluted.
It can easily be merged when we use pixel coordinates everywhere.


Alternatively, we can expose the size from gfx::Canvas, which is always in pixel
coordinates when this feature is
enabled. So instead of doing an UndoDeviceScaleFactor(), the developer can
instead call set_paint_to_pixel(true) 
and get the pixel_size of the view during paint via canvas->size(). Although
this is not very consistent since
canvas->size() is always in pixels whether |set_paint_to_pixel()| is set or not.

IMO this would be a very hacky alternative.

[sky, 2017-06-27 03:25:16]

The problem is it's not going to stop there. You'll quickly find you need
to convert other APIs  and you're more likely to run into conversion
issues. I'm fine with *just* this change. If we start wanting to expand it,
then we should instead focus on converting views to pixels.

  -Scott

On Mon, Jun 26, 2017 at 4:12 PM, <malaykeshav@chromium.org> wrote:

> On 2017/06/26 at 22:15:03, sky wrote:
> > I would prefer not to introduce another paint function. I think that's
> just
> > going to make things more complex.
> >
> > -Scott
>
> The primary reason to add another paint function is to pass down the
> pixel_size
> of the view during paint. Which
> would always be required when doing Pixel painting instead of DIP. Without
> the
> correct device_scale_factor or the
> accurate pixel size, OnPaint(canvas) cannot paint in pixels.
>
> I would be more inclined towards having another paint function (or
> overloaded or
> even with a default argument)
> - OnPixelPaint(canvas, pixel_size) or
> OR
> - OnPaint(canvas, pixel_size)
> , as it keeps things less error prone, clear and non convoluted.
> It can easily be merged when we use pixel coordinates everywhere.
>
>
> Alternatively, we can expose the size from gfx::Canvas, which is always in
> pixel
> coordinates when this feature is
> enabled. So instead of doing an UndoDeviceScaleFactor(), the developer can
> instead call set_paint_to_pixel(true)
> and get the pixel_size of the view during paint via canvas->size().
> Although
> this is not very consistent since
> canvas->size() is always in pixels whether |set_paint_to_pixel()| is set
> or not.
>
> IMO this would be a very hacky alternative.
>
>
> https://codereview.chromium.org/2877483003/
>

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

[danakj, 2017-06-27 16:28:36]

I think overall I'm concerned about the approach wrt code layering. How a view
wants to paint is up to the View, and that this is being done in PaintContext,
with ui-painting concepts also leaking into cc both seem incorrect. Why isn't
View::Paint setting its transforms up itself? If this is based on a command
line, why aren't we setting the PaintedDeviceScale on LayerTreeHost instead of
DeviceScale, the way blink does, so that cc won't apply the DSF to its raster
automatically?

https://codereview.chromium.org/2877483003/diff/480001/cc/raster/raster_sourc...
File cc/raster/raster_source.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/cc/raster/raster_sourc...
cc/raster/raster_source.cc:83: if (!display_list_->pixel_canvas_enabled())
I don't think this is okay, this assumes that the transform seen at painting is
always the same as at raster, but if a scale is applied in the compositor then
it doesn't have to be.

This also feels very weird WRT layering, in that DisplayItemList is knowing
things about ui painting things and this seems problematic to me.

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/layer.cc...
ui/compositor/layer.cc:923: PaintContext context(display_list.get(),
device_scale_factor_, invalidation,
why did you want to make this an lvalue?

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:42: pixel_bounds_(ScaleToCornerScaledRect(
It is weird to me that this context has a size now. A context is a pipe into
which we put recording commands. A recording doesn't have a size, it has a list
of commands. The layer has a size, the view has a size, the recording doesn't.

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.h:32: SCALE_TO_SCALE_FACTOR = 0,
Why is this a property of a PaintContext? A view can set up a transform as it
wants when inserting commands into the recording. PaintContext seems like the
wrong place for this to me.

[malaykeshav, 2017-06-29 01:03:32]

malaykeshav@chromium.org changed reviewers:
+ vmpstr@chromium.org

[malaykeshav, 2017-06-29 01:03:33]

+vmpstr

[malaykeshav, 2017-07-07 18:09:24]

Patchset #16 (id:500001) has been deleted

[malaykeshav, 2017-07-07 20:54:31]

Patchset #16 (id:520001) has been deleted

[malaykeshav, 2017-07-07 21:03:22]

Patchset #16 (id:540001) has been deleted

[malaykeshav, 2017-07-07 21:03:31]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 21:04:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 21:08:51]

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

[commit-bot: I haz the power, 2017-07-07 21:08:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-07-07 22:29:31]

Patchset #17 (id:580001) has been deleted

[malaykeshav, 2017-07-07 22:42:44]

Patchset #17 (id:600001) has been deleted

[malaykeshav, 2017-07-07 22:43:00]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 22:43:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-07-07 22:56:42]

https://codereview.chromium.org/2877483003/diff/620001/cc/raster/raster_sourc...
File cc/raster/raster_source.cc (right):

https://codereview.chromium.org/2877483003/diff/620001/cc/raster/raster_sourc...
cc/raster/raster_source.cc:249: gfx::ScaleToCornerScaledRect(layer_rect,
recording_scale_factor_);
Could you add a couple of tests to raster_source_unittest that has different
recording_scale_factors?

[malaykeshav, 2017-07-07 22:57:10]

@danakj @vmpstr
PTAL

https://codereview.chromium.org/2877483003/diff/480001/cc/raster/raster_sourc...
File cc/raster/raster_source.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/cc/raster/raster_sourc...
cc/raster/raster_source.cc:83: if (!display_list_->pixel_canvas_enabled())
On 2017/06/27 at 16:28:36, danakj wrote:
> I don't think this is okay, this assumes that the transform seen at painting
is always the same as at raster, but if a scale is applied in the compositor
then it doesn't have to be.
> 
> This also feels very weird WRT layering, in that DisplayItemList is knowing
things about ui painting things and this seems problematic to me.

This has been updated after offline discussion.
The value is now plumbed to the raster source as a recording_scale_factor. This
component is then removed from the raster transform.

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/layer.cc...
ui/compositor/layer.cc:923: PaintContext context(display_list.get(),
device_scale_factor_, invalidation,
On 2017/06/27 at 16:28:36, danakj wrote:
> why did you want to make this an lvalue?

Done

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:42: pixel_bounds_(ScaleToCornerScaledRect(
On 2017/06/27 at 16:28:36, danakj wrote:
> It is weird to me that this context has a size now. A context is a pipe into
which we put recording commands. A recording doesn't have a size, it has a list
of commands. The layer has a size, the view has a size, the recording doesn't.

Agreed on the fact that the PaintContext does not have a size. A better name
that reflects
what it represents would be paint_recording_bounds. The PaintContext would be
the correct 
place for it.

The concept of recording bounds only exists during Paint, so it cannot be a part
of views or layers.

OTOH |bounds_| can be removed, as they represent the bounds of a layer/view.

Adding paint_recording_bounds and paint_recording_scale would also make the
terminologies 
consistent with the cc side changes for recording_scale_factor. This also
removes the concept of 
pixels from PaintContext which was also confusing.

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.h:29: enum ScaleType {
On 2017/06/23 at 23:40:23, sky OOO wrote:
> enum class.

Done

https://codereview.chromium.org/2877483003/diff/480001/ui/compositor/paint_co...
ui/compositor/paint_context.h:32: SCALE_TO_SCALE_FACTOR = 0,
On 2017/06/27 at 16:28:36, danakj wrote:
> Why is this a property of a PaintContext? A view can set up a transform as it
wants when inserting commands into the recording. PaintContext seems like the
wrong place for this to me.

These are the scales that set the recording bounds for the paint recorder.
Depending on the enum type, when the recording bounds for the given PaintContext
is computed, the bounds may or may not be snapped.

This can also be moved to PaintRecorder if PaintContext does not look like the
right place for it.

[malaykeshav, 2017-07-08 00:14:35]

On 2017/07/07 at 22:56:42, vmpstr wrote:
>
https://codereview.chromium.org/2877483003/diff/620001/cc/raster/raster_sourc...
> File cc/raster/raster_source.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/620001/cc/raster/raster_sourc...
> cc/raster/raster_source.cc:249: gfx::ScaleToCornerScaledRect(layer_rect,
recording_scale_factor_);
> Could you add a couple of tests to raster_source_unittest that has different
recording_scale_factors?

Done

[vmpstr, 2017-07-10 16:20:34]

cc lgtm

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:1091: recording_scale_factor_ =
recording_scale_factor;
nit: I think we might need a SetNeedsCommit here.

[malaykeshav, 2017-07-10 18:03:53]

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:1091: recording_scale_factor_ =
recording_scale_factor;
On 2017/07/10 at 16:20:34, vmpstr wrote:
> nit: I think we might need a SetNeedsCommit here.

The value is not synced to the LayerTreeImpl. Does it still need a
SetNeedsCommit() ?

[vmpstr, 2017-07-10 18:06:26]

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2877483003/diff/640001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:1091: recording_scale_factor_ =
recording_scale_factor;
On 2017/07/10 18:03:53, malaykeshav wrote:
> On 2017/07/10 at 16:20:34, vmpstr wrote:
> > nit: I think we might need a SetNeedsCommit here.
> 
> The value is not synced to the LayerTreeImpl. Does it still need a
> SetNeedsCommit() ?

Oh you're right, it's just set on the recording source if there's a new one.
That makes sense. In that case, you probably don't want the if statement on 1089
(ie just set it unconditionally).

[malaykeshav, 2017-07-11 02:56:14]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 02:56:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-11 02:58:46]

Patchset #19 (id:660001) has been deleted

[malaykeshav, 2017-07-11 02:58:54]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 02:59:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 04:13:41]

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

[commit-bot: I haz the power, 2017-07-11 04:13:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[malaykeshav, 2017-07-11 05:26:21]

Patchset #19 (id:680001) has been deleted

[malaykeshav, 2017-07-11 05:26:29]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 05:26:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 06:34:26]

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

[commit-bot: I haz the power, 2017-07-11 06:34:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-07-11 08:31:21]

Patchset #19 (id:700001) has been deleted

[malaykeshav, 2017-07-11 08:31:28]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 08:31:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 10:20:40]

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

[commit-bot: I haz the power, 2017-07-11 10:20:41]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-07-11 17:37:22]

I looked at some UI stuff as well. I'm by no means an expert there, but
hopefully my comments can improve the direction a bit.

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:33: // to be done via corner points. See
go/pixel-canvas
can you use a public link shortener (like goo.gl) for the doc link, since it
should be accessible externally.

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:56: void SetPixelCanvasForTesting(bool
value) {
I think this function should be called EnablePixelCanvasForTesting or something
like that, since it can't really disable the pixel canvas if one is set. Or are
you relying on the fact that it's off by default?

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/paint_co...
ui/compositor/paint_context.h:95: gfx::Transform
TransformToParentRecordingSpace(
Just some general thoughts:

- It feels like ui::PaintContext is becoming more of transport for things that
seem to be specific to ui::View. PaintContext is also used in a bunch of places
outside of views that don't need any of this functionality. Is that correct?
- Can some of the functionality like this one move to ui::View? What I'm trying
to do is to limit the exposure of the pixel canvas and this new recording mode
to the place where it's used (ie views). 
- Some of this functionality can be moved to views I think, and some maybe to
the recording canvas. 

FWIW, I think the code looks reasonable, it's just it seems that PaintContext is
becoming a places where a bunch of unrelated concepts live, and I think we
should avoid that.

https://codereview.chromium.org/2877483003/diff/720001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/gfx/geometry/rect.h...
ui/gfx/geometry/rect.h:343: inline Rect ScaleToCornerScaledRect(const Rect&
rect,
Should this be called something like ScaleToRoundedRect? I'm not really sure
what CornerScaledRect is in this context

https://codereview.chromium.org/2877483003/diff/720001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/views/view.cc#newco...
ui/views/view.cc:1988: // If the View has a layer() then it is a paint root.
Otherwise, we need to
Can you update the comment?

[malaykeshav, 2017-07-11 18:58:33]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 18:58:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-11 19:26:23]

Patchset #20 (id:740001) has been deleted

[malaykeshav, 2017-07-11 19:26:30]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 19:26:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 20:01:58]

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

[commit-bot: I haz the power, 2017-07-11 20:02:00]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[malaykeshav, 2017-07-11 21:12:04]

Patchset #20 (id:760001) has been deleted

[malaykeshav, 2017-07-11 21:12:10]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 21:13:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 21:32:59]

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

[commit-bot: I haz the power, 2017-07-11 21:33:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[malaykeshav, 2017-07-11 21:40:09]

Patchset #20 (id:780001) has been deleted

[malaykeshav, 2017-07-11 21:40:20]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 21:40:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-12 00:55:37]

PTAL

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:33: // to be done via corner points. See
go/pixel-canvas
On 2017/07/11 at 17:37:21, vmpstr wrote:
> can you use a public link shortener (like goo.gl) for the doc link, since it
should be accessible externally.

Done

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:56: void SetPixelCanvasForTesting(bool
value) {
On 2017/07/11 at 17:37:21, vmpstr wrote:
> I think this function should be called EnablePixelCanvasForTesting or
something like that, since it can't really disable the pixel canvas if one is
set. Or are you relying on the fact that it's off by default?

During unittests, the switch is disabled, so kIsPixelCanvasRecordingEnabled is
always set to false. kEnablePixelCanvasForTesting becomes the only way to enable
or disable PixelCanvas.

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/compositor/paint_co...
ui/compositor/paint_context.h:95: gfx::Transform
TransformToParentRecordingSpace(
On 2017/07/11 at 17:37:22, vmpstr wrote:
> Just some general thoughts:
> 
> - It feels like ui::PaintContext is becoming more of transport for things that
seem to be specific to ui::View. PaintContext is also used in a bunch of places
outside of views that don't need any of this functionality. Is that correct?
> - Can some of the functionality like this one move to ui::View? What I'm
trying to do is to limit the exposure of the pixel canvas and this new recording
mode to the place where it's used (ie views). 
> - Some of this functionality can be moved to views I think, and some maybe to
the recording canvas. 
> 
> FWIW, I think the code looks reasonable, it's just it seems that PaintContext
is becoming a places where a bunch of unrelated concepts live, and I think we
should avoid that.

Done.
Moving this to views::View() and exposing paint_recording_scale_

https://codereview.chromium.org/2877483003/diff/720001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/gfx/geometry/rect.h...
ui/gfx/geometry/rect.h:343: inline Rect ScaleToCornerScaledRect(const Rect&
rect,
On 2017/07/11 at 17:37:22, vmpstr wrote:
> Should this be called something like ScaleToRoundedRect? I'm not really sure
what CornerScaledRect is in this context

Done. 
Forgot to rename when moved it here from PaintContext.

https://codereview.chromium.org/2877483003/diff/720001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/720001/ui/views/view.cc#newco...
ui/views/view.cc:1988: // If the View has a layer() then it is a paint root.
Otherwise, we need to
On 2017/07/11 at 17:37:22, vmpstr wrote:
> Can you update the comment?

Done

[commit-bot: I haz the power, 2017-07-12 01:13:13]

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

[commit-bot: I haz the power, 2017-07-12 01:13:15]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-07-12 17:48:19]

lgtm with comments

https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:72: if (!IsPixelCanvas()) {
Can you leave a comment here explaining? It's not immediately obvious to me why
we don't use device_scale_factor_ instead of 1.f

https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest...
ui/views/view_unittest.cc:529: auto list = make_scoped_refptr(new
cc::DisplayItemList);
Why this change?

[malaykeshav, 2017-07-12 18:05:06]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-12 18:05:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-12 18:08:23]

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

[malaykeshav, 2017-07-12 18:08:26]

+piman for OWNERS on ui/compositor/ changes. 
Dana is on leave this week. Trying to get this in by M61.

https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:72: if (!IsPixelCanvas()) {
On 2017/07/12 at 17:48:19, vmpstr wrote:
> Can you leave a comment here explaining? It's not immediately obvious to me
why we don't use device_scale_factor_ instead of 1.f

Done

https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest...
ui/views/view_unittest.cc:529: auto list = make_scoped_refptr(new
cc::DisplayItemList);
On 2017/07/12 at 17:48:19, vmpstr wrote:
> Why this change?

This must have happened during rebasing.
Done.

[commit-bot: I haz the power, 2017-07-12 19:57:30]

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

[commit-bot: I haz the power, 2017-07-12 19:57:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[malaykeshav, 2017-07-13 20:01:38]

On 2017/07/12 at 18:08:26, malaykeshav wrote:
> +piman for OWNERS on ui/compositor/ changes. 
> Dana is on leave this week. Trying to get this in by M61.
> 
>
https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
> File ui/compositor/paint_context.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/800001/ui/compositor/paint_co...
> ui/compositor/paint_context.cc:72: if (!IsPixelCanvas()) {
> On 2017/07/12 at 17:48:19, vmpstr wrote:
> > Can you leave a comment here explaining? It's not immediately obvious to me
why we don't use device_scale_factor_ instead of 1.f
> 
> Done
> 
>
https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest.cc
> File ui/views/view_unittest.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/800001/ui/views/view_unittest...
> ui/views/view_unittest.cc:529: auto list = make_scoped_refptr(new
cc::DisplayItemList);
> On 2017/07/12 at 17:48:19, vmpstr wrote:
> > Why this change?
> 
> This must have happened during rebasing.
> Done.

ping @piman @sky

[danakj, 2017-07-18 22:13:35]

Sorry, I'll have a look at ui/ tomorrow. Pinging myself to remember.

[danakj, 2017-07-19 17:00:59]

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:35: // If enabled, all draw commnads
recorded on canvas are done in pixel aligned
commands

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.cc:16: const gfx::Size& size)
I was thinking about this and wondering if the reason all this code is going in
PaintContext is because View::Paint() is the recursion function, and it is also
the public function called from ui::Layer?

What if you had View::Paint as a public method that takes PaintContext, and a
View::PaintInternal as a private method that takes PaintContext and some
View-specific data stuff, and View::Paint calls PaintInternal which recurses
thru PaintInternal not thru Paint? Would that change your strategy here?

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:31: SCALE_TO_SCALE_FACTOR = 0,
enums should be kFoo style now

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:118: // |parent bound|'s right and/or bottom edge
if required.
|parent_bounds|'

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:119: // This function returns |child_bounds| as is
if pixel canvas is disabled.
"as is if" reads a bit awkward, maybe reorder things in this sentence or choose
diff words

[malaykeshav, 2017-07-21 00:12:34]

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:35: // If enabled, all draw commnads
recorded on canvas are done in pixel aligned
On 2017/07/19 at 17:00:59, danakj wrote:
> commands

Done

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:31: SCALE_TO_SCALE_FACTOR = 0,
On 2017/07/19 at 17:00:59, danakj wrote:
> enums should be kFoo style now

Done

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:118: // |parent bound|'s right and/or bottom edge
if required.
On 2017/07/19 at 17:00:59, danakj wrote:
> |parent_bounds|'

Done

https://codereview.chromium.org/2877483003/diff/820001/ui/compositor/paint_co...
ui/compositor/paint_context.h:119: // This function returns |child_bounds| as is
if pixel canvas is disabled.
On 2017/07/19 at 17:00:59, danakj wrote:
> "as is if" reads a bit awkward, maybe reorder things in this sentence or
choose diff words

Done

https://codereview.chromium.org/2877483003/diff/860001/ui/compositor/paint_in...
File ui/compositor/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/860001/ui/compositor/paint_in...
ui/compositor/paint_info.h:13: class COMPOSITOR_EXPORT PaintInfo {
Could be renamed to ViewPaintContext.

[danakj, 2017-07-21 17:29:39]

Thanks, this looks better to me. We should avoid globals such as the one being
used to fake a command line (command lines are also globals, so we should limit
their scope to make unit tests easier to write to test all behaviours). I have
some concrete feedback to that aim below.

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
File ui/compositor/canvas_painter.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
ui/compositor/canvas_painter.cc:25: IsPixelCanvasRecordingEnabled() ?
raster_scale : 1.f)) {}
Rather than reading the command line and trying to intercept that for tests, can
you just move the decision to use this higher and pass in the information this
class needs?

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
ui/compositor/compositor.cc:326: if (IsPixelCanvasRecordingEnabled())
Can this be passed to the Constructor instead so unit tests can set it without
having to muck with trying to intercept command line?

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc...
ui/compositor/layer.cc:926: gfx::ScaleToRoundedRect(invalidation,
IsPixelCanvasRecordingEnabled()
Can this read the value off the compositor instead of the command line?

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:16: gfx::ScaleToRoundedRect(gfx::Rect(size),
paint_recording_scale_x_)),
Rounding is almost always not what you want to do when you scale. Scaling means
you can have partial pixels in your rect. Rounding means you don't know if those
partial pixels will be included in the resulting rect or not.

Can you explain why rounding is the right thing here? What are all the edge
cases and how it does the right thing?

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:45: return IsPixelCanvasRecordingEnabled();
pass this in the constructor, instead of reading the command line. Unit tests
should just be able to make objects and pass in how they want them to run,
rather than setting global states that get read later.

Two choices I think:
1) Leave the invalidation in DIP on the PaintContext. Consumer of PaintContext
(View::PaintFromPaintRoot) scales the invalidation if the
compositor->RecordPixelCanvas() or whatever is true, and passes that bool to the
PaintInfo if needed as well.

2) Scale the invalidation on the PaintContext. Then the user of PaintContext
needs to know what state the PaintContext uses, so pass the
compositor->RecordPixelCanvas() or whatever to the PaintContext constructor and
expose it like the DSF as a property.

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.h:13: class COMPOSITOR_EXPORT PaintInfo {
move this to ui/views/? It's not used by compositor APIs

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:23: PaintRecorder::PaintRecorder(const
PaintInfo& info, PaintCache* cache)
Can you pass in the various things it needs here instead of the whole PaintInfo?

[malaykeshav, 2017-07-21 22:18:06]

Patchset #25 (id:900001) has been deleted

[malaykeshav, 2017-07-21 22:19:23]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 22:19:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-21 22:24:12]

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

[commit-bot: I haz the power, 2017-07-21 22:24:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[malaykeshav, 2017-07-21 23:22:28]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 23:22:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-21 23:30:15]

malaykeshav@chromium.org changed reviewers:
- piman@chromium.org

[malaykeshav, 2017-07-21 23:30:17]

@sky @dana
PTAL

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
File ui/compositor/canvas_painter.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
ui/compositor/canvas_painter.cc:25: IsPixelCanvasRecordingEnabled() ?
raster_scale : 1.f)) {}
On 2017/07/21 at 17:29:39, danakj wrote:
> Rather than reading the command line and trying to intercept that for tests,
can you just move the decision to use this higher and pass in the information
this class needs?

Done

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
ui/compositor/compositor.cc:326: if (IsPixelCanvasRecordingEnabled())
On 2017/07/21 at 17:29:39, danakj wrote:
> Can this be passed to the Constructor instead so unit tests can set it without
having to muck with trying to intercept command line?

Done

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc...
ui/compositor/layer.cc:926: gfx::ScaleToRoundedRect(invalidation,
IsPixelCanvasRecordingEnabled()
On 2017/07/21 at 17:29:39, danakj wrote:
> Can this read the value off the compositor instead of the command line?

Done

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:16: gfx::ScaleToRoundedRect(gfx::Rect(size),
paint_recording_scale_x_)),
On 2017/07/21 at 17:29:39, danakj wrote:
> Rounding means you don't know if those partial pixels will be included in the
resulting rect or not.

That's true. But if we round at every location in the code the result is always
consistent and aligned. The idea is to use rounding logic to scale at all
locations and to use enclosing/enclosed only at places where rounding logic does
not satisfy or is a special case. Doing this for ui/views/ so far gave accurate
results and solved a lot(MOST) of the pixel misalignment issues.

I also did a dry run by replacing _EVERY_ {scale by enclosing and scale by
enclosed for gfx::Rect} in chromium to always use {scale by rounding logic}, and
everything looked good at first look. There was just 1 instance where I could
find a bug on Chrome OS which later turned out was an existing bug that was only
made more visible due to this dry run.

Caveat: The use of rounding logic everywhere only works with coordinates. That
is, it works for bounds (gfx::Rect) that has 4 corner points or a point with
{x,y} values. 
This cannot be applied in case of gfx::Size as it only has width and height, but
no coordinate points.

>What are all the edge cases and how it does the right thing? 
So far I have not been able to find many edge cases specifically related to
this, but all other minor bugs that this change causes is tracked in
crbug/731293. Most of those bugs already have a fix and would be pushed once
this core change is committed.

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:45: return IsPixelCanvasRecordingEnabled();
On 2017/07/21 at 17:29:39, danakj wrote:
> pass this in the constructor, instead of reading the command line. Unit tests
should just be able to make objects and pass in how they want them to run,
rather than setting global states that get read later.
> 
> Two choices I think:
> 1) Leave the invalidation in DIP on the PaintContext. Consumer of PaintContext
(View::PaintFromPaintRoot) scales the invalidation if the
compositor->RecordPixelCanvas() or whatever is true, and passes that bool to the
PaintInfo if needed as well.
> 
> 2) Scale the invalidation on the PaintContext. Then the user of PaintContext
needs to know what state the PaintContext uses, so pass the
compositor->RecordPixelCanvas() or whatever to the PaintContext constructor and
expose it like the DSF as a property.

Done. Passing it to PaintContext should propagate it to rest of the code.

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.h:13: class COMPOSITOR_EXPORT PaintInfo {
On 2017/07/21 at 17:29:39, danakj wrote:
> move this to ui/views/? It's not used by compositor APIs

Done

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:23: PaintRecorder::PaintRecorder(const
PaintInfo& info, PaintCache* cache)
On 2017/07/21 at 17:29:39, danakj wrote:
> Can you pass in the various things it needs here instead of the whole
PaintInfo?

Done

[commit-bot: I haz the power, 2017-07-22 00:01:00]

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

[commit-bot: I haz the power, 2017-07-22 00:01:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-07-24 18:34:00]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-24 18:34:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 19:52:59]

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

[commit-bot: I haz the power, 2017-07-24 19:53:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[malaykeshav, 2017-07-24 21:16:47]

Patchset #27 (id:960001) has been deleted

[malaykeshav, 2017-07-24 21:17:11]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-24 21:17:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 23:41:55]

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

[commit-bot: I haz the power, 2017-07-24 23:41:57]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-07-25 17:42:41]

On 2017/07/21 at 23:30:17, malaykeshav wrote:
> @sky @dana
> PTAL
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
> File ui/compositor/canvas_painter.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/canvas_p...
> ui/compositor/canvas_painter.cc:25: IsPixelCanvasRecordingEnabled() ?
raster_scale : 1.f)) {}
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Rather than reading the command line and trying to intercept that for tests,
can you just move the decision to use this higher and pass in the information
this class needs?
> 
> Done
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/composit...
> ui/compositor/compositor.cc:326: if (IsPixelCanvasRecordingEnabled())
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Can this be passed to the Constructor instead so unit tests can set it
without having to muck with trying to intercept command line?
> 
> Done
> 
> https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/layer.cc...
> ui/compositor/layer.cc:926: gfx::ScaleToRoundedRect(invalidation,
IsPixelCanvasRecordingEnabled()
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Can this read the value off the compositor instead of the command line?
> 
> Done
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
> File ui/compositor/paint_info.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
> ui/compositor/paint_info.cc:16: gfx::ScaleToRoundedRect(gfx::Rect(size),
paint_recording_scale_x_)),
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Rounding means you don't know if those partial pixels will be included in
the resulting rect or not.
> 
> That's true. But if we round at every location in the code the result is
always consistent and aligned. The idea is to use rounding logic to scale at all
locations and to use enclosing/enclosed only at places where rounding logic does
not satisfy or is a special case. Doing this for ui/views/ so far gave accurate
results and solved a lot(MOST) of the pixel misalignment issues.
> 
> I also did a dry run by replacing _EVERY_ {scale by enclosing and scale by
enclosed for gfx::Rect} in chromium to always use {scale by rounding logic}, and
everything looked good at first look. There was just 1 instance where I could
find a bug on Chrome OS which later turned out was an existing bug that was only
made more visible due to this dry run.
> 
> Caveat: The use of rounding logic everywhere only works with coordinates. That
is, it works for bounds (gfx::Rect) that has 4 corner points or a point with
{x,y} values. 
> This cannot be applied in case of gfx::Size as it only has width and height,
but no coordinate points.
> 
> >What are all the edge cases and how it does the right thing? 
> So far I have not been able to find many edge cases specifically related to
this, but all other minor bugs that this change causes is tracked in
crbug/731293. Most of those bugs already have a fix and would be pushed once
this core change is committed.
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
> ui/compositor/paint_info.cc:45: return IsPixelCanvasRecordingEnabled();
> On 2017/07/21 at 17:29:39, danakj wrote:
> > pass this in the constructor, instead of reading the command line. Unit
tests should just be able to make objects and pass in how they want them to run,
rather than setting global states that get read later.
> > 
> > Two choices I think:
> > 1) Leave the invalidation in DIP on the PaintContext. Consumer of
PaintContext (View::PaintFromPaintRoot) scales the invalidation if the
compositor->RecordPixelCanvas() or whatever is true, and passes that bool to the
PaintInfo if needed as well.
> > 
> > 2) Scale the invalidation on the PaintContext. Then the user of PaintContext
needs to know what state the PaintContext uses, so pass the
compositor->RecordPixelCanvas() or whatever to the PaintContext constructor and
expose it like the DSF as a property.
> 
> Done. Passing it to PaintContext should propagate it to rest of the code.
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
> File ui/compositor/paint_info.h (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
> ui/compositor/paint_info.h:13: class COMPOSITOR_EXPORT PaintInfo {
> On 2017/07/21 at 17:29:39, danakj wrote:
> > move this to ui/views/? It's not used by compositor APIs
> 
> Done
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
> File ui/compositor/paint_recorder.cc (right):
> 
>
https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_re...
> ui/compositor/paint_recorder.cc:23: PaintRecorder::PaintRecorder(const
PaintInfo& info, PaintCache* cache)
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Can you pass in the various things it needs here instead of the whole
PaintInfo?
> 
> Done

ping

[danakj, 2017-07-25 17:58:57]

The ui/compositor changes mostly LG.

I'm still extremely skeptical that you want to round, and suggest that you think
more robustly about edge cases. Can you give some concrete numbers where it
would round one way or the other, and demonstrate why rounding is giving you a
consistent result that you want?

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:16: gfx::ScaleToRoundedRect(gfx::Rect(size),
paint_recording_scale_x_)),
On 2017/07/21 23:30:17, malaykeshav wrote:
> On 2017/07/21 at 17:29:39, danakj wrote:
> > Rounding means you don't know if those partial pixels will be included in
the
> resulting rect or not.
> 
> That's true. But if we round at every location in the code the result is
always
> consistent and aligned. The idea is to use rounding logic to scale at all
> locations and to use enclosing/enclosed only at places where rounding logic
does
> not satisfy or is a special case. Doing this for ui/views/ so far gave
accurate
> results and solved a lot(MOST) of the pixel misalignment issues.
> 
> I also did a dry run by replacing _EVERY_ {scale by enclosing and scale by
> enclosed for gfx::Rect} in chromium to always use {scale by rounding logic},
and
> everything looked good at first look. There was just 1 instance where I could
> find a bug on Chrome OS which later turned out was an existing bug that was
only
> made more visible due to this dry run.
> 
> Caveat: The use of rounding logic everywhere only works with coordinates. That
> is, it works for bounds (gfx::Rect) that has 4 corner points or a point with
> {x,y} values. 
> This cannot be applied in case of gfx::Size as it only has width and height,
but
> no coordinate points.
> 
> >What are all the edge cases and how it does the right thing? 
> So far I have not been able to find many edge cases specifically related to
> this, but all other minor bugs that this change causes is tracked in
> crbug/731293. Most of those bugs already have a fix and would be pushed once
> this core change is committed.

Not finding bugs easily isn't surprising, they would be off-by-ones which would
hide until they came up. I can give you a specific example:

OcclusionTracker takes the enclosed rect when making a Rect for occlusion data.
And takes an enclosing rect when making a Rect to compare against it.

If it were to round instead, a partial pixel could be considered as occluding
pixels below it, but would actually blend with them. Then we'd not draw things
below and we'd get black (or undefined) ghosting in the pixels instead of the
correct output.

Partial pixel bugs may be hard for you to just find randomly using chrome. But
would show up in cases like partial swap, occlusion, or blending.

If you round here, then might you see that at different sizes, an object that it
goes from being 1px being partially filled on the edges to 1px being cropped off
the edge of the recording, or something like this?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:192: bool enable_pixel_canvas = false);
Can you avoid default parameter values?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:441: // If true, all paint commands are recorded at
pixel size instead of DIP.
Can you also comment on the accessor method?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:442: bool is_pixel_canvas_;
const

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:44: bool kEnablePixelCanvasForTesting =
false;
remove

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/layer.cc...
ui/compositor/layer.cc:937: bool is_pixel_canvas = GetCompositor() ?
GetCompositor()->is_pixel_canvas()
A layer can't paint without being part of a compositor, can it?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
ui/compositor/paint_context.h:50: float device_scale_factor() const { return
device_scale_factor_; }
Can you document these accessors?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
ui/compositor/paint_context.h:102: bool is_pixel_canvas_;
const

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:29: canvas_(&record_canvas_,
context_.device_scale_factor_),
why these changes?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:46: gfx::ScaleToRoundedSize(
Why is this constructor changing the recording_size, but the other isn't? Should
the caller be deciding this?

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:34: const gfx::PointF recording_scale,
&

[malaykeshav, 2017-07-25 21:21:00]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 21:21:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-25 21:25:28]

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

[commit-bot: I haz the power, 2017-07-25 21:25:30]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[malaykeshav, 2017-07-25 21:48:13]

Patchset #28 (id:1000001) has been deleted

[malaykeshav, 2017-07-25 21:48:24]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 21:48:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-25 22:07:04]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 22:07:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-25 22:12:27]

Patchset #29 (id:1040001) has been deleted

[malaykeshav, 2017-07-25 22:12:43]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 22:12:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-25 22:47:51]

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

[commit-bot: I haz the power, 2017-07-25 22:47:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[malaykeshav, 2017-07-25 22:48:06]

Patchset #29 (id:1060001) has been deleted

[malaykeshav, 2017-07-25 22:48:23]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 22:48:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-07-25 22:55:10]

> I'm still extremely skeptical that you want to round, and suggest that you
think more robustly about edge cases. Can you give some concrete numbers where
it would round one way or the other, and demonstrate why rounding is giving you
a consistent result that you want?

Rounding logic is an incorrect way to say it. It is actually scaling each of the
4 corner points in a gfx::Rect() and rounding it instead of performing an
enclosed or enclosing operation.
Using this within ui/views/ keeps the alignment pixel correct. The main goal for
using it is to keep any point (x,y) in the same relative position as compared to
the rest of the points or gfx elements irrespective of the combination of
enclosing or enclosed scale operation that has been performed.

With the rounding logic (or scaling by points logic), all scale operations would
be consistent. Any point on the screen at (x,y) would always be transformed to
the correct (x_scaled, y_scaled) point. Which is not true 
with enclosing/enclosed logic, where the same (x,y) point can be transformed
into 1 of the 4 scaled points depending on context: 
{
  (x_ceiled, y_ceiled), 
  (x_ceiled, y_floor), 
  (x_floor,  y_ceiled), 
  (x_floor,  y_floor)
}. 
Thus creating an inconsistent experience.
It is this difference in consistency that solves the Pixel alignment bugs. 

(eg.)
When scaling 2 adjacent rects of bounds gfx::Rect{(0,0) to (5,5)} and
gfx::Rect{(5,0) to (10,5)} by 1.5 we get the following: 
 - Enclosing logic: {(0, 0) to (8, 8)} and {(7, 0) to (15, 8)} <- Overlap of 1
px in the middle 
 - Enclosed logic : {(0, 0) to (7, 7)} and {(8, 0) to (15, 7)} <- An extra pixel
in the middle. 
 - Rounding logic : {(0, 0) to (8, 8)} and {(8, 0) to (15, 8)} <- Everything is
aligned as expected.
If there were a point at (2, 2) in the above example, its relative position to
the 2 rects would depend on whether enclosing or enclosed was used. While in the
case of rounding it will always lie in the correct relative location to any
gfx::rects on the screen, since all of them have been scaled by the same
rounding logic.

> and suggest that you think more robustly about edge cases.
As mentioned before, this is hidden behind a flag and we are already testing
this with internal dog food devices. Also this only affects ui/views/ so it
should not effect the cc side.. All the bugs related to this change are tracked
under crbug/731293.

[malaykeshav, 2017-07-25 22:57:58]

PTAL

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
File ui/compositor/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/880001/ui/compositor/paint_in...
ui/compositor/paint_info.cc:16: gfx::ScaleToRoundedRect(gfx::Rect(size),
paint_recording_scale_x_)),
> Not finding bugs easily isn't surprising, they would be off-by-ones which
would hide until they came up.

For pixel canvas we are only modifying the scaling logic for ui/views/view
coordinates. The rounding logic does not effect the top level layer.
Can doing just this cause the occlusion bug as well?

> OcclusionTracker takes the enclosed rect when making a Rect for occlusion
data. And takes an enclosing rect when making a Rect to compare against it.
> 
> If it were to round instead, a partial pixel could be considered as occluding
pixels below it, but would actually blend with them. Then we'd not draw things
below and we'd get black (or undefined) ghosting in the pixels instead of the
correct output.

This is the bug that I observed when I changed _EVERYTHING_ to rounding logic.
Something similar happens even without this change that I have been unable to
track. But reading this it seems this might be related.

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:192: bool enable_pixel_canvas = false);
On 2017/07/25 at 17:58:56, danakj wrote:
> Can you avoid default parameter values?

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:441: // If true, all paint commands are recorded at
pixel size instead of DIP.
On 2017/07/25 at 17:58:56, danakj wrote:
> Can you also comment on the accessor method?

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor.h:442: bool is_pixel_canvas_;
On 2017/07/25 at 17:58:56, danakj wrote:
> const

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/composit...
ui/compositor/compositor_switches.cc:44: bool kEnablePixelCanvasForTesting =
false;
On 2017/07/25 at 17:58:56, danakj wrote:
> remove

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/layer.cc...
ui/compositor/layer.cc:937: bool is_pixel_canvas = GetCompositor() ?
GetCompositor()->is_pixel_canvas()
On 2017/07/25 at 17:58:56, danakj wrote:
> A layer can't paint without being part of a compositor, can it?

yes, but it does not work for layers set as mask. Adding a fix for it.

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
ui/compositor/paint_context.h:50: float device_scale_factor() const { return
device_scale_factor_; }
On 2017/07/25 at 17:58:56, danakj wrote:
> Can you document these accessors?

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_co...
ui/compositor/paint_context.h:102: bool is_pixel_canvas_;
On 2017/07/25 at 17:58:56, danakj wrote:
> const

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:29: canvas_(&record_canvas_,
context_.device_scale_factor_),
On 2017/07/25 at 17:58:56, danakj wrote:
> why these changes?

Done

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:46: gfx::ScaleToRoundedSize(
On 2017/07/25 at 17:58:56, danakj wrote:
> Why is this constructor changing the recording_size, but the other isn't?
Should the caller be deciding this?

There are 32 occurrences to this and they need to be handled case by case. Some
of them need a simple scaling while others need to call
paint_info->paint_recording_size(). Since this is behind a flag, I am trying to
keep the core changes to a minimum. In the end, this will be moved out to the
caller.

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.h:34: const gfx::PointF recording_scale,
On 2017/07/25 at 17:58:57, danakj wrote:
> &

Done

[commit-bot: I haz the power, 2017-07-25 23:18:24]

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

[commit-bot: I haz the power, 2017-07-25 23:18:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[danakj, 2017-07-26 16:06:04]

On 2017/07/25 22:55:10, malaykeshav wrote:
> > I'm still extremely skeptical that you want to round, and suggest that you
> think more robustly about edge cases. Can you give some concrete numbers where
> it would round one way or the other, and demonstrate why rounding is giving
you
> a consistent result that you want?
> 
> Rounding logic is an incorrect way to say it. It is actually scaling each of
the
> 4 corner points in a gfx::Rect() and rounding it instead of performing an
> enclosed or enclosing operation.
> Using this within ui/views/ keeps the alignment pixel correct. The main goal
for
> using it is to keep any point (x,y) in the same relative position as compared
to
> the rest of the points or gfx elements irrespective of the combination of
> enclosing or enclosed scale operation that has been performed.
> 
> With the rounding logic (or scaling by points logic), all scale operations
would
> be consistent. Any point on the screen at (x,y) would always be transformed to
> the correct (x_scaled, y_scaled) point. Which is not true 
> with enclosing/enclosed logic, where the same (x,y) point can be transformed
> into 1 of the 4 scaled points depending on context: 
> {
>   (x_ceiled, y_ceiled), 
>   (x_ceiled, y_floor), 
>   (x_floor,  y_ceiled), 
>   (x_floor,  y_floor)
> }. 
> Thus creating an inconsistent experience.
> It is this difference in consistency that solves the Pixel alignment bugs. 
> 
> (eg.)
> When scaling 2 adjacent rects of bounds gfx::Rect{(0,0) to (5,5)} and
> gfx::Rect{(5,0) to (10,5)} by 1.5 we get the following: 
>  - Enclosing logic: {(0, 0) to (8, 8)} and {(7, 0) to (15, 8)} <- Overlap of 1
> px in the middle 
>  - Enclosed logic : {(0, 0) to (7, 7)} and {(8, 0) to (15, 7)} <- An extra
pixel
> in the middle. 
>  - Rounding logic : {(0, 0) to (8, 8)} and {(8, 0) to (15, 8)} <- Everything
is
> aligned as expected.
> If there were a point at (2, 2) in the above example, its relative position to
> the 2 rects would depend on whether enclosing or enclosed was used. While in
the
> case of rounding it will always lie in the correct relative location to any
> gfx::rects on the screen, since all of them have been scaled by the same
> rounding logic.

Thanks, I see what you're saying. Rounding here works for this because the
layout is happening in DIP instead of after converting to pixel size. However
note that this will make the right/bottom edges inconsistent:

A 12x5 DIP layer at 1.2x will be 15px wide.

If you have two 6x5 DIP views in it, at 0->6 and 6->12, they would become
0->6  =>  0->7
6->12 => 7->14

Which would leave the right-most pixel in the layer empty. At 13x5 DIP, the
right edge would jump from 14px to 16px. So you have a situation where the layer
grows one pixel at a time, (and other views in other layers could grow at that
time), but the view here would grow 2 pixels at a time.

So you may have off-by-one problems where one layer appears a pixel larger than
another.

As long as layout is being done in DIP you have to choose between these problems
though, I guess.

> > and suggest that you think more robustly about edge cases.
> As mentioned before, this is hidden behind a flag and we are already testing
> this with internal dog food devices. Also this only affects ui/views/ so it
> should not effect the cc side.. All the bugs related to this change are
tracked
> under crbug/731293.

ui/compositor LGTM w/ a few more comments and 1 question

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/2877483003/diff/980001/ui/compositor/paint_re...
ui/compositor/paint_recorder.cc:46: gfx::ScaleToRoundedSize(
On 2017/07/25 22:57:58, malaykeshav wrote:
> On 2017/07/25 at 17:58:56, danakj wrote:
> > Why is this constructor changing the recording_size, but the other isn't?
> Should the caller be deciding this?
> 
> There are 32 occurrences to this and they need to be handled case by case.
Some
> of them need a simple scaling while others need to call
> paint_info->paint_recording_size(). Since this is behind a flag, I am trying
to
> keep the core changes to a minimum. In the end, this will be moved out to the
> caller.

Can you add a TODO that explains this and points to a bug for this cleanup?

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/composi...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/composi...
ui/compositor/compositor_switches.cc:51: static const bool
kIsPixelCanvasRecordingEnabled =
oh, kFooBar is for compile-time constants, which this is not, so should be
hacker_case. You could also just return HasSwitch() result now if u like.

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.c...
ui/compositor/layer.cc:452: if (layer_mask_ == layer_mask)
usually nice to keep DCHECKs at the top of the function. is there a reason this
moved?

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.h...
ui/compositor/layer.h:418: const Layer* layer_mask_back_link() const { return
layer_mask_back_link_; }
can you add a comment explaining what this is?

[malaykeshav, 2017-07-26 18:34:15]

Patchset #28 (id:1020001) has been deleted

[malaykeshav, 2017-07-26 18:34:46]

Resolving comments.

+sky
PTAL

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/composi...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/composi...
ui/compositor/compositor_switches.cc:51: static const bool
kIsPixelCanvasRecordingEnabled =
On 2017/07/26 at 16:06:04, danakj wrote:
> oh, kFooBar is for compile-time constants, which this is not, so should be
hacker_case. You could also just return HasSwitch() result now if u like.

Done

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.c...
ui/compositor/layer.cc:452: if (layer_mask_ == layer_mask)
On 2017/07/26 16:06:04, danakj wrote:
> usually nice to keep DCHECKs at the top of the function. is there a reason
this
> moved?

This modification is not a part of this CL. 
https://chromium-review.googlesource.com/#/c/578232/

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2877483003/diff/1080001/ui/compositor/layer.h...
ui/compositor/layer.h:418: const Layer* layer_mask_back_link() const { return
layer_mask_back_link_; }
On 2017/07/26 at 16:06:04, danakj wrote:
> can you add a comment explaining what this is?

Done. I am not why we need to duplicate the comment for |layer_mask_back_link_|
at the getter as well as the member variable declaration.

[malaykeshav, 2017-07-26 19:01:46]

Patchset #29 (id:1100001) has been deleted

[malaykeshav, 2017-07-26 19:02:03]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-26 19:02:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-07-26 22:12:12]

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
File ui/compositor/canvas_painter.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
ui/compositor/canvas_painter.h:33: bool is_pixel_canvas);
Document what is_pixel_canvas means. Also, I'm wondering if is_pixel_canvas
should have a default value to avoid having to supply false everywhere.

https://codereview.chromium.org/2877483003/diff/1120001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/gfx/geometry/rect....
ui/gfx/geometry/rect.h:343: inline Rect ScaleToRoundedRect(const Rect& rect,
float x_scale, float y_scale) {
Please add a description. Also, given the description I thought this was
creating a 'rounded' rect, which isn't what it does. Maybe ScaleUsingRounding? I
realize that is rather verbose, but it makes it obvious what is happening.

I also really worry about yet another way to scale, especially given this
function has no description as to *when* it should be used. Please motivate when
and why this should be used. Lastly, please add a description.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights
reserved.
no (c)

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:22: : paint_recording_bounds_(
Make sure to member initialize paint_recording_scale_x_/y_. I realize you reset
them in ComputePaintRecordingScales, but it it's easy to miss that and if an
early return is inserted the value go uninitialized.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:27: root_context_(nullptr) {
How come this doesn't set root_context_? I think the problem with these two
constructors is they are intended for different uses and it isn't clear
immediately clear which to use and when. Factory functions would clear that up.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:73: const gfx::Rect& parent_bounds,
Don't the coordinate system of the two rects differ? I suspect parent_bounds
should really be parent_size, or am I misunderstanding what this is?

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:89: if (right == parent_bounds.width()) {
no {}

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:89: if (right == parent_bounds.width()) {
Might rounding give you a size > parent_bounds.width()? Are you sure you don't
need >= ? Same comment below.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h
File ui/views/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:1: // Copyright (c) 2017 The Chromium Authors. All rights
reserved.
no (c) (see style guide).

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:14: class VIEWS_EXPORT PaintInfo {
Please add a description.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:17: // Scales the context by the default dsf. Use this if
you dont want any form
dont => don't

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:19: kScaleToScaleFactor = 0,
Do we really need both of these? I don't see any usage of kScaleToScaleFactor
(other than early returns).

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:42: bool IsPixelCanvas() const;
Add description.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:44: gfx::Vector2d offset_from_parent() const { return
offset_from_parent_; }
const gfx::Vector2&

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:66: // Paint Recording bounds for this view. The offset is
relative to the root
Please document coordinates. It isn't readily clear what they are, and this is
tricky code.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:78: void ComputePaintRecordingScales(const gfx::Size&
size, ScaleType scale_type);
functions then members.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
File ui/views/paint_info_unittest.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:41: const gfx::Rect r_0(kLayerSize);
constexpr on these.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:57: // | 3 | 4 | 5 | <-- 2
The <-- 2 isn't clear at all. Maybe you mean there 2 wraps 3/4/5? Not sure.
Please make the rectangle names line up with the rectangles you have. I would
expect a rectangle named r1 to match 1 in the diagram.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:63: std::vector<PaintInfo*>
GetPaintInfoSetup(const ui::PaintContext& context) {
Passing around raw pointers like this is a recipe for leaks. Return a
std::vector<std::unique_ptr<PaintInfo>>

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:92: void
VerifyChildBoundsCoversParent(PaintInfo* parent_paint_info,
Can both of these parameters be const refs? That makes it clear the arguments
aren't mofieid.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.cc#newc...
ui/views/view.cc:863: if (!context.is_pixel_canvas()) {
Why is this only done for !pixel_canvas?

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1278: // PaintInfo::ScaleType.
Remove second sentence as this is obvious given the return type.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1279: virtual PaintInfo::ScaleType GetPaintScaleType() const;
Move this up with the other painting functions. Say line 1170.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1279: virtual PaintInfo::ScaleType GetPaintScaleType() const;
Are we really going to have different scale types for different views? Wouldn't
we do the same scale type for all views?

[sky, 2017-07-26 23:16:41]

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
File ui/compositor/canvas_painter.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
ui/compositor/canvas_painter.h:33: bool is_pixel_canvas);
On 2017/07/26 22:12:11, sky wrote:
> Document what is_pixel_canvas means. Also, I'm wondering if is_pixel_canvas
> should have a default value to avoid having to supply false everywhere.

Dana says she prefers no default args, which I'm ok with. So, ignore this
comment. Sorry for the churn.

[commit-bot: I haz the power, 2017-07-27 00:46:48]

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

[commit-bot: I haz the power, 2017-07-27 00:46:50]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[malaykeshav, 2017-07-27 01:34:40]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-27 01:34:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-27 03:00:55]

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

[commit-bot: I haz the power, 2017-07-27 03:00:56]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[malaykeshav, 2017-07-27 21:45:18]

Patchset #31 (id:1160001) has been deleted

[malaykeshav, 2017-07-27 21:45:33]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-27 21:45:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-27 23:04:56]

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

[commit-bot: I haz the power, 2017-07-27 23:05:00]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[malaykeshav, 2017-07-28 01:24:38]

@sky 
PTAL

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
File ui/compositor/canvas_painter.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
ui/compositor/canvas_painter.h:33: bool is_pixel_canvas);
On 2017/07/26 at 22:12:11, sky wrote:
> Document what is_pixel_canvas means. Also, I'm wondering if is_pixel_canvas
should have a default value to avoid having to supply false everywhere.

Do we need to document this here? It is only required to initialize the
ui::PaintContext where it is documented.

https://codereview.chromium.org/2877483003/diff/1120001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/gfx/geometry/rect....
ui/gfx/geometry/rect.h:343: inline Rect ScaleToRoundedRect(const Rect& rect,
float x_scale, float y_scale) {
On 2017/07/26 at 22:12:11, sky wrote:
> Please add a description. Also, given the description I thought this was
creating a 'rounded' rect, which isn't what it does. Maybe ScaleUsingRounding? I
realize that is rather verbose, but it makes it obvious what is happening.
> 
> I also really worry about yet another way to scale, especially given this
function has no description as to *when* it should be used. Please motivate when
and why this should be used. Lastly, please add a description.

This specific name was used to give consistency as it is used elsewhere in the
code as well. (gfx::ScaleToRoundedSize).

Adding a description of what it does and when it should be used.
Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights
reserved.
On 2017/07/26 at 22:12:11, sky wrote:
> no (c)

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:22: : paint_recording_bounds_(
On 2017/07/26 at 22:12:11, sky wrote:
> Make sure to member initialize paint_recording_scale_x_/y_. I realize you
reset them in ComputePaintRecordingScales, but it it's easy to miss that and if
an early return is inserted the value go uninitialized.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:27: root_context_(nullptr) {
On 2017/07/26 at 22:12:11, sky wrote:
> How come this doesn't set root_context_? I think the problem with these two
constructors is they are intended for different uses and it isn't clear
immediately clear which to use and when. Factory functions would clear that up.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:73: const gfx::Rect& parent_bounds,
On 2017/07/26 at 22:12:11, sky wrote:
> Don't the coordinate system of the two rects differ? I suspect parent_bounds
should really be parent_size, or am I misunderstanding what this is?

Yes. parent_bounds are not needed. parent_size is enough.
Done.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:89: if (right == parent_bounds.width()) {
On 2017/07/26 at 22:12:11, sky wrote:
> Might rounding give you a size > parent_bounds.width()? Are you sure you don't
need >= ? Same comment below.

Both the values are in DIP, so there should be no problem with rounding.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:89: if (right == parent_bounds.width()) {
On 2017/07/26 22:12:11, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h
File ui/views/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:1: // Copyright (c) 2017 The Chromium Authors. All rights
reserved.
On 2017/07/26 at 22:12:12, sky wrote:
> no (c) (see style guide).

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:14: class VIEWS_EXPORT PaintInfo {
On 2017/07/26 at 22:12:11, sky wrote:
> Please add a description.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:17: // Scales the context by the default dsf. Use this if
you dont want any form
On 2017/07/26 at 22:12:12, sky wrote:
> dont => don't

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:19: kScaleToScaleFactor = 0,
On 2017/07/26 at 22:12:11, sky wrote:
> Do we really need both of these? I don't see any usage of kScaleToScaleFactor
(other than early returns).

This is needed in the subsequent changes when dealing with views and layers that
have images and icons.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:42: bool IsPixelCanvas() const;
On 2017/07/26 at 22:12:12, sky wrote:
> Add description.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:44: gfx::Vector2d offset_from_parent() const { return
offset_from_parent_; }
On 2017/07/26 at 22:12:12, sky wrote:
> const gfx::Vector2&

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:66: // Paint Recording bounds for this view. The offset is
relative to the root
On 2017/07/26 at 22:12:12, sky wrote:
> Please document coordinates. It isn't readily clear what they are, and this is
tricky code.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:78: void ComputePaintRecordingScales(const gfx::Size&
size, ScaleType scale_type);
On 2017/07/26 at 22:12:12, sky wrote:
> functions then members.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
File ui/views/paint_info_unittest.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:41: const gfx::Rect r_0(kLayerSize);
On 2017/07/26 at 22:12:12, sky wrote:
> constexpr on these.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:57: // | 3 | 4 | 5 | <-- 2
On 2017/07/26 at 22:12:12, sky wrote:
> The <-- 2 isn't clear at all. Maybe you mean there 2 wraps 3/4/5? Not sure.
Please make the rectangle names line up with the rectangles you have. I would
expect a rectangle named r1 to match 1 in the diagram.

Done. Adding further comments

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:63: std::vector<PaintInfo*>
GetPaintInfoSetup(const ui::PaintContext& context) {
On 2017/07/26 at 22:12:12, sky wrote:
> Passing around raw pointers like this is a recipe for leaks. Return a
std::vector<std::unique_ptr<PaintInfo>>

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info_u...
ui/views/paint_info_unittest.cc:92: void
VerifyChildBoundsCoversParent(PaintInfo* parent_paint_info,
On 2017/07/26 at 22:12:12, sky wrote:
> Can both of these parameters be const refs? That makes it clear the arguments
aren't mofieid.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.cc#newc...
ui/views/view.cc:863: if (!context.is_pixel_canvas()) {
On 2017/07/26 at 22:12:12, sky wrote:
> Why is this only done for !pixel_canvas?

With pixel canvas, the context.PaintOffset() returns the offset in pixels. This
cannot be compared with the DIP offset stored in views.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1278: // PaintInfo::ScaleType.
On 2017/07/26 at 22:12:12, sky wrote:
> Remove second sentence as this is obvious given the return type.

Done

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1279: virtual PaintInfo::ScaleType GetPaintScaleType() const;
On 2017/07/26 at 22:12:12, sky wrote:
> Are we really going to have different scale types for different views?
Wouldn't we do the same scale type for all views?

All except the ones that don't want any distortion in scaling. Specifically the
images and icons. There may be other cases as well where we do not want any
distorted scaling.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/view.h#newco...
ui/views/view.h:1279: virtual PaintInfo::ScaleType GetPaintScaleType() const;
On 2017/07/26 22:12:12, sky wrote:
> Move this up with the other painting functions. Say line 1170.

Done.

[malaykeshav, 2017-07-28 01:44:34]

Patchset #31 (id:1180001) has been deleted

[malaykeshav, 2017-07-28 01:44:44]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-28 01:44:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-28 04:39:08]

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

[commit-bot: I haz the power, 2017-07-28 04:39:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2017-07-28 16:02:13]

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
File ui/compositor/canvas_painter.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/compositor/canvas_...
ui/compositor/canvas_painter.h:33: bool is_pixel_canvas);
On 2017/07/28 01:24:37, malaykeshav wrote:
> On 2017/07/26 at 22:12:11, sky wrote:
> > Document what is_pixel_canvas means. Also, I'm wondering if is_pixel_canvas
> should have a default value to avoid having to supply false everywhere.
> 
> Do we need to document this here? It is only required to initialize the
> ui::PaintContext where it is documented.

It wasn't obvious to me what this means on first look, so yes, it needs some
documentation. We don't want to duplicate documentation though, so you can say
see PaintContext for details on |is_pixel_canvas|.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.c...
ui/views/paint_info.cc:89: if (right == parent_bounds.width()) {
On 2017/07/28 01:24:37, malaykeshav wrote:
> On 2017/07/26 at 22:12:11, sky wrote:
> > Might rounding give you a size > parent_bounds.width()? Are you sure you
don't
> need >= ? Same comment below.
> 
> Both the values are in DIP, so there should be no problem with rounding.

Actually, I'm a little confused by why you need to special case this. Please add
a comment. Also, remember that there is nothing stopping a child view from going
outside the bounds of it's parent.

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h
File ui/views/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:18: // of distortion during scaling.
Distortion is bad, so based on this description I would assume *all* views want
this. Why would they go with scaletofit? This is basically a repeat of my next
question, why use scaletofit?

https://codereview.chromium.org/2877483003/diff/1120001/ui/views/paint_info.h...
ui/views/paint_info.h:19: kScaleToScaleFactor = 0,
On 2017/07/28 01:24:37, malaykeshav wrote:
> On 2017/07/26 at 22:12:11, sky wrote:
> > Do we really need both of these? I don't see any usage of
kScaleToScaleFactor
> (other than early returns).
> 
> This is needed in the subsequent changes when dealing with views and layers
that
> have images and icons.

Please document (in View::GetPaintScaleType) *why* a View would override this
function.

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect....
ui/gfx/geometry/rect.h:347: // All rectangles (or any point) within the same
coordinate space will
I'm not sure what this last sentence means. This sentence implies it isn't
lossy, yet it is. Also, you don't give guidance on *when* this should be used vs
the others. That is the important thing to document.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:23: PaintInfo::PaintInfo(const ui::PaintContext&
root_context,
Style guide says order of declarations and definitions should match.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:63: void PaintInfo::ComputePaintRecordingScales(const
gfx::Size& size,
You only call this from one place, how about moving it to the place it's called?
I could see keeping it if the calling function was long, but it's not.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:68: paint_recording_scale_x_ = paint_recording_scale_y_ =
1.f;
When you inline this function in the constructor that calls it you should be
able to remove this line, because the constructor already sets it.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h
File ui/views/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:35: static PaintInfo CreateRootPaintInfo(const
ui::PaintContext& root_context,
Thanks for the improved names, they make it much easier to understand the
context.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:40: static PaintInfo CreateChildPaintInfo(const PaintInfo&
other,
other -> parent_paint_info.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:45: PaintInfo(const PaintInfo& other);
The way you have this class set up the assignment operator is also generated.
Are you sure you want that?

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:63: gfx::PointF paint_recording_scale() const {
Why are you using a Point for something that isn't a point but a scale? Instead
expose getters for the scales, for example:
float paint_recording_scale_x() const { return ... };

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/view.h#newco...
ui/views/view.h:1336: ui::TransformRecorder* recorder,
Style guide says out params should be last.

[malaykeshav, 2017-07-29 06:42:36]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-29 06:42:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-29 08:36:32]

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

[commit-bot: I haz the power, 2017-07-29 08:36:35]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[malaykeshav, 2017-07-29 10:56:51]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[malaykeshav, 2017-07-29 10:57:00]

Patchset #32 (id:1220001) has been deleted

[commit-bot: I haz the power, 2017-07-29 10:57:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-29 13:17:46]

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

[commit-bot: I haz the power, 2017-07-29 13:17:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[malaykeshav, 2017-07-30 07:03:03]

Patchset #32 (id:1240001) has been deleted

[malaykeshav, 2017-07-30 07:03:05]

Patchset #32 (id:1240001) has been deleted

[malaykeshav, 2017-07-30 07:03:14]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-30 07:03:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-30 08:17:38]

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

[commit-bot: I haz the power, 2017-07-30 08:17:41]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[malaykeshav, 2017-07-31 18:54:30]

@sky
PTAL

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect....
ui/gfx/geometry/rect.h:347: // All rectangles (or any point) within the same
coordinate space will
On 2017/07/28 at 16:02:11, sky wrote:
> I'm not sure what this last sentence means. This sentence implies it isn't
lossy, yet it is. Also, you don't give guidance on *when* this should be used vs
the others. That is the important thing to document.

The relative positions _isn't_ lossy, even though the absolute position may be.
Adding a comment that this scheme of scaling should be used when performing
layout operations to give lossless relative positioning.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:23: PaintInfo::PaintInfo(const ui::PaintContext&
root_context,
On 2017/07/28 at 16:02:11, sky wrote:
> Style guide says order of declarations and definitions should match.

The style guide mentions that classes should be defined before any other methods
but below the static methods. So I am not sure what the right order is in this
case.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:63: void PaintInfo::ComputePaintRecordingScales(const
gfx::Size& size,
On 2017/07/28 at 16:02:11, sky wrote:
> You only call this from one place, how about moving it to the place it's
called? I could see keeping it if the calling function was long, but it's not.

Done

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:68: paint_recording_scale_x_ = paint_recording_scale_y_ =
1.f;
On 2017/07/28 at 16:02:11, sky wrote:
> When you inline this function in the constructor that calls it you should be
able to remove this line, because the constructor already sets it.

Ack

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h
File ui/views/paint_info.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:40: static PaintInfo CreateChildPaintInfo(const PaintInfo&
other,
On 2017/07/28 at 16:02:11, sky wrote:
> other -> parent_paint_info.

Done

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:45: PaintInfo(const PaintInfo& other);
On 2017/07/28 at 16:02:12, sky wrote:
> The way you have this class set up the assignment operator is also generated.
Are you sure you want that?
You are right, this will create a PaintContext clone without the invalidation at
every assignment operator.

Creating a factory method for cloning in that case.

Also creating a copy constructor for all the factory function assignments to
work.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.h...
ui/views/paint_info.h:63: gfx::PointF paint_recording_scale() const {
On 2017/07/28 at 16:02:11, sky wrote:
> Why are you using a Point for something that isn't a point but a scale?
Instead expose getters for the scales, for example:
> float paint_recording_scale_x() const { return ... };

Done

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/view.h#newco...
ui/views/view.h:1336: ui::TransformRecorder* recorder,
On 2017/07/28 at 16:02:12, sky wrote:
> Style guide says out params should be last.

Done

[malaykeshav, 2017-07-31 21:33:30]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-31 21:33:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-31 21:38:12]

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

[commit-bot: I haz the power, 2017-07-31 21:38:15]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[malaykeshav, 2017-07-31 21:39:41]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-31 21:39:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-31 22:58:45]

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

[commit-bot: I haz the power, 2017-07-31 22:58:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-08-01 15:10:25]

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/gfx/geometry/rect....
ui/gfx/geometry/rect.h:347: // All rectangles (or any point) within the same
coordinate space will
On 2017/07/31 18:54:29, malaykeshav wrote:
> On 2017/07/28 at 16:02:11, sky wrote:
> > I'm not sure what this last sentence means. This sentence implies it isn't
> lossy, yet it is. Also, you don't give guidance on *when* this should be used
vs
> the others. That is the important thing to document.
> 
> The relative positions _isn't_ lossy, even though the absolute position may
be.
> Adding a comment that this scheme of scaling should be used when performing
> layout operations to give lossless relative positioning.

At the end of the day you are rounding to ints, so I don't know how it can be
'not lossy.' It all depends upon how you are using this.

But again, the important thing is the comment. Thanks for adding it.

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:23: PaintInfo::PaintInfo(const ui::PaintContext&
root_context,
On 2017/07/31 18:54:29, malaykeshav wrote:
> On 2017/07/28 at 16:02:11, sky wrote:
> > Style guide says order of declarations and definitions should match.
> 
> The style guide mentions that classes should be defined before any other
methods
> but below the static methods. So I am not sure what the right order is in this
> case.

Here's the relevant item, "Function declaration order should match function
definition order." So, in the header you have CreateRootPaintInfo, which means
that should be the first function you have here.

https://codereview.chromium.org/2877483003/diff/1280001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1280001/ui/views/view.h#newco...
ui/views/view.h:1171: // Returns the type of scaling to be done for this View.
You still haven't outlined when and why this should be overriden. That is key.

[malaykeshav, 2017-08-01 18:05:10]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-01 18:05:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-08-01 19:31:02]

@sky PTAL

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.cc
File ui/views/paint_info.cc (right):

https://codereview.chromium.org/2877483003/diff/1200001/ui/views/paint_info.c...
ui/views/paint_info.cc:23: PaintInfo::PaintInfo(const ui::PaintContext&
root_context,
On 2017/08/01 at 15:10:24, sky wrote:
> On 2017/07/31 18:54:29, malaykeshav wrote:
> > On 2017/07/28 at 16:02:11, sky wrote:
> > > Style guide says order of declarations and definitions should match.
> > 
> > The style guide mentions that classes should be defined before any other
methods
> > but below the static methods. So I am not sure what the right order is in
this
> > case.
> 
> Here's the relevant item, "Function declaration order should match function
definition order." So, in the header you have CreateRootPaintInfo, which means
that should be the first function you have here.

Done

https://codereview.chromium.org/2877483003/diff/1280001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2877483003/diff/1280001/ui/views/view.h#newco...
ui/views/view.h:1171: // Returns the type of scaling to be done for this View.
On 2017/08/01 at 15:10:24, sky wrote:
> You still haven't outlined when and why this should be overriden. That is key.

Done

[sky, 2017-08-01 23:23:10]

LGTM

[commit-bot: I haz the power, 2017-08-02 00:57:51]

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

[commit-bot: I haz the power, 2017-08-02 00:57:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[malaykeshav, 2017-08-02 01:35:57]

malaykeshav@chromium.org changed reviewers:
+ boliu@chromium.org, jbauman@chromium.org

[malaykeshav, 2017-08-02 01:36:00]

Adding:

+boliu for OWNERS approval on components/web_contents_delegate_android/
- Moving function from anonymous namepsace to avoid conflict 
 

+jbauman for OWNERS approval on
content/browser/renderer_host/browser/compositor_view_mac
- Update call to constructor for ui::Compositor()

PTAL

[boliu, 2017-08-02 01:49:44]

components/web_contents_delegate_android lgtm

[jbauman, 2017-08-02 01:53:36]

On 2017/08/02 01:36:00, malaykeshav wrote:
> Adding:
> 
> +boliu for OWNERS approval on components/web_contents_delegate_android/
> - Moving function from anonymous namepsace to avoid conflict 
>  
> 
> +jbauman for OWNERS approval on
> content/browser/renderer_host/browser/compositor_view_mac
> - Update call to constructor for ui::Compositor()
> 
> PTAL

Sorry, I don't think I own
content/browser/renderer_host/browser_compositor_view_mac.mm. Maybe ccameron or
kbr instead?

[malaykeshav, 2017-08-02 02:53:15]

malaykeshav@chromium.org changed reviewers:
+ ccameron@chromium.org
- jbauman@chromium.org

[malaykeshav, 2017-08-02 02:53:16]

malaykeshav@chromium.org changed reviewers:
+ ccameron@chromium.org
- jbauman@chromium.org

[malaykeshav, 2017-08-02 02:53:17]

malaykeshav@chromium.org changed reviewers:
+ ccameron@chromium.org
- jbauman@chromium.org

[malaykeshav, 2017-08-02 02:53:18]

+ccameron for OWNERS approval on
content/browser/renderer_host/browser/compositor_view_mac
- Update call to constructor for ui::Compositor()

[malaykeshav, 2017-08-02 02:53:18]

+ccameron for OWNERS approval on
content/browser/renderer_host/browser/compositor_view_mac
- Update call to constructor for ui::Compositor()

[malaykeshav, 2017-08-02 02:53:19]

+ccameron for OWNERS approval on
content/browser/renderer_host/browser/compositor_view_mac
- Update call to constructor for ui::Compositor()

[ccameron, 2017-08-02 19:48:02]

On 2017/08/02 02:53:19, malaykeshav wrote:
> +ccameron for OWNERS approval on
> content/browser/renderer_host/browser/compositor_view_mac
> - Update call to constructor for ui::Compositor()

content/browser/renderer_host/browser_compositor_view_mac.mm lgtm

[malaykeshav, 2017-08-02 20:34:22]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 20:34:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2017-08-02 22:15:45]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 22:16:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-02 22:54:22]

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

[commit-bot: I haz the power, 2017-08-02 22:54:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[malaykeshav, 2017-08-03 00:51:51]

The CQ bit was checked by malaykeshav@chromium.org

[malaykeshav, 2017-08-03 00:51:53]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
danakj@chromium.org, ccameron@chromium.org, boliu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2877483003/#ps1340001
(title: "Sync with ToT")

[commit-bot: I haz the power, 2017-08-03 00:52:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 01:05:47]

CQ is committing da patch.

Bot data: {"patchset_id": 1340001, "attempt_start_ts": 1501721511985940,
"parent_rev": "c5b3034ef05c531fbe1f699445a4ec8468ef9041", "commit_rev":
"1716ced8d970f5b2c409fc0849b71c3efe95b7fe"}

[commit-bot: I haz the power, 2017-08-03 01:06:07]

CQ is committing da patch.

Bot data: {"patchset_id": 1340001, "attempt_start_ts": 1501721511985940,
"parent_rev": "499b248c899248d34afcbef534769c6eff331dd6", "commit_rev":
"55c9f6d21086e418dac45a58e5f51a3a5ef7bc1e"}

[commit-bot: I haz the power, 2017-08-03 01:06:26]

Description was changed from

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slide
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implements core logic for Pixel Canvas

This patch implements the core logic for Pixel Canvas while hiding it
behind a compositor switch (Disabled by default).

Pixel Canvas enables all canvas draw commands to be recorded at pixel
size, helping eliminate a majority of fractional device scale factor
bugs. This also allows devices to work at all non discrete fractional
scales with reduced(or no) overhead.

Subsequent changes will fix bugs that happen when this mode is enabled.
This is to avoid one massive CL.

DesignDoc=go/pixel-canvas
Slides=go/pixel-canvas-slide
BUG=720596
COMPONENT=Canvas, View, Layer, Paint Context, Paint Recorder
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2877483003
Cr-Commit-Position: refs/heads/master@{#491587}
Committed:
https://chromium.googlesource.com/chromium/src/+/55c9f6d21086e418dac45a58e5f5...
==========

[commit-bot: I haz the power, 2017-08-03 01:06:32]

Committed patchset #36 (id:1340001) as
https://chromium.googlesource.com/chromium/src/+/55c9f6d21086e418dac45a58e5f5...