Suppress frame requests in InputMethodController::textInputType()

Suppress frame requests in InputMethodController::textInputType()

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in InputMethodController::textInputType().

This is a temporary workaround for a performance regression bug where Blink
would continuously request main thread frames during a composited animation.

This patch is intended to be non invasive and suitable for merging. The proper
fix for this issue involves much larger changes. See design doc:
https://docs.google.com/document/d/1A22HVDbVh2xhPU0OeXVWTSAriZMsd_rH5seO7xIuzCI

BUG=704763

[alancutter (OOO until 2018), 2017-04-03 01:48:45]

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

[commit-bot: I haz the power, 2017-04-03 01:48:51]

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

[alancutter (OOO until 2018), 2017-04-03 01:56:15]

Description was changed from

==========
+pageAnimatorFrameSuppression

BUG=704763
==========

to

==========
Suppress frame requests in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated.
This is a workaround for a performance regression bug where Blink would
continuously request main thread frames during a composited animation.

BUG=704763
==========

[alancutter (OOO until 2018), 2017-04-03 01:56:15]

alancutter@chromium.org changed reviewers:
+ dstockwell@chromium.org, yosin@chromium.org

[alancutter (OOO until 2018), 2017-04-03 01:59:09]

Description was changed from

==========
Suppress frame requests in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated.
This is a workaround for a performance regression bug where Blink would
continuously request main thread frames during a composited animation.

BUG=704763
==========

to

==========
Suppress frame requests in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated.

This is a temporary workaround for a performance regression bug where Blink
would continuously request main thread frames during a composited animation.

This patch is intended to be non invasive and suitable for merging. The proper
fix for this issue involves much larger changes. See design doc:
https://docs.google.com/document/d/1A22HVDbVh2xhPU0OeXVWTSAriZMsd_rH5seO7xIuzCI

BUG=704763
==========

[alancutter (OOO until 2018), 2017-04-03 04:34:50]

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

[commit-bot: I haz the power, 2017-04-03 04:34:57]

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

[alancutter (OOO until 2018), 2017-04-03 04:57:57]

alancutter@chromium.org changed reviewers:
+ esprehn@chromium.org

[dstockwell, 2017-04-03 05:07:03]

lgtm

[esprehn, 2017-04-03 05:14:16]

I don't think this is a good idea, using a frame suppression scope is almost
always a bug. Also what else calls this function? Anything that calls it that
happens to flush pending sheets will then fail to schedule a frame and we won't
repaint. This patch makes me very worried we're adding race condition where the
page becomes busted and stops painting.

This frame suppression logic is literally only okay inside BeginMainFrame. I'm
also weary of something named so generally getting used widely once it exists
while is painful technical debt and usually means bugs.

[yosin_UTC9, 2017-04-03 05:17:14]

lgtm

Thanks for workaround!

Patch [1] will make not to call update layout for opacity animation case, but
there are unseen
update layout call some path during animation. This patch fix perf regression in
such path.

We're investigating why [1] causes crash.

[1]  http://crrev.com/2782413002: Get rid of
computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController

[alancutter (OOO until 2018), 2017-04-03 05:35:08]

Description was changed from

==========
Suppress frame requests in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in
FrameSelection::computeVisibleSelectionInDOMTreeDeprecated.

This is a temporary workaround for a performance regression bug where Blink
would continuously request main thread frames during a composited animation.

This patch is intended to be non invasive and suitable for merging. The proper
fix for this issue involves much larger changes. See design doc:
https://docs.google.com/document/d/1A22HVDbVh2xhPU0OeXVWTSAriZMsd_rH5seO7xIuzCI

BUG=704763
==========

to

==========
Suppress frame requests in InputMethodController::textInputType()

This change generalises and exposes the frame request suppression logic in
PageAnimator for use in InputMethodController::textInputType().

This is a temporary workaround for a performance regression bug where Blink
would continuously request main thread frames during a composited animation.

This patch is intended to be non invasive and suitable for merging. The proper
fix for this issue involves much larger changes. See design doc:
https://docs.google.com/document/d/1A22HVDbVh2xhPU0OeXVWTSAriZMsd_rH5seO7xIuzCI

BUG=704763
==========

[esprehn, 2017-04-03 05:39:41]

not lgtm, this method is also called here:
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?l=2201

which is not inside BeginMainFrame. Also I don't think we should have a generic
suppression system that any code inside blink can use from anywhere.

[alancutter (OOO until 2018), 2017-04-03 05:42:02]

