Avoid rotation anchor during transitional fullscreen states.

Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG=698315
Review-Url: https://codereview.chromium.org/2745313002
Cr-Commit-Position: refs/heads/master@{#456495}
Committed: https://chromium.googlesource.com/chromium/src/+/80da37a9ae40452f920fdb8a5bbd55454f7e560c

[aelias_OOO_until_Jul13, 2017-03-13 20:43:44]

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

[commit-bot: I haz the power, 2017-03-13 20:44:42]

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

[aelias_OOO_until_Jul13, 2017-03-13 20:51:24]

Description was changed from

==========
Avoid rotation anchor during transitional fullscreen states.

BUG=698315
==========

to

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.
    
This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case as it doesn't
touch page scale.

BUG=698315
==========

[aelias_OOO_until_Jul13, 2017-03-13 20:51:52]

aelias@chromium.org changed reviewers:
+ skobes@chromium.org

[aelias_OOO_until_Jul13, 2017-03-13 20:55:13]

Description was changed from

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.
    
This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case as it doesn't
touch page scale.

BUG=698315
==========

to

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

BUG=698315
==========

[aelias_OOO_until_Jul13, 2017-03-13 20:57:18]

Hi skobes@, this is a quick mitigation for an M57 blocker.

There's further work to do here: testing, but more than that fixing the problem
for the resize anchor as well.  That would involve something like reordering or
suppressing the fullscreencontroller's restoration action so it doesn't happen
during the anchor, or disabling the anchor entirely in this specific condition. 
However, that's too risky for M57 cherry-pick so I'd like to go with this very
well-understood mitigation for now.

[mlamouri (slow - plz ping), 2017-03-13 21:00:55]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-13 21:00:56]

https://codereview.chromium.org/2745313002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.h (right):

https://codereview.chromium.org/2745313002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.h:63: bool isFullscreen() {
return m_state != State::Initial; }
nit: should it be renamed "isFullscreenOrTransitionning() const"?

[skobes, 2017-03-13 21:04:22]

Do we know which change caused the regression and how?  It wasn't clear to me
from skimming the bug.

[aelias_OOO_until_Jul13, 2017-03-13 21:08:27]

It was bisected to
https://chromium.googlesource.com/chromium/src/+/1a6bd5037fc10e45646715d675e6...
.  It's a longstanding issue (I've occasionally seen similar symptoms even a
year ago) but the repro is sensitive to ordering.  Any random resize timing
change that causes there more often to be a single resize for the fullscreen
exit rather than two, therefore to cause the fullscreen restoration to happen
during the resize anchor, increases the frequency of the problem.  But there's
nothing incorrect in principle about this scenario of having a single resize
(it's even preferable in theory, IMO).

[skobes, 2017-03-13 21:09:49]

lgtm

[aelias_OOO_until_Jul13, 2017-03-13 21:27:25]

Description was changed from

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

BUG=698315
==========

to

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG=698315
==========

[aelias_OOO_until_Jul13, 2017-03-13 21:27:33]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2017-03-13 21:28:15]

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, 2017-03-13 21:34:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489440453495600,
"parent_rev": "7c5c6039abbbba1156a568d28154139f4fcc86ce", "commit_rev":
"80da37a9ae40452f920fdb8a5bbd55454f7e560c"}

[commit-bot: I haz the power, 2017-03-13 21:35:24]

Description was changed from

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG=698315
==========

to

==========
Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG=698315

Review-Url: https://codereview.chromium.org/2745313002
Cr-Commit-Position: refs/heads/master@{#456495}
Committed:
https://chromium.googlesource.com/chromium/src/+/80da37a9ae40452f920fdb8a5bbd...
==========

[commit-bot: I haz the power, 2017-03-13 21:35:25]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/80da37a9ae40452f920fdb8a5bbd...