Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572433002:
Disable tracking of visits to bookmarks in incognito (Closed)
DescriptionDisable tracking of visits to bookmarks in incognito
Date of the last visit to any bookmarked page is stored to
BookmarkModel. Previously, this happens in off-the-record profiles as
well. This CL fixes the issue.
BUG=662382
Committed: https://crrev.com/1e385fa1c19d96f343594a0623a7985adb50c483
Cr-Commit-Position: refs/heads/master@{#438845}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Bernhard's comments + a shiny new unittest #
Total comments: 4
Patch Set 3 : Bernhard's comments #2 #
Messages
Total messages: 34 (21 generated)
The CQ bit was checked by jkrcal@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...
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, could you PTAL? Do you think this bug / situation is worth adding a unit-test?
if it's fixing a bug, we should always add a unit-test. If you add functionality requiring an if statement (especially including negations ;-)), you should add a test to verify it. You wouldn't believe how often I get conditions wrong ;-)
mastiz@chromium.org changed reviewers: + mastiz@chromium.org
Also, please see https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... Bookmark-related exceptions seem intentional.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/12 14:41:49, mastiz wrote: > Also, please see > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... > > Bookmark-related exceptions seem intentional. I still wouldn't want to store the date of a visit in incognito. If in doubt, please check with chrome-privacy. https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:31: return; FWIW, using a base::ScopedObserver would let us avoid this check, but maybe we can avoid creating this object completely (see below). https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:50: // Switch off tracking visits for off the record profiles. Nit: the code here uses the term "off the record" because no one has renamed the method, but outside of the most commonly used term is "incognito", so I think we should use that as much as possible. https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:55: if (!bookmark_model_ || !tracking_enabled_) { So, what does this class actually do if tracking is disabled (or we don't have a bookmark model)? Could we just skip creating it completely?
Thanks for the comments. PTAL, again. https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:31: return; On 2016/12/12 15:06:37, Bernhard Bauer wrote: > FWIW, using a base::ScopedObserver would let us avoid this check, but maybe we > can avoid creating this object completely (see below). Done. https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:50: // Switch off tracking visits for off the record profiles. On 2016/12/12 15:06:37, Bernhard Bauer wrote: > Nit: the code here uses the term "off the record" because no one has renamed the > method, but outside of the most commonly used term is "incognito", so I think we > should use that as much as possible. Done. https://codereview.chromium.org/2572433002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:55: if (!bookmark_model_ || !tracking_enabled_) { On 2016/12/12 15:06:38, Bernhard Bauer wrote: > So, what does this class actually do if tracking is disabled (or we don't have a > bookmark model)? Could we just skip creating it completely? Done.
Description was changed from ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 ========== to ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 ==========
mastiz@chromium.org changed reviewers: - mastiz@chromium.org
The CQ bit was checked by jkrcal@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...)
Thanks! https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:33: void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel( Name this method Maybe... now? https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:84: bool tracking_enabled_; This is now not necessary anymore.
jkrcal@chromium.org changed reviewers: + avi@chromium.org
Oh, sorry for the omissions. PTAL, again! Avi, could you PTAL at tab_helpers.cc? https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:33: void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel( On 2016/12/14 15:58:45, Bernhard Bauer wrote: > Name this method Maybe... now? Done. https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2572433002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:84: bool tracking_enabled_; On 2016/12/14 15:58:45, Bernhard Bauer wrote: > This is now not necessary anymore. Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks! Avi, could you PTAL at tab_helpers.cc?
lgtm
The CQ bit was checked by jkrcal@chromium.org
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": 40001, "attempt_start_ts": 1481818703460860,
"parent_rev": "2049943cd871f6f399fc35fed84ca7f54a106f7c", "commit_rev":
"67de56f957f0ab59c495ce7fbd9fddab029d0f35"}
Message was sent while issue was closed.
Description was changed from ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 ========== to ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 Review-Url: https://codereview.chromium.org/2572433002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 Review-Url: https://codereview.chromium.org/2572433002 ========== to ========== Disable tracking of visits to bookmarks in incognito Date of the last visit to any bookmarked page is stored to BookmarkModel. Previously, this happens in off-the-record profiles as well. This CL fixes the issue. BUG=662382 Committed: https://crrev.com/1e385fa1c19d96f343594a0623a7985adb50c483 Cr-Commit-Position: refs/heads/master@{#438845} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e385fa1c19d96f343594a0623a7985adb50c483 Cr-Commit-Position: refs/heads/master@{#438845} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
