Translate page in the new UI.

Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

Demo record:
https://googleo.users.x20web.corp.google.com/chrome/videos/transdemo1.mp4


BUG=703887
Review-Url: https://codereview.chromium.org/2788343002
Cr-Commit-Position: refs/heads/master@{#462780}
Committed: https://chromium.googlesource.com/chromium/src/+/92d27790b430374991505d62025876708e54b6ba

[Leo, 2017-04-03 07:33:33]

Description was changed from

==========
Translate page in the new UI.

This CL implements the basic functions of new translate UI.

BUG=703887
==========

to

==========
Translate page in the new UI.
This CL implements the basic functions of new translate UI.

1, Add TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page tranlated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

BUG=703887
==========

[Leo, 2017-04-03 07:37:38]

Description was changed from

==========
Translate page in the new UI.
This CL implements the basic functions of new translate UI.

1, Add TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page tranlated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

BUG=703887
==========

to

==========
Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a
protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

BUG=703887
==========

[Leo, 2017-04-03 07:37:39]

googleo@chromium.org changed reviewers:
+ dfalcantara@chromium.org, groby@chromium.org

[Leo, 2017-04-03 07:49:51]

Description was changed from

==========
Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a
protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

BUG=703887
==========

to

==========
Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a
protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

Demo record:
https://googleo.users.x20web.corp.google.com/chrome/videos/transdemo1.mp4


BUG=703887
==========

[Leo, 2017-04-03 07:51:55]

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

[commit-bot: I haz the power, 2017-04-03 07:52:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 08:31:59]

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

[commit-bot: I haz the power, 2017-04-03 08:32:00]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-04 18:44:38]

Looking good... are you at the point where you should be considering some tests?

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/infobar_translate_compact_content.xml
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_translate_compact_content.xml:35:
android:src="@drawable/btn_menu" />
I think this thing needs to be tinted gray like the close button is.  See how we
use TintedImageButton in empty_background_view_tablet.xml.  I'm not sure exactly
what tint to use though... Try "@color/light_mode_tint", maybe?

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:103: <!-- Dimensions for compcat
translate infobar. -->
compact*

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:24:
private final TranslateOptions mOptions;
nit: private final above private fields

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:44:
sourceLanguageCode, targetLanguageCode, languageList, false, false);
Move this copy and the original in TranslateInfoBar into TranslateUtils.java

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:67:
private class TranslateCallback implements TabLayout.OnTabSelectedListener {
1) Try not to sandwich classes in between functions; prefer to keep them at the
top of the class with other ones so that they're easy to find later.

2) This thing should be final since it's not being subclassed.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:68:
private final TranslateTabLayout mTabs;
mTabLayout; mTabs sounds like a group of tabs that don't necessarily show up in
the layout.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:87:
} else {
There was an onInterceptTouchEvent here somewhere right?  Something that
prevented two translations from colliding?

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:104:
public void hidePresentProgressBar() {
What's a "present progress bar"?

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:105:
if (mTabShowingProgressBar != null) {
Save a level of indentation by early returning.

if (mTabShowingProgressBar == null) return;

[gone, 2017-04-04 18:49:19]

Also: you need to hit "publish+mail comments" or I don't get emails about it.  I
only found this because I was diving through my review queue.

[Leo, 2017-04-05 08:37:47]

Thanks for the review. Sorry for missing the "Publish+Mail Comments".

I did some code refactoring to make it clearer. Will consider the tests after
researching how a proper test works in the Chrome.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/infobar_translate_compact_content.xml
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_translate_compact_content.xml:35:
android:src="@drawable/btn_menu" />
On 2017/04/04 18:44:37, dfalcantara (load balance plz) wrote:
> I think this thing needs to be tinted gray like the close button is.  See how
we
> use TintedImageButton in empty_background_view_tablet.xml.  I'm not sure
exactly
> what tint to use though... Try "@color/light_mode_tint", maybe?

Thanks for the tips. I will try it in a separate CL. Seems need to add a custom
UI component for this button.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:103: <!-- Dimensions for compcat
translate infobar. -->
On 2017/04/04 18:44:37, dfalcantara (load balance plz) wrote:
> compact*

Acknowledged.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:24:
private final TranslateOptions mOptions;
On 2017/04/04 18:44:37, dfalcantara (load balance plz) wrote:
> nit: private final above private fields

Done.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:44:
sourceLanguageCode, targetLanguageCode, languageList, false, false);
On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> Move this copy and the original in TranslateInfoBar into TranslateUtils.java

Some thoughts for the code refactoring.

1, The code above is building a TranslateOptions. How about moving it to a
static build method in TranslateOptions. This one has been done.

2, If we found there are other methods/logic shared by TranslateInfo and
TranslateCompactInfobar later. I will create a base class for them and abstract
same methods/logic.

WDYT?

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:67:
private class TranslateCallback implements TabLayout.OnTabSelectedListener {
On 2017/04/04 18:44:37, dfalcantara (load balance plz) wrote:
> 1) Try not to sandwich classes in between functions; prefer to keep them at
the
> top of the class with other ones so that they're easy to find later.
> 
> 2) This thing should be final since it's not being subclassed.

Thanks for the tips. After the code-refactor TranslateCompactInfoBar implements
TabSelectedListener directly, which makes the logic much clearer.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:68:
private final TranslateTabLayout mTabs;
On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> mTabLayout; mTabs sounds like a group of tabs that don't necessarily show up
in
> the layout.

Done.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:87:
} else {
On 2017/04/04 18:44:37, dfalcantara (load balance plz) wrote:
> There was an onInterceptTouchEvent here somewhere right?  Something that
> prevented two translations from colliding?

Yes, once a tab was selected, all the TouchEvents on the tabs will not
propagate. Implementation is in onInterceptTouchEvent of
translate/TranslateTabLayout.java

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:104:
public void hidePresentProgressBar() {
On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> What's a "present progress bar"?

Removed "present"

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:105:
if (mTabShowingProgressBar != null) {
On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> Save a level of indentation by early returning.
> 
> if (mTabShowingProgressBar == null) return;

Acknowledged.

[gone, 2017-04-05 18:32:36]

lgtm then

A very basic test could just be to make sure that the infobar pops up, but the
problem with that is that you'd need to mock out the backend to make the infobar
pop up.  Our bots don't allow devices to connect to Wi-fi (which would result in
flaky tests anyway), and I don't know how the backend can be mocked out.

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:44:
sourceLanguageCode, targetLanguageCode, languageList, false, false);
On 2017/04/05 08:37:47, Leo wrote:
> On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> > Move this copy and the original in TranslateInfoBar into TranslateUtils.java
> 
> Some thoughts for the code refactoring.
> 
> 1, The code above is building a TranslateOptions. How about moving it to a
> static build method in TranslateOptions. This one has been done.
> 
> 2, If we found there are other methods/logic shared by TranslateInfo and
> TranslateCompactInfobar later. I will create a base class for them and
abstract
> same methods/logic.
> 
> WDYT?

Yeah, sounds fine.

[groby-ooo-7-16, 2017-04-05 21:25:25]

On 2017/04/05 18:32:36, dfalcantara (load balance plz) wrote:
> lgtm then
> 
> A very basic test could just be to make sure that the infobar pops up, but the
> problem with that is that you'd need to mock out the backend to make the
infobar
> pop up.  Our bots don't allow devices to connect to Wi-fi (which would result
in
> flaky tests anyway), and I don't know how the backend can be mocked out.

