Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2817533004: Improve PasswordProtectionService and PasswordProtectionRequest (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by Jialiu Lin
Modified:
6 months ago
Reviewers:
Nathan Parker, lpz
CC:
chromium-reviews, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, timvolodine, blundell+watchlist_chromium.org, lpz
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Improve PasswordProtectionService and PasswordProtectionRequest (1) refactor PaswordProtectionRequest to be RefCountedThreadSafe, (2) use CancelableTaskTracker in PasswordProtectionRequest to make sure tasks posted on IO thread can be canceled correctly upon destructing PasswordProtectionService, (3) Add the skeleton of PasswordProtectionFrame struct, which will be used to communicate password form information between ChromePasswordManagerClient and PasswordProtectionService BUG=698899 Review-Url: https://codereview.chromium.org/2817533004 Cr-Commit-Position: refs/heads/master@{#465695} Committed: https://chromium.googlesource.com/chromium/src/+/3cffb12884df959d94c1e3bb67e9d8bcae2c339a

Patch Set 1 #

Patch Set 2 : rebase #

Total comments: 20

Patch Set 3 : address nparker's comments #

Total comments: 6

Patch Set 4 : address comments from lpz #

Patch Set 5 : revert fieldtrial #

Total comments: 10

Patch Set 6 : address more comments #

Total comments: 6

Patch Set 7 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -94 lines) Patch
M chrome/browser/safe_browsing/chrome_password_protection_service.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/safe_browsing/password_protection/password_protection_request.h View 1 2 3 4 5 6 7 chunks +52 lines, -21 lines 0 comments Download
M components/safe_browsing/password_protection/password_protection_request.cc View 1 2 3 4 5 6 5 chunks +28 lines, -23 lines 0 comments Download
M components/safe_browsing/password_protection/password_protection_service.h View 1 2 3 4 5 6 7 chunks +41 lines, -10 lines 0 comments Download
M components/safe_browsing/password_protection/password_protection_service.cc View 1 2 3 4 5 6 8 chunks +48 lines, -27 lines 0 comments Download
M components/safe_browsing/password_protection/password_protection_service_unittest.cc View 1 2 3 4 5 6 8 chunks +41 lines, -13 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 49 (37 generated)
Jialiu Lin
Hi lpz@, Can I bother you with one more CL? Thanks!
6 months ago (2017-04-13 23:40:08 UTC) #13
Jialiu Lin
-lpz@ Sorry, I didn't realize it is a public holiday in Canada. +nparker@, Could you ...
6 months ago (2017-04-17 17:04:01 UTC) #18
Nathan Parker
I'm not clear on why at refcounted ptr is required (I may just be missing ...
6 months ago (2017-04-17 20:28:02 UTC) #19
Jialiu Lin
Thanks nparker@! According to http://dev.chromium.org/developers/design-documents/threading#base_Bind "...By default, the object that PostTask uses must be a ...
6 months ago (2017-04-18 00:58:27 UTC) #22
lpz
Just driving by with mostly nits. https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsing/csd.proto File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsing/csd.proto#newcode254 components/safe_browsing/csd.proto:254: // A token ...
6 months ago (2017-04-18 14:43:28 UTC) #26
Jialiu Lin
Thanks lpz@! :-) https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsing/csd.proto File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsing/csd.proto#newcode254 components/safe_browsing/csd.proto:254: // A token which is consistent ...
6 months ago (2017-04-18 20:38:04 UTC) #28
Nathan Parker
Thanks for the explanations, this makes sense to me now. Threading is hard. :-) Just ...
6 months ago (2017-04-18 23:07:27 UTC) #32
Jialiu Lin
Thanks! https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsing/password_protection/password_protection_request.cc File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsing/password_protection/password_protection_request.cc#newcode18 components/safe_browsing/password_protection/password_protection_request.cc:18: namespace safe_browsing { On 2017/04/18 23:07:26, Nathan Parker ...
6 months ago (2017-04-19 00:21:14 UTC) #33
Nathan Parker
lgtm https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsing/password_protection/password_protection_request.h File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsing/password_protection/password_protection_request.h#newcode29 components/safe_browsing/password_protection/password_protection_request.h:29: // (1) | UI | If incognito or ...
6 months ago (2017-04-19 17:20:53 UTC) #38
Jialiu Lin
Thank you nparker and lpz! All comments addressed. https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsing/password_protection/password_protection_request.h File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsing/password_protection/password_protection_request.h#newcode29 components/safe_browsing/password_protection/password_protection_request.h:29: // ...
6 months ago (2017-04-19 17:52:27 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2817533004/140001
6 months ago (2017-04-19 19:04:33 UTC) #46
commit-bot: I haz the power
6 months ago (2017-04-19 19:13:02 UTC) #49
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3cffb12884df959d94c1e3bb67e9...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa