Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783773002:
Link PasswordProtectionService to Profile and SB Service (Closed)
|
Created:
3 years, 8 months ago by Jialiu Lin Modified:
3 years, 8 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, gcasto+watchlist_chromium.org, grt+watch_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, timvolodine, vabr+watchlistpasswordmanager_chromium.org, vakh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLink PasswordProtectionService to Profile and SB Service
1. Refactor PasswordProtectionService to be a per Profile class.
2. Create ChromePasswordProtectionService subclass in c/b/safe_browsing/ to
to link Profile instances to password protection.
3. Make SafeBrowsingService observe the creation and destruction of profiles,
and create/remove ChromePasswordProtection instances accordingly.
BUG=698899
Review-Url: https://codereview.chromium.org/2783773002
Cr-Commit-Position: refs/heads/master@{#462233}
Committed: https://chromium.googlesource.com/chromium/src/+/4522d139cf6832a2f3393d374dd9b025902935dc
Patch Set 1 #Patch Set 2 : fix build #Patch Set 3 : Make HistoryServiceObserver scopedobserver #Patch Set 4 : fix broken browser tests #Patch Set 5 : revert website_settings_registry #Patch Set 6 : nit #
Total comments: 31
Patch Set 7 : RemovePasswordProtectionService(..) #
Total comments: 3
Patch Set 8 : Gate whitelist checking by SAFE_BROWSING_DB_LOCAL only #
Total comments: 2
Patch Set 9 : rebase #
Total comments: 14
Patch Set 10 : add a UMA metric to track the number of cached verdict before shutdown #
Total comments: 6
Patch Set 11 : address comments from nparker #Patch Set 12 : refine histogram description #Messages
Total messages: 100 (80 generated)
The CQ bit was checked by jialiul@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 jialiul@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...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jialiul@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 checked by jialiul@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 jialiul@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 #4 (id:60001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:50016) has been deleted
Description was changed from ========== Wire PasswordProtectionService up with Profile and SB Service Refactor PasswordProtectionService to be a per Profile class. Create ChromePasswordProtectionService subclass in c/b/safe_browsing/ to to link Profile instances to password protection. Make SafeBrowsingService observe the creation and destruction of profiles, and create/remove ChromePasswordProtection instances accordingly. BUG=698899 ========== to ========== Link PasswordProtectionService to Profile and SB Service 1. Refactor PasswordProtectionService to be a per Profile class. 2. Create ChromePasswordProtectionService subclass in c/b/safe_browsing/ to to link Profile instances to password protection. 3. Make SafeBrowsingService observe the creation and destruction of profiles, and create/remove ChromePasswordProtection instances accordingly. BUG=698899 ==========
The CQ bit was checked by jialiul@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...
jialiul@chromium.org changed reviewers: + nparker@chromium.org
The CQ bit was checked by jialiul@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/2783773002/diff/150001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:654: ->GetPasswordProtectionService(profile_); When would this be null but safe_browsing_service() not be null? Could we simplify it by ensuring this was always not null? Then I think you could drop the CanSetPasswordProtectionService() func. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:26: "PasswordProtectionPingOnly", base::FEATURE_DISABLED_BY_DEFAULT}; Do you imagine we'd later add something like "PasswordProtectionShowWarning," that would also enable IsPingingEnabled()? https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.h:10: #include "base/feature_list.h" I think this can be moved to the .cc file https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.h:44: // Profile associated with instance. Profile expected to outlive this class, yes? https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:770: if (it != password_protection_service_map_.end() && it->second) should this be an error? https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:783: password_protection_service_map_.erase(it); Also, maybe an error if it's not there. Could be a dcheck. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:168: PasswordProtectionService* GetPasswordProtectionService(Profile* profile); const? https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:103: password_protection_service_->FillReferrerChain(main_frame_url_, -1, Is there a TODO here to add other iframes? Or would that go elsewhere? https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:118: if (!url.is_valid() || !content_settings_) { when would !content_settings be true? Should that be a dcheck? https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:285: if (verdict_dictionary.get() && verdict_dictionary->empty()) Do you mean !empty()? If it's empty, the ->size() would be 0. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:288: return stored_verdict_count_; It might be interesting to log this value to UMA, like on startup or on profile registration. Not necessary in this CL. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:34: // class per profile. Therefore, one every PasswordProtectionService instance nit: confusing wording around "one every" https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:103: // Obtains referrer chain of |event_url| and |event_tab_id| and add this nit: s/add/adds https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:175: // Stored verdict count for each HostContentSettingsMap. ... for this profile (?) https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:88: bool IsIncognito() override { return false; } Do you test anywhere that these get used correctly? Like if IsIncognito returns false? https://codereview.chromium.org/2783773002/diff/170001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/170001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:27: How about tests for this class?
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 jialiul@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...
Thanks! https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:654: ->GetPasswordProtectionService(profile_); On 2017/03/30 at 21:38:12, Nathan Parker wrote: > When would this be null but safe_browsing_service() not be null? Could we simplify it by ensuring this was always not null? Then I think you could drop the CanSetPasswordProtectionService() func. Agree. As long as SafeBrowsingService is not null, there should always be a passwordProtectionService instance associated with this profile. Removing this function. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:26: "PasswordProtectionPingOnly", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Do you imagine we'd later add something like "PasswordProtectionShowWarning," that would also enable IsPingingEnabled()? Yes. that's my plan. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.h:10: #include "base/feature_list.h" On 2017/03/30 at 21:38:12, Nathan Parker wrote: > I think this can be moved to the .cc file indeed. Done. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.h:44: // Profile associated with instance. On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Profile expected to outlive this class, yes? Yes. Its creation is triggered by profile creation, and its destruction happens before profile's destruction. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:770: if (it != password_protection_service_map_.end() && it->second) On 2017/03/30 at 21:38:12, Nathan Parker wrote: > should this be an error? Right, it is impossible to create the same profile twice. Change to DCHECK https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:783: password_protection_service_map_.erase(it); On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Also, maybe an error if it's not there. Could be a dcheck. Done. https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:168: PasswordProtectionService* GetPasswordProtectionService(Profile* profile); On 2017/03/30 at 21:38:12, Nathan Parker wrote: > const? Done. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:103: password_protection_service_->FillReferrerChain(main_frame_url_, -1, On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Is there a TODO here to add other iframes? Or would that go elsewhere? Added a TODO https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:118: if (!url.is_valid() || !content_settings_) { On 2017/03/30 at 21:38:12, Nathan Parker wrote: > when would !content_settings be true? Should that be a dcheck? Yes.... object lifetime scares me... so I tend to use if condition more often than DCHECK. And I'll try to get rid of this habit. Changed to DCHECK. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:285: if (verdict_dictionary.get() && verdict_dictionary->empty()) On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Do you mean !empty()? If it's empty, the ->size() would be 0. Oops. it should be !empty(). https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:288: return stored_verdict_count_; On 2017/03/30 at 21:38:12, Nathan Parker wrote: > It might be interesting to log this value to UMA, like on startup or on profile registration. Not necessary in this CL. This number is sent with every ping. I think that's probably enough. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:34: // class per profile. Therefore, one every PasswordProtectionService instance On 2017/03/30 at 21:38:12, Nathan Parker wrote: > nit: confusing wording around "one every" Done. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:103: // Obtains referrer chain of |event_url| and |event_tab_id| and add this On 2017/03/30 at 21:38:12, Nathan Parker wrote: > nit: s/add/adds Done. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:175: // Stored verdict count for each HostContentSettingsMap. On 2017/03/30 at 21:38:12, Nathan Parker wrote: > ... for this profile (?) Oops, forget to update comment. Fixed. https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:88: bool IsIncognito() override { return false; } On 2017/03/30 at 21:38:12, Nathan Parker wrote: > Do you test anywhere that these get used correctly? Like if IsIncognito returns false? Yes, these are just default returns. Situations like incognito, non-SBER, are covered in TestNoRequestSentForIncognito, TestNoRequestSentForNonExtendedReporting. I have refactored the testing code a little to make it easier to read. https://codereview.chromium.org/2783773002/diff/170001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/170001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:27: On 2017/03/30 at 21:38:12, Nathan Parker wrote: > How about tests for this class? Very little to test... This class only has a couple of getters, and the FillReferrerChain() function is pretty much a wrapper. Its main logic is already thoroughly tested in safe_browsing_navigation_observer_browsertest.cc I'll add tests later when there is more non-trivial logic/funcions.
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_...)
It seems ChromePasswordManagerClient is created before each ChromePasswordProtectionService. I'm working on it. I'll send it out for review after I fix this problem. Thanks,
[I'll wait for your later update for final review] https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/150001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:118: if (!url.is_valid() || !content_settings_) { On 2017/03/30 23:23:07, Jialiu Lin wrote: > On 2017/03/30 at 21:38:12, Nathan Parker wrote: > > when would !content_settings be true? Should that be a dcheck? > > Yes.... object lifetime scares me... so I tend to use if condition more often > than DCHECK. And I'll try to get rid of this habit. > > Changed to DCHECK. I totally agree! Better safe than sorry... But then we don't want to hide a bug. I sorta want to have it both ways: if (!ptr) { DCHECK(false); return; } But then that's saying we're not confident in our testing, and it adds complexity (you have to handle that early return, and its side effects, etc).
The CQ bit was checked by jialiul@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_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 jialiul@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_...)
The CQ bit was checked by jialiul@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jialiul@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 #9 (id:210001) has been deleted
Patchset #9 (id:230001) has been deleted
Patchset #9 (id:250001) has been deleted
Patchset #9 (id:270001) has been deleted
Patchset #9 (id:290001) has been deleted
Patchset #8 (id:190001) has been deleted
Patchset #8 (id:310001) has been deleted
Patchset #7 (id:170001) has been deleted
There are two major changes in the last patch: (1) During the browser starting up, ChromePasswordManagerClient's constructor may be called before the creation of the first PasswordProtectionService. Therefore, calling GetPasswordProtectionService() inside its constructor will return nullptr. New patch delays calling of GetPasswordProtectionService() to the time when it is needed. (2) In SafeBrowsingService::RemovePasswordProtectionService(), DCHECK will fail a number of browsertests (due to their special instrumentation of testing profile). Therefore, change it to a if statement instead. https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:778: if (it != password_protection_service_map_.end()) It turns out this line cannot be a DCHECK, since in some browsertests, creating profile may not trigger NOTIFICATION_PROFILE_CREATED, but during shutdown NOTIFICATION_PROFILE_DESTROYED will be triggered. In regular running of Chrome, this will never happen.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jialiul@chromium.org changed reviewers: + dvadym@chromium.org
+dvadym@ for password_manager side of code. Some explanation: Previously, PasswordProtectionService (PPS) is set in ChromePasswordManagerClient's (CPMC) constructor. Now since I changed PPS to be created upon NOTIFICATION_PROFILE_CREATED, during browser startup, the 1st CPMC is created before the 1st PPS. Therefore, calling password_protection_service() at CPMC's constructor will return nullptr. This CL does not keep a pointer of PPS in the PasswordReuseDetectionManager, instead it calls CPMC to get a pointer of PPS on demand to avoid getting a nullptr. Thanks!
[Adding one quick comment... I didn't do a full pass again. I'll let Vadym look at ptr ownership] https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.h:108: safe_browsing::PasswordProtectionService* GetPasswordProtectionService() Re: SAFE_BROWSING_DB_REMOTE -- On Android we don't yet have the CSD Whitelist (tracked in http://b/36806770), and it'll fail on a NOTREACHED() if you call MatchCsdWhitelistUrl(). So we should keep this all disabled on SAFE_BROWSING_DB_REMOTE until then. You could do that here, or keep it here and disable it someplace further down.
The CQ bit was checked by jialiul@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...
Thanks for catching this nparker! I completely forgot there is no csd whitelist on Android. https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2783773002/diff/330001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.h:108: safe_browsing::PasswordProtectionService* GetPasswordProtectionService() On 2017/04/03 at 20:33:34, Nathan Parker wrote: > Re: SAFE_BROWSING_DB_REMOTE -- On Android we don't yet have the CSD Whitelist (tracked in http://b/36806770), and it'll fail on a NOTREACHED() if you call MatchCsdWhitelistUrl(). So we should keep this all disabled on SAFE_BROWSING_DB_REMOTE until then. You could do that here, or keep it here and disable it someplace further down. Thanks for pointing this out. It completely slipped my mind. I'll make sure it is gated in password_reuse_detection_manager.cc (where the actual calling happens).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for a password part of this CL Please note that today I've landed CL https://codereview.chromium.org/2777283004/ for not building Password Reuse detection for IOS and Android, since anyway they don't work for them (I'm going to fix this for Android later), please rebase. https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:66: #if defined(SAFE_BROWSING_DB_LOCAL) Just wondering in which cases SAFE_BROWSING_DB_LOCAL and SAFE_BROWSING_DB_REMOTE are defined ?
The CQ bit was checked by jialiul@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...
Thanks dvadym@! I have rebased this CL to the latest HEAD. https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:66: #if defined(SAFE_BROWSING_DB_LOCAL) On 2017/04/04 at 13:02:38, dvadym wrote: > Just wondering in which cases > SAFE_BROWSING_DB_LOCAL and SAFE_BROWSING_DB_REMOTE are defined ? SAFE_BROWSING_DB_LOCAL means (OS_MAC || OS_WIN || OS_CHROMEOS), SAFE_BROWSING_DB_REMOTE means OS_ANDROID. These are defined in build/config/BUILD.gn
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_...)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #9 (id:370001) 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...
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_...)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #9 (id:390001) 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/04 at 20:24:10, Jialiu Lin wrote: > Thanks dvadym@! I have rebased this CL to the latest HEAD. > > https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... > File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): > > https://codereview.chromium.org/2783773002/diff/350001/components/password_ma... > components/password_manager/core/browser/password_reuse_detection_manager.cc:66: #if defined(SAFE_BROWSING_DB_LOCAL) > On 2017/04/04 at 13:02:38, dvadym wrote: > > Just wondering in which cases > > SAFE_BROWSING_DB_LOCAL and SAFE_BROWSING_DB_REMOTE are defined ? > > SAFE_BROWSING_DB_LOCAL means (OS_MAC || OS_WIN || OS_CHROMEOS), > SAFE_BROWSING_DB_REMOTE means OS_ANDROID. > > These are defined in build/config/BUILD.gn Oops, forgot Linux, should be SAFE_BROWSING_DB_LOCAL means (OS_MAC || OS_WIN || OS_CHROMEOS || OS_LINUX).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jialiul@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson@ for owner review of histograms.xml Thanks!
lgtm Mostly nits, otherwise LGTM! https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:25: const base::Feature Nit: Could this be declared here rather than in the .h? https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:64: DCHECK(profile_); nit: I'm pretty sure the DCHECKs here are not helpful since you deref the ptr immediately. Either way is fine. https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:285: void OnProfileDestroyed(Profile* profile); Comment for this one? Isn't not obvious what ti does. https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:364: profile_shutdown_notifiers_; Is this used? I didn't see it. https://codereview.chromium.org/2783773002/diff/410001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2783773002/diff/410001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:71: DCHECK(password_protection_service); nit: again if you're deref'ing it immediately, I think the DCHECK doesn't add much. https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:103: password_protection_service_->FillReferrerChain(main_frame_url_, -1, nit: add a comment as to what the -1 is. https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:93: scoped_refptr<net::URLRequestContextGetter> request_context_getter() { If any of these could be protected or private, that'd be better.
https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48090: +<histogram name="PasswordProtection.NumberOfCachedVerdictBeforeShutdown"> units=? https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48094: + Number of cached verdict of a profile before destructing What is a "cached verdict" https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48095: + PasswordProtectionService. Roughly when is a PasswordProtectionService destructed? https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
Thank you nparker@! https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:25: const base::Feature On 2017/04/05 at 18:05:22, Nathan Parker wrote: > Nit: Could this be declared here rather than in the .h? Sure. Removed from .h https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:64: DCHECK(profile_); On 2017/04/05 at 18:05:22, Nathan Parker wrote: > nit: I'm pretty sure the DCHECKs here are not helpful since you deref the ptr immediately. Either way is fine. DCHECK removed. https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:285: void OnProfileDestroyed(Profile* profile); On 2017/04/05 at 18:05:22, Nathan Parker wrote: > Comment for this one? Isn't not obvious what ti does. Oops, I forgot to remove this function. It is not implemented. https://codereview.chromium.org/2783773002/diff/410001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:364: profile_shutdown_notifiers_; On 2017/04/05 at 18:05:22, Nathan Parker wrote: > Is this used? I didn't see it. You're right. this is not needed. https://codereview.chromium.org/2783773002/diff/410001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2783773002/diff/410001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:71: DCHECK(password_protection_service); On 2017/04/05 at 18:05:23, Nathan Parker wrote: > nit: again if you're deref'ing it immediately, I think the DCHECK doesn't add much. Removed DCHECK(). https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:103: password_protection_service_->FillReferrerChain(main_frame_url_, -1, On 2017/04/05 at 18:05:23, Nathan Parker wrote: > nit: add a comment as to what the -1 is. Done. https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2783773002/diff/410001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:93: scoped_refptr<net::URLRequestContextGetter> request_context_getter() { On 2017/04/05 at 18:05:23, Nathan Parker wrote: > If any of these could be protected or private, that'd be better. Moved most of them to protected and private
Thanks mpearson! https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48090: +<histogram name="PasswordProtection.NumberOfCachedVerdictBeforeShutdown"> On 2017/04/05 at 18:39:28, Mark P wrote: > units=? units="count" https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48094: + Number of cached verdict of a profile before destructing On 2017/04/05 at 18:39:28, Mark P wrote: > What is a "cached verdict" it refers to the password protection verdict we stored in content settings https://codereview.chromium.org/2783773002/diff/430001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48095: + PasswordProtectionService. On 2017/04/05 at 18:39:28, Mark P wrote: > Roughly when is a PasswordProtectionService destructed? > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... It is destructed with its corresponding user profile.
histograms.xml lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2783773002/#ps470001 (title: "refine histogram description")
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": 470001, "attempt_start_ts": 1491423300931680,
"parent_rev": "4b4c3ab4242415baad151e3614837cd02ffeb7e4", "commit_rev":
"4522d139cf6832a2f3393d374dd9b025902935dc"}
Message was sent while issue was closed.
Description was changed from ========== Link PasswordProtectionService to Profile and SB Service 1. Refactor PasswordProtectionService to be a per Profile class. 2. Create ChromePasswordProtectionService subclass in c/b/safe_browsing/ to to link Profile instances to password protection. 3. Make SafeBrowsingService observe the creation and destruction of profiles, and create/remove ChromePasswordProtection instances accordingly. BUG=698899 ========== to ========== Link PasswordProtectionService to Profile and SB Service 1. Refactor PasswordProtectionService to be a per Profile class. 2. Create ChromePasswordProtectionService subclass in c/b/safe_browsing/ to to link Profile instances to password protection. 3. Make SafeBrowsingService observe the creation and destruction of profiles, and create/remove ChromePasswordProtection instances accordingly. BUG=698899 Review-Url: https://codereview.chromium.org/2783773002 Cr-Commit-Position: refs/heads/master@{#462233} Committed: https://chromium.googlesource.com/chromium/src/+/4522d139cf6832a2f3393d374dd9... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:470001) as https://chromium.googlesource.com/chromium/src/+/4522d139cf6832a2f3393d374dd9... |
