Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(198)

Issue 1185703007: Disable translate when there is no API key (Closed)

Created:
5 years, 6 months ago by paulmiller
Modified:
5 years, 5 months ago
CC:
chromium-reviews, klobag.chromium
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disable translate when there is no API key There is a new ChromePublic.apk target which is to Android Chrome as desktop Chromium is to desktop Chrome. ChromePublic lacks an API key, so many Google APIs are not available. I recently surveyed all the Chrome features which seem to depend an Google APIs. Some still worked. Most failed gracefully. Translate was the only one that failed ungracefully; in ChromePublic, the dialog pops up offering to translate a foreign page, but if you accept, nothing happens. We should disable translate (not show the dialog) in this case to avoid this bad UX. BUG=500025 Committed: https://crrev.com/8788f3c1260e54476d13d29acc02cf1412bb73f0 Cr-Commit-Position: refs/heads/master@{#339292}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : don't disable translate when testing #

Total comments: 2

Patch Set 4 : comment #

Total comments: 1

Patch Set 5 : fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -12 lines) Patch
M chrome/browser/autofill/autofill_interactive_uitest.cc View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_browsertest.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_service.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M components/translate/core/browser/translate_browser_metrics.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/translate/core/browser/translate_browser_metrics_unittest.cc View 1 3 chunks +20 lines, -12 lines 0 comments Download
M components/translate/core/browser/translate_manager.h View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
M components/translate/core/browser/translate_manager.cc View 1 2 3 3 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 57 (14 generated)
paulmiller
PTAL. Things I tested: -Regular Chrome.apk builds still translate. -Official Chrome.apk builds still translate. -ChromePublic.apk ...
5 years, 6 months ago (2015-06-12 22:20:28 UTC) #2
hajimehoshi
Could you give me more context? Please add more description.
5 years, 6 months ago (2015-06-16 08:39:18 UTC) #3
paulmiller
There is a new ChromePublic.apk target which is to Android Chrome as desktop Chromium is ...
5 years, 6 months ago (2015-06-18 17:14:11 UTC) #4
paulmiller
This is going to need to be merged to m44 (stable cut July 13), so ...
5 years, 6 months ago (2015-06-19 17:23:08 UTC) #5
hajimehoshi
Sorry for my late review. lgtm Please add the description to this CL.
5 years, 6 months ago (2015-06-22 08:03:14 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185703007/20001
5 years, 6 months ago (2015-06-22 17:22:59 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/30604)
5 years, 6 months ago (2015-06-22 18:08:31 UTC) #10
paulmiller
I'm now told this can wait until m45, which is good, since it's not working...
5 years, 6 months ago (2015-06-23 16:20:31 UTC) #11
paulmiller
This seems to work. I'm going to be out-of-office for a while, but I'll need ...
5 years, 6 months ago (2015-06-25 00:09:20 UTC) #12
hajimehoshi
On 2015/06/25 00:09:20, paulmiller wrote: > This seems to work. I'm going to be out-of-office ...
5 years, 6 months ago (2015-06-25 02:22:19 UTC) #13
Miguel Garcia
https://chromiumcodereview.appspot.com/1185703007/diff/40001/components/translate/core/browser/translate_manager.cc File components/translate/core/browser/translate_manager.cc (right): https://chromiumcodereview.appspot.com/1185703007/diff/40001/components/translate/core/browser/translate_manager.cc#newcode99 components/translate/core/browser/translate_manager.cc:99: if (!ignore_missing_key_for_testing_ && can you add a flag to ...
5 years, 6 months ago (2015-06-25 07:10:26 UTC) #15
paulmiller
To what default quota are you referring? Without an API key, the translate API doesn't ...
5 years, 5 months ago (2015-07-06 20:31:54 UTC) #16
paulmiller
I tested patch set 3 a bit more and it works. Hajime, PTAL again.
5 years, 5 months ago (2015-07-07 23:49:10 UTC) #17
hajimehoshi
https://codereview.chromium.org/1185703007/diff/40001/components/translate/core/browser/translate_manager.h File components/translate/core/browser/translate_manager.h (right): https://codereview.chromium.org/1185703007/diff/40001/components/translate/core/browser/translate_manager.h#newcode102 components/translate/core/browser/translate_manager.h:102: static void SetIgnoreMissingKeyForTesting(bool ignore); Add a comment
5 years, 5 months ago (2015-07-08 04:41:35 UTC) #18
paulmiller
done (translate_manager.cc changes are from rebase)
5 years, 5 months ago (2015-07-08 17:50:19 UTC) #19
Miguel Garcia
On 2015/07/06 20:31:54, paulmiller wrote: > To what default quota are you referring? Without an ...
5 years, 5 months ago (2015-07-08 17:52:23 UTC) #20
paulmiller
This change will not disable translate whenever the key is present. So if you ever ...
5 years, 5 months ago (2015-07-08 17:56:03 UTC) #21
Miguel Garcia
On 2015/07/08 17:56:03, paulmiller wrote: > This change will not disable translate whenever the key ...
5 years, 5 months ago (2015-07-08 17:56:41 UTC) #22
Miguel Garcia
lgtm
5 years, 5 months ago (2015-07-08 17:57:02 UTC) #23
paulmiller
Is this acceptable, Hajime? We're now aiming for m45 so time is once again limited.
5 years, 5 months ago (2015-07-10 00:43:11 UTC) #24
hajimehoshi
lgtm
5 years, 5 months ago (2015-07-10 02:24:42 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185703007/60001
5 years, 5 months ago (2015-07-10 18:08:07 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/86228)
5 years, 5 months ago (2015-07-10 19:17:46 UTC) #29
paulmiller
Bah. It seems linux_chromium_chromeos_rel_ng is flaky and mac_chromium_rel_ng is an unrelated build break.
5 years, 5 months ago (2015-07-10 21:24:30 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185703007/60001
5 years, 5 months ago (2015-07-10 21:26:38 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/86335)
5 years, 5 months ago (2015-07-10 22:43:06 UTC) #34
Takashi Toyoshima
Failed TranslateBrowserTest.TranslateInIsolatedWorld is disabled for other platforms than mac. https://code.google.com/p/chromium/issues/detail?id=383235 That's the reason why it ...
5 years, 5 months ago (2015-07-14 11:53:31 UTC) #35
Takashi Toyoshima
The test fails due to timeout. Can you check the following point? https://codereview.chromium.org/1185703007/diff/60001/chrome/browser/translate/translate_service.cc File chrome/browser/translate/translate_service.cc ...
5 years, 5 months ago (2015-07-14 12:06:22 UTC) #37
Takashi Toyoshima
Ah, can you remove "for Android" from the CL subject, too? The title is confusing ...
5 years, 5 months ago (2015-07-14 12:10:51 UTC) #38
paulmiller
did all that, thanks. Now I have a different failure...
5 years, 5 months ago (2015-07-15 00:43:24 UTC) #39
Takashi Toyoshima
Hum.... I have no idea this time.
5 years, 5 months ago (2015-07-15 04:04:11 UTC) #40
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185703007/80001
5 years, 5 months ago (2015-07-15 05:14:24 UTC) #42
paulmiller
Okay, then. I re-ran linux_chromium_rel_ng and it passed. So I'm going to say that's not ...
5 years, 5 months ago (2015-07-15 20:44:57 UTC) #45
Evan Stade
autofill_interactive_uitest.cc rslgtm
5 years, 5 months ago (2015-07-15 22:02:38 UTC) #46
paulmiller
On 2015/07/15 22:02:38, Evan Stade wrote: > autofill_interactive_uitest.cc rslgtm codereview didn't seem to recognize that ...
5 years, 5 months ago (2015-07-15 22:19:07 UTC) #47
Evan Stade
On 2015/07/15 22:19:07, paulmiller wrote: > On 2015/07/15 22:02:38, Evan Stade wrote: > > autofill_interactive_uitest.cc ...
5 years, 5 months ago (2015-07-15 23:34:20 UTC) #48
paulmiller
We're going to instruct people to build ChromePublic from the LKGR, rather than m45. So ...
5 years, 5 months ago (2015-07-16 17:30:54 UTC) #49
Takashi Toyoshima
Ah, sorry. I forgot to say. LGTM.
5 years, 5 months ago (2015-07-17 04:26:57 UTC) #50
hajimehoshi
lgtm
5 years, 5 months ago (2015-07-17 04:28:17 UTC) #51
paulmiller
may our tree be healthy and may our trybots be not flaky http://www.evilenglish.net/wp-content/uploads/2015/01/20090916_fingers_crossed_2743.jpg
5 years, 5 months ago (2015-07-17 17:13:59 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185703007/80001
5 years, 5 months ago (2015-07-17 17:15:15 UTC) #55
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 5 months ago (2015-07-17 18:39:53 UTC) #56
commit-bot: I haz the power
5 years, 5 months ago (2015-07-17 18:42:49 UTC) #57
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/8788f3c1260e54476d13d29acc02cf1412bb73f0
Cr-Commit-Position: refs/heads/master@{#339292}

Powered by Google App Engine
This is Rietveld 408576698