Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650003004:
Don't offer to translate between Simplified and Traditional Chinese (Closed)
DescriptionDon't offer to translate between Simplified and Traditional Chinese.
BUG=683164
Review-Url: https://codereview.chromium.org/2650003004
Cr-Commit-Position: refs/heads/master@{#446213}
Committed: https://chromium.googlesource.com/chromium/src/+/889fb1a6efab0b3e425b81575bbd7a62ed0c4eb1
Patch Set 1 #Patch Set 2 : Adds unittests for this change. #Patch Set 3 : Updates comments #
Total comments: 5
Patch Set 4 : Fixes unittests #
Total comments: 1
Patch Set 5 : Fixes unittests by simulating online mode and setting behavior for mock function. #
Total comments: 1
Messages
Total messages: 30 (18 generated)
Description was changed from ========== Don't offer to translate between Simplified and Traditional Chinese BUG=683164 ========== to ========== Don't offer to translate between Simplified and Traditional Chinese. BUG=683164 ==========
riesa@chromium.org changed reviewers: + groby@chromium.org
The CQ bit was checked by riesa@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...
In general l-g-t-m, but I'm confused why the tests pass? https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:321: prefs_.SetBoolean(prefs::kEnableTranslate, false); I'm surprised the tests work for translate being disabled? They should report INITIATION_STATUS_DISABLED_BY_PREFS as result. https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:328: // In the offline case, Initiate will early-out before even hitting the API Might want to fix the comment :) https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:330: network_notifier_.SimulateOffline(); And remove SimulateOffline https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:335: network_notifier_.SimulateOnline(); See above https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:362: // key test. See previous test
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by riesa@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 2017/01/25 01:52:07, groby wrote: > In general l-g-t-m, but I'm confused why the tests pass? > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > File components/translate/core/browser/translate_manager_unittest.cc (right): > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:321: > prefs_.SetBoolean(prefs::kEnableTranslate, false); > I'm surprised the tests work for translate being disabled? They should report > INITIATION_STATUS_DISABLED_BY_PREFS as result. > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:328: // In the > offline case, Initiate will early-out before even hitting the API > Might want to fix the comment :) > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:330: > network_notifier_.SimulateOffline(); > And remove SimulateOffline > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:335: > network_notifier_.SimulateOnline(); > See above > > https://codereview.chromium.org/2650003004/diff/40001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:362: // key > test. > See previous test thanks for reviewing! The two added unittests are indeed starting to fail in some of the bots that have run. I think I have fixed them in the latest patch set and am re-running.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2650003004/diff/60001/components/translate/co... File components/translate/core/browser/translate_manager_unittest.cc (right): https://codereview.chromium.org/2650003004/diff/60001/components/translate/co... components/translate/core/browser/translate_manager_unittest.cc:327: histogram_tester.ExpectTotalCount(kMetricName, 0); That's the offending line, sorry for not spotting it in the review. You expect a total count of 1 sample in the histogram, not 0. (You can just delete this - ExpectUniqueSample tests there is exactly one sample added) Both here and below.
The CQ bit was checked by riesa@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 2017/01/25 16:20:10, groby wrote: > https://codereview.chromium.org/2650003004/diff/60001/components/translate/co... > File components/translate/core/browser/translate_manager_unittest.cc (right): > > https://codereview.chromium.org/2650003004/diff/60001/components/translate/co... > components/translate/core/browser/translate_manager_unittest.cc:327: > histogram_tester.ExpectTotalCount(kMetricName, 0); > That's the offending line, sorry for not spotting it in the review. You expect a > total count of 1 sample in the histogram, not 0. (You can just delete this - > ExpectUniqueSample tests there is exactly one sample added) > > Both here and below. Done. Also added a call setting behavior for mock function IsTranslatableURL. Tests now pass.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by riesa@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": 80001, "attempt_start_ts": 1485398403391910,
"parent_rev": "178529af33287d1459efceca6454f768f62afe7b", "commit_rev":
"889fb1a6efab0b3e425b81575bbd7a62ed0c4eb1"}
Message was sent while issue was closed.
Description was changed from ========== Don't offer to translate between Simplified and Traditional Chinese. BUG=683164 ========== to ========== Don't offer to translate between Simplified and Traditional Chinese. BUG=683164 Review-Url: https://codereview.chromium.org/2650003004 Cr-Commit-Position: refs/heads/master@{#446213} Committed: https://chromium.googlesource.com/chromium/src/+/889fb1a6efab0b3e425b81575bbd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/889fb1a6efab0b3e425b81575bbd...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2653183009/ by dbeam@chromium.org. The reason for reverting is: Causing sizes failure on Linux: https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/31922/steps... # translate_manager.cc std::ios_base::Init::Init()@plt # translate_manager.cc std::__ioinit [#includes <iostream>, use <ostream> instead] # translate_manager.cc __cxa_atexit@plt [registers a dtor to run at exit] # translate_manager.cc __dso_handle # translate_manager.cc __init_array_end+0x968.
Message was sent while issue was closed.
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2650003004/diff/80001/components/translate/co... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2650003004/diff/80001/components/translate/co... components/translate/core/browser/translate_manager.cc:7: #include <iostream> I'm confused how this could pass the _CheckNoIOStreamInHeaders check in https://chromium.googlesource.com/chromium/src/+/master/PRESUBMIT.py#395 iostream will give you static initializers, which are not allowed in Chromium. I filed crbug.com/685512 for that.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2655223002/ by kjellander@chromium.org. The reason for reverting is: Introduces static initializers; failing the sizes step in https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/31922.
Message was sent while issue was closed.
On 2017/01/26 05:30:50, kjellander_chromium wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/2655223002/ by mailto:kjellander@chromium.org. > > The reason for reverting is: Introduces static initializers; failing the sizes > step in > https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/31922. Ah, this was left over from debugging and I missed it. Sorry for the trouble. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
