Paint text cursor in LAYER_SOLID_COLOR

Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink costs 
130KB memory to raster. It is now painted in the solid color layer and it is free to 
raster it.

BUG=680536
Review-Url: https://codereview.chromium.org/2660593002
Cr-Commit-Position: refs/heads/master@{#451179}
Committed: https://chromium.googlesource.com/chromium/src/+/a33c8f5acb10c229da14134b9987e3b16eaa74f4

[yiyix, 2017-01-27 03:25:40]

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

[commit-bot: I haz the power, 2017-01-27 03:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-01-27 04:19:36]

Description was changed from

==========
cursor

BUG=
==========

to

==========
Paint text cursor in LAYER_SOLID_COLOR



BUG=
==========

[commit-bot: I haz the power, 2017-01-27 04:47:06]

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

[commit-bot: I haz the power, 2017-01-27 04:47:07]

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

[yiyix, 2017-01-27 22:14:59]

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

[commit-bot: I haz the power, 2017-01-27 22:15: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-01-27 23:23:39]

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

[commit-bot: I haz the power, 2017-01-27 23:23:40]

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

[yiyix, 2017-01-29 00:41:08]

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

[commit-bot: I haz the power, 2017-01-29 00:41:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-01-29 00:54:33]

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

[commit-bot: I haz the power, 2017-01-29 00:54: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-01-29 01:47:54]

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

[commit-bot: I haz the power, 2017-01-29 01:47:55]

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

[yiyix, 2017-01-29 05:07:05]

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

[commit-bot: I haz the power, 2017-01-29 05:07:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-01-29 05:09:37]

Patchset #1 (id:1) has been deleted

[yiyix, 2017-01-29 05:10:26]

Patchset #1 (id:20001) has been deleted

[yiyix, 2017-01-29 05:18:08]

Description was changed from

==========
Paint text cursor in LAYER_SOLID_COLOR



BUG=
==========

to

==========
Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink
costs 130KB memory to raster. It is now painted in the solid co

BUG=680536
==========

[yiyix, 2017-01-29 05:22:25]

Description was changed from

==========
Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink
costs 130KB memory to raster. It is now painted in the solid co

BUG=680536
==========

to

==========
Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink
costs 
130KB memory to raster. It is now painted in the solid color layer and it is
free to 
raster it.

BUG=680536
==========

[yiyix, 2017-01-29 05:31:31]

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

[commit-bot: I haz the power, 2017-01-29 05:31:35]

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-01-29 05:43:06]

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

[commit-bot: I haz the power, 2017-01-29 05:43:07]

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

[yiyix, 2017-01-30 05:14:07]

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

[commit-bot: I haz the power, 2017-01-30 05:14:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-01-30 05:14:30]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-30 06:02:48]

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

[commit-bot: I haz the power, 2017-01-30 06:02:49]

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

[yiyix, 2017-01-31 02:57:04]

Patchset #1 (id:40001) has been deleted

[yiyix, 2017-01-31 02:57:20]

yiyix@chromium.org changed reviewers:
+ sadrul@chromium.org

[yiyix, 2017-01-31 02:59:32]

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

[commit-bot: I haz the power, 2017-01-31 03:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-01-31 03:01:50]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-31 04:22:28]

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

[commit-bot: I haz the power, 2017-01-31 04:22:29]

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

[yiyix, 2017-01-31 06:56:47]

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

[yiyix, 2017-01-31 06:56:58]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-01-31 06:56: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-01-31 07:55:06]

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

[commit-bot: I haz the power, 2017-01-31 07:55:07]

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

[yiyix, 2017-01-31 15:50:15]

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

[commit-bot: I haz the power, 2017-01-31 15:50: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-01-31 16:57:44]

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

[commit-bot: I haz the power, 2017-01-31 16:57:44]

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

[yiyix, 2017-01-31 19:59:40]

yiyix@chromium.org changed reviewers:
- sadrul@chromium.org

[yiyix, 2017-01-31 20:00:28]

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

[yiyix, 2017-01-31 20:00:32]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-01-31 20:01:13]

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-01-31 20:52:33]

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

