Split FaviconService and FaviconServiceImpl.

Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=694312
Review-Url: https://codereview.chromium.org/2698473004
Cr-Commit-Position: refs/heads/master@{#451982}
Committed: https://chromium.googlesource.com/chromium/src/+/d7bd35142ea07f588ba2decdd4c184d68e34d35a

[mastiz, 2017-02-15 09:42:35]

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

[commit-bot: I haz the power, 2017-02-15 09:46:12]

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-02-15 09:53:51]

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

[commit-bot: I haz the power, 2017-02-15 09:54:08]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mastiz, 2017-02-15 10:04:08]

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

[commit-bot: I haz the power, 2017-02-15 10:04:19]

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-02-15 10:11:35]

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

[commit-bot: I haz the power, 2017-02-15 10:11:36]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[mastiz, 2017-02-15 10:41:16]

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

[commit-bot: I haz the power, 2017-02-15 10:41:31]

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-02-15 10:47:47]

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

[commit-bot: I haz the power, 2017-02-15 10:47:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-02-15 10:54:01]

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

[commit-bot: I haz the power, 2017-02-15 10:54:13]

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-02-15 11:01:20]

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

[commit-bot: I haz the power, 2017-02-15 11:01:21]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[mastiz, 2017-02-15 11:55:18]

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

[commit-bot: I haz the power, 2017-02-15 11:55:44]

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-02-15 12:03:53]

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

[commit-bot: I haz the power, 2017-02-15 12:03:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-02-15 12:16:34]

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

[commit-bot: I haz the power, 2017-02-15 12:16:55]

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-02-15 13:08:20]

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

[commit-bot: I haz the power, 2017-02-15 13:08:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2017-02-15 13:51:38]

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

[commit-bot: I haz the power, 2017-02-15 13:52:02]

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

[mastiz, 2017-02-15 13:59:58]

Description was changed from

==========
Split FaviconService and FaviconServiceImpl.

BUG=
==========

to

==========
Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=
==========

[commit-bot: I haz the power, 2017-02-15 14:58:55]

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

[commit-bot: I haz the power, 2017-02-15 14:58:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-02-15 19:43:31]

mastiz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[mastiz, 2017-02-15 19:43:32]

Please don't pay too much attention to tests in
content_favicon_driver_unittest.cc, they will likely be replaced by the
dependent patch.

There's one failing test on Mac that I need to investigate, but otherwise the CL
is ready for review.

[pkotwicz, 2017-02-16 04:07:27]

LGTM with nits

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/con...
File components/favicon/content/content_favicon_driver_unittest.cc (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/con...
components/favicon/content/content_favicon_driver_unittest.cc:97: 
These tests were written with the intention of testing the implementation of
FaviconService::WasUnableToDownloadFavicon(),
FaviconService::UnableToDownloadFavicon() and
FaviconService::ClearUnableToDownloadFavicons()

lines 97 - 107 in the old code test the behavior of
FaviconService::ClearUnableToDownloadFavicons(). As you have removed the call to
FaviconService::ClearUnableToDownloadFavicons() you might as well delete the
tests in lines 98 - 108

The mocking of FaviconService::WasUnableToDownloadFavicon() makes the test on
lines 90 - 96 uninteresting too. You might as well delete that too

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
File components/favicon/core/favicon_service.h (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
components/favicon/core/favicon_service.h:30: const
std::vector<favicon_base::FaviconRawBitmapResult>* results);
In a separate CL we should find a new home for this method. favicon_util.h
perhaps?

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.h (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
components/favicon/core/favicon_service_impl.h:43: // FaviconServiceInterface
implementation.
Nit: "FaviconServiceInterface implementation." -> "FaviconService:"

https://codereview.chromium.org/2698473004/diff/130001/ios/chrome/app/spotlig...
File ios/chrome/app/spotlight/spotlight_manager_unittest.mm (right):

https://codereview.chromium.org/2698473004/diff/130001/ios/chrome/app/spotlig...
ios/chrome/app/spotlight/spotlight_manager_unittest.mm:79:
WithArg<3>(PostCallbackWithResult(CreateTestBitmap(24, 24))));
This is ok because there aren't a lot of places where we implement
FaviconService methods via the EXPECT_CALL() macro. However, I wouldn't want to
implement FaviconService via the EXPECT_CALL macro in many dozens of places.

[mastiz, 2017-02-16 08:40:59]

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

[commit-bot: I haz the power, 2017-02-16 08:42:28]

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

[mastiz, 2017-02-16 09:01:57]

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

[commit-bot: I haz the power, 2017-02-16 09:03:34]

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-02-16 09:08:38]

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

