Textfield: suppress cursor repaints when there's a selection

Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
Committed: https://crrev.com/1c77b2b6c04efe4e5d5d3ec5688616d65b8d0138
Cr-Commit-Position: refs/heads/master@{#420667}

[Elly Fong-Jones, 2016-09-09 14:11:55]

Description was changed from

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
==========

to

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
==========

[Elly Fong-Jones, 2016-09-09 14:11:55]

ellyjones@chromium.org changed reviewers:
+ pkasting@chromium.org

[Elly Fong-Jones, 2016-09-09 14:12:03]

pkasting: ptal? :)

[Peter Kasting, 2016-09-09 21:47:15]

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
!HasFocus())
Are the two lines above this still correct in this case?

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1799: cursor_repaint_timer_.Stop();
What starts this timer again when one of the conditions above changes?  Do we
get called back to here again with another "cursor_changed" event?

[Elly Fong-Jones, 2016-09-12 11:39:16]

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
!HasFocus())
On 2016/09/09 21:47:15, Peter Kasting wrote:
> Are the two lines above this still correct in this case?

Yes - we always have to do a repaint when the cursor changes so that the cursor
visually moves to where it should be now. We just don't want to "blink" it if
there's no actual insertion point.

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1799: cursor_repaint_timer_.Stop();
On 2016/09/09 21:47:15, Peter Kasting wrote:
> What starts this timer again when one of the conditions above changes?  Do we
> get called back to here again with another "cursor_changed" event?

We get another UpdateAfterChange() with cursor_changed = true, yeah. Changes to
the selection always entail cursor_changed, including losing the selection
altogether.

[Peter Kasting, 2016-09-13 05:16:07]

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
!HasFocus())
On 2016/09/12 11:39:16, Elly Jones wrote:
> On 2016/09/09 21:47:15, Peter Kasting wrote:
> > Are the two lines above this still correct in this case?
> 
> Yes - we always have to do a repaint when the cursor changes so that the
cursor
> visually moves to where it should be now. We just don't want to "blink" it if
> there's no actual insertion point.

Well, if we won't be drawing the cursor, we aren't visually moving it anywhere
-- or are you referring to erasing the old cursor?  But then setting
|cursor_visible_| to true unconditionally seems wrong.

The premise of my question is that, when there's no focus or no selection,
there's no visible cursor.

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1799: cursor_repaint_timer_.Stop();
On 2016/09/12 11:39:15, Elly Jones wrote:
> On 2016/09/09 21:47:15, Peter Kasting wrote:
> > What starts this timer again when one of the conditions above changes?  Do
we
> > get called back to here again with another "cursor_changed" event?
> 
> We get another UpdateAfterChange() with cursor_changed = true, yeah. Changes
to
> the selection always entail cursor_changed, including losing the selection
> altogether.

The gain/lose focus case is the one I was more worried about.

I don't know if it's worth a comment about this.

[Elly Fong-Jones, 2016-09-14 15:26:27]

pkasting: ptal? :)

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
!HasFocus())
On 2016/09/13 05:16:07, Peter Kasting wrote:
> On 2016/09/12 11:39:16, Elly Jones wrote:
> > On 2016/09/09 21:47:15, Peter Kasting wrote:
> > > Are the two lines above this still correct in this case?
> > 
> > Yes - we always have to do a repaint when the cursor changes so that the
> cursor
> > visually moves to where it should be now. We just don't want to "blink" it
if
> > there's no actual insertion point.
> 
> Well, if we won't be drawing the cursor, we aren't visually moving it anywhere
> -- or are you referring to erasing the old cursor?  But then setting
> |cursor_visible_| to true unconditionally seems wrong.
> 
> The premise of my question is that, when there's no focus or no selection,
> there's no visible cursor.

|cursor_visible_| is used only to track whether the blinking cursor is currently
"on" or not, so it is only set in OnFocus/OnBlur (when the textfield becomes or
stops being editable) and when the cursor blinks in |UpdateCursor()|. Setting
|cursor_visible_| to true here is the right thing to do, because the only times
it is false here are:

1) Dialog is blurred (can't happen)
2) Cursor blink cycle was "off", in which case we must redraw it since it just
moved

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1799: cursor_repaint_timer_.Stop();
On 2016/09/13 05:16:07, Peter Kasting wrote:
> On 2016/09/12 11:39:15, Elly Jones wrote:
> > On 2016/09/09 21:47:15, Peter Kasting wrote:
> > > What starts this timer again when one of the conditions above changes?  Do
> we
> > > get called back to here again with another "cursor_changed" event?
> > 
> > We get another UpdateAfterChange() with cursor_changed = true, yeah. Changes
> to
> > the selection always entail cursor_changed, including losing the selection
> > altogether.
> 
> The gain/lose focus case is the one I was more worried about.
> 
> I don't know if it's worth a comment about this.

