[CRD iOS] Preserve viewport state when toggling the soft keyboard

[CRD iOS] Preserve viewport state when toggling the soft keyboard

Previously toggling the soft keyboard will always reset the viewport state.
This should only be done when the view frame is undergoing significant
changes, e.g. screen rotated.

For now I just define "significant change" as the change of both of the surface
dimensions, by doing so the viewport will not be reset when it's scrolled up to
show temporary elements like software keyboard.

BUG=714193
Review-Url: https://codereview.chromium.org/2909703002
Cr-Commit-Position: refs/heads/master@{#475656}
Committed: https://chromium.googlesource.com/chromium/src/+/61bed118775724243545580086dc1186e6f66949

[Yuwei, 2017-05-26 18:59:19]

yuweih@chromium.org changed reviewers:
+ nicholss@chromium.org

[Yuwei, 2017-05-26 18:59:20]

PTAL thanks!

[Yuwei, 2017-05-26 19:10:57]

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

[commit-bot: I haz the power, 2017-05-26 19:11:30]

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, 2017-05-26 19:19:29]

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

[commit-bot: I haz the power, 2017-05-26 19:19:29]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yuwei, 2017-05-26 19:23:08]

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

[commit-bot: I haz the power, 2017-05-26 19:23:41]

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, 2017-05-26 19:31:30]

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

[commit-bot: I haz the power, 2017-05-26 19:31:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-05-26 20:12:50]

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
File remoting/client/ui/desktop_viewport.cc (right):

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
remoting/client/ui/desktop_viewport.cc:39: // preserve the viewport state in
case the surface is just scrolled up.
The keyboard showing changes the size of the viewport. so this comment is not
correct

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
File remoting/client/ui/desktop_viewport.h (right):

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
remoting/client/ui/desktop_viewport.h:41: // Sets the |surface_size_| and
(re)initializes the viewport if both
I think this is too much to assume.

[Yuwei, 2017-05-26 21:38:21]

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
File remoting/client/ui/desktop_viewport.h (right):

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
remoting/client/ui/desktop_viewport.h:41: // Sets the |surface_size_| and
(re)initializes the viewport if both
On 2017/05/26 20:12:49, nicholss wrote:
> I think this is too much to assume.

I agree that we bring too many conditional branching into one function.
Basically:

Neither dimension has changed => No-op
Both dimension changed => ResizeToFit
Only one dimension changed => Only update the constraints

I like to hide the idea of the constraint outside the class.

Maybe we can remove the ResizeToFit() call from the function and give it to the
caller to control, and make SetSurfaceSize always update the constraints.
Potentially this will trigger two viewport changes but they will be synchronized
into the same frame so I think it's fine.

I think the bigger question is what is the criteria for triggering
ResizeToFit(). Both rotating screen and toggling the keyboard (maybe later
opening the side menu) will trigger the same codepath of viewDidLayoutSubviews.

For use cases we have:
* Rotation => Reset
* Keyboard => No reset
* Settings menu => No reset

Checking whether two dimensions match covers all use cases above. I'm not sure
whether that's an overfitting for this problem thought..

[nicholss, 2017-05-30 16:38:43]

lgtm, I do think this is overfit for the problem but it works for now and we
might revisit it later when we do iPad Pro integrations.

[Yuwei, 2017-05-30 19:27:09]

Thanks!

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
File remoting/client/ui/desktop_viewport.cc (right):

https://codereview.chromium.org/2909703002/diff/20001/remoting/client/ui/desk...
remoting/client/ui/desktop_viewport.cc:39: // preserve the viewport state in
case the surface is just scrolled up.
On 2017/05/26 20:12:49, nicholss wrote:
> The keyboard showing changes the size of the viewport. so this comment is not
> correct

Done.

[Yuwei, 2017-05-30 19:27:14]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-30 19:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2909703002/#ps40001
(title: "fix feedback")

[commit-bot: I haz the power, 2017-05-30 19:27: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, 2017-05-30 19:48:06]

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

[commit-bot: I haz the power, 2017-05-30 19:48:07]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Yuwei, 2017-05-30 19:52:46]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-30 19:52:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2909703002/#ps60001
(title: "Merge ToT")

[commit-bot: I haz the power, 2017-05-30 19:53:17]

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-05-30 20:26:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496173966513310,
"parent_rev": "1d7b16f9b7854268e4a0ff8c94d5989fb84cd1ce", "commit_rev":
"61bed118775724243545580086dc1186e6f66949"}

[commit-bot: I haz the power, 2017-05-30 20:26:10]

Description was changed from

==========
[CRD iOS] Preserve viewport state when toggling the soft keyboard

Previously toggling the soft keyboard will always reset the viewport state.
This should only be done when the view frame is undergoing significant
changes, e.g. screen rotated.

For now I just define "significant change" as the change of both of the surface
dimensions, by doing so the viewport will not be reset when it's scrolled up to
show temporary elements like software keyboard.

BUG=714193
==========

to

==========
[CRD iOS] Preserve viewport state when toggling the soft keyboard

Previously toggling the soft keyboard will always reset the viewport state.
This should only be done when the view frame is undergoing significant
changes, e.g. screen rotated.

For now I just define "significant change" as the change of both of the surface
dimensions, by doing so the viewport will not be reset when it's scrolled up to
show temporary elements like software keyboard.

BUG=714193

Review-Url: https://codereview.chromium.org/2909703002
Cr-Commit-Position: refs/heads/master@{#475656}
Committed:
https://chromium.googlesource.com/chromium/src/+/61bed118775724243545580086dc...
==========

[commit-bot: I haz the power, 2017-05-30 20:26:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/61bed118775724243545580086dc...