[commit-bot: I haz the power, 2017-01-31 20:52:34]

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_...)

[yiyix, 2017-01-31 21:13:15]

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

[commit-bot: I haz the power, 2017-01-31 21:13: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-01-31 22:21:21]

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

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

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

[yiyix, 2017-02-01 01:29:34]

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

[commit-bot: I haz the power, 2017-02-01 01:30:13]

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-02-01 02:39:20]

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

[commit-bot: I haz the power, 2017-02-01 02:39:20]

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

[yiyix, 2017-02-01 19:32:43]

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

[commit-bot: I haz the power, 2017-02-01 19:33:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-02-01 19:38:59]

Patchset #1 (id:140001) has been deleted

[yiyix, 2017-02-01 19:39:08]

Patchset #1 (id:160001) has been deleted

[yiyix, 2017-02-01 19:40:37]

Patchset #1 (id:180001) has been deleted

[yiyix, 2017-02-01 19:40:54]

Patchset #1 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-02-01 20:37:11]

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

[commit-bot: I haz the power, 2017-02-01 20:37:13]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-02-01 21:07:27]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-02-01 21:19:47]

yiyix@chromium.org changed reviewers:
+ sadrul@chromium.org

[yiyix, 2017-02-01 21:20:05]

@sadrul, could you please take a look? Thank you.

[yiyix, 2017-02-01 21:23:28]

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

[yiyix, 2017-02-01 21:23:32]

Patchset #1 (id:220001) has been deleted

[yiyix, 2017-02-01 21:23:40]

Patchset #1 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-02-01 21:23: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-02-01 23:19:08]

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

[commit-bot: I haz the power, 2017-02-01 23:19:09]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-02-02 00:14:19]

Patchset #1 (id:260001) has been deleted

[yiyix, 2017-02-02 00:14:29]

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

[commit-bot: I haz the power, 2017-02-02 00:15:05]

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-02-02 01:20:28]

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

[commit-bot: I haz the power, 2017-02-02 01:20:30]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-02-02 17:15:32]

https://codereview.chromium.org/2660593002/diff/280001/ui/gfx/render_text.h
File ui/gfx/render_text.h (left):

https://codereview.chromium.org/2660593002/diff/280001/ui/gfx/render_text.h#o...
ui/gfx/render_text.h:772: SkColor cursor_color_;
Remove this too?

https://codereview.chromium.org/2660593002/diff/280001/ui/views/accessibility...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/accessibility...
ui/views/accessibility/ax_widget_obj_wrapper.cc:36:
!widget_->GetRootView()->visible()) {
It'd be better to move this into a separate CL with a test for it. (where the
test attempts to trigger some AX event when the Widget is in the process of
being destroyed)

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:504: SchedulePaint();
We should update the layer color here too.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1908: SchedulePaint();
You shouldn't reschedule a paint here, because then we are essentially
repainting the entire textfield. Just make sure the position and visibility of
the cursor-view are set correctly. That should be sufficient.

To reflect this, maybe rename this function to UpdateCursorView() or something
like that.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1929: render_text->DrawCursor(canvas,
drop_cursor_position_);
Add a TODO.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:2067:
cursor_view_->SetVisible(!cursor_view_->visible());
If you update the visibility in UpdateCursorView() above, then this can just
call UpdateCursorView().

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:528: std::unique_ptr<views::View>
cursor_view_;
Don't need views.

Don't need unique_ptr (i.e. just 'View cursor_view_;')

[yiyix, 2017-02-06 21:36:18]

https://codereview.chromium.org/2660593002/diff/280001/ui/gfx/render_text.h
File ui/gfx/render_text.h (left):

https://codereview.chromium.org/2660593002/diff/280001/ui/gfx/render_text.h#o...
ui/gfx/render_text.h:772: SkColor cursor_color_;
On 2017/02/02 17:15:31, sadrul wrote:
> Remove this too?

This is used in DrawCursor. I will remove this variable as I remove the
drop_cursor related stuff.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/accessibility...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/accessibility...
ui/views/accessibility/ax_widget_obj_wrapper.cc:36:
!widget_->GetRootView()->visible()) {
On 2017/02/02 17:15:31, sadrul wrote:
> It'd be better to move this into a separate CL with a test for it. (where the
> test attempts to trigger some AX event when the Widget is in the process of
> being destroyed)