We do have the ability to initialize the TranslateService for testing purposes
and simulate URL fetches - see translate_manager_render_view_host_unittest.cc
for details.

> 
>
https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
> (right):
> 
>
https://codereview.chromium.org/2788343002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:44:
> sourceLanguageCode, targetLanguageCode, languageList, false, false);
> On 2017/04/05 08:37:47, Leo wrote:
> > On 2017/04/04 18:44:38, dfalcantara (load balance plz) wrote:
> > > Move this copy and the original in TranslateInfoBar into
TranslateUtils.java
> > 
> > Some thoughts for the code refactoring.
> > 
> > 1, The code above is building a TranslateOptions. How about moving it to a
> > static build method in TranslateOptions. This one has been done.
> > 
> > 2, If we found there are other methods/logic shared by TranslateInfo and
> > TranslateCompactInfobar later. I will create a base class for them and
> abstract
> > same methods/logic.
> > 
> > WDYT?
> 
> Yeah, sounds fine.

[groby-ooo-7-16, 2017-04-05 21:26:16]

Also, components/translate LGTM - but please add a unit test.

[Leo, 2017-04-06 00:26:54]

Thanks fore the review and approval. I will fix the unit test then submit.

[Leo, 2017-04-07 04:52:24]

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

[commit-bot: I haz the power, 2017-04-07 04:52:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 05:33:37]

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

[commit-bot: I haz the power, 2017-04-07 05:33:38]

Dry run: This issue passed the CQ dry run.

[Leo, 2017-04-07 05:45:47]

The CQ bit was checked by googleo@chromium.org

[Leo, 2017-04-07 05:45:48]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2788343002/#ps60001
(title: "compact unit test")

[commit-bot: I haz the power, 2017-04-07 05:46:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 05:50:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491543947782940,
"parent_rev": "3387639806215545d562fa99b93054a6754779b5", "commit_rev":
"92d27790b430374991505d62025876708e54b6ba"}

[commit-bot: I haz the power, 2017-04-07 05:51:09]

Description was changed from

==========
Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a
protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

Demo record:
https://googleo.users.x20web.corp.google.com/chrome/videos/transdemo1.mp4


BUG=703887
==========

to

==========
Translate page in the new UI.
This CL implements the basic functions of the new translate infobar under a
protection of the experiment flag.

1, Make TranslateCompactInfoBar as an observer for the ContentTranslateDriver to
response page translated event.
2, Stop translate UI reloading before translate infobar was closed explicitly.
3, Polish UI.

Demo record:
https://googleo.users.x20web.corp.google.com/chrome/videos/transdemo1.mp4


BUG=703887

Review-Url: https://codereview.chromium.org/2788343002
Cr-Commit-Position: refs/heads/master@{#462780}
Committed:
https://chromium.googlesource.com/chromium/src/+/92d27790b430374991505d620258...
==========

[commit-bot: I haz the power, 2017-04-07 05:51:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/92d27790b430374991505d620258...