Fix DCHECK when shutting down safe browsing DownloadMetadataManager.

Fix DCHECK when shutting down safe browsing DownloadMetadataManager.

Chrome announces that a DownloadManager is going down before it destroys
its items. This change makes the unit test behave more like chrome does
and makes the metadata manager handle this shutdown order properly.

BUG=389123, 386915, 389643
R=robertshield@chromium.org

Committed: https://crrev.com/f1f59477f54feacd508eee215d66fe7086700179
Cr-Commit-Position: refs/heads/master@{#302746}

[grt (UTC plus 2), 2014-11-05 00:58:55]

Quickie for you. Please take a look.

[robertshield, 2014-11-05 01:04:50]

robertshield@chromium.org changed reviewers:
+ mad@chromium.org

[robertshield, 2014-11-05 01:04:50]

Code LGTM, MAD claims knowledge of the GMock warning and that it might be
squashable.

https://codereview.chromium.org/703463006/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
(right):

https://codereview.chromium.org/703463006/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc:188:
// returned as desired.
I don't know enough gmock to know whether this is work-aroundable. Can
expectations be added for these calls? MAD claims knowledge of such things.

[grt (UTC plus 2), 2014-11-05 01:25:35]

Thanks. Happy to learn from MAD if he has ideas. See my comment below with more
info about what's going on.

https://codereview.chromium.org/703463006/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
(right):

https://codereview.chromium.org/703463006/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc:188:
// returned as desired.
On 2014/11/05 01:04:50, robertshield wrote:
> I don't know enough gmock to know whether this is work-aroundable. Can
> expectations be added for these calls? MAD claims knowledge of such things.

The right thing when you don't want an expectation (something that would make
the test fail) but rather to specify the mock's behavior is to use ON_CALL
(https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Action...)
as on line 200. This returns the desired value and will emit the "Uninteresting"
warning as noted in the comment. The right way to suppress that warning is to
use a NiceMock
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...),
which means "don't let me know about calls that don't have expectations." The
problem, as I perhaps poorly explained in the comment here, is that
MockDownloadItem's dtor runs callbacks which will invoke methods on the item.
These work just fine, but the NiceMock's dtor (which removes the "suppress
unintersting calls" hook for this mock) has already run at that point, so the
uninteresting calls are logged.

[grt (UTC plus 2), 2014-11-05 01:25:41]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-11-05 01:27:33]

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

[commit-bot: I haz the power, 2014-11-05 02:16:19]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-11-05 02:17:41]

Patchset 1 (id:??) landed as
https://crrev.com/f1f59477f54feacd508eee215d66fe7086700179
Cr-Commit-Position: refs/heads/master@{#302746}