Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640033006:
Convert AutoBlocker static class to KeyedService. (Closed)
DescriptionConvert PermissionDecisionAutoBlocker to a KeyedService.
In order to avoid passing around the profile, safe browsing database
PermissionDecisionAutoBlocker now keeps track of the database manager
and timeout. This means that testing this is now done within a unit test
of this class, rather than PermissionContextBase. The autoblocker now
handles the database manager entirely as well as the timeout. This allows
for the blacklisting pathway to be unit tested, as well as integration
tested in PermissionContextBase.
BUG=679877
Review-Url: https://codereview.chromium.org/2640033006
Cr-Commit-Position: refs/heads/master@{#445954}
Committed: https://chromium.googlesource.com/chromium/src/+/03b128552363baf102bafecf0dc45d201cfb43f1
Patch Set 1 #Patch Set 2 : Remove ShouldChangeDismissalToBlock #
Total comments: 34
Patch Set 3 : Nits and incognito browser context #
Total comments: 30
Patch Set 4 : Add clock, move browsing data tests, nits #
Total comments: 28
Patch Set 5 : Remove static methods, nits #
Total comments: 14
Patch Set 6 : Reformat conversion method + nits #
Total comments: 4
Patch Set 7 : Switch statement. #Patch Set 8 : Rebase #Patch Set 9 : Rebase pt 2. #Patch Set 10 : Git surgery. #Dependent Patchsets: Messages
Total messages: 53 (34 generated)
meredithl@google.com changed reviewers: + dominickn@chromium.org
Hey Dom, PTAL!
Looks pretty good! Change the title to be: "Convert PermissionDecisionAutoBlocker to a KeyedService" https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2766: content::PermissionType::MIDI_SYSEX); Nit: indentation https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2769: tester.RecordDismissAndEmbargo(kOrigin2, Nit: indentation https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2811: tester.RecordDismissAndEmbargo(kOrigin1, Nit: indentation https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2815: tester.RecordDismissAndEmbargo(kOrigin2, Nit: indentation https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:15: #include "base/strings/stringprintf.h" #include "base/time/time.h" https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:162: // TODO(meredithl): Consider setting the content setting and persisting You can remove this TODO :) https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:186: PermissionDecisionAutoBlocker* autoblocker = Nit: newline above this line. Also consider not having a variable and just doing: PermissionDecisionAutoBlocker::GetForProfile(profile_)->IsUnderEmbargo https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:187: int safe_browsing_timeout_; Remove? https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:188: scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager_; Remove? https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:171: // test. Nit: comment indentation https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:131: : profile_(profile) {} Initialize safe_browsing_timeout_: move kCheckUrlTimeoutMs into this file. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:197: void PermissionDecisionAutoBlocker::UpdateFromVariations() { This method can actually stay static - there's no need for an instance here since it's just updated globals in this compilation unit. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:225: // TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database Remove the TODO! https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:6: #include <map> https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:41: // TODO(meredithl): Write unit tests to simulate entering Permissions I think you implemented this TODO? https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:59: // Returns false if scheme is HTTP/HTTPS and able to be checked. This comment seems out of context. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:113: // Manually placing an origin, permission pair under embargo for blacklisting. Nit: "Manually place an (origin, permission) pair"
Description was changed from ========== Convert AutoBlocker static class to KeyedService. In order to avoid passing around the profile, safe browsing database PermissionDecisionAutoBlocker now keeps track of the database manager and timeout. This means that testing this is now done within a unit test of this class, rather than PermissionContextBase. The autoblocker now handles the database manager entirely as well as the timeout. This allows for the blacklisting pathway to be unit tested, as well as integration tested in PermissionContextBase. BUG=679877 ========== to ========== Convert PermissionDecisionAutoBlocker to a KeyedService. In order to avoid passing around the profile, safe browsing database PermissionDecisionAutoBlocker now keeps track of the database manager and timeout. This means that testing this is now done within a unit test of this class, rather than PermissionContextBase. The autoblocker now handles the database manager entirely as well as the timeout. This allows for the blacklisting pathway to be unit tested, as well as integration tested in PermissionContextBase. BUG=679877 ==========
Thanks! https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2766: content::PermissionType::MIDI_SYSEX); On 2017/01/20 04:03:47, dominickn wrote: > Nit: indentation Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2769: tester.RecordDismissAndEmbargo(kOrigin2, On 2017/01/20 04:03:47, dominickn wrote: > Nit: indentation Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2811: tester.RecordDismissAndEmbargo(kOrigin1, On 2017/01/20 04:03:46, dominickn wrote: > Nit: indentation Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2815: tester.RecordDismissAndEmbargo(kOrigin2, On 2017/01/20 04:03:47, dominickn wrote: > Nit: indentation Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:15: #include "base/strings/stringprintf.h" On 2017/01/20 04:03:47, dominickn wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:162: // TODO(meredithl): Consider setting the content setting and persisting On 2017/01/20 04:03:47, dominickn wrote: > You can remove this TODO :) Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:186: PermissionDecisionAutoBlocker* autoblocker = On 2017/01/20 04:03:47, dominickn wrote: > Nit: newline above this line. Also consider not having a variable and just > doing: > > PermissionDecisionAutoBlocker::GetForProfile(profile_)->IsUnderEmbargo Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:187: int safe_browsing_timeout_; On 2017/01/20 04:03:47, dominickn wrote: > Remove? Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:188: scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager_; On 2017/01/20 04:03:47, dominickn wrote: > Remove? Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:171: // test. On 2017/01/20 04:03:47, dominickn wrote: > Nit: comment indentation Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:131: : profile_(profile) {} On 2017/01/20 04:03:47, dominickn wrote: > Initialize safe_browsing_timeout_: move kCheckUrlTimeoutMs into this file. Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:197: void PermissionDecisionAutoBlocker::UpdateFromVariations() { On 2017/01/20 04:03:47, dominickn wrote: > This method can actually stay static - there's no need for an instance here > since it's just updated globals in this compilation unit. Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:225: // TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database On 2017/01/20 04:03:47, dominickn wrote: > Remove the TODO! Done. Ahhh, feelsgoodman. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:6: On 2017/01/20 04:03:47, dominickn wrote: > #include <map> Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:41: // TODO(meredithl): Write unit tests to simulate entering Permissions On 2017/01/20 04:03:47, dominickn wrote: > I think you implemented this TODO? Done. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:59: // Returns false if scheme is HTTP/HTTPS and able to be checked. On 2017/01/20 04:03:47, dominickn wrote: > This comment seems out of context. Fair enough. It just felt weird to arbitrarily return false but I've removed it. https://codereview.chromium.org/2640033006/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:113: // Manually placing an origin, permission pair under embargo for blacklisting. On 2017/01/20 04:03:47, dominickn wrote: > Nit: "Manually place an (origin, permission) pair" Done.
lgtm :) https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:182: PermissionDecisionAutoBlockerFactory::GetForProfile(profile_) Nit: remove factory
meredithl@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
Hey, sorry I didn't get this to you sooner. PTAL, thanks!
lgtm https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:133: // and the url will be treated as not blacklisted. "not blacklisted" -> "safe" (just to keep consistent language with SafeBrowsing).
This looks really good. Thanks for doing this. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:184: base::Time::Now())) { Rather than pass the time in here I think PermissionDecisionAutoBlocker can hold a base::Clock and call Now() to get the time itself. In tests we can override the clock to be a SimpleTestClock for testing. In the implementation it will be a DefaultClock. Same with the calls to UpdateEmbargoedStatus. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:647: if (response == CONTENT_SETTING_ALLOW) { How come this changed? https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:250: } nit: Is it worth just putting this in the constructor now? I don't think there is any benefit to having it here. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:45: ~PermissionDecisionAutoBlocker() override; You should be able to make the constructor/destrutor private and the factory could be a friend class or it could just be a nested class (see e.g. https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...) https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:76: // but may also make a call to Safe Browsing to check their API blacklist, their->the https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:93: int timeout); nit: this should be private https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: const char* key); nit: convert these to member functions or just standalone functions that live in the anonymous namespace of the implementation https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:119: int safe_browsing_timeout_; nit: please document the units that the timeout is in https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:87: // Mock client to return timer to be manually fired from the test nit: is this comment accurate? https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:373: EXPECT_TRUE(base::FeatureList::IsEnabled(features::kPermissionsBlacklist)); nit (here and above) I don't think this checks for features enabled are really necessary. It should be clear in the test driver. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:385: true /* expected result */); nit: It's not a big deal but a slightly nicer pattern here might be to set a private instance variable in the callback with the result and then check it in the test with: EXPECT_EQ(true, last_embargoed_status());
msramek@chromium.org changed reviewers: + msramek@chromium.org
Drive-by! I'm currently in the process of refactoring c/b/browsing_data/, and specifically in https://codereview.chromium.org/2641853002/ I split browsing_data_remover_unittest.cc into two files. The test that you updated is now in the chrome_browsing_data_remover_delegate.cc file. Please rebase and move your changes verbatim there. After that, browsing_data/ LGTM.
Hey Raymes. Feel free to come chat to me re: static methods in the autoblocker. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:182: PermissionDecisionAutoBlockerFactory::GetForProfile(profile_) On 2017/01/20 04:34:24, dominickn wrote: > Nit: remove factory Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:647: if (response == CONTENT_SETTING_ALLOW) { On 2017/01/23 06:14:23, raymes wrote: > How come this changed? Because when the permission isn't embargoed and automatically blocked, it needed to be responded to other wise it would crash the test when destroying the permission context, because it still had an outstanding request. This was a way to only set the permission to be responded to if it needed to be, and test both cases when the permission is embargoed and when it isn't. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:133: // and the url will be treated as not blacklisted. On 2017/01/23 02:52:41, kcarattini wrote: > "not blacklisted" -> "safe" (just to keep consistent language with > SafeBrowsing). Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:250: } On 2017/01/23 06:14:23, raymes wrote: > nit: Is it worth just putting this in the constructor now? I don't think there > is any benefit to having it here. Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:45: ~PermissionDecisionAutoBlocker() override; On 2017/01/23 06:14:23, raymes wrote: > You should be able to make the constructor/destrutor private and the factory > could be a friend class or it could just be a nested class (see e.g. > https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...) Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:76: // but may also make a call to Safe Browsing to check their API blacklist, On 2017/01/23 06:14:23, raymes wrote: > their->the Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:93: int timeout); On 2017/01/23 06:14:23, raymes wrote: > nit: this should be private Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: const char* key); On 2017/01/23 06:14:23, raymes wrote: > nit: convert these to member functions or just standalone functions that live in > the anonymous namespace of the implementation CheckSafeBrowsingResult is static to avoid having to pass the client a bound method, so it has safer lifetime semantics. Internally, this can call PlaceUnderEmbargo, so this was made also static. These were both in the anonymous namespace originally, which is why I was using the PlaceUnderEmbargoForTest methods in the autoblocker to still access them for testing, which was removed in a previous CL in favour of the current way, so as to only test the public API. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:119: int safe_browsing_timeout_; On 2017/01/23 06:14:23, raymes wrote: > nit: please document the units that the timeout is in Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:87: // Mock client to return timer to be manually fired from the test On 2017/01/23 06:14:23, raymes wrote: > nit: is this comment accurate? Not at all. Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:373: EXPECT_TRUE(base::FeatureList::IsEnabled(features::kPermissionsBlacklist)); On 2017/01/23 06:14:23, raymes wrote: > nit (here and above) I don't think this checks for features enabled are really > necessary. It should be clear in the test driver. Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:385: true /* expected result */); On 2017/01/23 06:14:23, raymes wrote: > nit: It's not a big deal but a slightly nicer pattern here might be to set a > private instance variable in the callback with the result and then check it in > the test with: > > EXPECT_EQ(true, last_embargoed_status()); Done. (I think)
Added the clock. Sorry it took a bit longer. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:184: base::Time::Now())) { On 2017/01/23 06:14:23, raymes wrote: > Rather than pass the time in here I think PermissionDecisionAutoBlocker can hold > a base::Clock and call Now() to get the time itself. In tests we can override > the clock to be a SimpleTestClock for testing. In the implementation it will be > a DefaultClock. > > Same with the calls to UpdateEmbargoedStatus. Done.
https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:647: if (response == CONTENT_SETTING_ALLOW) { On 2017/01/24 01:20:46, meredithl wrote: > On 2017/01/23 06:14:23, raymes wrote: > > How come this changed? > > Because when the permission isn't embargoed and automatically blocked, it needed > to be responded to other wise it would crash the test when destroying the > permission context, because it still had an outstanding request. This was a way > to only set the permission to be responded to if it needed to be, and test both > cases when the permission is embargoed and when it isn't. I see. I think the confusing thing here is what |response| means. I would suggest renaming it to expected_permission_status. We can add a comment above this saying that we only set the repsonse callback if we don't expect the permission to be blacklisted. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: const char* key); On 2017/01/24 01:20:46, meredithl wrote: > On 2017/01/23 06:14:23, raymes wrote: > > nit: convert these to member functions or just standalone functions that live > in > > the anonymous namespace of the implementation > > CheckSafeBrowsingResult is static to avoid having to pass the client a bound > method, so it has safer lifetime semantics. Internally, this can call > PlaceUnderEmbargo, so this was made also static. > > These were both in the anonymous namespace originally, which is why I was using > the PlaceUnderEmbargoForTest methods in the autoblocker to still access them for > testing, which was removed in a previous CL in favour of the current way, so as > to only test the public API. I don't think it is actually safer from a lifetime perspective. The lifetime of this object is tied to the profile's lifetime so either way we need to make sure the profile is still around. I think it might be slightly nicer to make these member functions because it means that the test isn't accessing stuff without getting a handle on this object first. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:121: nit: unnecessary newline https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:141: if (!db_manager_) { The db_manager_ will always be null at the start, so this check is unncessary (actually right now it is undefined, so this could result in a bug). To fix: -Initialize db_manager_ to null in the initializer list -Remove this line https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:151: PermissionDecisionAutoBlocker* PermissionDecisionAutoBlocker::GetForProfile( nit: // static https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:254: PermissionBlacklistClient::CheckSafeBrowsingBlacklist( nit: I discussed about lifetimes with Dom. Could you add a comment here, as well as in the header for this function, that the callback passed in will not be called if the profile is destroyed in the process. Something like // The CheckSafeBrowsingResult callback won't be called if the profile is destroyed before a result is received. In that case this object will have been destroyed by that point. And in the header for CheckSafeBrowsingBlacklist // The callback passed in will not be called if the |web_contents| passed in is destroyed. Thus if the callback is run, the profile associated with |web_contents| is guaranteed to be alive. We should think about how to make those guarantees clearer by better API design at some point (but not now :) https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:351: // PermissionDecisionAutoBlocker::Factory -------------------------------------- nit: this section should probably move to the top to match the header. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:131: bool embargo); Hmm, what is this for? https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:148: // timeout in ms. nit: Timeout (with capital) https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:133: bool LastEmbargoStatus() { return last_embargoed_status_; } nit: just last_embargoed_status() for these types of getters https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:135: base::SimpleTestClock* GetClock() { return clock_; } nit: clock() instead of GetClock likewise GetAutoBlockerInstance should just be autoblocker() above https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:249: // Test that an origin that has been blacklisted for a permission nit: this sentence looks unfinished. ... is embargoed. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:263: EXPECT_EQ(true, LastEmbargoStatus()); EXPECT_TRUE( https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:271: GetClock()->SetNow(base::Time::Now()); I just realised it's best not to use Now() in tests because it can make them not reproducible. It would be better to set a reference time. I'm ok if we do this in a followup CL though. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:392: EXPECT_EQ(false, LastEmbargoStatus()); nit: this was my fault, but this can just be EXPECT_FALSE(... https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:401: EXPECT_EQ(true, LastEmbargoStatus()); EXPECT_TRUE(
Making the static methods private cleaned up the api a lot actually, since the time/profile didn't need to be passed around between callbacks so thanks :) PTAL! https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:647: if (response == CONTENT_SETTING_ALLOW) { On 2017/01/24 03:31:08, raymes wrote: > On 2017/01/24 01:20:46, meredithl wrote: > > On 2017/01/23 06:14:23, raymes wrote: > > > How come this changed? > > > > Because when the permission isn't embargoed and automatically blocked, it > needed > > to be responded to other wise it would crash the test when destroying the > > permission context, because it still had an outstanding request. This was a > way > > to only set the permission to be responded to if it needed to be, and test > both > > cases when the permission is embargoed and when it isn't. > > I see. I think the confusing thing here is what |response| means. I would > suggest renaming it to expected_permission_status. We can add a comment above > this saying that we only set the repsonse callback if we don't expect the > permission to be blacklisted. Done. https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: const char* key); On 2017/01/24 03:31:08, raymes wrote: > On 2017/01/24 01:20:46, meredithl wrote: > > On 2017/01/23 06:14:23, raymes wrote: > > > nit: convert these to member functions or just standalone functions that > live > > in > > > the anonymous namespace of the implementation > > > > CheckSafeBrowsingResult is static to avoid having to pass the client a bound > > method, so it has safer lifetime semantics. Internally, this can call > > PlaceUnderEmbargo, so this was made also static. > > > > These were both in the anonymous namespace originally, which is why I was > using > > the PlaceUnderEmbargoForTest methods in the autoblocker to still access them > for > > testing, which was removed in a previous CL in favour of the current way, so > as > > to only test the public API. > > I don't think it is actually safer from a lifetime perspective. The lifetime of > this object is tied to the profile's lifetime so either way we need to make sure > the profile is still around. I think it might be slightly nicer to make these > member functions because it means that the test isn't accessing stuff without > getting a handle on this object first. Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:121: On 2017/01/24 03:31:08, raymes wrote: > nit: unnecessary newline Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:141: if (!db_manager_) { On 2017/01/24 03:31:08, raymes wrote: > The db_manager_ will always be null at the start, so this check is unncessary > (actually right now it is undefined, so this could result in a bug). > > To fix: > -Initialize db_manager_ to null in the initializer list > -Remove this line Done. But that means I still need to add a null check before calling the client method, yes? Again, I'm not sure under which circumstances, if any, caling database_manager() will return a nullptr. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:151: PermissionDecisionAutoBlocker* PermissionDecisionAutoBlocker::GetForProfile( On 2017/01/24 03:31:08, raymes wrote: > nit: // static Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:254: PermissionBlacklistClient::CheckSafeBrowsingBlacklist( On 2017/01/24 03:31:08, raymes wrote: > nit: I discussed about lifetimes with Dom. Could you add a comment here, as well > as in the header for this function, that the callback passed in will not be > called if the profile is destroyed in the process. Something like > > // The CheckSafeBrowsingResult callback won't be called if the profile is > destroyed before a result is received. In that case this object will have been > destroyed by that point. > > And in the header for CheckSafeBrowsingBlacklist > // The callback passed in will not be called if the |web_contents| passed in is > destroyed. Thus if the callback is run, the profile associated with > |web_contents| is guaranteed to be alive. > > We should think about how to make those guarantees clearer by better API design > at some point (but not now :) Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:351: // PermissionDecisionAutoBlocker::Factory -------------------------------------- On 2017/01/24 03:31:08, raymes wrote: > nit: this section should probably move to the top to match the header. Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:131: bool embargo); On 2017/01/24 03:31:08, raymes wrote: > Hmm, what is this for? Oh goodness, just a result of a bad rebase I think, sorry. Good catch, thanks. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:148: // timeout in ms. On 2017/01/24 03:31:08, raymes wrote: > nit: Timeout (with capital) Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:133: bool LastEmbargoStatus() { return last_embargoed_status_; } On 2017/01/24 03:31:09, raymes wrote: > nit: just last_embargoed_status() for these types of getters Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:135: base::SimpleTestClock* GetClock() { return clock_; } On 2017/01/24 03:31:08, raymes wrote: > nit: clock() instead of GetClock > likewise GetAutoBlockerInstance should just be autoblocker() above Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:249: // Test that an origin that has been blacklisted for a permission On 2017/01/24 03:31:09, raymes wrote: > nit: this sentence looks unfinished. > ... is embargoed. Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:263: EXPECT_EQ(true, LastEmbargoStatus()); On 2017/01/24 03:31:09, raymes wrote: > EXPECT_TRUE( Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:271: GetClock()->SetNow(base::Time::Now()); On 2017/01/24 03:31:08, raymes wrote: > I just realised it's best not to use Now() in tests because it can make them not > reproducible. It would be better to set a reference time. > > I'm ok if we do this in a followup CL though. Acknowledged. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:392: EXPECT_EQ(false, LastEmbargoStatus()); On 2017/01/24 03:31:09, raymes wrote: > nit: this was my fault, but this can just be EXPECT_FALSE(... Done. https://codereview.chromium.org/2640033006/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:401: EXPECT_EQ(true, LastEmbargoStatus()); On 2017/01/24 03:31:09, raymes wrote: > EXPECT_TRUE( Done.
The CQ bit was checked by meredithl@google.com 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...)
I would have lg'd except for the new stuff that was added. It can sometimes be better to add new stuff to followup CLs to get things lg'd quicker :) In any case, there's nothing major here. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:78: for (const auto& i : metadata.api_permissions) { nit: auto can be helpful sometimes, but in other cases it can be clearer to specify the full type. In this case I think the type might be clearer (which is just std::string?). (Unfortunately there are no hard rules around this and people lean in different directions :( ) https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:84: permission_types.find(permission_type_) != permission_types.end(); Would this be simpler if we just converted in the other direction? e.g. bool permission_blocked = metadata.api_permissions.find(PermissionUtil::PermissionTypeToSafeBrowsingName(permission_type_)) != metadata.api_permissions.end() ? https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:168: const int kCheckUrlTimeoutMs = 2000; nit: this should probably move up into the anonymous namespace above https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: // profile associated with |web_contents| is guaranteed to be alive. nit: sorry I meant for this comment to be in PermissionBlacklistClient::CheckSafeBrowsingBlacklist rather than here (in which case we could remove the reference to UpdateEmbargoStatus). https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:142: PermissionDecisionAutoBlocker* autoblocker_ptr = autoblocker(); nit: I would just inline autoblocker_ptr to be autoblocker() https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:60: if (sb_name == "GEOLOCATION") nit: please make a note that the name is a stringified version of the enum https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.h:30: const char kFlashPermissionName[] = "Flash"; Is it necessary to define these as constants?
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Yeah, sorry Raymes! I didn't really think about it. Thankyou for reviewing the extra stuff anyway :D https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:78: for (const auto& i : metadata.api_permissions) { On 2017/01/24 05:15:16, raymes wrote: > nit: auto can be helpful sometimes, but in other cases it can be clearer to > specify the full type. In this case I think the type might be clearer (which is > just std::string?). > (Unfortunately there are no hard rules around this and people lean in different > directions :( ) Done. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:84: permission_types.find(permission_type_) != permission_types.end(); On 2017/01/24 05:15:16, raymes wrote: > Would this be simpler if we just converted in the other direction? > > e.g. bool permission_blocked = > metadata.api_permissions.find(PermissionUtil::PermissionTypeToSafeBrowsingName(permission_type_)) > != metadata.api_permissions.end() ? Done. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:168: const int kCheckUrlTimeoutMs = 2000; On 2017/01/24 05:15:16, raymes wrote: > nit: this should probably move up into the anonymous namespace above Done. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:110: // profile associated with |web_contents| is guaranteed to be alive. On 2017/01/24 05:15:17, raymes wrote: > nit: sorry I meant for this comment to be in > PermissionBlacklistClient::CheckSafeBrowsingBlacklist rather than here (in which > case we could remove the reference to UpdateEmbargoStatus). Done. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:142: PermissionDecisionAutoBlocker* autoblocker_ptr = autoblocker(); On 2017/01/24 05:15:17, raymes wrote: > nit: I would just inline autoblocker_ptr to be autoblocker() Done. https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:60: if (sb_name == "GEOLOCATION") On 2017/01/24 05:15:17, raymes wrote: > nit: please make a note that the name is a stringified version of the enum I've now changed this method around so there is no sb_name, and updated the comment in the header. Is that okay? https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/2640033006/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.h:30: const char kFlashPermissionName[] = "Flash"; On 2017/01/24 05:15:17, raymes wrote: > Is it necessary to define these as constants? Not since I've changed the conversion to return a Safe Browsing string rather than the PermissionType.
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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:60: if (permission_type == content::PermissionType::GEOLOCATION) Please convert this to a switch statement as in GetRequestType below (but without a default entry). That will ensure that we don't miss any entries. It's also faster. https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:74: if (permission_type == content::PermissionType::MIDI) MIDI->VIDEO_CAPTURE
The CQ bit was checked by meredithl@google.com to run a CQ dry run
https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:60: if (permission_type == content::PermissionType::GEOLOCATION) On 2017/01/24 23:44:23, raymes wrote: > Please convert this to a switch statement as in GetRequestType below (but > without a default entry). That will ensure that we don't miss any entries. It's > also faster. Done. https://codereview.chromium.org/2640033006/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:74: if (permission_type == content::PermissionType::MIDI) On 2017/01/24 23:44:23, raymes wrote: > MIDI->VIDEO_CAPTURE 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: 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...)
lgtm
The CQ bit was checked by meredithl@google.com to run a CQ dry run
The CQ bit was checked by meredithl@google.com 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...
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...)
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...)
The CQ bit was checked by meredithl@google.com 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...)
The CQ bit was checked by meredithl@google.com 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 meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, kcarattini@chromium.org, msramek@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2640033006/#ps180001 (title: "Git surgery.")
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": 180001, "attempt_start_ts": 1485320590808260,
"parent_rev": "a8cd0049f8f708a0b0058eadfec737ef1e768568", "commit_rev":
"03b128552363baf102bafecf0dc45d201cfb43f1"}
Message was sent while issue was closed.
Description was changed from ========== Convert PermissionDecisionAutoBlocker to a KeyedService. In order to avoid passing around the profile, safe browsing database PermissionDecisionAutoBlocker now keeps track of the database manager and timeout. This means that testing this is now done within a unit test of this class, rather than PermissionContextBase. The autoblocker now handles the database manager entirely as well as the timeout. This allows for the blacklisting pathway to be unit tested, as well as integration tested in PermissionContextBase. BUG=679877 ========== to ========== Convert PermissionDecisionAutoBlocker to a KeyedService. In order to avoid passing around the profile, safe browsing database PermissionDecisionAutoBlocker now keeps track of the database manager and timeout. This means that testing this is now done within a unit test of this class, rather than PermissionContextBase. The autoblocker now handles the database manager entirely as well as the timeout. This allows for the blacklisting pathway to be unit tested, as well as integration tested in PermissionContextBase. BUG=679877 Review-Url: https://codereview.chromium.org/2640033006 Cr-Commit-Position: refs/heads/master@{#445954} Committed: https://chromium.googlesource.com/chromium/src/+/03b128552363baf102bafecf0dc4... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/03b128552363baf102bafecf0dc4... |
