|
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 7 months ago Reviewers:
jkarlin CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Move driver factory tests to the SB throttle
BUG=717590
Review-Url: https://codereview.chromium.org/2887253004
Cr-Commit-Position: refs/heads/master@{#474000}
Committed: https://chromium.googlesource.com/chromium/src/+/fb7da5949a9529ba0b63ab06e76cd4a8c351db26
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : VERBATIM TEST COPY #Patch Set 6 : FORWARD DIFF from 4 #
Total comments: 8
Patch Set 7 : jkarlin review #
Total comments: 2
Patch Set 8 : enum class : int #
Dependent Patchsets: Messages
Total messages: 36 (28 generated)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_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_...)
The CQ bit was checked by csharrison@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: 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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...) 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_...)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + jkarlin@chromium.org
Josh, PTAL I tried to do what you asked with patch 4->5 to see the exact diff between tests. Note that some tests I stopped using parameterized harnesses in favor of data driven tests because there was only one consumer of the harness.
The CQ bit was checked by csharrison@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: 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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks. Admittedly not the closest ever inspection of the code but overall lgtm. https://codereview.chromium.org/2887253004/diff/100001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:192: ruleset_dealer_.reset(); Why this? It leaves a dangling pointer in MockSubresourceFilterClient. https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:219: // Returns the frame host the navigation commit in, or nullptr if it did not s/commit/committed/ https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:646: test_data.activation_level)); Android compiler is complaining that activation_level isn't an int.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2887253004/diff/100001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:192: ruleset_dealer_.reset(); On 2017/05/23 13:04:33, jkarlin wrote: > Why this? It leaves a dangling pointer in MockSubresourceFilterClient. The ruleset dealer has a member which posts a task to delete itself. I've moved it so the mock client owns the dealer and is deleted here. https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:219: // Returns the frame host the navigation commit in, or nullptr if it did not On 2017/05/23 13:04:33, jkarlin wrote: > s/commit/committed/ Done. https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:646: test_data.activation_level)); On 2017/05/23 13:04:33, jkarlin wrote: > Android compiler is complaining that activation_level isn't an int. Just used a static_cast. One of these days I'll add some logging utils to ActivationDecision but today is not that day.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2887253004/diff/100001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:646: test_data.activation_level)); On 2017/05/23 15:32:23, Charlie Harrison wrote: > On 2017/05/23 13:04:33, jkarlin wrote: > > Android compiler is complaining that activation_level isn't an int. > > Just used a static_cast. One of these days I'll add some logging utils to > ActivationDecision but today is not that day. Be sure to set the type of the enum class to int as well then. https://codereview.chromium.org/2887253004/diff/120001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/120001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:20: Remove newline
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
thanks https://codereview.chromium.org/2887253004/diff/100001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/100001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:646: test_data.activation_level)); On 2017/05/23 15:40:25, jkarlin wrote: > On 2017/05/23 15:32:23, Charlie Harrison wrote: > > On 2017/05/23 13:04:33, jkarlin wrote: > > > Android compiler is complaining that activation_level isn't an int. > > > > Just used a static_cast. One of these days I'll add some logging utils to > > ActivationDecision but today is not that day. > > Be sure to set the type of the enum class to int as well then. Done. https://codereview.chromium.org/2887253004/diff/120001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2887253004/diff/120001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:20: On 2017/05/23 15:40:26, jkarlin wrote: > Remove newline Oops done
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
slgtm
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2887253004/#ps140001 (title: "enum class : int")
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": 140001, "attempt_start_ts": 1495556520375460, "parent_rev": "0a2243c09c7513d55bde53ceeae0ea89a8334e79", "commit_rev": "fb7da5949a9529ba0b63ab06e76cd4a8c351db26"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Move driver factory tests to the SB throttle BUG=717590 ========== to ========== [subresource_filter] Move driver factory tests to the SB throttle BUG=717590 Review-Url: https://codereview.chromium.org/2887253004 Cr-Commit-Position: refs/heads/master@{#474000} Committed: https://chromium.googlesource.com/chromium/src/+/fb7da5949a9529ba0b63ab06e76c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/fb7da5949a9529ba0b63ab06e76c... |