Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2418413003:
predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle (Closed)
Descriptionpredictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle
And also use more convenient operator<< instead of PrintTo for tests.
BUG=631966, 272091
Committed: https://crrev.com/241eb5ff377402abdd0a322d9cebb7e958967dea
Cr-Commit-Position: refs/heads/master@{#425698}
Patch Set 1 #
Total comments: 9
Patch Set 2 : '\n' -> std::endl #
Messages
Total messages: 21 (13 generated)
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
I've deleted deprecated class from unittest code + operator<< is more convenient that PrintTo function for test purposes so I've updated resource_prefetch_predictor_test_utils. It also will work with gtest for nice printing.
lgtm with minor suggestions and if bots pass Yeah, seems to be changing the way threads are shutting down, so there might be occasional dragons there: git cl try!
https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_test_util.cc (right): https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:63: os << "\t\t" << resource << "\n"; nit: std::endl instead of '\n' for extra pretty on hypothetical Windows? https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Please add the deprecation bug to the list of bugs this change is addressing: BUG=631966,272091 https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:36: std::ostream& operator<<(std::ostream& stream, const PrefetchData& data); that's neat, I did not know it was possible to use both operator<< and PrintTo in googletest
The CQ bit was checked by alexilin@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Description was changed from ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966 ========== to ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966,272091 ==========
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_test_util.cc (right): https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:63: os << "\t\t" << resource << "\n"; On 2016/10/17 14:46:42, pasko wrote: > nit: std::endl instead of '\n' for extra pretty on hypothetical Windows? Uh, I've thought that std::endl is kinda undesirable thing. I've heard that it's slower than simple '\n'. https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:80: os << "\t\t" << redirect << os; oh, wow https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/17 14:46:42, pasko wrote: > Please add the deprecation bug to the list of bugs this change is addressing: > > BUG=631966,272091 Done. https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:36: std::ostream& operator<<(std::ostream& stream, const PrefetchData& data); On 2016/10/17 14:46:42, pasko wrote: > that's neat, I did not know it was possible to use both operator<< and PrintTo > in googletest Yeah, I think that it makes sense to use PrintTo only if there is already decleared some operator<< for type T. In other cases << is more convenient. For more information: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm, thanks https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_test_util.cc (right): https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:63: os << "\t\t" << resource << "\n"; On 2016/10/17 15:32:35, alexilin wrote: > On 2016/10/17 14:46:42, pasko wrote: > > nit: std::endl instead of '\n' for extra pretty on hypothetical Windows? > > Uh, I've thought that std::endl is kinda undesirable thing. I've heard that it's > slower than simple '\n'. TIL: 1. std::endl also does flushing of the buffer 2. it would output '\r\n' on Windows only if the stream is a file that is open in text mode. So: std::endl sounds better in tests, where portability and flushing messages ASAP is more important than speed. https://codereview.chromium.org/2418413003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:80: os << "\t\t" << redirect << os; On 2016/10/17 15:32:35, alexilin wrote: > oh, wow :)
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 alexilin@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 ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966,272091 ========== to ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966,272091 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966,272091 ========== to ========== predictors: Replace deprecated TestBrowserThread by TestBrowserThreadBundle And also use more convenient operator<< instead of PrintTo for tests. BUG=631966,272091 Committed: https://crrev.com/241eb5ff377402abdd0a322d9cebb7e958967dea Cr-Commit-Position: refs/heads/master@{#425698} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/241eb5ff377402abdd0a322d9cebb7e958967dea Cr-Commit-Position: refs/heads/master@{#425698} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