Done.

[Peter Kasting, 2016-09-15 21:07:11]

I'm worried that I'm blocking submitting what seems like a win because I'm not
understanding enough context to know whether more improvements make sense.  I'd
still like to figure out the answers to my questions below, and if necessary
improve the code more accordingly, but I'm giving you an LGTM so you can make a
call about how best to do that.  If you feel like it makes sense to submit this
in the meantime now you don't have to wait for me :)

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
!HasFocus())
On 2016/09/14 15:26:27, Elly Jones wrote:
> On 2016/09/13 05:16:07, Peter Kasting wrote:
> > On 2016/09/12 11:39:16, Elly Jones wrote:
> > > On 2016/09/09 21:47:15, Peter Kasting wrote:
> > > > Are the two lines above this still correct in this case?
> > > 
> > > Yes - we always have to do a repaint when the cursor changes so that the
> > cursor
> > > visually moves to where it should be now. We just don't want to "blink" it
> if
> > > there's no actual insertion point.
> > 
> > Well, if we won't be drawing the cursor, we aren't visually moving it
anywhere
> > -- or are you referring to erasing the old cursor?  But then setting
> > |cursor_visible_| to true unconditionally seems wrong.
> > 
> > The premise of my question is that, when there's no focus or no selection,
> > there's no visible cursor.
> 
> |cursor_visible_| is used only to track whether the blinking cursor is
currently
> "on" or not, so it is only set in OnFocus/OnBlur (when the textfield becomes
or
> stops being editable) and when the cursor blinks in |UpdateCursor()|. Setting
> |cursor_visible_| to true here is the right thing to do, because the only
times
> it is false here are:
> 
> 1) Dialog is blurred (can't happen)

Wait, so "!HasFocus()" cannot be true?  (If so why add a condition check for
it?)  I'm confused.

> 2) Cursor blink cycle was "off", in which case we must redraw it since it just
> moved

It sounds like you're saying "we're trying to blink the cursor on after it had
been off".  But if HasSelection() is true, then we don't want to blink the
cursor on, because we're not going to draw it.  That's why I was asking.

I have the nagging feeling we're talking past each other :(

[Elly Fong-Jones, 2016-09-16 11:57:33]

On 2016/09/15 21:07:11, Peter Kasting wrote:
> I'm worried that I'm blocking submitting what seems like a win because I'm not
> understanding enough context to know whether more improvements make sense. 
I'd
> still like to figure out the answers to my questions below, and if necessary
> improve the code more accordingly, but I'm giving you an LGTM so you can make
a
> call about how best to do that.  If you feel like it makes sense to submit
this
> in the meantime now you don't have to wait for me :)

No. This code needs to be understandable and we should go back and forth until
it really is :)

>
https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/2322303002/diff/20001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:1798: if (HasSelection() ||
> !HasFocus())
> On 2016/09/14 15:26:27, Elly Jones wrote:
> > On 2016/09/13 05:16:07, Peter Kasting wrote:
> > > On 2016/09/12 11:39:16, Elly Jones wrote:
> > > > On 2016/09/09 21:47:15, Peter Kasting wrote:
> > > > > Are the two lines above this still correct in this case?
> > > > 
> > > > Yes - we always have to do a repaint when the cursor changes so that the
> > > cursor
> > > > visually moves to where it should be now. We just don't want to "blink"
it
> > if
> > > > there's no actual insertion point.
> > > 
> > > Well, if we won't be drawing the cursor, we aren't visually moving it
> anywhere
> > > -- or are you referring to erasing the old cursor?  But then setting
> > > |cursor_visible_| to true unconditionally seems wrong.
> > > 
> > > The premise of my question is that, when there's no focus or no selection,
> > > there's no visible cursor.
> > 
> > |cursor_visible_| is used only to track whether the blinking cursor is
> currently
> > "on" or not, so it is only set in OnFocus/OnBlur (when the textfield becomes
> or
> > stops being editable) and when the cursor blinks in |UpdateCursor()|.
Setting
> > |cursor_visible_| to true here is the right thing to do, because the only
> times
> > it is false here are:
> > 
> > 1) Dialog is blurred (can't happen)
> 
> Wait, so "!HasFocus()" cannot be true?  (If so why add a condition check for
> it?)  I'm confused.

Hm, sorry, I was very unclear here. UpdateAfterChange() cannot be called while
!HasFocus() *because of user input*, but there are API functions of Textfield
that code using it can use (Textfield::SelectAll() or whatever) that may result
in UpdateAfterChange() being called while !HasFocus(). The "can't happen" here
is that if !HasFocus(), the timer was *already* stopped in Textfield::OnBlur(),
so it "can't happen" that it needs to be stopped again. I'll change the code to
make it more clear what's going on and add a comment.

> > 2) Cursor blink cycle was "off", in which case we must redraw it since it
just
> > moved
> 
> It sounds like you're saying "we're trying to blink the cursor on after it had
> been off".  But if HasSelection() is true, then we don't want to blink the
> cursor on, because we're not going to draw it.  That's why I was asking.

