Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176893002:
[Sync] Ignore urls which contain username/password (Closed)
|
Created:
4 years, 5 months ago by Gang Wu Modified:
4 years, 4 months ago Reviewers:
Nicolas Zea CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Ignore urls which contain username/password.
Since HistoryBackend would remove username/password in an url
when write it into DB, Chrome Sync would ignore such url.
But ignore that url won't lost the url since HistoryBackend
will send username/password removed url to sync later.
BUG=629170
Committed: https://crrev.com/8da033de405cfea2418da4798b757ab8c671546e
Cr-Commit-Position: refs/heads/master@{#407682}
Patch Set 1 #Patch Set 2 : add more test #
Total comments: 4
Patch Set 3 : add comments #
Messages
Total messages: 32 (25 generated)
The CQ bit was checked by gangwu@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 gangwu@chromium.org
Description was changed from ========== ignore urls which contain username/password BUG=629170 ========== to ========== Ignore urls which contain username/password BUG=629170 ==========
gangwu@chromium.org changed reviewers: + zea@chromium.org
PTAL
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly LG, a couple nits. Also, could you update the commit message with a bit more detail? Note that you have [Sync] in the CL title, but not in the commit message itself. https://codereview.chromium.org/2176893002/diff/20001/components/history/core... File components/history/core/browser/typed_url_syncable_service.cc (right): https://codereview.chromium.org/2176893002/diff/20001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:753: if (url.has_username() || url.has_password()) Comment why we ignore these here? https://codereview.chromium.org/2176893002/diff/20001/components/history/core... File components/history/core/browser/typed_url_syncable_service_unittest.cc (right): https://codereview.chromium.org/2176893002/diff/20001/components/history/core... components/history/core/browser/typed_url_syncable_service_unittest.cc:1078: TEST_F(TypedUrlSyncableServiceTest, MergeUrlsWithUsernameAndPassword) { Add comment describing test?
Description was changed from ========== Ignore urls which contain username/password BUG=629170 ========== to ========== Ignore urls which contain username/password. BUG=629170 Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. ==========
The CQ bit was checked by gangwu@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 gangwu@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gangwu@chromium.org
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Comments added. https://codereview.chromium.org/2176893002/diff/20001/components/history/core... File components/history/core/browser/typed_url_syncable_service.cc (right): https://codereview.chromium.org/2176893002/diff/20001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:753: if (url.has_username() || url.has_password()) On 2016/07/25 18:30:56, Nicolas Zea wrote: > Comment why we ignore these here? Done. https://codereview.chromium.org/2176893002/diff/20001/components/history/core... File components/history/core/browser/typed_url_syncable_service_unittest.cc (right): https://codereview.chromium.org/2176893002/diff/20001/components/history/core... components/history/core/browser/typed_url_syncable_service_unittest.cc:1078: TEST_F(TypedUrlSyncableServiceTest, MergeUrlsWithUsernameAndPassword) { On 2016/07/25 18:30:56, Nicolas Zea wrote: > Add comment describing test? 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: This issue passed the CQ dry run.
Could you add " [Sync] " to the commit title (it makes it easier to find sync related commits in the revision history)? Also move the BUG= line to below the commit message. It's good to be consistent with that kind of thing. Lastly, the grammar in the commit message makes it tough to understand. I've rewritten it for clarity: Since the HistoryBackend will remove the username/password in a url before it writes it into the DB, Chrome Sync should ignore such a url. Ignoring that url won't result in data loss because the HistoryBackend will send the url with the username/password removed back to sync. LGTM otherwise
Description was changed from ========== Ignore urls which contain username/password. BUG=629170 Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. ========== to ========== [Sync] Ignore urls which contain username/password. BUG=629170 Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. ==========
Description was changed from ========== [Sync] Ignore urls which contain username/password. BUG=629170 Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. ========== to ========== [Sync] Ignore urls which contain username/password. Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. BUG=629170 ==========
The CQ bit was checked by gangwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Sync] Ignore urls which contain username/password. Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. BUG=629170 ========== to ========== [Sync] Ignore urls which contain username/password. Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. BUG=629170 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Ignore urls which contain username/password. Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. BUG=629170 ========== to ========== [Sync] Ignore urls which contain username/password. Since HistoryBackend would remove username/password in an url when write it into DB, Chrome Sync would ignore such url. But ignore that url won't lost the url since HistoryBackend will send username/password removed url to sync later. BUG=629170 Committed: https://crrev.com/8da033de405cfea2418da4798b757ab8c671546e Cr-Commit-Position: refs/heads/master@{#407682} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8da033de405cfea2418da4798b757ab8c671546e Cr-Commit-Position: refs/heads/master@{#407682} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
