Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690413002:
Refactor safe browsing service tests. (Closed)
DescriptionRefactor safe browsing service tests.
Extracts code which helps to mock communication with the V4 database. This code will be reused in the browser tests for the Subrsource Filter.
Part 1, https://codereview.chromium.org/2690413002 (this one)
Part 2, https://codereview.chromium.org/2645283007
BUG=671962
Review-Url: https://codereview.chromium.org/2690413002
Cr-Commit-Position: refs/heads/master@{#453372}
Committed: https://chromium.googlesource.com/chromium/src/+/994815ee0b5bbf09286abeb06ae25fda6aa50e07
Patch Set 1 : . #Patch Set 2 : rename #
Total comments: 6
Patch Set 3 : addressed comments #
Messages
Total messages: 38 (28 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:10001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:50001) has been deleted
melandory@chromium.org changed reviewers: + vakh@chromium.org
Hey, PTAL. Here (in te second comment) are more details why this is needed: https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br...
Description was changed from ========== Extract some code from the Safe BRowsing tests, so it can be used in browser tests for the Subrsource Filter. BUG=671962 ========== to ========== Refactor safe browsing service tests. Extracts some code from the Safe Browsing service tests, so it can be used in browser tests for the Subrsource Filter. BUG=671962 ==========
Patchset #1 (id:30001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:70001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor safe browsing service tests. Extracts some code from the Safe Browsing service tests, so it can be used in browser tests for the Subrsource Filter. BUG=671962 ========== to ========== Refactor safe browsing service tests. Extracts some code from the Safe Browsing service tests, so it can be used in browser tests for the Subrsource Filter. Part 1, https://codereview.chromium.org/2690413002 (this one) Part 2, https://codereview.chromium.org/2645283007 BUG=671962 ==========
LGTM. Modulo one nit but I don't feel super strongly about it: It might be better to call the file as comp/safe_browsing_db/v4_test_utils.* -- that would make them easier to discover since they'll be right next to the classes that they are overriding.
Still lgtm. One more thing, please revise the CL description to make it clearer to understand what you are changing.
Description was changed from ========== Refactor safe browsing service tests. Extracts some code from the Safe Browsing service tests, so it can be used in browser tests for the Subrsource Filter. Part 1, https://codereview.chromium.org/2690413002 (this one) Part 2, https://codereview.chromium.org/2645283007 BUG=671962 ========== to ========== Refactor safe browsing service tests. Extracts code which helps to mock communication with the V4 database. This code will be reused in the browser tests for the Subrsource Filter. Part 1, https://codereview.chromium.org/2690413002 (this one) Part 2, https://codereview.chromium.org/2645283007 BUG=671962 ==========
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
melandory@chromium.org changed reviewers: + mattm@chromium.org
mattm@chromium.org: Please review changes in this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/v4_test_utils.cc (right): https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.cc:10: #include "components/safe_browsing_db/v4_get_hash_protocol_manager.h" Don't need to duplicate includes that are in the v4_test_utils.h file https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/v4_test_utils.h (right): https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.h:8: #include "chrome/browser/safe_browsing/test_safe_browsing_service.h" Is this needed? https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.h:50: // Owned by V4LocalDatabaseManager. Each test in the V4SafeBrowsingServiceTest update comment (no longer specific to V4SafeBrowsingServiceTest)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/v4_test_utils.cc (right): https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.cc:10: #include "components/safe_browsing_db/v4_get_hash_protocol_manager.h" On 2017/02/17 21:49:53, mattm wrote: > Don't need to duplicate includes that are in the v4_test_utils.h file Done. https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/v4_test_utils.h (right): https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.h:8: #include "chrome/browser/safe_browsing/test_safe_browsing_service.h" On 2017/02/17 21:49:53, mattm wrote: > Is this needed? Done. https://codereview.chromium.org/2690413002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/v4_test_utils.h:50: // Owned by V4LocalDatabaseManager. Each test in the V4SafeBrowsingServiceTest On 2017/02/17 21:49:53, mattm wrote: > update comment (no longer specific to V4SafeBrowsingServiceTest) Done.
note for future reference: when updating a codereview it's generally appreciated to upload the rebase as one patchset (with no changes other than the rebase) and then the local changes in another patchset, keeps things easier to review. lgtm
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vakh@chromium.org Link to the patchset: https://codereview.chromium.org/2690413002/#ps130001 (title: "addressed comments")
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": 130001, "attempt_start_ts": 1488235149691050,
"parent_rev": "7085aa755cacbf3906608d930347a6b214428f76", "commit_rev":
"994815ee0b5bbf09286abeb06ae25fda6aa50e07"}
Message was sent while issue was closed.
Description was changed from ========== Refactor safe browsing service tests. Extracts code which helps to mock communication with the V4 database. This code will be reused in the browser tests for the Subrsource Filter. Part 1, https://codereview.chromium.org/2690413002 (this one) Part 2, https://codereview.chromium.org/2645283007 BUG=671962 ========== to ========== Refactor safe browsing service tests. Extracts code which helps to mock communication with the V4 database. This code will be reused in the browser tests for the Subrsource Filter. Part 1, https://codereview.chromium.org/2690413002 (this one) Part 2, https://codereview.chromium.org/2645283007 BUG=671962 Review-Url: https://codereview.chromium.org/2690413002 Cr-Commit-Position: refs/heads/master@{#453372} Committed: https://chromium.googlesource.com/chromium/src/+/994815ee0b5bbf09286abeb06ae2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:130001) as https://chromium.googlesource.com/chromium/src/+/994815ee0b5bbf09286abeb06ae2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