cursor_visible_ does not have any effect while there's a selection; it tracks
whether an "I-beam" type insertion cursor would be visible at the moment or not.
This is actually surpassingly confusing and causes an auxiliary bug, which is
that we display an insertion cursor for readonly textfields. I renamed it to
editing_cursor_visible_, which might help some.

> I have the nagging feeling we're talking past each other :(

Same :\

[Peter Kasting, 2016-09-16 18:52:27]

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:258: editing_cursor_visible_(false),
I still don't really know what "editing cursor visible" means:

* Does it read "editing; cursor visible"?  Or "Editing Cursor is visible"?
* If the latter, what is an "editing cursor" as distinct from a non-editing
cursor?

To me, there's just one cursor, a blinking line at the insertion point.  This
would be invisible when there's a selection or when the field has no focus. 
There's also the mouse pointer, which may look like an arrow or an i-beam in
different scenarios, and might (I guess?) be invisible sometimes, like if you
start typing maybe we hide the cursor until it moves.

From your last reply, I read you as talking about the mouse pointer, in which
I'd call this |mouse_pointer_visible_|.  But from reading the code, I think
that's not the case, in which case I'd still just call it the cursor, though it
would be nice to more clearly distinguish between "the cursor is visible in
general, i.e. blinking on and off" vs. "the cursor is currently drawn on the
screen, i.e. in the 'blink on' state".  The former could be named
|cursor_blinking_|.  The latter (which is what I think this actually means) is
the hardest to name; |cursor_visible_| is not terrible, or perhaps
|cursor_drawn_| or even |cursor_currently_drawn_|.  No matter what, I'd extend
the comment in the .h to be very precise about the meaning here.

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:977: editing_cursor_visible_ = true;
Why not make this mutually exclusive with HasSelection()?

Seems like part of the problem is that you have a variable that supposedly means
that your cursor is onscreen, except that when there's a selection, it doesn't. 
Just making this variable false in that case seems better.

This would mean OnBlur() wouldn't do an unnecessary repaint call when there's a
selection (and thus the cursor is not actually visible), and it would allow you
to simplify the line you touched in PaintTextAndCursor() to check fewer
conditions.

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1798: RepaintCursor();
Nit: Since RepaintCursor() merely schedules a paint, it doesn't actually matter
where it's called.  I wonder if it would be clearer to call it at the end of
this whole conditional, since where it is now, it appears as if we "force the
cursor off and erase it, then do other stuff", which is not what I think the
code actually does.

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1804:
DCHECK(!cursor_repaint_timer_.IsRunning());
Nit: Just:

    DCHECK(HasFocus() || !cursor_repaint_timer_.IsRunning());