I will submit a separate cl for this change. Remove the change here.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:504: SchedulePaint();
On 2017/02/02 17:15:32, sadrul wrote:
> We should update the layer color here too.

I was not sure if the cursor color has to be the same as the text color. In
render_text, the set color method does not change the cursor color:
void RenderText::SetColor(SkColor value) {
  colors_.SetValue(value);
  OnTextColorChanged();
}

Furthermore, The only caller of set_cursor_color in render_text is in
OnNativeThemeChange.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1908: SchedulePaint();
On 2017/02/02 17:15:32, sadrul wrote:
> You shouldn't reschedule a paint here, because then we are essentially
> repainting the entire textfield. Just make sure the position and visibility of
> the cursor-view are set correctly. That should be sufficient.
> 
> To reflect this, maybe rename this function to UpdateCursorView() or something
> like that.

Done.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1929: render_text->DrawCursor(canvas,
drop_cursor_position_);
On 2017/02/02 17:15:31, sadrul wrote:
> Add a TODO.

Done.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:2067:
cursor_view_->SetVisible(!cursor_view_->visible());
On 2017/02/02 17:15:32, sadrul wrote:
> If you update the visibility in UpdateCursorView() above, then this can just
> call UpdateCursorView().

As we have discussed offline, keep the set visibility related code here.

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2660593002/diff/280001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:528: std::unique_ptr<views::View>
cursor_view_;
On 2017/02/02 17:15:32, sadrul wrote:
> Don't need views.
> 
> Don't need unique_ptr (i.e. just 'View cursor_view_;')
As we have discussed offline, done.

[yiyix, 2017-02-06 21:36:26]

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

[yiyix, 2017-02-06 21:36:45]

@sadrul, could you please take another look? Thank you very much.

[commit-bot: I haz the power, 2017-02-06 21:37: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-02-06 23:45:17]

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

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

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

[sadrul, 2017-02-07 04:19:35]

https://codereview.chromium.org/2660593002/diff/300001/ui/views/accessibility...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (left):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/accessibility...
ui/views/accessibility/ax_widget_obj_wrapper.cc:37: 
Restore?

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:962: cursor_view_.SetVisible(true);
Call UpdateCursorView() before calling SetVisible(true)?

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:986: UpdateCursorView();
We don't need to call UpdateCursorView() since it's hidden anyway, right?

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:376: // Repaint the cursor.
Update the comment.

[yiyix, 2017-02-08 19:47:40]

https://codereview.chromium.org/2660593002/diff/300001/ui/views/accessibility...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (left):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/accessibility...
ui/views/accessibility/ax_widget_obj_wrapper.cc:37: 
On 2017/02/07 04:19:35, sadrul wrote:
> Restore?

Done.

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:962: cursor_view_.SetVisible(true);
On 2017/02/07 04:19:35, sadrul wrote:
> Call UpdateCursorView() before calling SetVisible(true)?

Good Call! Cursor should show at the correct position.

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:986: UpdateCursorView();
On 2017/02/07 04:19:35, sadrul wrote:
> We don't need to call UpdateCursorView() since it's hidden anyway, right?

Right. It doesn't matter where the cursor is as it is not shown to the users.

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2660593002/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:376: // Repaint the cursor.
On 2017/02/07 04:19:35, sadrul wrote:
> Update the comment.

Done.

[yiyix, 2017-02-08 19:47:52]

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

[commit-bot: I haz the power, 2017-02-08 19:48: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-02-08 20:33:27]

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

[commit-bot: I haz the power, 2017-02-08 20:33: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_...)

[yiyix, 2017-02-08 21:18:44]

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

[commit-bot: I haz the power, 2017-02-08 21:19:14]

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-02-08 22:10:16]

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

[commit-bot: I haz the power, 2017-02-08 22:10:17]

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

[yiyix, 2017-02-08 22:54:28]

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

[commit-bot: I haz the power, 2017-02-08 22:55: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-02-09 00:12:08]

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

