Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
598023006:
Add browser test for initial languages. (Closed)
|
Created:
6 years, 2 months ago by Alexander Alekseev Modified:
6 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd browser test for initial languages.
BUG=418573
Committed: https://crrev.com/e4016bbbb73b9abd385174e1c11cb3949e4211e0
Cr-Commit-Position: refs/heads/master@{#303852}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Update after review. #
Total comments: 20
Patch Set 3 : Update after review. #Patch Set 4 : Rebased. #Patch Set 5 : Revert l10n_util. #Patch Set 6 : Fix test. #
Messages
Total messages: 15 (4 generated)
alemate@chromium.org changed reviewers: + dpolukhin@chromium.org, maruel@chromium.org
Please review. dpolukhin@ chrome/browser/chromeos/* maruel@ PRESUBMIT.py
PRESUBMIT.py rubberstamp lgtm even if it does smell wrong a bit.
https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:293: if (enforce_initial_locale) You override before and after checking machine statistics. I think you need it only once and I prefer doing it before checking machine statistics, i.e. just provide default value. But if ii doesn't work, please keep only last overriders. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document_browsertest.cc (right): https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:47: using namespace locale_util; Please write 'using locale_util::SwitchLanguageCallback' instead. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:61: std::string GetExpectedLanguage(const std::string& required) { Please add comment why this mismatch happens. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:92: std::ostringstream os; It is just simple string concatenation, you don't need ostringstream. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:129: virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE { It looks like there is no need to override function just to call base class function. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:133: virtual void SetUpOnMainThread() OVERRIDE { Ditto. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:157: public testing::WithParamInterface<const char*> { Indent. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:162: virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE { Ditto. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:166: virtual void SetUpOnMainThread() OVERRIDE { Ditto.
alemate@chromium.org changed reviewers: + davemoore@chromium.org
+ devemoore@ for chromeos/system/fake_statistics_provider* I've moved FakeStatisticsProvider to a separate file. Please review. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document_browsertest.cc (right): https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:47: using namespace locale_util; On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Please write 'using locale_util::SwitchLanguageCallback' instead. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:61: std::string GetExpectedLanguage(const std::string& required) { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Please add comment why this mismatch happens. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:92: std::ostringstream os; On 2014/09/30 09:29:55, Dmitry Polukhin wrote: > It is just simple string concatenation, you don't need ostringstream. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:133: virtual void SetUpOnMainThread() OVERRIDE { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Ditto. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:157: public testing::WithParamInterface<const char*> { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Indent. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:162: virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Ditto. Done. https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:166: virtual void SetUpOnMainThread() OVERRIDE { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > Ditto. Done.
LGTM after fixing nits. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document_browsertest.cc (right): https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:22: using locale_util::SwitchLanguageCallback; Nit, usually we put such using on global level of the file just after includes. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:30: runner_(new content::MessageLoopRunner){}; There is no need in ';' please remove. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:31: ~LanguageSwitchedWaiter(){}; Nit, there is no need in such empty d-tor. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:48: using namespace locale_util; Remove this line, it is not needed anymore after line #22. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:66: if (required == "en-AU") { To simplify future test extension, I would create static table with mapping and iterate over it. So adding new language pair will require only 1 line change in data. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:178: virtual ~CustomizationLocaleTest() {} You don't need both c-tor and d-tor, compiler will do it for you. If you would liked to keep d-tor please add OVERRIDE. Or that is even better use: typedef InProcessBrowserTest CustomizationLocaleTest; If you don't need anything to add to InProcessBrowserTest. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:210: virtual ~CustomizationVPDTest() {} You don't need it and OVERRIDE is missing. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:224: chromeos::StartupCustomizationDocument::GetInstance() You already in chromeos:: namespace. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:246: EXPECT_EQ(chromeos::kMostRelevantLanguagesDivider, value) Ditto. https://codereview.chromium.org/598023006/diff/20001/chromeos/system/fake_sta... File chromeos/system/fake_statistics_provider.h (right): https://codereview.chromium.org/598023006/diff/20001/chromeos/system/fake_sta... chromeos/system/fake_statistics_provider.h:17: virtual ~FakeStatisticsProvider() {} Missing OVERRIDE.
https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document_browsertest.cc (right): https://codereview.chromium.org/598023006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document_browsertest.cc:129: virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE { On 2014/09/30 09:29:54, Dmitry Polukhin wrote: > It looks like there is no need to override function just to call base class > function. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document_browsertest.cc (right): https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:22: using locale_util::SwitchLanguageCallback; On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > Nit, usually we put such using on global level of the file just after includes. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:30: runner_(new content::MessageLoopRunner){}; On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > There is no need in ';' please remove. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:31: ~LanguageSwitchedWaiter(){}; On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > Nit, there is no need in such empty d-tor. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:48: using namespace locale_util; On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > Remove this line, it is not needed anymore after line #22. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:66: if (required == "en-AU") { On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > To simplify future test extension, I would create static table with mapping and > iterate over it. So adding new language pair will require only 1 line change in > data. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:178: virtual ~CustomizationLocaleTest() {} On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > You don't need both c-tor and d-tor, compiler will do it for you. If you would > liked to keep d-tor please add OVERRIDE. > > Or that is even better use: > > typedef InProcessBrowserTest CustomizationLocaleTest; > > If you don't need anything to add to InProcessBrowserTest. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:210: virtual ~CustomizationVPDTest() {} On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > You don't need it and OVERRIDE is missing. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:224: chromeos::StartupCustomizationDocument::GetInstance() On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > You already in chromeos:: namespace. Done. https://codereview.chromium.org/598023006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document_browsertest.cc:246: EXPECT_EQ(chromeos::kMostRelevantLanguagesDivider, value) On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > Ditto. Done. https://codereview.chromium.org/598023006/diff/20001/chromeos/system/fake_sta... File chromeos/system/fake_statistics_provider.h (right): https://codereview.chromium.org/598023006/diff/20001/chromeos/system/fake_sta... chromeos/system/fake_statistics_provider.h:17: virtual ~FakeStatisticsProvider() {} On 2014/10/03 07:49:52, Dmitry Polukhin wrote: > Missing OVERRIDE. Done.
lgtm
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/598023006/80001
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/598023006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e4016bbbb73b9abd385174e1c11cb3949e4211e0 Cr-Commit-Position: refs/heads/master@{#303852} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
