Ensure clean layout before calling plainText() (flat tree version)

Ensure clean layout before calling plainText() (flat tree version)

BUG=590369
Committed: https://crrev.com/2f9c93110386ac6742a7bb55d7057aff368113e5
Cr-Commit-Position: refs/heads/master@{#416204}

[Xiaocheng, 2016-08-30 10:02:04]

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

[Xiaocheng, 2016-08-30 10:02:16]

Description was changed from

==========
Update layout before calling plainText() (flat tree version)

BUG=590369
==========

to

==========
Ensure clean layout before calling plainText() (flat tree version)

BUG=590369
==========

[commit-bot: I haz the power, 2016-08-30 10:02:22]

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, 2016-08-30 10:17:35]

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

[commit-bot: I haz the power, 2016-08-30 10:17:36]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xiaocheng, 2016-08-30 10:20:11]

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

[commit-bot: I haz the power, 2016-08-30 10:20:21]

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

[Xiaocheng, 2016-08-30 11:50:03]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-08-30 11:50:04]

PTAL.

[commit-bot: I haz the power, 2016-08-30 13:43:01]

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

[commit-bot: I haz the power, 2016-08-30 13:43:02]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-08-31 04:58:22]

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1084: String
FrameSelection::selectedTextForClipboard() const
There are only three call sites of selectedTextForClipboard(), we should call
updateLayout() in there.

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1095: return
selectedText();
It seems we can replace |selectedText()| call to |extractSelectedText(*this,
TextIteratorDefaultBehavior);|.

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionController.cpp:324:
pos.deepEquivalent().document()->updateStyleAndLayoutIgnorePendingStylesheets();
There are only two call sites of selectClosestWordFromHitTestResult(),
updateLayout() should be there.
For SelectionController::handleGestureLongPress(), we don't need to call
updateLayout()

[Xiaocheng, 2016-09-02 05:35:47]

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

[commit-bot: I haz the power, 2016-09-02 05:36:01]

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

[Xiaocheng, 2016-09-02 05:37:23]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2016-09-02 05:37:24]

Updated. PTAL.

Also +tkent for reviewing the affected files outside editing.

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1084: String
FrameSelection::selectedTextForClipboard() const
On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> There are only three call sites of selectedTextForClipboard(), we should call
updateLayout() in there.

I guess the layout is already clean when called from
DragController::populateDragDataTransfer() since it computes a HitTestResult
which can only be done with clean layout?

Done for the other two call sites.

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1095: return
selectedText();
On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> It seems we can replace |selectedText()| call to |extractSelectedText(*this,
TextIteratorDefaultBehavior);|.

Makes sense. Done.

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionController.cpp:324:
pos.deepEquivalent().document()->updateStyleAndLayoutIgnorePendingStylesheets();
On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> There are only two call sites of selectClosestWordFromHitTestResult(),
updateLayout() should be there.

Done.

> For SelectionController::handleGestureLongPress(), we don't need to call
updateLayout()

Is it because of the call to updateStyleAndLayoutTree()? What's its difference
with updateStyleAndLayout()?

[yosin_UTC9, 2016-09-02 06:06:11]

On 2016/09/02 at 05:37:24, xiaochengh wrote:
> Updated. PTAL.
> 
> Also +tkent for reviewing the affected files outside editing.
> 
>
https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:1084: String
FrameSelection::selectedTextForClipboard() const
> On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> > There are only three call sites of selectedTextForClipboard(), we should
call updateLayout() in there.
> 
> I guess the layout is already clean when called from
DragController::populateDragDataTransfer() since it computes a HitTestResult
which can only be done with clean layout?
> 
> Done for the other two call sites.
> 
>
https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:1095: return
selectedText();
> On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> > It seems we can replace |selectedText()| call to |extractSelectedText(*this,
TextIteratorDefaultBehavior);|.
> 
> Makes sense. Done.
> 
>
https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):
> 
>
https://codereview.chromium.org/2292913002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/SelectionController.cpp:324:
pos.deepEquivalent().document()->updateStyleAndLayoutIgnorePendingStylesheets();
> On 2016/08/31 at 04:58:21, Yosi_UTC9 wrote:
> > There are only two call sites of selectClosestWordFromHitTestResult(),
updateLayout() should be there.
> 
> Done.
> 
> > For SelectionController::handleGestureLongPress(), we don't need to call
updateLayout()
> 
> Is it because of the call to updateStyleAndLayoutTree()? What's its difference
with updateStyleAndLayout()?

Sorry for confusion, I mean "updateStyleAndLayoutTree()".

[yosin_UTC9, 2016-09-02 06:06:20]

lgtm

[tkent, 2016-09-02 06:13:26]

lgtm

[commit-bot: I haz the power, 2016-09-02 06:54:27]

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

[commit-bot: I haz the power, 2016-09-02 06:54:28]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-09-02 06:54:49]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2016-09-02 06:54:57]

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-02 06:58:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 07:00:16]

Description was changed from

==========
Ensure clean layout before calling plainText() (flat tree version)

BUG=590369
==========

to

==========
Ensure clean layout before calling plainText() (flat tree version)

BUG=590369

Committed: https://crrev.com/2f9c93110386ac6742a7bb55d7057aff368113e5
Cr-Commit-Position: refs/heads/master@{#416204}
==========

[commit-bot: I haz the power, 2016-09-02 07:00:17]

Patchset 3 (id:??) landed as
https://crrev.com/2f9c93110386ac6742a7bb55d7057aff368113e5
Cr-Commit-Position: refs/heads/master@{#416204}