Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804573002:
Add FaviconHandler test reflecting behavior of bookmarks in incognito (Closed)
DescriptionAdd FaviconHandler test reflecting behavior of bookmarks in incognito
It's not clear whether this is the desired behavior, but let's at least
document it in the form of tests.
BUG=708447
Review-Url: https://codereview.chromium.org/2804573002
Cr-Commit-Position: refs/heads/master@{#462801}
Committed: https://chromium.googlesource.com/chromium/src/+/d65c1d7370f67352f3fc7052d71b5ddfe88a18f1
Patch Set 1 #Patch Set 2 : Reverted unrelated changes. #
Total comments: 4
Patch Set 3 : Verified downloads. #Messages
Total messages: 16 (6 generated)
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:480: Can you please add: EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
PTAL. https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:480: On 2017/04/05 15:19:42, pkotwicz wrote: > Can you please add: > > EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16)); Done. Can you please elaborate why this verification is relevant for this test, or why it is different to db_requests()?
LGTM https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:480: It is relevant to the test because "the downloaded favicon" is what is saved to the database via SetFavicons().
LGTM https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:480: It is relevant to the test because "the downloaded favicon" is what is saved to the database via SetFavicons().
https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2804573002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:480: On 2017/04/06 14:46:02, pkotwicz wrote: > It is relevant to the test because "the downloaded favicon" is what is saved to > the database via SetFavicons(). Acknowledged, thx.
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491548688635570,
"parent_rev": "fb96ed753ed737db5befc119248c110e41134814", "commit_rev":
"d65c1d7370f67352f3fc7052d71b5ddfe88a18f1"}
Message was sent while issue was closed.
Description was changed from ========== Add FaviconHandler test reflecting behavior of bookmarks in incognito It's not clear whether this is the desired behavior, but let's at least document it in the form of tests. BUG=708447 ========== to ========== Add FaviconHandler test reflecting behavior of bookmarks in incognito It's not clear whether this is the desired behavior, but let's at least document it in the form of tests. BUG=708447 Review-Url: https://codereview.chromium.org/2804573002 Cr-Commit-Position: refs/heads/master@{#462801} Committed: https://chromium.googlesource.com/chromium/src/+/d65c1d7370f67352f3fc7052d71b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d65c1d7370f67352f3fc7052d71b... |
