Upstream //ios/chrome/browser/snapshots

Upstream //ios/chrome/browser/snapshots

snapshots_util is also moved from ui/ to snapshots/ as part of this CL.

NOPRESUBMIT=true

Committed: https://crrev.com/17f3dfc806ea91dd71f3ba7d4b172a191caa2351
Cr-Commit-Position: refs/heads/master@{#316584}

[droger, 2015-01-20 16:49:03]

Patchset #1 (id:1) has been deleted

[droger, 2015-01-20 16:53:54]

droger@chromium.org changed reviewers:
+ msarda@chromium.org

[msarda, 2015-01-21 16:45:02]

lgtm

[droger, 2015-01-21 16:45:51]

droger@chromium.org changed reviewers:
+ thakis@chromium.org

[droger, 2015-01-21 16:45:52]

+thakis for base/test/ios

[droger, 2015-01-26 08:53:34]

thakis: friendly ping.

[droger, 2015-01-29 12:58:46]

thakis: ping.

[Nico, 2015-01-29 19:24:39]

thakis@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Nico, 2015-01-29 19:24:39]

Wait-and-poll code in tests is usually a bad idea. Can you restructure your code
to use regular async patterns to prevent this?

If not, can you use the timeouts from base/test/test_timeouts.h ?


Also, this is ios-specific and should only be called in testing code, so if you
need this code it should be in namespace base::test::ios

Also, can you use the existing runloop / messageloop abstractions instead of
using nsrunloop?

Also, do you need all these overloads? Can't tests just call this with the
arguments they need? (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...
)

[Paweł Hajdan Jr., 2015-01-30 12:24:29]

https://codereview.chromium.org/862693003/diff/20001/base/test/ios/wait_util.h
File base/test/ios/wait_util.h (right):

https://codereview.chromium.org/862693003/diff/20001/base/test/ios/wait_util....
base/test/ios/wait_util.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/base/test/ios/wait_util.mm
File base/test/ios/wait_util.mm (right):

https://codereview.chromium.org/862693003/diff/20001/base/test/ios/wait_util....
base/test/ios/wait_util.mm:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/base/test/ios/wait_util....
base/test/ios/wait_util.mm:27: while ((-[startTime timeIntervalSinceNow]) <
timeout_in_seconds &&
I strongly second Nico's comment about waiting properly for events.

Could you explain why this code was written in this way (as opposed to waiting
for an event)?

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_cache.h (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_cache.h:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_cache.mm:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_cache_unittest.mm (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_cache_unittest.mm:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_cache_unittest.mm:135: NSTimeInterval
kTimeoutSec = 3;
I strongly second Nico's comment about using base/test/test_timeouts instead of
hardcoding things in random places (applies to entire CL).

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_manager.h (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_manager.h:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_manager.mm (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_manager.mm:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_overlay.h (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_overlay.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
File ios/chrome/browser/snapshots/snapshot_overlay.mm (right):

https://codereview.chromium.org/862693003/diff/20001/ios/chrome/browser/snaps...
ios/chrome/browser/snapshots/snapshot_overlay.mm:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: 2015

[droger, 2015-01-30 12:54:47]

Thanks for the comments.
I don't know currently why this code was written like this, I'm trying to
investigate and see if we can do differently.

About the dates in the copyright notice: so far when upstreaming files we have
been keeping the date unchanged (i.e. the date refers to the creation of the
file rather than its upstreaming). Let me know if you think it is wrong.

[droger, 2015-01-30 15:09:21]

I have a downstream CL to remove the polling in snapshot cache:
https://chromereviews.googleplex.com/148707013/

Basically I'm replacing the native high priority dispatch queue by the
FILE_USER_BLOCKING thread, and then I can replace polling by
base::RunLoop().RunUntilIdle()
and  web::WebThread::GetBlockingPool()->FlushForTesting()

[Paweł Hajdan Jr., 2015-01-30 21:25:09]

On 2015/01/30 15:09:21, droger wrote:
> I have a downstream CL to remove the polling in snapshot cache:
> https://chromereviews.googleplex.com/148707013/
> 
> Basically I'm replacing the native high priority dispatch queue by the
> FILE_USER_BLOCKING thread, and then I can replace polling by
> base::RunLoop().RunUntilIdle()
> and  web::WebThread::GetBlockingPool()->FlushForTesting()

Sounds good (didn't take a deeper look at that CL), thanks!

I'll be OOO next week, Nico can finish the review as far as I'm concerned
(OWNERS of base/ also work for base/test).

[stuartmorgan, 2015-02-16 12:09:01]

On 2015/01/29 19:24:39, Nico wrote:
> Wait-and-poll code in tests is usually a bad idea. Can you restructure your
code
> to use regular async patterns to prevent this?

The wait_utils code is mostly used for interacting with web views, where we
don't control how the async code is run. For instance, anything running JS is
async, and spinning the runloop and waiting for a callback is the only way to
let the test finish. I don't think there's much we can do other than that. So
for this particular test we can change, but I don't think it'll generalize.

David suggested we move it to ios/web/; that would work, but we'd probably need
to make it public there in case components built on it wanted to test JS-related
paths.

> Also, can you use the existing runloop / messageloop abstractions instead of
> using nsrunloop?

See above; IIRC in the iOS model this is the only way to spin the runloop. I
could be mistaken though.

> Also, do you need all these overloads? Can't tests just call this with the
> arguments they need? (
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...
> )

We could, but this doesn't seem to go against the guidance there; the section
right after it specifically suggests using overloading instead of default
arguments. The timeout specifically we wanted a default for since the majority
of cases don't care and pasting a constant everywhere that happened seemed worse
for readability.

[droger, 2015-02-17 16:16:44]

After rebasing, this CL no longer touches //base.
Landing as is.

[droger, 2015-02-17 16:16:49]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:17:08]

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

[commit-bot: I haz the power, 2015-02-17 16:22:26]

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

[commit-bot: I haz the power, 2015-02-17 16:22:27]

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

[droger, 2015-02-17 16:39:58]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:40:28]

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

[commit-bot: I haz the power, 2015-02-17 16:41:33]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-02-17 16:44:24]

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