Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793913002:
Added profile deletion browsertests. (Closed)
DescriptionAdded profile deletion browsertests.
BUG=
R=bauerb@chromium.org, anthonyvd@chromium.org, mlerman@chromium.org
Review-Url: https://codereview.chromium.org/2793913002
Cr-Commit-Position: refs/heads/master@{#461688}
Committed: https://chromium.googlesource.com/chromium/src/+/f670a4bb1851e14d8a58046bb39f31cb7cd2bd6c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review fixes. #
Total comments: 1
Patch Set 3 : Tests will now wait for profile deletion not profile creation. #Patch Set 4 : Hopefully provide a better grammar for the sentence. #Patch Set 5 : Added 'override' for virtual d-tors. #
Messages
Total messages: 19 (7 generated)
Here we go again. Previous attempt: https://codereview.chromium.org/2747293003/ Reverted due https://crbug.com/704601 Flakiness is fixed by adding delay till BrowserData is actually removed.
Just FYI for the next time, the ideal flow would be: 1) Upload a pure reland as a new CL 2) Make changes to fix the CL 3) Upload fixed CL as a new patch set That way, one CL still corresponds to one commit, but reviewers can see the diff between the previous version of the CL. https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:77: EXPECT_LT(0u, expected_count_); Nit: I would do EXPECT_GT(expected_count_, 0u) -- if it's not EQ, we don't get a better error message by putting the expected value first, and `expected_count_ > 0` reads more straightforward to me than the other way around. https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:96: ++profiles_created_count_; Nit: I would use postincrement -- the performance difference is negligible, and again it reads a bit more straightforward to me. https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:100: // TODO(bug 704601): remove it when bug is fixed. Nit: Use TODO(704601) or TODO(https://crbug.com/704601). https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:111: LOG(INFO) << profiles_created_count_ << " profiles created & " Make this a DLOG. https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager_browsertest.cc:292: // fulfill |profile_deletion_observer| expectataion. This seems a bit strange. If a new profile is not created in all cases, why have the deletion observer expect that? Would it make more sense to pass in one value for the expected number of deletions and one for additions? https://codereview.chromium.org/2793913002/diff/1/chrome/browser/ui/webui/pro... File chrome/browser/ui/webui/profile_helper_browsertest.cc (right): https://codereview.chromium.org/2793913002/diff/1/chrome/browser/ui/webui/pro... chrome/browser/ui/webui/profile_helper_browsertest.cc:70: class BrowsingDataRemoverObserver There is an actual BrowsingDataRemover::Observer class if you don't want to inhibit completion.
On 2017/04/03 10:57:39, Bernhard (slow until Apr 6) wrote: > Just FYI for the next time, the ideal flow would be: > 1) Upload a pure reland as a new CL > 2) Make changes to fix the CL > 3) Upload fixed CL as a new patch set > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/ui/webui/pro... > chrome/browser/ui/webui/profile_helper_browsertest.cc:70: class > BrowsingDataRemoverObserver > There is an actual BrowsingDataRemover::Observer class if you don't want to > inhibit completion. It is a my first option, but obviously its works only then observer supplied with BrowsingDataRemoverImpl::Remove[WithFilter]AndReply call. see chrome/browser/browsing_data/browsing_data_remover_impl.cc:632
> https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager_browsertest.cc:111: LOG(INFO) << > profiles_created_count_ << " profiles created & " > Make this a DLOG. Isn't browser_tests always run with DCHECK_IS_ON() mode? > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager_browsertest.cc:292: // fulfill > |profile_deletion_observer| expectataion. > This seems a bit strange. If a new profile is not created in all cases, why have > the deletion observer expect that? Would it make more sense to pass in one value > for the expected number of deletions and one for additions? It costs more lines, but you're right I shouldn't be so lazy.
On 2017/04/03 12:16:11, palar wrote: > > > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... > > chrome/browser/profiles/profile_manager_browsertest.cc:111: LOG(INFO) << > > profiles_created_count_ << " profiles created & " > > Make this a DLOG. > Isn't browser_tests always run with DCHECK_IS_ON() mode? > > > > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... > > chrome/browser/profiles/profile_manager_browsertest.cc:292: // fulfill > > |profile_deletion_observer| expectataion. > > This seems a bit strange. If a new profile is not created in all cases, why > have > > the deletion observer expect that? Would it make more sense to pass in one > value > > for the expected number of deletions and one for additions? > It costs more lines, but you're right I shouldn't be so lazy. Wait. I think about profile creation callback, and on second thought it's not look like reliable thing. I'll redone expectation on ProfileAttributesStorage::Observer it makes more sense.
done.
LGTM On 2017/04/03 12:16:11, palar wrote: > > > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/profiles/pro... > > chrome/browser/profiles/profile_manager_browsertest.cc:111: LOG(INFO) << > > profiles_created_count_ << " profiles created & " > > Make this a DLOG. > Isn't browser_tests always run with DCHECK_IS_ON() mode? Hm, true, this is not actually production code. Basically, what I would like to avoid is log spamming, but that ship has probably sailed anyway, and the test runner should swallow log messages (and then show them only if a test is failing), and given that this log statement is directly relevant to the test, it's not that spammy. So, cool :) On 2017/04/03 11:31:31, palar wrote: > On 2017/04/03 10:57:39, Bernhard (slow until Apr 6) wrote: > > Just FYI for the next time, the ideal flow would be: > > 1) Upload a pure reland as a new CL > > 2) Make changes to fix the CL > > 3) Upload fixed CL as a new patch set > > > > > https://codereview.chromium.org/2793913002/diff/1/chrome/browser/ui/webui/pro... > > chrome/browser/ui/webui/profile_helper_browsertest.cc:70: class > > BrowsingDataRemoverObserver > > There is an actual BrowsingDataRemover::Observer class if you don't want to > > inhibit completion. > It is a my first option, but obviously its works only then observer supplied > with > BrowsingDataRemoverImpl::Remove[WithFilter]AndReply call. > see chrome/browser/browsing_data/browsing_data_remover_impl.cc:632 Ugh. +msramek, can you remind me again why that is so? I vaguely remember a discussion about it, but not the outcome. What that does tell me though is that we should probably add some comments :)
profiles LGTM https://codereview.chromium.org/2793913002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_browsertest.cc (right): https://codereview.chromium.org/2793913002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_browsertest.cc:68: // It has ScopedKeepAlive object to prevent browser shutdown started in case nit: this sentence needs better grammar
The CQ bit was checked by palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/2793913002/#ps60001 (title: "Hopefully provide a better grammar for the sentence.")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 palar@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/2793913002/#ps80001 (title: "Added 'override' for virtual d-tors.")
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": 80001, "attempt_start_ts": 1491306574609150,
"parent_rev": "5ee4ef4db4ece43fdb9994b90a6d30f4239ad756", "commit_rev":
"f670a4bb1851e14d8a58046bb39f31cb7cd2bd6c"}
Message was sent while issue was closed.
Description was changed from ========== Added profile deletion browsertests. BUG= R=bauerb@chromium.org, anthonyvd@chromium.org, mlerman@chromium.org ========== to ========== Added profile deletion browsertests. BUG= R=bauerb@chromium.org, anthonyvd@chromium.org, mlerman@chromium.org Review-Url: https://codereview.chromium.org/2793913002 Cr-Commit-Position: refs/heads/master@{#461688} Committed: https://chromium.googlesource.com/chromium/src/+/f670a4bb1851e14d8a58046bb39f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f670a4bb1851e14d8a58046bb39f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
