Disable translate when there is no API key

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}

[paulmiller, 2015-06-12 21:46:27]

paulmiller@chromium.org changed reviewers:
+ hajimehoshi@chromium.org

[paulmiller, 2015-06-12 22:20:28]

PTAL.

Things I tested:
-Regular Chrome.apk builds still translate.
-Official Chrome.apk builds still translate.
-ChromePublic.apk builds no longer offer to translate. (They were offering and
failing.)
-TranslateBrowserMetricsTest.ReportInitiationStatus still passes.

[hajimehoshi, 2015-06-16 08:39:18]

Could you give me more context? Please add more description.

[paulmiller, 2015-06-18 17:14:11]

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 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.

[paulmiller, 2015-06-19 17:23:08]

This is going to need to be merged to m44 (stable cut July 13), so time is
limited.

[hajimehoshi, 2015-06-22 08:03:14]

Sorry for my late review. lgtm

Please add the description to this CL.

[paulmiller, 2015-06-22 17:22:21]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2015-06-22 17:22:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185703007/20001

[commit-bot: I haz the power, 2015-06-22 18:08:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-22 18:08:31]

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_...)

[paulmiller, 2015-06-23 16:20:31]

I'm now told this can wait until m45, which is good, since it's not working...

[paulmiller, 2015-06-25 00:09:20]

This seems to work. I'm going to be out-of-office for a while, but I'll need to
trouble you for another review when I get back.

[hajimehoshi, 2015-06-25 02:22:19]

On 2015/06/25 00:09:20, paulmiller wrote:
> This seems to work. I'm going to be out-of-office for a while, but I'll need
to
> trouble you for another review when I get back.

No problem.

[Miguel Garcia, 2015-06-25 07:10:26]

miguelg@chromium.org changed reviewers:
+ miguelg@chromium.org

[Miguel Garcia, 2015-06-25 07:10:26]

https://chromiumcodereview.appspot.com/1185703007/diff/40001/components/trans...
File components/translate/core/browser/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/1185703007/diff/40001/components/trans...
components/translate/core/browser/translate_manager.cc:99: if
(!ignore_missing_key_for_testing_ &&
can you add a flag to enable it manually? the default quota you get is still
very useful for testing purposes.

[paulmiller, 2015-07-06 20:31:54]

To what default quota are you referring? Without an API key, the translate API
doesn't work at all. When would such a flag be useful?

[paulmiller, 2015-07-07 23:49:10]

I tested patch set 3 a bit more and it works. Hajime, PTAL again.

[hajimehoshi, 2015-07-08 04:41:35]

https://codereview.chromium.org/1185703007/diff/40001/components/translate/co...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/1185703007/diff/40001/components/translate/co...
components/translate/core/browser/translate_manager.h:102: static void
SetIgnoreMissingKeyForTesting(bool ignore);
Add a comment

[paulmiller, 2015-07-08 17:50:19]

done (translate_manager.cc changes are from rebase)

[Miguel Garcia, 2015-07-08 17:52:23]

On 2015/07/06 20:31:54, paulmiller wrote:
> To what default quota are you referring? Without an API key, the translate API
> doesn't work at all. When would such a flag be useful?

Well my build of Chromium (desktop) does work out of the box for several times.
So does clank when I compile it from source. I think we get developer keys as
part of compiling it manually

[paulmiller, 2015-07-08 17:56:03]

This change will not disable translate whenever the key is present. So if you
ever build in a way that includes the key, translate will not be disabled. Which
is what you want, right?

[Miguel Garcia, 2015-07-08 17:56:41]

On 2015/07/08 17:56:03, paulmiller wrote:
> This change will not disable translate whenever the key is present. So if you
> ever build in a way that includes the key, translate will not be disabled.
Which
> is what you want, right?

ah understood. Great then

[Miguel Garcia, 2015-07-08 17:57:02]

lgtm

[paulmiller, 2015-07-10 00:43:11]

Is this acceptable, Hajime? We're now aiming for m45 so time is once again
limited.

[hajimehoshi, 2015-07-10 02:24:42]

lgtm

[paulmiller, 2015-07-10 18:06:46]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2015-07-10 18:08:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185703007/60001

[commit-bot: I haz the power, 2015-07-10 19:17:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 19:17:46]

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_...)

