|
|
Created:
3 years, 8 months ago by melandory Modified:
3 years, 7 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, jam, Randy Smith (Not in Mondays), timvolodine, darin-cc_chromium.org, asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the SocEng as a type for checking in CheckUrlForSubresourceFilter.
The main change in this CL -- CheckUrlForSubresourceFilter will check
not only against GetUrlSubresourceFilterId(), but also against
GetUrlSocEngId().
This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle.
BUG=671962
Review-Url: https://codereview.chromium.org/2814733002
Cr-Commit-Position: refs/heads/master@{#472099}
Committed: https://chromium.googlesource.com/chromium/src/+/cc872706b26db043caa7d57f4c10661ff9d0ae40
Patch Set 1 : remove tests which are not neede anymore #
Total comments: 29
Patch Set 2 : . #
Total comments: 44
Patch Set 3 : some engedy@ comments #
Total comments: 3
Patch Set 4 : rebased and changes sync strategy #
Total comments: 2
Patch Set 5 : . #Patch Set 6 : . #Messages
Total messages: 166 (132 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...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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:1) 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:20001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
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: + engedy@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Are you sure you uploaded all files, e.g., the SubresourceFilterSafeBrowsingActivationThrottle? :-)
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:70001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/12 18:53:55, engedy wrote: > Are you sure you uploaded all files, e.g., the > SubresourceFilterSafeBrowsingActivationThrottle? :-) 80% sure. I've tried hard to find what I've missed, but failed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
melandory@chromium.org changed reviewers: + nparker@chromium.org
nparker@chromium.org I have a question to you, it's in components/safe_browsing_db/v4_local_database_manager.cc Thanks! https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:77: ListInfo(kSyncAlways, "UrlSubresourceFilter.store", +nparker@ Before start debugging I want to check if I'm hitting something known. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). These two lists have different fetch_updates strategies, SocEng is kSyncAlways and SubresourceFilter is kSyncOnlyOnChromeBuilds. The symptom I observe is for SubresourceFilterBrowserTest: 1.fetch_update strategy of SB_THREAT_TYPE_SUBRESOURCE_FILTER is kSyncOnlyOnChromeBuilds. 2. Configure the url to be a SocEng via test wrappers from chrome/browser/safe_browsing/v4_test_utils.cc (MarkPrefixAsBad from safe_browsing::TestV4DatabaseFactory and AddToFullHashCache from safe_browsing::TestV4GetHashProtocolManagerFactory) 3. Navigate to this URL. The call to CheckUrlForSubresourceFilter identifies the URL as SB_THREAT_TYPE_SAFE. But if I change the fetch_update strategy as in this patch (to kSyncAlways) everything works. I'll start debugging if the symptoms doesn't ring a bell. Thanks!
Description was changed from ========== Move SocEng check toi the Safe browsing activation navigation throttle. It makes more sense to have all checks related to the Subresource Filter in one place. BUG=671962 ========== to ========== Add the SocEng as a type for cheking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962 ==========
Description was changed from ========== Add the SocEng as a type for cheking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962 ========== to ========== Add the SocEng as a type for checking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962 ==========
Those fetch_stragegies are there because non-Chrome-branded builds aren't authorized to fetch certain lists. Safe Browsing made some of them Chrome-only, so you need Chrome's API keys. If you build the branded Chrome binary w/ API keys, does it work? -- Nathan On Wed, Apr 19, 2017 at 7:47 AM, <melandory@chromium.org> wrote: > nparker@chromium.org > > I have a question to you, it's in > components/safe_browsing_db/v4_local_database_manager.cc > > > Thanks! > > > > > https://codereview.chromium.org/2814733002/diff/90001/ > components/safe_browsing_db/v4_local_database_manager.cc > File components/safe_browsing_db/v4_local_database_manager.cc (right): > > https://codereview.chromium.org/2814733002/diff/90001/ > components/safe_browsing_db/v4_local_database_manager.cc#newcode77 > components/safe_browsing_db/v4_local_database_manager.cc:77: > ListInfo(kSyncAlways, "UrlSubresourceFilter.store", > +nparker@ > > Before start debugging I want to check if I'm hitting something known. > > The main change in this CL -- CheckUrlForSubresourceFilter will check > not only against GetUrlSubresourceFilterId(), but also against > GetUrlSocEngId(). These two lists have different fetch_updates > strategies, SocEng is kSyncAlways and SubresourceFilter is > kSyncOnlyOnChromeBuilds. > > The symptom I observe is for SubresourceFilterBrowserTest: > 1.fetch_update strategy of SB_THREAT_TYPE_SUBRESOURCE_FILTER is > kSyncOnlyOnChromeBuilds. > 2. Configure the url to be a SocEng via test wrappers from > chrome/browser/safe_browsing/v4_test_utils.cc (MarkPrefixAsBad from > safe_browsing::TestV4DatabaseFactory and AddToFullHashCache from > safe_browsing::TestV4GetHashProtocolManagerFactory) > 3. Navigate to this URL. > > The call to CheckUrlForSubresourceFilter identifies the URL as > SB_THREAT_TYPE_SAFE. > > But if I change the fetch_update strategy as in this patch (to > kSyncAlways) everything works. > > I'll start debugging if the symptoms doesn't ring a bell. Thanks! > > https://codereview.chromium.org/2814733002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oops, I shouldn't reply from Google email.. Trying again: Those fetch_stragegies are there because non-Chrome-branded builds aren't authorized to fetch certain lists. Safe Browsing made some of them Chrome-only, so you need Chrome's API keys. If you build the branded Chrome binary w/ API keys, does it work? -- Nathan
Looking good, a couple of comments. https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { Would it be possible to move these tests to the SubresourceFilter browsertest? https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:280: {GetUrlSocEngId(), GetUrlSubresourceFilterId()}); Just to double check: does this cover both SOCIAL_ENG_ADS_INTERSTITIAL as well as PHISHING_INTERSTITIAL? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:51: UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.Match." suffix, \ Have you considered making this UMA_HISTOGRAM_BOOLEAN, and record this histogram all the time, so that we would have a baseline that we can compare the number of matches to? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:349: bool ContentSubresourceFilterDriverFactory::CalculateMatchForActivationList( nit: Let's just inline this method now that it became so trivial. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:351: return navigation_chain_.size() && nit: Can |navigation_chain_| ever be empty here? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:51: const char kMatchesHistogramName[] = "SubresourceFilter.PageLoad.Match."; How about `SubresourecFilter.PageLoad.FinalURLMatch.`? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:57: enum RecordHistograms { naming nit: RecordedHistograms https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:60: NUM_MATCH_PATTERNS, nit: Never used. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:144: {ActivationDecision::ACTIVATED, Could you please add corresponding NOT_MATCHED test cases for this activation list as well? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:286: RecordHistograms expected_pattern, nit: expected_histograms https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:334: tester.ExpectTotalCount(kMatchesHistogramName + suffix, 1); Let's also check that the correct value is recorded here! https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:656: for (size_t i = 0U; i < arraysize(kRedirectRecordHistogramsTestData); ++i) { nit: If you are already changing this line, could you make this a range-based for loop? https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:667: #if defined(GOOGLE_CHROME_BUILD) nit: Do we need this anymore? https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70136: + Obsolete, since the don't have correct data to record anymore. nit: Is Charlie planning to reinstate it in the future? If so, we should not that here.
On 2017/04/20 00:30:18, Nathan Parker wrote: > Oops, I shouldn't reply from Google email.. Trying again: > > Those fetch_stragegies are there because non-Chrome-branded builds aren't > authorized to fetch certain lists. Safe Browsing made some of them Chrome-only, > so you need Chrome's API keys. If you build the branded Chrome binary w/ API > keys, does it work? Yes, it works. But I find it really strange that Soc Eng check stops working in non branded build. I guard all subresource filter browser tests with #if defined(GOOGLE_CHROME_BUILD) > > -- Nathan
Ok, I filed http://crbug.com/713902 to investigate what I think you're describing. As for this change, you should verify with awoz@ that the subresource list can be synced from Chromium, and also test it as follows: Build chromium, point it at an empty data-dir and run it w/ pver4 enabled. Verify it populates all the lists on disk (in "Safe Browsing/" directory). If the backend rejects this list, it'll reject updating ALL of them.
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...
On 2017/04/20 22:30:18, Nathan Parker wrote: > Ok, I filed http://crbug.com/713902 to investigate what I think you're > describing. As for this change, you should verify with awoz@ that the > subresource list can be synced from Chromium, and also test it as follows: Build > chromium, point it at an empty data-dir and run it w/ pver4 enabled. Verify it > populates all the lists on disk (in "Safe Browsing/" directory). If the backend > rejects this list, it'll reject updating ALL of them. Ha! I've investigated. The cause in fact is very primitive: V4LocalDatabaseManager::AreStoresAvailableNow checks if _every_ store in |stores_to_check| is available. And in case of chromium build it's not true, since only SocEng store is available (kAlwaysSync). So I see several alternatives: 1. Change the sync strategy for SubresourceFilter list (as done in 1st patch). 2. Change logic for AreStoresAvailableNow as in second patch (check changes in v4_database.cc). I prefer 2. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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: 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_...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Yes that was the intention, since the code that follows that checks the DB will DCHECK if you ask it for a store that's not available. I think it's important that the default action what to do when store(s) are unavailable should be handled up top in the Check*() calls, since that's where it's most clear what the caller expects. So I'd go for #1 if that's possible (not sure if backend will reject it), or else explicitly check the availability of each list separately and craft a check that asks just for the store(s) that are available.
Patchset #3 (id:130001) has been deleted
On 2017/04/21 17:33:06, Nathan Parker wrote: > Yes that was the intention, since the code that follows that checks the DB will > DCHECK if you ask it for a store that's not available. I think it's important > that the default action what to do when store(s) are unavailable should be > handled up top in the Check*() calls, since that's where it's most clear what > the caller expects. > > So I'd go for #1 if that's possible (not sure if backend will reject it), or > else explicitly check the availability of each list separately and craft a check > that asks just for the store(s) that are available. Ok, will do #1 and check with awoz@ if it doesn't break anything. Thanks!
https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:237: if (!IsStoreAvailable(identifier)) I thought you were going to sync both lists, so you wouldn't need any of these changes?
https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:224: if (IsStoreAvailable(identifier)) (codereview ate my comment) I think we'd need two versions: AreAllStoresAvailable() AreSomeStoresAvailable() And plumb that up through V4LocalDatabaseManager so it's really clear in the Check*() calls what the requirements are.
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { On 2017/04/20 11:16:10, engedy wrote: > Would it be possible to move these tests to the SubresourceFilter browsertest? Hm, I thought that we have them there in a bit different flavor. And I don't think we need to test interaction with the interstitial, since it's not relevant to our check anymore. https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:280: {GetUrlSocEngId(), GetUrlSubresourceFilterId()}); On 2017/04/20 11:16:10, engedy wrote: > Just to double check: does this cover both SOCIAL_ENG_ADS_INTERSTITIAL as well > as PHISHING_INTERSTITIAL? Yes https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:51: UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.Match." suffix, \ On 2017/04/20 11:16:11, engedy wrote: > Have you considered making this UMA_HISTOGRAM_BOOLEAN, and record this histogram > all the time, so that we would have a baseline that we can compare the number of > matches to? Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:349: bool ContentSubresourceFilterDriverFactory::CalculateMatchForActivationList( On 2017/04/20 11:16:11, engedy wrote: > nit: Let's just inline this method now that it became so trivial. Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:351: return navigation_chain_.size() && On 2017/04/20 11:16:11, engedy wrote: > nit: Can |navigation_chain_| ever be empty here? Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:51: const char kMatchesHistogramName[] = "SubresourceFilter.PageLoad.Match."; On 2017/04/20 11:16:11, engedy wrote: > How about `SubresourecFilter.PageLoad.FinalURLMatch.`? Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:57: enum RecordHistograms { On 2017/04/20 11:16:11, engedy wrote: > naming nit: RecordedHistograms Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:60: NUM_MATCH_PATTERNS, On 2017/04/20 11:16:11, engedy wrote: > nit: Never used. Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:286: RecordHistograms expected_pattern, On 2017/04/20 11:16:11, engedy wrote: > nit: expected_histograms Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:334: tester.ExpectTotalCount(kMatchesHistogramName + suffix, 1); On 2017/04/20 11:16:11, engedy wrote: > Let's also check that the correct value is recorded here! Done. https://codereview.chromium.org/2814733002/diff/90001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:667: #if defined(GOOGLE_CHROME_BUILD) On 2017/04/20 11:16:11, engedy wrote: > nit: Do we need this anymore? ditto https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70136: + Obsolete, since the don't have correct data to record anymore. On 2017/04/20 11:16:11, engedy wrote: > nit: Is Charlie planning to reinstate it in the future? If so, we should not > that here. Not quite following you. Given new strategy of checking URLs, we will never be able to record this histogram exactly. https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:237: if (!IsStoreAvailable(identifier)) On 2017/04/24 22:17:50, Nathan Parker wrote: > I thought you were going to sync both lists, so you wouldn't need any of these > changes? Yep, it was not final patchset, sorry. I've just replied to you that I;ll go with #1, but haven't reverted changes yet.
melandory@chromium.org changed reviewers: + jwd@chromium.org
jwd, PTAL at tools/metrics/histograms/histograms.xml
jwd, PTAL at tools/metrics/histograms/histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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: 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_...)
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.
LGTM % comments, thanks! https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { On 2017/04/25 13:48:13, melandory wrote: > On 2017/04/20 11:16:10, engedy wrote: > > Would it be possible to move these tests to the SubresourceFilter browsertest? > > Hm, I thought that we have them there in a bit different flavor. And I don't > think we need to test interaction with the interstitial, since it's not relevant > to our check anymore. That's a fair point. In that case, could we just re-enable all the tests in subresource_filter_browsertest.cc for Chromium builds as well? https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/90001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70136: + Obsolete, since the don't have correct data to record anymore. On 2017/04/25 13:48:14, melandory wrote: > On 2017/04/20 11:16:11, engedy wrote: > > nit: Is Charlie planning to reinstate it in the future? If so, we should not > > that here. > > Not quite following you. Given new strategy of checking URLs, we will never be > able to record this histogram exactly. I think Charlie is changing that strategy back, but I guess he can un-deprecate this histogram once that's done. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:43: #include "chrome/browser/subresource_filter/test_ruleset_publisher.h" nit: No longer needed. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:64: #include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h" nit: This and below no longer needed. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:542: command_line->AppendSwitchASCII( nit: Probably no longer needed and can be removed with the features.h header. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:595: bool WasSubresourceFilterProbeScriptLoaded() { nit: No longer needed. https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... File components/safe_browsing/base_resource_throttle.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... components/safe_browsing/base_resource_throttle.cc:15: #include "content/public/browser/web_contents.h" nit: Is this still used? https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:77: ListInfo(kSyncAlways, "UrlSubresourceFilter.store", Would this allow us to re-enable the browsertests for this list for subresource_filter_browsertest.cc for Chromium builds now? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:43: // Records histograms about the length of redirect chains, and about the pattern nit: s/about the pattern of// https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:44: // of whether each URL in the chain matched the activation list. nit: s/each/the last/ https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:45: #define REPORT_REDIRECT_PATTERN_FOR_SUFFIX(suffix, match_pattern, chain_size) \ nit: s/match_pattern/did_match|is_match|matched/, and then use that name consistently throughout this file. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:287: if (!navigation_chain_.back().has_host() || nit: Add DCHECK(!navigation_chain_.empty()); before. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:55: "SubresourecFilter.PageLoad.FinalURLMatch."; nit: Sorry, I introduced a typo here: Subresource* https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:61: enum RecordedHistograms { Could you also please remove RedirectChainMatchPattern in subresource_filter_browsertest.cc? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:62: EMPTY, nit: Let's see if we can make these names more informative. How about: NO_HISTOGRAMS = 0, FINAL_URL_MATCHES = 1 << 0, REDIRECT_CHAIN_LENGTH = 1 << 1, BOTH_HISTOGRAMS = ... Then we could make the checking starting at line 350 also easier to understand. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:153: subresource_filter::kActivationListPhishingInterstitial, I think this should be kActivationListSubresourceFilter. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:204: if (url.SchemeIsHTTPOrHTTPS()) nit: Can we ASSERT_TRUE(url.SchemeIsHTTPOrHTTPS()) here instead? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:216: // Owned by the test harness. nit: Add blank line before comment. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: size_t all_pattern = nit: s//all_matches/ https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:24: safe_browsing::ThreatPatternType pattern_type) { nit: Could we just make NONE the default value for the last argument? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:36: "SubresourecFilter.PageLoad.FinalURLMatch."; Same typo here. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:118: void ExpectSampleForSuffix(const std::string& suffix, nit: This method is a bit hard to understand. How about passing just one suffix, doing a loop over all suffixes inside, and naming it in a more informative way? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/renderer/subresource_filter_agent.h (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/renderer/subresource_filter_agent.h:61: void RecordedHistogramsOnLoadCommitted(); nit: I think accidental rename? https://codereview.chromium.org/2814733002/diff/210001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/210001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:71030: + Records the fact of a match of the last url in the redirect chain for the Phrasing nit: Records, for each main frame navigation, whether the last URL in the redirect chain matched the Safe Browsing blacklist specified by the histogram suffix.
melandory@chromium.org changed reviewers: + csharrison@chromium.org
csharrison@: Please review changes in chrome/browser/loader/safe_browsing_resource_throttle.cc https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:43: #include "chrome/browser/subresource_filter/test_ruleset_publisher.h" On 2017/04/26 13:47:09, engedy wrote: > nit: No longer needed. Done. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:64: #include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h" On 2017/04/26 13:47:09, engedy wrote: > nit: This and below no longer needed. Done. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:542: command_line->AppendSwitchASCII( On 2017/04/26 13:47:09, engedy wrote: > nit: Probably no longer needed and can be removed with the features.h header. Done. https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:595: bool WasSubresourceFilterProbeScriptLoaded() { On 2017/04/26 13:47:09, engedy wrote: > nit: No longer needed. Done. https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... File components/safe_browsing/base_resource_throttle.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... components/safe_browsing/base_resource_throttle.cc:15: #include "content/public/browser/web_contents.h" On 2017/04/26 13:47:09, engedy wrote: > nit: Is this still used? Yes, for displaying the blocking page. https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:77: ListInfo(kSyncAlways, "UrlSubresourceFilter.store", On 2017/04/26 13:47:09, engedy wrote: > Would this allow us to re-enable the browsertests for this list for > subresource_filter_browsertest.cc for Chromium builds now? Yes! https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:43: // Records histograms about the length of redirect chains, and about the pattern On 2017/04/26 13:47:09, engedy wrote: > nit: s/about the pattern of// Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:44: // of whether each URL in the chain matched the activation list. On 2017/04/26 13:47:09, engedy wrote: > nit: s/each/the last/ Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:45: #define REPORT_REDIRECT_PATTERN_FOR_SUFFIX(suffix, match_pattern, chain_size) \ On 2017/04/26 13:47:09, engedy wrote: > nit: s/match_pattern/did_match|is_match|matched/, and then use that name > consistently throughout this file. Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:287: if (!navigation_chain_.back().has_host() || On 2017/04/26 13:47:09, engedy wrote: > nit: Add DCHECK(!navigation_chain_.empty()); before. Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:55: "SubresourecFilter.PageLoad.FinalURLMatch."; On 2017/04/26 13:47:09, engedy wrote: > nit: Sorry, I introduced a typo here: Subresource* Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:61: enum RecordedHistograms { On 2017/04/26 13:47:10, engedy wrote: > Could you also please remove RedirectChainMatchPattern in > subresource_filter_browsertest.cc? Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:62: EMPTY, On 2017/04/26 13:47:10, engedy wrote: > nit: Let's see if we can make these names more informative. How about: > > NO_HISTOGRAMS = 0, > FINAL_URL_MATCHES = 1 << 0, > REDIRECT_CHAIN_LENGTH = 1 << 1, > BOTH_HISTOGRAMS = ... > > Then we could make the checking starting at line 350 also easier to understand. Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:153: subresource_filter::kActivationListPhishingInterstitial, On 2017/04/26 13:47:10, engedy wrote: > I think this should be kActivationListSubresourceFilter. Isn't it the test case above? https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:204: if (url.SchemeIsHTTPOrHTTPS()) On 2017/04/26 13:47:10, engedy wrote: > nit: Can we ASSERT_TRUE(url.SchemeIsHTTPOrHTTPS()) here instead? Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:216: // Owned by the test harness. On 2017/04/26 13:47:10, engedy wrote: > nit: Add blank line before comment. Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: size_t all_pattern = On 2017/04/26 13:47:10, engedy wrote: > nit: s//all_matches/ Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:24: safe_browsing::ThreatPatternType pattern_type) { On 2017/04/26 13:47:10, engedy wrote: > nit: Could we just make NONE the default value for the last argument? Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:36: "SubresourecFilter.PageLoad.FinalURLMatch."; On 2017/04/26 13:47:10, engedy wrote: > Same typo here. Done. https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/renderer/subresource_filter_agent.h (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/renderer/subresource_filter_agent.h:61: void RecordedHistogramsOnLoadCommitted(); On 2017/04/26 13:47:10, engedy wrote: > nit: I think accidental rename? Done. https://codereview.chromium.org/2814733002/diff/210001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/210001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:71030: + Records the fact of a match of the last url in the redirect chain for the On 2017/04/26 13:47:10, engedy wrote: > Phrasing nit: Records, for each main frame navigation, whether the last URL in > the redirect chain matched the Safe Browsing blacklist specified by the > histogram suffix. Done.
jwd@ friendlt ping nparker@ please take another look
Did you upload the latest patch set?
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:127174: - <suffix name="SubresourceFilterOnly" label="subresource filter only patern"/> Rather than delete, add an obsolete child tag. Same for the affected-histograms. https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:70930: + Obsolete, since the don't have correct data to record anymore. Can you add the year and month when it is being obsoleted.
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: 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_...)
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...
chrome/browser/loader LGTM but I didn't take a close look at the other files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_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...)
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: 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_...)
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:127174: - <suffix name="SubresourceFilterOnly" label="subresource filter only patern"/> On 2017/04/26 23:34:10, jwd wrote: > Rather than delete, add an obsolete child tag. Same for the affected-histograms. Hm, this line is not deleted, just type was fixed. otherwise done. https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:70930: + Obsolete, since the don't have correct data to record anymore. On 2017/04/26 23:34:10, jwd wrote: > Can you add the year and month when it is being obsoleted. Done.
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:70930: + Obsolete, since the don't have correct data to record anymore. On 2017/04/26 23:34:10, jwd wrote: > Can you add the year and month when it is being obsoleted. Done.
lgtm
LGTM for safe_browsing*/* https://codereview.chromium.org/2814733002/diff/310001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/310001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:77: ListInfo(kSyncAlways, "UrlSubresourceFilter.store", l-g-t-m assuming you've tested this on a chromium build and you've checked with awoz@ that the backend intends to support this for chromium builds.
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: 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_...)
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: 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_...)
https://codereview.chromium.org/2814733002/diff/210001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:153: subresource_filter::kActivationListPhishingInterstitial, On 2017/04/26 15:02:21, melandory wrote: > On 2017/04/26 13:47:10, engedy wrote: > > I think this should be kActivationListSubresourceFilter. > > Isn't it the test case above? Nevermind me. :-)
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: 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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:150001) has been deleted
Patchset #7 (id:250001) has been deleted
Patchset #7 (id:270001) has been deleted
Patchset #10 (id:350001) has been deleted
Patchset #8 (id:310001) 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...
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...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:170001) has been deleted
Patchset #6 (id:290001) has been deleted
Patchset #6 (id:330001) has been deleted
Patchset #8 (id:410001) has been deleted
Patchset #7 (id:390001) has been deleted
Patchset #6 (id:370001) has been deleted
Patchset #3 (id:190001) has been deleted
Patchset #2 (id:110001) 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...
nparker@, please take another look. As per e-mail discussion it was decided that we can't have kSyncAlways for the Subresource filter list. The main changes are in following files: components/safe_browsing_db/v4_database.h components/safe_browsing_db/v4_database.cc components/safe_browsing_db/v4_database_unittest.cc components/safe_browsing_db/v4_local_database_manager.cc Thanks in advance!
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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:207: bool V4Database::AreStoresAvailable( I had the following comment on an earlier vers, and couldn't find a response. Since you're changing the semantics of this function: I think we'd need two versions: AreAllStoresAvailable() AreAnyStoresAvailable() And plumb that up through V4LocalDatabaseManager so it's really clear in the Check*() calls what the requirements are.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 #5 (id:450001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #5 (id:470001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:207: bool V4Database::AreStoresAvailable( On 2017/05/12 17:30:10, Nathan Parker wrote: > I had the following comment on an earlier vers, and couldn't find a response. > Since you're changing the semantics of this function: > > I think we'd need two versions: > AreAllStoresAvailable() > AreAnyStoresAvailable() > And plumb that up through V4LocalDatabaseManager so it's really clear in the > Check*() calls what the requirements are. Sorry, haven't noticed it earlier. Done.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Great, thanks. LGTM.
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 #5 (id:490001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, csharrison@chromium.org, jwd@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2814733002/#ps470002 (title: ".")
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": 470002, "attempt_start_ts": 1494944958081490, "parent_rev": "8691cc8301afdcbf13422ebce335b727bf7fa090", "commit_rev": "cc872706b26db043caa7d57f4c10661ff9d0ae40"}
Message was sent while issue was closed.
Description was changed from ========== Add the SocEng as a type for checking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962 ========== to ========== Add the SocEng as a type for checking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962 Review-Url: https://codereview.chromium.org/2814733002 Cr-Commit-Position: refs/heads/master@{#472099} Committed: https://chromium.googlesource.com/chromium/src/+/cc872706b26db043caa7d57f4c10... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:470002) as https://chromium.googlesource.com/chromium/src/+/cc872706b26db043caa7d57f4c10... |