Let FrameSelection::localCaretRect on a text input field avoid synchronous layout.

Let FrameSelection::localCaretRect on a text input field avoid synchronous  
layout. 

We use VisiblePosition canonicalize to know where to draw a caret.
However if selection is on a text input, we need concern just whether the text 
input is visible or not.
It means we can change VisiblePosition to PositionWithAffinity.

Source/core/editing/Caret.cpp/h:
- Add a override method of updateCaretRect using PositionWithAffinity.

Source/core/editing/FrameSelection.cpp:
- if selection is on a TextFormControl, use the updateCaretRect function.

Source/core/editing/PositionWithAffinity.h, VisiblePosition.h:
- Move the localCaretRect function to PositionWithAffinity. It uses position and 
affinity so moving is straightforward.

Source/core/html/HTMLTextFormControlElementTest.cpp:
- Add a test to confirm FrameSelection::localCaretRect not to cause layout.

BUG=382809
TEST=HTMLTextFormControlElementTest.cpp

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179133

[yoichio, 2014-07-29 06:14:14]

PTAL

[yosin_UTC9, 2014-07-29 07:00:20]

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1227: return
!!enclosingTextFormControl(selection.start());
nit: I don't think we need to use |!!| for casting pointer to |bool|.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1236: if (isTextFormControl(m_selection))
nit: We can combine |else| and |if|.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1237: m_absCaretBoundsDirty |=
updateCaretRect(m_frame->document(),
PositionWithAffinity(m_selection.start().isCandidate() ? m_selection.start() :
Position(), m_selection.affinity()));
Q: Why do we need to check |start()| is editable position?

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
File Source/core/editing/PositionWithAffinity.cpp (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.cpp:26: LayoutRect
PositionWithAffinity::localCaretRect(RenderObject*& renderer) const
It seems we can return just renderer and computing local caret rect on call
sites.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.cpp:29: renderer = 0;
nit: Let's use |nullptr|.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
File Source/core/editing/PositionWithAffinity.h (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.h:24: LayoutRect
localCaretRect(RenderObject*&) const;
We would like to keep PositionWithAninity as simple as possible and keep away
from rendering, which is different layer.

This function should be in VisibleUnit.{cpp,h}.

[yoichio, 2014-07-29 08:05:17]

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1227: return
!!enclosingTextFormControl(selection.start());
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> nit: I don't think we need to use |!!| for casting pointer to |bool|.

Done.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1236: if (isTextFormControl(m_selection))
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> nit: We can combine |else| and |if|.

Done.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1237: m_absCaretBoundsDirty |=
updateCaretRect(m_frame->document(),
PositionWithAffinity(m_selection.start().isCandidate() ? m_selection.start() :
Position(), m_selection.affinity()));
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> Q: Why do we need to check |start()| is editable position?

updateCaretRect suppose that an argument Visible position is Null if start is
not on editable position.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
File Source/core/editing/PositionWithAffinity.cpp (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.cpp:26: LayoutRect
PositionWithAffinity::localCaretRect(RenderObject*& renderer) const
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> It seems we can return just renderer and computing local caret rect on call
> sites.
calling localCaretRect uses local variables |inlineBox| and |caretOffset| so it
is not simple.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.cpp:29: renderer = 0;
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> nit: Let's use |nullptr|.

Done.

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
File Source/core/editing/PositionWithAffinity.h (right):

https://codereview.chromium.org/429453004/diff/100001/Source/core/editing/Pos...
Source/core/editing/PositionWithAffinity.h:24: LayoutRect
localCaretRect(RenderObject*&) const;
On 2014/07/29 07:00:20, Yosi_UTC9 wrote:
> We would like to keep PositionWithAninity as simple as possible and keep away
> from rendering, which is different layer.
> 
> This function should be in VisibleUnit.{cpp,h}.

Done.

[yosin_UTC9, 2014-07-29 09:09:02]

LGTM w/ nit

Please add TEST=HTMLTextFormControlElementTest.cpp in description.

You need to ask OWNERS review as usual.

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Vis...
File Source/core/editing/VisibleUnits.h (right):

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Vis...
Source/core/editing/VisibleUnits.h:99: // Rect is local to the returned renderer
Oops, sorry this function is only used in VisiblePosition. We would like to this
function as static function in VisiblePosition.cpp.

[yoichio, 2014-07-29 09:14:16]

On 2014/07/29 09:09:02, Yosi_UTC9 wrote:
> LGTM w/ nit
> 
> Please add TEST=HTMLTextFormControlElementTest.cpp in description.
> 
> You need to ask OWNERS review as usual.
> 
>
https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Vis...
> File Source/core/editing/VisibleUnits.h (right):
> 
>
https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Vis...
> Source/core/editing/VisibleUnits.h:99: // Rect is local to the returned
renderer
> Oops, sorry this function is only used in VisiblePosition. We would like to
this
> function as static function in VisiblePosition.cpp.
No. CaretBase::updateCaretRect calls without VisiblePosition.

[yoichio, 2014-07-29 09:17:21]

Add tkent@ as an OWNER.
Thanks in advance.

[tkent, 2014-07-29 09:30:20]

lgtm

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Car...
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Car...
Source/core/editing/Caret.h:51: bool updateCaretRect(Document*, const
PositionWithAffinity& caretPosition);
Please add a comment about differences between two versions of updateCaretRect.

https://codereview.chromium.org/429453004/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElementTest.cpp (right):

https://codereview.chromium.org/429453004/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElementTest.cpp:41: private:
nit: We put one blank line before private:.

[yoichio, 2014-07-29 10:32:23]

The CQ bit was checked by yoichio@chromium.org

[yoichio, 2014-07-29 10:32:46]

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Car...
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/429453004/diff/120001/Source/core/editing/Car...
Source/core/editing/Caret.h:51: bool updateCaretRect(Document*, const
PositionWithAffinity& caretPosition);
On 2014/07/29 09:30:20, tkent wrote:
> Please add a comment about differences between two versions of
updateCaretRect.

Done.

https://codereview.chromium.org/429453004/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElementTest.cpp (right):

https://codereview.chromium.org/429453004/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElementTest.cpp:41: private:
On 2014/07/29 09:30:20, tkent wrote:
> nit: We put one blank line before private:.

Done.

[commit-bot: I haz the power, 2014-07-29 10:32:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/429453004/140001

[commit-bot: I haz the power, 2014-07-29 11:39:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17506)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19433)

[commit-bot: I haz the power, 2014-07-29 12:26:09]

Change committed as 179133

[tkent, 2014-07-30 00:29:33]

https://codereview.chromium.org/429453004/diff/140001/Source/core/editing/Car...
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/429453004/diff/140001/Source/core/editing/Car...
Source/core/editing/Caret.h:52: // Simply calls above updateCaretRect using
deepEquivalent() and affinity().
This comment is not helpful.
Please write a comment to help a developer who wants to call updateCaretRect.
The important difference is synchronous layout, and we should use the former
version if possible, right?