On 2017/04/03 at 05:14:16, esprehn wrote:
> I don't think this is a good idea, using a frame suppression scope is almost
always a bug. Also what else calls this function? Anything that calls it that
happens to flush pending sheets will then fail to schedule a frame and we won't
repaint. This patch makes me very worried we're adding race condition where the
page becomes busted and stops painting.

I'm in complete agreement with these concerns. I see that
computeVisibleSelectionInDOMTreeDeprecated() has >200 callsites, this is far too
many to reason about and very very scary.
I moved the suppression up to InputMethodController::textInputType() instead.
This only has 2 call sites, both of which are part of the web interface and not
used by Blink internally. Doesn't alleviate all concerns for me but that's
mostly because I'm unfamiliar with this method and how it's used. WDYT?

> This frame suppression logic is literally only okay inside BeginMainFrame. I'm
also weary of something named so generally getting used widely once it exists
while is painful technical debt and usually means bugs.

Added comments to the header.

[alancutter (OOO until 2018), 2017-04-03 05:42:16]

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

[commit-bot: I haz the power, 2017-04-03 05:42:22]

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

[alancutter (OOO until 2018), 2017-04-03 05:43:40]

On 2017/04/03 at 05:42:02, alancutter wrote:
> On 2017/04/03 at 05:14:16, esprehn wrote:
> > I don't think this is a good idea, using a frame suppression scope is almost
always a bug. Also what else calls this function? Anything that calls it that
happens to flush pending sheets will then fail to schedule a frame and we won't
repaint. This patch makes me very worried we're adding race condition where the
page becomes busted and stops painting.
> 
> I'm in complete agreement with these concerns. I see that
computeVisibleSelectionInDOMTreeDeprecated() has >200 callsites, this is far too
many to reason about and very very scary.
> I moved the suppression up to InputMethodController::textInputType() instead.
This only has 2 call sites, both of which are part of the web interface and not
used by Blink internally. Doesn't alleviate all concerns for me but that's
mostly because I'm unfamiliar with this method and how it's used. WDYT?

Nevermind, the not lgtm covers textInputType().

[esprehn, 2017-04-03 06:03:55]

On 2017/04/03 at 05:43:40, alancutter wrote:
> On 2017/04/03 at 05:42:02, alancutter wrote:
> > On 2017/04/03 at 05:14:16, esprehn wrote:
> > > I don't think this is a good idea, using a frame suppression scope is
almost always a bug. Also what else calls this function? Anything that calls it
that happens to flush pending sheets will then fail to schedule a frame and we
won't repaint. This patch makes me very worried we're adding race condition
where the page becomes busted and stops painting.
> > 
> > I'm in complete agreement with these concerns. I see that
computeVisibleSelectionInDOMTreeDeprecated() has >200 callsites, this is far too
many to reason about and very very scary.
> > I moved the suppression up to InputMethodController::textInputType()
instead. This only has 2 call sites, both of which are part of the web interface
and not used by Blink internally. Doesn't alleviate all concerns for me but
that's mostly because I'm unfamiliar with this method and how it's used. WDYT?
> 
> Nevermind, the not lgtm covers textInputType().

Based on what you did here I had an idea:
https://bugs.chromium.org/p/chromium/issues/detail?id=704763#c35

What if instead we add a new callback in the blink public API where
WillBeginMainFrame() disables frame updates until the call to
updateAllLifecyclePhases?

[yosin_UTC9, 2017-04-03 06:27:42]

On 2017/04/03 at 05:17:14, yosin_UTC9 wrote:
> lgtm
> 
> Thanks for workaround!
> 
> Patch [1] will make not to call update layout for opacity animation case, but
there are unseen
> update layout call some path during animation. This patch fix perf regression
in such path.
> 
> We're investigating why [1] causes crash.
> 
> [1]  http://crrev.com/2782413002: Get rid of
computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController

The crash reason of patch[1] is Blink paints with dirty layout tree[2]
Without [1], we make clean layout tree in IMC::textInputType(), by calling
updateStyleAndLayoutTreeIgnorePendingStylesheets(), during WillBeignMainFrame().
With [1], Blink paints with dirty layout tree then call Element::clientHeight()
during painting media play button and which calls update layout.
It seems that dirty layout  LayoutTextTrackContainer::layout() which modifies
inline style property[3].

[2] http://crbug.com/706865 ⚐	Crash during system_health.common_mobile
browse:media:youtube on Nexus5 Perf bot
[3] http://crbug.com/372245 ⚐	LayoutTextTrackContainer::layout() should not
modify DOM tree

[commit-bot: I haz the power, 2017-04-03 07:24:48]

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

[commit-bot: I haz the power, 2017-04-03 07:24:49]

Dry run: This issue passed the CQ dry run.