[paulmiller, 2015-07-10 21:24:30]

Bah. It seems linux_chromium_chromeos_rel_ng is flaky and mac_chromium_rel_ng is
an unrelated build break.

[paulmiller, 2015-07-10 21:24:48]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2015-07-10 21:26:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185703007/60001

[commit-bot: I haz the power, 2015-07-10 22:43:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 22:43:06]

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_...)

[Takashi Toyoshima, 2015-07-14 11:53:31]

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 fails only on mac.

[Takashi Toyoshima, 2015-07-14 12:06:21]

toyoshim@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-07-14 12:06:22]

The test fails due to timeout. Can you check the following point?

https://codereview.chromium.org/1185703007/diff/60001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/1185703007/diff/60001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:67: void
TranslateService::InitializeForTesting() {
This function is not called for browser tests. So you should call
translate::TranslateManager::SetIgnoreMissingKeyForTesting(true) from
chrome/browser/translate/translate_browsertest.cc.

[Takashi Toyoshima, 2015-07-14 12:10:51]

Ah, can you remove "for Android" from the CL subject, too?
The title is confusing since this change should affect all platforms.

[paulmiller, 2015-07-15 00:43:24]

did all that, thanks. Now I have a different failure...

[Takashi Toyoshima, 2015-07-15 04:04:11]

Hum.... I have no idea this time.

[hajimehoshi, 2015-07-15 05:14:17]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-15 05:14:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185703007/80001

[hajimehoshi, 2015-07-15 05:14:35]

The CQ bit was unchecked by hajimehoshi@chromium.org

[paulmiller, 2015-07-15 20:43:36]

paulmiller@chromium.org changed reviewers:
+ estade@chromium.org

[paulmiller, 2015-07-15 20:44:57]

Okay, then. I re-ran linux_chromium_rel_ng and it passed. So I'm going to say
that's not my fault. linux_chromium_compile_dbg_32_ng looks like an unrelated
build break. adding estade from autofill/OWNERS to review
autofill_interactive_uitest.cc. PTAL.

[Evan Stade, 2015-07-15 22:02:38]

autofill_interactive_uitest.cc rslgtm

[paulmiller, 2015-07-15 22:19:07]

On 2015/07/15 22:02:38, Evan Stade wrote:
> autofill_interactive_uitest.cc rslgtm

codereview didn't seem to recognize that ^ (at least, it's not turning green).
Do we need a green comment for presubmit to pass?

[Evan Stade, 2015-07-15 23:34:20]

On 2015/07/15 22:19:07, paulmiller wrote:
> On 2015/07/15 22:02:38, Evan Stade wrote:
> > autofill_interactive_uitest.cc rslgtm
> 
> codereview didn't seem to recognize that ^ (at least, it's not turning green).
> Do we need a green comment for presubmit to pass?

sorry, lgtm

[paulmiller, 2015-07-16 17:30:54]

We're going to instruct people to build ChromePublic from the LKGR, rather than
m45. So the pressure to merge to m45 is off. waiting for review from Takashi or
Hajime.

[Takashi Toyoshima, 2015-07-17 04:26:57]

Ah, sorry. I forgot to say. LGTM.

[hajimehoshi, 2015-07-17 04:28:17]

lgtm

[paulmiller, 2015-07-17 17:13:59]

may our tree be healthy and may our trybots be not flaky
http://www.evilenglish.net/wp-content/uploads/2015/01/20090916_fingers_crosse...

[paulmiller, 2015-07-17 17:14:10]

The CQ bit was checked by paulmiller@chromium.org

[paulmiller, 2015-07-17 17:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/1185703007/#ps80001
(title: "fix tests")

[commit-bot: I haz the power, 2015-07-17 17:15:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185703007/80001

[commit-bot: I haz the power, 2015-07-17 18:39:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-17 18:42:49]

Patchset 5 (id:??) landed as
https://crrev.com/8788f3c1260e54476d13d29acc02cf1412bb73f0
Cr-Commit-Position: refs/heads/master@{#339292}