https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1810: if (HasSelection()) {
Nit: Inspired by my suggestion above to move the RepaintCursor() call, I think
it might also be clearer to eliminate the unconditional clear of
|editing_cursor_visible_| above and change the code here to:

    editing_cursor_visible_=
        enabled() && !read_only() && HasFocus() && !HasSelection();
    if (editing_cursor_visible_)
      cursor_repaint_timer_.Reset();
    else
      cursor_repaint_timer_.Stop();

Admittedly the Stop() call is not always necessary, but it's not always
necessary in the current way you wrote the code either.  This reads more like
your comment did, and makes it less likely someone later breaks something by
early-returning (or whatever) between when |editing_cursor_visible_| gets
cleared above and when it sometimes gets reset here.  Finally, it means the code
no longer says "we always want to hide the cursor -- oh no wait, keep reading, I
guess we don't"; instead it directly sets the variable to just what it
ultimately means.

[Elly Fong-Jones, 2016-09-19 12:41:50]

On 2016/09/16 18:52:27, Peter Kasting wrote:
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:258: editing_cursor_visible_(false),
> I still don't really know what "editing cursor visible" means:
> 
> * Does it read "editing; cursor visible"?  Or "Editing Cursor is visible"?
> * If the latter, what is an "editing cursor" as distinct from a non-editing
> cursor?
> 
> To me, there's just one cursor, a blinking line at the insertion point.  This
> would be invisible when there's a selection or when the field has no focus. 
> There's also the mouse pointer, which may look like an arrow or an i-beam in
> different scenarios, and might (I guess?) be invisible sometimes, like if you
> start typing maybe we hide the cursor until it moves.

I don't think that is right. There are three kinds of cursor in textfields: the
editing/insertion cursor (a blinking vertical line), the drag cursor (a solid
vertical line, to indicate where dragged text will be dropped), and a selection
cursor (looks like a background on the selected area). This variable was
supposed to track whether the first was currently visible.

> From your last reply, I read you as talking about the mouse pointer, in which
> I'd call this |mouse_pointer_visible_|.  But from reading the code, I think
> that's not the case, in which case I'd still just call it the cursor, though
it
> would be nice to more clearly distinguish between "the cursor is visible in
> general, i.e. blinking on and off" vs. "the cursor is currently drawn on the
> screen, i.e. in the 'blink on' state".  The former could be named
> |cursor_blinking_|.  The latter (which is what I think this actually means) is
> the hardest to name; |cursor_visible_| is not terrible, or perhaps
> |cursor_drawn_| or even |cursor_currently_drawn_|.  No matter what, I'd extend
> the comment in the .h to be very precise about the meaning here.
> 
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:977: editing_cursor_visible_ = true;
> Why not make this mutually exclusive with HasSelection()?
> 
> Seems like part of the problem is that you have a variable that supposedly
means
> that your cursor is onscreen, except that when there's a selection, it
doesn't. 
> Just making this variable false in that case seems better.
> 
> This would mean OnBlur() wouldn't do an unnecessary repaint call when there's
a
> selection (and thus the cursor is not actually visible), and it would allow
you
> to simplify the line you touched in PaintTextAndCursor() to check fewer
> conditions.
> 
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:1798: RepaintCursor();
> Nit: Since RepaintCursor() merely schedules a paint, it doesn't actually
matter
> where it's called.  I wonder if it would be clearer to call it at the end of
> this whole conditional, since where it is now, it appears as if we "force the
> cursor off and erase it, then do other stuff", which is not what I think the
> code actually does.
> 
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:1804:
> DCHECK(!cursor_repaint_timer_.IsRunning());
> Nit: Just:
> 
>     DCHECK(HasFocus() || !cursor_repaint_timer_.IsRunning());
> 
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:1810: if (HasSelection()) {
> Nit: Inspired by my suggestion above to move the RepaintCursor() call, I think
> it might also be clearer to eliminate the unconditional clear of
> |editing_cursor_visible_| above and change the code here to:
> 
>     editing_cursor_visible_=
>         enabled() && !read_only() && HasFocus() && !HasSelection();
>     if (editing_cursor_visible_)
>       cursor_repaint_timer_.Reset();
>     else
>       cursor_repaint_timer_.Stop();
> 
> Admittedly the Stop() call is not always necessary, but it's not always
> necessary in the current way you wrote the code either.  This reads more like
> your comment did, and makes it less likely someone later breaks something by
> early-returning (or whatever) between when |editing_cursor_visible_| gets
> cleared above and when it sometimes gets reset here.  Finally, it means the
code
> no longer says "we always want to hide the cursor -- oh no wait, keep reading,
I
> guess we don't"; instead it directly sets the variable to just what it
> ultimately means.

I ended up entirely rewriting this chunk of code; I hope it is now easier to
understand.

[Peter Kasting, 2016-09-19 18:31:33]

On 2016/09/19 12:41:50, Elly Jones wrote:
> On 2016/09/16 18:52:27, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> > File ui/views/controls/textfield/textfield.cc (right):
> > 
> >
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> > ui/views/controls/textfield/textfield.cc:258:
editing_cursor_visible_(false),
> > I still don't really know what "editing cursor visible" means:
> > 
> > * Does it read "editing; cursor visible"?  Or "Editing Cursor is visible"?
> > * If the latter, what is an "editing cursor" as distinct from a non-editing
> > cursor?
> > 
> > To me, there's just one cursor, a blinking line at the insertion point. 
This
> > would be invisible when there's a selection or when the field has no focus. 
> > There's also the mouse pointer, which may look like an arrow or an i-beam in
> > different scenarios, and might (I guess?) be invisible sometimes, like if
you
> > start typing maybe we hide the cursor until it moves.
> 
> I don't think that is right. There are three kinds of cursor in textfields:
the
> editing/insertion cursor (a blinking vertical line), the drag cursor (a solid
> vertical line, to indicate where dragged text will be dropped), and a
selection
> cursor (looks like a background on the selected area).

Hmm.  The drag cursor I forgot about (although "drag insertion point" seems
clearer), but the selection background I never thought of as a "cursor", that's
just the background style for the selected region.

> I ended up entirely rewriting this chunk of code; I hope it is now easier to
> understand.

Looks like you didn't upload the new version?

[Elly Fong-Jones, 2016-09-20 10:40:53]

On 2016/09/19 18:31:33, Peter Kasting wrote:
> On 2016/09/19 12:41:50, Elly Jones wrote:
> > On 2016/09/16 18:52:27, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> > > File ui/views/controls/textfield/textfield.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2322303002/diff/50001/ui/views/controls/textf...
> > > ui/views/controls/textfield/textfield.cc:258:
> editing_cursor_visible_(false),
> > > I still don't really know what "editing cursor visible" means:
> > > 
> > > * Does it read "editing; cursor visible"?  Or "Editing Cursor is visible"?
> > > * If the latter, what is an "editing cursor" as distinct from a
non-editing
> > > cursor?
> > > 
> > > To me, there's just one cursor, a blinking line at the insertion point. 
> This
> > > would be invisible when there's a selection or when the field has no
focus. 
> > > There's also the mouse pointer, which may look like an arrow or an i-beam
in
> > > different scenarios, and might (I guess?) be invisible sometimes, like if
> you
> > > start typing maybe we hide the cursor until it moves.
> > 
> > I don't think that is right. There are three kinds of cursor in textfields:
> the
> > editing/insertion cursor (a blinking vertical line), the drag cursor (a
solid
> > vertical line, to indicate where dragged text will be dropped), and a
> selection
> > cursor (looks like a background on the selected area).
> 
> Hmm.  The drag cursor I forgot about (although "drag insertion point" seems
> clearer), but the selection background I never thought of as a "cursor",
that's
> just the background style for the selected region.
> 
> > I ended up entirely rewriting this chunk of code; I hope it is now easier to
> > understand.
> 
> Looks like you didn't upload the new version?

D'oh! Uploaded now :)

[Peter Kasting, 2016-09-20 17:15:41]

LGTM

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:408: // |Textfield::GetCaretBlinkMs()|.
Nit: No || on function names

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:409: void BlinkCursor();
Nit: OnCursorBlinkTimerFired()?

[Peter Kasting, 2016-09-20 17:16:58]

(This definitely seems clearer now!  Yay!)

You may want to update the CL description, especially if you made behavioral
fixes.

[Elly Fong-Jones, 2016-09-21 14:00:10]

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:408: // |Textfield::GetCaretBlinkMs()|.
On 2016/09/20 17:15:41, Peter Kasting wrote:
> Nit: No || on function names

Done.

https://codereview.chromium.org/2322303002/diff/70001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:409: void BlinkCursor();
On 2016/09/20 17:15:41, Peter Kasting wrote:
> Nit: OnCursorBlinkTimerFired()?

Done.

[Elly Fong-Jones, 2016-09-21 14:00:15]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-09-21 14:00:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2322303002/#ps90001
(title: "final nit")

[commit-bot: I haz the power, 2016-09-21 14:00:30]

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, 2016-09-21 14:31:51]

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

[commit-bot: I haz the power, 2016-09-21 14:31:52]

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

[Elly Fong-Jones, 2016-09-23 12:42:59]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-09-23 12:42:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2322303002/#ps110001
(title: "remove spurious DCHECK")

[commit-bot: I haz the power, 2016-09-23 12:43:08]

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, 2016-09-23 13:04:17]

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

[commit-bot: I haz the power, 2016-09-23 13:04:18]

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

[Elly Fong-Jones, 2016-09-23 17:31:47]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:32:35]

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, 2016-09-23 18:22:22]

Description was changed from

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
==========

to

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
==========

[commit-bot: I haz the power, 2016-09-23 18:22:23]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-09-23 18:24:47]

Description was changed from

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262
==========

to

==========
Textfield: suppress cursor repaints when there's a selection

Without this, we do a considerable amount of repainting every timer tick.

BUG=645262

Committed: https://crrev.com/1c77b2b6c04efe4e5d5d3ec5688616d65b8d0138
Cr-Commit-Position: refs/heads/master@{#420667}
==========

[commit-bot: I haz the power, 2016-09-23 18:24:48]

Patchset 7 (id:??) landed as
https://crrev.com/1c77b2b6c04efe4e5d5d3ec5688616d65b8d0138
Cr-Commit-Position: refs/heads/master@{#420667}