Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297863003:
Fix initialization for a few more unit_tests under PlzNavigate (Closed)
|
Created:
4 years, 3 months ago by scottmg Modified:
4 years, 3 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix initialization for a few more unit_tests under PlzNavigate
Follows https://codereview.chromium.org/2296483002/.
R=clamy@chromium.org
BUG=510836
Committed: https://crrev.com/af2f7ed196372bcf7cbfcf673a3f3c05d146c78b
Cr-Commit-Position: refs/heads/master@{#416046}
Patch Set 1 #Patch Set 2 : tabsapiunittest #
Total comments: 2
Patch Set 3 : rebase #
Depends on Patchset: Messages
Total messages: 24 (11 generated)
Thanks! https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc (left): https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc:38: TestingBrowserProcess::CreateInstance(); Why do we need to remove this, and isn't it a problem if we do it?
https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc (left): https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc:38: TestingBrowserProcess::CreateInstance(); On 2016/08/30 21:23:21, clamy wrote: > Why do we need to remove this, and isn't it a problem if we do it? It's initialized twice at the moment, here, but also very early on in https://cs.chromium.org/chromium/src/chrome/test/base/chrome_unit_test_suite.... . I guess no mainline bots notice because the whole file is in #if GOOGLE_CHROME_BUILD, only happened to notice because I run with that locally on my Windows box. So... not too important either way, I guess.
On 2016/08/30 22:03:15, scottmg wrote: > https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc > (left): > > https://codereview.chromium.org/2297863003/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc:38: > TestingBrowserProcess::CreateInstance(); > On 2016/08/30 21:23:21, clamy wrote: > > Why do we need to remove this, and isn't it a problem if we do it? > > It's initialized twice at the moment, here, but also very early on in > https://cs.chromium.org/chromium/src/chrome/test/base/chrome_unit_test_suite.... > . > > I guess no mainline bots notice because the whole file is in #if > GOOGLE_CHROME_BUILD, only happened to notice because I run with that locally on > my Windows box. So... not too important either way, I guess. (In fact, I think it'll probably dcheck completely unrelated to PlzNavigate now that I think about it. Let me confirm that, and if so, I'll move to a separate CL so as not to confuse things.)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
metrics_reporting_handler_unittest.cc lgtm
Thanks! Lgtm.
The CQ bit was checked by scottmg@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...
On 2016/08/30 22:22:08, Dan Beam wrote: > metrics_reporting_handler_unittest.cc lgtm I split this out to here https://codereview.chromium.org/2296053002 since it's unrelated and it might be a bit before the rest of this change can land because its dependent is large.
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 scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2297863003/#ps40001 (title: "rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
scottmg@chromium.org changed reviewers: + asargent@chromium.org
oops, +asargent for OWNERS review
lgtm
The CQ bit was checked by scottmg@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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix initialization for a few more unit_tests under PlzNavigate Follows https://codereview.chromium.org/2296483002/. R=clamy@chromium.org BUG=510836 ========== to ========== Fix initialization for a few more unit_tests under PlzNavigate Follows https://codereview.chromium.org/2296483002/. R=clamy@chromium.org BUG=510836 Committed: https://crrev.com/af2f7ed196372bcf7cbfcf673a3f3c05d146c78b Cr-Commit-Position: refs/heads/master@{#416046} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/af2f7ed196372bcf7cbfcf673a3f3c05d146c78b Cr-Commit-Position: refs/heads/master@{#416046} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