[commit-bot: I haz the power, 2017-02-16 09:08:39]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mastiz, 2017-02-16 09:19:13]

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

[commit-bot: I haz the power, 2017-02-16 09:20:02]

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-02-16 09:24:41]

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

[commit-bot: I haz the power, 2017-02-16 09:24:44]

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_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-02-16 09:42:07]

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

[commit-bot: I haz the power, 2017-02-16 09:42:56]

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

[mastiz, 2017-02-16 09:46:24]

mastiz@chromium.org changed reviewers:
+ noyau@chromium.org

[mastiz, 2017-02-16 09:46:26]

pkotwicz@: thanks for the review!

noyau@: please review ios/

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/con...
File components/favicon/content/content_favicon_driver_unittest.cc (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/con...
components/favicon/content/content_favicon_driver_unittest.cc:97: 
On 2017/02/16 04:07:27, pkotwicz wrote:
> These tests were written with the intention of testing the implementation of
> FaviconService::WasUnableToDownloadFavicon(),
> FaviconService::UnableToDownloadFavicon() and
> FaviconService::ClearUnableToDownloadFavicons()

Since testing FaviconService doesn't belong here, I started
favicon_service_impl_unittest.cc and added specific unit tests there.

> 
> lines 97 - 107 in the old code test the behavior of
> FaviconService::ClearUnableToDownloadFavicons(). As you have removed the call
to
> FaviconService::ClearUnableToDownloadFavicons() you might as well delete the
> tests in lines 98 - 108

Done, removed the lines, but I fail to see what changed: the old code already
verifies the internal state of FaviconService in lines 97 and 98, and still they
were followed up by the re-exercising of the driver.

> 
> The mocking of FaviconService::WasUnableToDownloadFavicon() makes the test on
> lines 90 - 96 uninteresting too. You might as well delete that too

Done.

Let's keep these removals in mind for the follow-up patches, in case you want to
reintroduce them.

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
File components/favicon/core/favicon_service.h (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
components/favicon/core/favicon_service.h:30: const
std::vector<favicon_base::FaviconRawBitmapResult>* results);
On 2017/02/16 04:07:27, pkotwicz wrote:
> In a separate CL we should find a new home for this method. favicon_util.h
> perhaps?

Ack. I think a better alternative is to get rid of this function altogether,
since the types are movable (and, to be sure, prevent copy constructors for most
favicon types).

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.h (right):

https://codereview.chromium.org/2698473004/diff/130001/components/favicon/cor...
components/favicon/core/favicon_service_impl.h:43: // FaviconServiceInterface
implementation.
On 2017/02/16 04:07:27, pkotwicz wrote:
> Nit: "FaviconServiceInterface implementation." -> "FaviconService:"

Done, thx!

https://codereview.chromium.org/2698473004/diff/130001/ios/chrome/app/spotlig...
File ios/chrome/app/spotlight/spotlight_manager_unittest.mm (right):

https://codereview.chromium.org/2698473004/diff/130001/ios/chrome/app/spotlig...
ios/chrome/app/spotlight/spotlight_manager_unittest.mm:79:
WithArg<3>(PostCallbackWithResult(CreateTestBitmap(24, 24))));
On 2017/02/16 04:07:27, pkotwicz wrote:
> This is ok because there aren't a lot of places where we implement
> FaviconService methods via the EXPECT_CALL() macro. However, I wouldn't want
to
> implement FaviconService via the EXPECT_CALL macro in many dozens of places.

The upside is, only a single method can be implemented. It also allows tests to
optionally specify parameter expectations, as it is the case today in
favicon_view_provider_unittest.mm, which wants to verify that the passed in URL
is kTestFaviconURL.

[mastiz, 2017-02-16 09:50:58]

mastiz@chromium.org changed reviewers:
+ rsesek@chromium.org

[mastiz, 2017-02-16 09:50:59]

rsesek@chromium.org: Please review changes in
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm

[commit-bot: I haz the power, 2017-02-16 10:37:01]

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

[commit-bot: I haz the power, 2017-02-16 10:37:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Robert Sesek, 2017-02-16 14:59:12]

FaviconServiceImplTest.ShouldCacheUnableToDownloadFavicons seems to be failing
on Mac still.

[mastiz, 2017-02-17 09:41:58]

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

[commit-bot: I haz the power, 2017-02-17 09:42:21]

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