[commit-bot: I haz the power, 2017-02-09 00:12:09]

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

[yiyix, 2017-02-11 21:34:14]

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

[commit-bot: I haz the power, 2017-02-11 21:34: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-02-11 22:30:34]

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

[commit-bot: I haz the power, 2017-02-11 22:30:35]

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_...)

[yiyix, 2017-02-12 07:50:00]

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

[commit-bot: I haz the power, 2017-02-12 07:50: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-02-12 08:33:18]

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

[commit-bot: I haz the power, 2017-02-12 08:33:19]

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_...)

[yiyix, 2017-02-12 18:14:28]

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

[commit-bot: I haz the power, 2017-02-12 18:14:37]

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-02-12 18:52:13]

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

[commit-bot: I haz the power, 2017-02-12 18:52:14]

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_...)

[yiyix, 2017-02-12 22:40:11]

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

[commit-bot: I haz the power, 2017-02-12 22:40: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-02-12 23:21:40]

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

[commit-bot: I haz the power, 2017-02-12 23:21:41]

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_...)

[yiyix, 2017-02-13 01:42:41]

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

[commit-bot: I haz the power, 2017-02-13 01:42: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-02-13 02:23:15]

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

[commit-bot: I haz the power, 2017-02-13 02:23:16]

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_...)

[yiyix, 2017-02-13 03:46:28]

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

[commit-bot: I haz the power, 2017-02-13 03:46: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-02-13 04:28:51]

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

[commit-bot: I haz the power, 2017-02-13 04:28:52]

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_...)

[yiyix, 2017-02-13 19:20:19]

Patchset #3 (id:320001) has been deleted

[yiyix, 2017-02-13 19:27:31]

Patchset #3 (id:340001) has been deleted

[yiyix, 2017-02-13 19:27:41]

Patchset #3 (id:360001) has been deleted

[yiyix, 2017-02-13 19:27:49]

Patchset #3 (id:380001) has been deleted

[yiyix, 2017-02-13 19:27:54]

Patchset #3 (id:400001) has been deleted

[yiyix, 2017-02-13 19:28:01]

Patchset #3 (id:420001) has been deleted

[yiyix, 2017-02-13 19:28:07]

Patchset #3 (id:440001) has been deleted

[yiyix, 2017-02-13 19:28:14]

Patchset #3 (id:460001) has been deleted

[yiyix, 2017-02-13 19:28:29]

Patchset #3 (id:480001) has been deleted

[yiyix, 2017-02-13 20:07:05]

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

[commit-bot: I haz the power, 2017-02-13 20:08:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-02-13 20:33:29]

sadrul@chromium.org changed reviewers:
+ msw@chromium.org

[sadrul, 2017-02-13 20:33:30]

+msw@

lgtm Thanks! Please wait for msw@'s approval too before landing.

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

https://codereview.chromium.org/2660593002/diff/500001/ui/views/view.cc#newco...
ui/views/view.cc:307: }
Remove these changes?

[commit-bot: I haz the power, 2017-02-13 21:00:50]

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

[commit-bot: I haz the power, 2017-02-13 21:00:51]

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

[yiyix, 2017-02-13 21:12:24]

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

https://codereview.chromium.org/2660593002/diff/500001/ui/views/view.cc#newco...
ui/views/view.cc:307: }
On 2017/02/13 20:33:30, sadrul wrote:
> Remove these changes?

so sorry, i fixed the changes but i forget to save it after. I will try to check
in details before giving to you.

[yiyix, 2017-02-13 21:12:57]

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

[commit-bot: I haz the power, 2017-02-13 21:14:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-02-13 21:27:12]

https://codereview.chromium.org/2660593002/diff/500001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
(right):

