Don't zoom in TextFinder if autosizing is disabled

Don't zoom in TextFinder if autosizing is disabled

On android devices the zoom-level is always being reset if
the viewed page doesn't use/need text autosizing. Therefore,
only zoom to content if autosizing is set.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199027

[sebastianl, 2015-07-14 13:23:16]

sebastianl@opera.com changed reviewers:
+ johnme@chromium.org, pdr@chromium.org, rbyers@chromium.org,
skobes@chromium.org

[sebastianl, 2015-07-14 13:26:12]

Only change zoom-level when calling TextFinder.Find if autosizing is set for the
page. This to not reset the zoom-level on android-devices (which is annoying)
when using textwrap.

[philipj_slow, 2015-07-14 13:29:14]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-14 13:29:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231673007/1

[philipj_slow, 2015-07-14 13:29:40]

Hi all, Sebastian showed me the email exchange you all had where it seemed like
the #if OS(ANDROID) is just being used as a proxy for "if autosizing enabled" so
this ought not change any behavior in Chrome for Android. Opera for Android on
the other hand disables text autosizing, so that's why this was noticed at all.

It seems like this code is entirely untested, but if there is a way to write a
unit test for this that would be nice.

[commit-bot: I haz the power, 2015-07-14 14:36:45]

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

[commit-bot: I haz the power, 2015-07-14 14:36:46]

Dry run: This issue passed the CQ dry run.

[skobes, 2015-07-14 17:11:28]

If possible, add a test case to TextFinderTest.

https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.cpp
File Source/web/TextFinder.cpp (right):

https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.cpp#n...
Source/web/TextFinder.cpp:158: if
(ownerFrame().frame()->document()->textAutosizer()->pageNeedsAutosizing()) {
Can you add a comment explaining why the check is here?

https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.h
File Source/web/TextFinder.h (right):

https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.h#new...
Source/web/TextFinder.h:35: #include "core/layout/TextAutosizer.h"
I think this #include belongs in TextFinder.cpp, not TextFinder.h.

[sebastianl, 2015-07-15 09:15:40]

On 2015/07/14 17:11:28, skobes wrote:
> If possible, add a test case to TextFinderTest.

Found a way to create a simple unittest, it is in the second patchset, PTAL! :)

On 2015/07/14 17:11:28, skobes wrote:
> https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.cpp
> File Source/web/TextFinder.cpp (right):
> 
>
https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.cpp#n...
> Source/web/TextFinder.cpp:158: if
> (ownerFrame().frame()->document()->textAutosizer()->pageNeedsAutosizing()) {
> Can you add a comment explaining why the check is here?
> 
> https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.h
> File Source/web/TextFinder.h (right):
> 
>
https://codereview.chromium.org/1231673007/diff/1/Source/web/TextFinder.h#new...
> Source/web/TextFinder.h:35: #include "core/layout/TextAutosizer.h"
> I think this #include belongs in TextFinder.cpp, not TextFinder.h.

Included these changes in the second patchset as well

[skobes, 2015-07-15 17:43:28]

lgtm

[sebastianl, 2015-07-16 07:30:24]

The CQ bit was checked by sebastianl@opera.com

[sebastianl, 2015-07-16 07:30:30]

The CQ bit was unchecked by sebastianl@opera.com

[philipj_slow, 2015-07-16 07:38:36]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-16 07:39:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231673007/20001

[philipj_slow, 2015-07-16 07:44:18]

https://codereview.chromium.org/1231673007/diff/20001/Source/web/tests/TextFi...
File Source/web/tests/TextFinderTest.cpp (right):

https://codereview.chromium.org/1231673007/diff/20001/Source/web/tests/TextFi...
Source/web/tests/TextFinderTest.cpp:157: PinchViewport& pinchViewport =
frame().page()->frameHost().pinchViewport();
Would document().page() do the same thing? If so you could drop the new frame()
helper.

[sebastianl, 2015-07-16 07:53:58]

On 2015/07/16 07:44:18, philipj wrote:
>
https://codereview.chromium.org/1231673007/diff/20001/Source/web/tests/TextFi...
> File Source/web/tests/TextFinderTest.cpp (right):
> 
>
https://codereview.chromium.org/1231673007/diff/20001/Source/web/tests/TextFi...
> Source/web/tests/TextFinderTest.cpp:157: PinchViewport& pinchViewport =
> frame().page()->frameHost().pinchViewport();
> Would document().page() do the same thing? If so you could drop the new
frame()
> helper.

You're quite right, thanks! I'll upload a new patch with the change :)

[philipj_slow, 2015-07-16 07:58:12]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-16 07:58:12]

lgtm

[philipj_slow, 2015-07-16 07:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1231673007/#ps40001
(title: "removed frame() helper from test")

[commit-bot: I haz the power, 2015-07-16 07:58:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231673007/40001

[commit-bot: I haz the power, 2015-07-16 09:36:27]

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

[commit-bot: I haz the power, 2015-07-16 09:36:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[philipj_slow, 2015-07-16 09:41:45]

On 2015/07/16 09:36:29, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

Looks like inspector/editor/text-editor-ctrl-d.html is being flaky.

[philipj_slow, 2015-07-16 09:41:51]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-07-16 09:42:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231673007/40001

[commit-bot: I haz the power, 2015-07-16 11:16:15]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199027