[mastiz, 2017-02-17 09:42:55]

noyau@: friendly ping for ios/, thanks!

[noyau (Ping after 24h), 2017-02-17 09:58:44]

ios lgtm.

[mastiz, 2017-02-17 10:01:18]

mastiz@chromium.org changed reviewers:
+ sfiera@chromium.org

[mastiz, 2017-02-17 10:01:19]

sfiera@chromium.org: Please review changes in components/ntp_tiles

[sfiera, 2017-02-17 10:05:02]

ntp_tiles LGTM

[mastiz, 2017-02-17 10:11:03]

On 2017/02/16 14:59:12, Robert Sesek wrote:
> FaviconServiceImplTest.ShouldCacheUnableToDownloadFavicons seems to be failing
> on Mac still.

Should be green now, PTAL.

[mastiz, 2017-02-17 10:13:07]

mastiz@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mastiz, 2017-02-17 10:13:08]

sdefresne@chromium.org: Please review changes in
components/history/core/browser/history_service.h

[sdefresne, 2017-02-17 10:38:04]

components/history/ lgtm

[commit-bot: I haz the power, 2017-02-17 13:13:06]

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

[commit-bot: I haz the power, 2017-02-17 13:13:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Robert Sesek, 2017-02-17 15:49:41]

cocoa/ LGTM

[mastiz, 2017-02-17 16:08:20]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-02-17 16:08:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2698473004/#ps210001
(title: "Fix tests due to missing MessageLoop")

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

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-02-17 19:47:19]

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

[commit-bot: I haz the power, 2017-02-17 19:47:21]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-02-17 20:23:24]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-02-17 20:24:43]

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

[mastiz, 2017-02-17 20:33:50]

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

[commit-bot: I haz the power, 2017-02-17 20:35:31]

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

[mastiz, 2017-02-17 20:40:19]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2017-02-20 18:07:26]

Description was changed from

==========
Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=
==========

to

==========
Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=694312
==========

[mastiz, 2017-02-21 13:38:54]

FYI, I rebased on  https://codereview.chromium.org/2703003002 and renamed the
introduced gmock action to avoid the need for WithArg (making the test
expectations shorter).

Since the changes are trivial, I will go ahead and land the CL, please reach out
if you have further comments. Thanks!

[mastiz, 2017-02-21 13:39:06]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-02-21 13:39:07]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkotwicz@chromium.org, noyau@chromium.org, sdefresne@chromium.org,
sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2698473004/#ps290001
(title: "Rebase and minor renames.")

[commit-bot: I haz the power, 2017-02-21 13:39:27]

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-02-21 13:42:06]

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

[commit-bot: I haz the power, 2017-02-21 13:42:07]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-02-21 14:39:01]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-02-21 14:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkotwicz@chromium.org, noyau@chromium.org, sdefresne@chromium.org,
sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2698473004/#ps310001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-02-21 14:39:23]

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-02-21 15:38:51]

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

[commit-bot: I haz the power, 2017-02-21 15:38:52]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-02-21 21:19:33]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-02-21 21:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkotwicz@chromium.org, noyau@chromium.org, sdefresne@chromium.org,
sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2698473004/#ps330001
(title: "Revert changes in
ios/.../history_collection_view_controller_unittest.mm")

[commit-bot: I haz the power, 2017-02-21 21:21:11]

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-02-21 23:25:51]

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

[commit-bot: I haz the power, 2017-02-21 23:25:53]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[mastiz, 2017-02-22 06:53:54]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-02-22 06:55:38]

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-02-22 10:42:50]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1487746434955630,
"parent_rev": "25ca749f44e7a1f8955f51fb30ed4e0a6287496c", "commit_rev":
"d7bd35142ea07f588ba2decdd4c184d68e34d35a"}

[commit-bot: I haz the power, 2017-02-22 10:43:31]

Description was changed from

==========
Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=694312
==========

to

==========
Split FaviconService and FaviconServiceImpl

Many existing tests inject test doubles by subclassing from
FaviconServiceImpl, or simply inject a null service and implement
special-handling in the production code.

In this patch, some simple cases are migrated to non-partial mocks.
Follow-up patches will address the more advanced cases.

BUG=694312

Review-Url: https://codereview.chromium.org/2698473004
Cr-Commit-Position: refs/heads/master@{#451982}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7bd35142ea07f588ba2decdd4c1...
==========

[commit-bot: I haz the power, 2017-02-22 10:43:33]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/d7bd35142ea07f588ba2decdd4c1...