https://codereview.chromium.org/2660593002/diff/500001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc:101:
ASSERT_EQ(1U, textfield_children.size());
Should we test that the new child view of the textfield (the cursor's view) is
not focusable, etc.? (maybe ping someone on the accessibility team?)

https://codereview.chromium.org/2660593002/diff/500001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/2660593002/diff/500001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:878: void RenderText::DrawCursor(Canvas* canvas, const
SelectionModel& position) {
Can you remove this function and inline similar functionality in textfield.cc
for the drop cursor? It doesn't make sense to keep this function for just that.
Once this is removed, please also remove |cursor_color_|. It looks like we'll
need to keep |cursor_enabled_| to reserve space for rendering the cursor.

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:994: if (cursor_view_.visible())
nit: maybe this check isn't really needed? (always set not visible)

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1930: // TODO(yiyix): Use
UpdateCursorView to draw drop_cursor instead.
Please fix this now or prepare a dependent patch set.

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:267:
cursor_view_.set_owned_by_client();
It's unusual to use the pattern; is there any reason to not let the view
hierarchy own this view?

[commit-bot: I haz the power, 2017-02-13 23:10:07]

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

[commit-bot: I haz the power, 2017-02-13 23:10:08]

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_...)

[yiyix, 2017-02-15 16:56:30]

yiyix@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[yiyix, 2017-02-15 16:56:32]

@dmazzoni, could you please review changes in
accessibility/ax_tree_source_aura_unittest.cc, do you think more accessibility
tests should be added to test cursor_view?
Thank you very much

@msw, could you review this patch? Thank you

https://codereview.chromium.org/2660593002/diff/500001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
(right):

https://codereview.chromium.org/2660593002/diff/500001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc:101:
ASSERT_EQ(1U, textfield_children.size());
On 2017/02/13 21:27:12, msw wrote:
> Should we test that the new child view of the textfield (the cursor's view) is
> not focusable, etc.? (maybe ping someone on the accessibility team?)

I don't think we need to add more test cases. I was calling
GetRenderText()->GetUpdatedCursorBounds() to get the current cursor location and
this function has been well tested. I will ping someone from accessibility to
review this change. Thank you for the suggestion.

https://codereview.chromium.org/2660593002/diff/500001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/2660593002/diff/500001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:878: void RenderText::DrawCursor(Canvas* canvas, const
SelectionModel& position) {
On 2017/02/13 21:27:12, msw wrote:
> Can you remove this function and inline similar functionality in textfield.cc
> for the drop cursor? It doesn't make sense to keep this function for just
that.
> Once this is removed, please also remove |cursor_color_|. It looks like we'll
> need to keep |cursor_enabled_| to reserve space for rendering the cursor.

Inline it it's a good idea. I will remove the todo and delete cursor_color_.
Thanks.

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:994: if (cursor_view_.visible())
On 2017/02/13 21:27:12, msw wrote:
> nit: maybe this check isn't really needed? (always set not visible)

Right, it checks if the visibility changes in SetVisible function in view.cc.
Update.

https://codereview.chromium.org/2660593002/diff/500001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1930: // TODO(yiyix): Use
UpdateCursorView to draw drop_cursor instead.
On 2017/02/13 21:27:12, msw wrote:
> Please fix this now or prepare a dependent patch set.

By adding DrawCursor inline, it is also fixed.

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:267:
cursor_view_.set_owned_by_client();
On 2017/02/13 21:27:12, msw wrote:
> It's unusual to use the pattern; is there any reason to not let the view
> hierarchy own this view?

I feel that this view is owned by textfield view.

[yiyix, 2017-02-15 16:57:52]

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

[commit-bot: I haz the power, 2017-02-15 16:58:37]

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-02-15 18:03:38]

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

[commit-bot: I haz the power, 2017-02-15 18:03:40]

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_...)

[msw, 2017-02-15 22:50:04]

lgtm with nits, thanks for the cleanup.

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:267:
cursor_view_.set_owned_by_client();
On 2017/02/15 16:56:32, yiyix wrote:
> On 2017/02/13 21:27:12, msw wrote:
> > It's unusual to use the pattern; is there any reason to not let the view
> > hierarchy own this view?
> 
> I feel that this view is owned by textfield view.

It's more common that view instances are owned by the view hierarchy, so most
developers familiar with the views ui toolkit will find this a bit strange, and
wonder at the reason it's owned by the client here. I would encourage you to
follow that pattern, but I won't block this Cl on that nuance.

https://codereview.chromium.org/2660593002/diff/540001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/540001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:279: // |cursor_view_| is owned by
TextField view.
nit: 'Textfield' to match the class name.

