Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in InputMethodController::setSelectio…

Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in InputMethodController::setSelectionOffsets

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

[Xiaocheng, 2016-09-07 09:51:22]

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

[commit-bot: I haz the power, 2016-09-07 09:51: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, 2016-09-07 13:13:55]

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

[commit-bot: I haz the power, 2016-09-07 13:13:56]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-09-08 01:40:11]

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

[Xiaocheng, 2016-09-08 01:40:12]

PTAL.

[yosin_UTC9, 2016-09-08 04:04:41]

https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:262:
frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
This is the first pattern which updates layout after calling mutator.
Could you explain why do we do this?
I think we should avoid this pattern. 
Caller of |confirmCompositionOrInsertText()| should update layout.

[Xiaocheng, 2016-09-08 06:39:10]

https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:262:
frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
On 2016/09/08 at 04:04:41, Yosi_UTC9 wrote:
> This is the first pattern which updates layout after calling mutator.
> Could you explain why do we do this?
> I think we should avoid this pattern. 
> Caller of |confirmCompositionOrInsertText()| should update layout.

It is in fact the dtor of the SelectionOffsetsScope that requires clean layout.
Is there anything we can do in this case?

[yosin_UTC9, 2016-09-08 06:49:39]

On 2016/09/08 at 06:39:10, xiaochengh wrote:
>
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:262:
frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
> On 2016/09/08 at 04:04:41, Yosi_UTC9 wrote:
> > This is the first pattern which updates layout after calling mutator.
> > Could you explain why do we do this?
> > I think we should avoid this pattern. 
> > Caller of |confirmCompositionOrInsertText()| should update layout.
> 
> It is in fact the dtor of the SelectionOffsetsScope that requires clean
layout. Is there anything we can do in this case?

Why not calling in dtor of SelectionOffsetsScope, just before calling
:setSelectionOffsets()?

[Xiaocheng, 2016-09-08 06:58:19]

On 2016/09/08 at 06:49:39, yosin wrote:
> On 2016/09/08 at 06:39:10, xiaochengh wrote:
> >
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:262:
frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
> > On 2016/09/08 at 04:04:41, Yosi_UTC9 wrote:
> > > This is the first pattern which updates layout after calling mutator.
> > > Could you explain why do we do this?
> > > I think we should avoid this pattern. 
> > > Caller of |confirmCompositionOrInsertText()| should update layout.
> > 
> > It is in fact the dtor of the SelectionOffsetsScope that requires clean
layout. Is there anything we can do in this case?
> 
> Why not calling in dtor of SelectionOffsetsScope, just before calling
:setSelectionOffsets()?

Do you mean we just leave it in the dtor and do not try to further hoist it for
now due to the bad pattern?

[yosin_UTC9, 2016-09-08 07:03:29]

On 2016/09/08 at 06:58:19, xiaochengh wrote:
> On 2016/09/08 at 06:49:39, yosin wrote:
> > On 2016/09/08 at 06:39:10, xiaochengh wrote:
> > >
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/2316053002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/editing/InputMethodController.cpp:262:
frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
> > > On 2016/09/08 at 04:04:41, Yosi_UTC9 wrote:
> > > > This is the first pattern which updates layout after calling mutator.
> > > > Could you explain why do we do this?
> > > > I think we should avoid this pattern. 
> > > > Caller of |confirmCompositionOrInsertText()| should update layout.
> > > 
> > > It is in fact the dtor of the SelectionOffsetsScope that requires clean
layout. Is there anything we can do in this case?
> > 
> > Why not calling in dtor of SelectionOffsetsScope, just before calling
:setSelectionOffsets()?
> 
> Do you mean we just leave it in the dtor and do not try to further hoist it
for now due to the bad pattern?

How about get rid of SelectionOffsetsScope? It seems it is used only one place
now.
We don't need to have it anymore. (^_^)

[Xiaocheng, 2016-09-08 07:34:14]

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

[commit-bot: I haz the power, 2016-09-08 07:50:07]

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-08 08:30:47]

Updated with the scope unfolded and removed.

PTAL.

[commit-bot: I haz the power, 2016-09-08 09:17:44]

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

[commit-bot: I haz the power, 2016-09-08 09:17:45]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-09-08 09:27:45]

lgtm

Excellent!
(^_^)b

[Xiaocheng, 2016-09-08 09:31:34]

The CQ bit was checked by xiaochengh@chromium.org

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

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-08 09:37:39]

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

[commit-bot: I haz the power, 2016-09-08 09:37:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tkent, 2016-09-08 09:47:34]

lgtm

[Xiaocheng, 2016-09-08 10:59:25]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2016-09-08 10:59:38]

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-08 11:04:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 11:06:34]

Description was changed from

==========
Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in
InputMethodController::setSelectionOffsets

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

to

==========
Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in
InputMethodController::setSelectionOffsets

BUG=590369

Committed: https://crrev.com/a6979e66d796e1dbee9b907dea5ef3b8e4b144c4
Cr-Commit-Position: refs/heads/master@{#417250}
==========

[commit-bot: I haz the power, 2016-09-08 11:06:35]

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