[dmazzoni, 2017-02-16 20:11:57]

lgtm for accessibility, I don't think this
will affect anything

[yiyix, 2017-02-16 20:33:25]

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/520001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:267:
cursor_view_.set_owned_by_client();
On 2017/02/15 22:50:04, msw wrote:
> On 2017/02/15 16:56:32, yiyix wrote:
> > On 2017/02/13 21:27:12, msw wrote:
> > > It's unusual to use the pattern; is there any reason to not let the view
> > > hierarchy own this view?
> > 
> > I feel that this view is owned by textfield view.
> 
> It's more common that view instances are owned by the view hierarchy, so most
> developers familiar with the views ui toolkit will find this a bit strange,
and
> wonder at the reason it's owned by the client here. I would encourage you to
> follow that pattern, but I won't block this Cl on that nuance.

I guess there is no special benefits to make this view owned by textfield rather
than view hierarchy as textfield itself is not owned_by_client. I will remove
this line in the next patch

[yiyix, 2017-02-16 20:33:36]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2017-02-16 20:33:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
msw@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2660593002/#ps560001
(title: "nits")

[commit-bot: I haz the power, 2017-02-16 20:35:09]

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

[msw, 2017-02-16 20:44:24]

Thanks for the sanity-check, Dominick!
Just one more comment.

https://codereview.chromium.org/2660593002/diff/560001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2660593002/diff/560001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:278:
cursor_view_.layer()->SetColor(GetTextColor());
You'll need to change the cursor_view_ member to a raw pointer, so that it may
be owned by the views hierarchy:

In textfield.h (with optional comment):
  View* cursor_view_;  // Owned by the views hierarchy.
Above this, in textfield.cc's initializer list:
  cursor_view_(new View),
  weak_ptr_fact...
And then just change use here to reflect that.

[yiyix, 2017-02-16 21:28:48]

The CQ bit was unchecked by yiyix@chromium.org

[yiyix, 2017-02-16 22:24:42]

On 2017/02/16 20:44:24, msw wrote:
> Thanks for the sanity-check, Dominick!
> Just one more comment.
> 
>
https://codereview.chromium.org/2660593002/diff/560001/ui/views/controls/text...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/2660593002/diff/560001/ui/views/controls/text...
> ui/views/controls/textfield/textfield.cc:278:
> cursor_view_.layer()->SetColor(GetTextColor());
> You'll need to change the cursor_view_ member to a raw pointer, so that it may
> be owned by the views hierarchy:
> 
> In textfield.h (with optional comment):
>   View* cursor_view_;  // Owned by the views hierarchy.
> Above this, in textfield.cc's initializer list:
>   cursor_view_(new View),
>   weak_ptr_fact...
> And then just change use here to reflect that.

As we have discussed offline, i will revert set_owned_by_client and keep
|cursor_view_| as a raw type.

[yiyix, 2017-02-16 22:24:45]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2017-02-16 22:24:45]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
dmazzoni@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2660593002/#ps580001
(title: "comments")

[commit-bot: I haz the power, 2017-02-16 22:26:11]

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-02-17 02:13:39]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1487283885489860,
"parent_rev": "a255f7468a598ea8cf8a5ab0e21ab0047f52568a", "commit_rev":
"a33c8f5acb10c229da14134b9987e3b16eaa74f4"}

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

Description was changed from

==========
Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink
costs 
130KB memory to raster. It is now painted in the solid color layer and it is
free to 
raster it.

BUG=680536
==========

to

==========
Paint text cursor in LAYER_SOLID_COLOR

The text cursor used to painted in a texture layer, where each cursor blink
costs 
130KB memory to raster. It is now painted in the solid color layer and it is
free to 
raster it.

BUG=680536

Review-Url: https://codereview.chromium.org/2660593002
Cr-Commit-Position: refs/heads/master@{#451179}
Committed:
https://chromium.googlesource.com/chromium/src/+/a33c8f5acb10c229da14134b9987...
==========

[commit-bot: I haz the power, 2017-02-17 02:14:25]

Committed patchset #7 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/a33c8f5acb10c229da14134b9987...