Upstream //ios/chrome/browser/translate

Upstream //ios/chrome/browser/translate

This CL upstreams the iOS translate client code, and the iOS resources
infrastructure.

TBR=asvitkine

Committed: https://crrev.com/4b2901c16880fbab6d20beefd8a07e2633340dd8
Cr-Commit-Position: refs/heads/master@{#310513}

[droger, 2015-01-07 17:31:37]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[droger, 2015-01-07 17:31:38]

This is a straight upstreaming + formatting.

[sdefresne, 2015-01-08 09:22:34]

Code is missing some files as it does not compile on iOS.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar.mm (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar.mm:81: case TranslateInfoBarIos::AFTER_DONE:
nit: TranslateInfoBarIOS

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/after_translate_infobar_controller.mm (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/after_translate_infobar_controller.mm:70:
[[NSString alloc] initWithFormat:@"%@ %@ %@%@ %@.", label1, original,
nit: is this concatenating translated strings? if so, can you create a bug to
change this as concatenating translated strings may not work in all languages.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/before_translate_infobar_controller.mm
(right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/before_translate_infobar_controller.mm:194: //
TODO(translate): Update the position of the infobar.
nit: is this still relevant? is there a tracking bug? if so, please update the
comment.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/before_translate_infobar_controller.mm:220:
languagePicker_.reset();
I'm afraid that this may lead to blockLanguagePicker pointing to a dangling
object if languagePicker_.reset() remove the last reference to the object.
Shouldn't blockLanguagePicker and blockNavigationBar be scoped_nsobject<>?

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/before_translate_infobar_controller.mm:227:
[languageSelectionView_ removeFromSuperview];
animations block goes to great length to avoid keeping a reference to self, but
this block does capture self. Shouldn't this be changed to also use a
blockLanguageSelectionView local variable?

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_infobar_tags.h (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_infobar_tags.h:10: enum Tag {
You can use enum class here instead of using a namespace:

  enum class TranslateInfoBarIOSTag {
  };

then it can be used as:

  TranslateInfoBarIOSTag::AFTER_DONE;

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_message_infobar_controller.mm
(right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_message_infobar_controller.mm:48: //
TODO: Check if we should remove the close button.
nit: same remark about the TODO.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_service_ios.cc (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_service_ios.cc:68: // - an internal URL
(chrome:// and others)
nit: the comment does not agree with the code (the "and others" part is
incorrect)

[droger, 2015-01-08 09:35:16]

Patchset #8 (id:140001) has been deleted

[droger, 2015-01-08 09:35:30]

Patchset #8 (id:140001) has been deleted

[droger, 2015-01-08 10:26:10]

Patchset #8 (id:160001) has been deleted

[droger, 2015-01-08 10:26:22]

Patchset #8 (id:180001) has been deleted

[droger, 2015-01-08 10:49:26]

Patchset #8 (id:200001) has been deleted

[droger, 2015-01-08 10:57:26]

Patchset #8 (id:220001) has been deleted

[droger, 2015-01-08 11:41:17]

Patchset #8 (id:240001) has been deleted

[droger, 2015-01-08 11:57:33]

Thanks for the review. I addressed most of the comments.
The 2 I did not address are the string concatenation and one block copy, see
below.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/after_translate_infobar_controller.mm (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/after_translate_infobar_controller.mm:70:
[[NSString alloc] initWithFormat:@"%@ %@ %@%@ %@.", label1, original,
On 2015/01/08 09:22:33, sdefresne wrote:
> nit: is this concatenating translated strings? if so, can you create a bug to
> change this as concatenating translated strings may not work in all languages.

I think this is similar to what is done upstream here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

They don't concatenate the strings but layout the labels directly on the view
which is similar.

I tried in debug and the strings are:
label1: "This page has been translated from"
original: "French"
label2: "to"
label3: ""
target: "English"

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/before_translate_infobar_controller.mm
(right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/before_translate_infobar_controller.mm:220:
languagePicker_.reset();
On 2015/01/08 09:22:33, sdefresne wrote:
> I'm afraid that this may lead to blockLanguagePicker pointing to a dangling
> object if languagePicker_.reset() remove the last reference to the object.
> Shouldn't blockLanguagePicker and blockNavigationBar be scoped_nsobject<>?

Right.
I think it's not a problem in practice because the views are displayed, but it
is nonetheless a good point.
I moved the reset() a bit further down.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/before_translate_infobar_controller.mm:227:
[languageSelectionView_ removeFromSuperview];
On 2015/01/08 09:22:33, sdefresne wrote:
> animations block goes to great length to avoid keeping a reference to self,
but
> this block does capture self. Shouldn't this be changed to also use a
> blockLanguageSelectionView local variable?

I would rather not change this one as part of upstreaming, because it's not
clear when languageSelectionView_.reset() should be called if it is no longer
called in the block.

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_infobar_tags.h (right):

https://codereview.chromium.org/809333003/diff/120001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_infobar_tags.h:10: enum Tag {
On 2015/01/08 09:22:33, sdefresne wrote:
> You can use enum class here instead of using a namespace:
> 
>   enum class TranslateInfoBarIOSTag {
>   };
> 
> then it can be used as:
> 
>   TranslateInfoBarIOSTag::AFTER_DONE;

Unfortunately this does not work, because these tags must be convertible to
NSUInteger, which is against the point of strongly typed enums.

[sdefresne, 2015-01-08 15:29:36]

lgtm

[droger, 2015-01-08 15:51:01]

Translate OWNERS:
I added you as owners of //ios/chrome/browser/translate
"for simple changes".

The "for simple changes" is here because a lot of code there is in Objective-C,
which you may not be familiar with.

If you don't want to be listed here, or if you want to be listed as a main owner
(outside of the "for simple changes" section), just let me know and I'll make a
follow up CL to adjust.

Thanks

[chromium-reviews, 2015-01-08 15:54:37]

SGTM. Thanks! Exciting stuff ahead!

On Thu Jan 08 2015 at 3:51:02 PM <droger@chromium.org> wrote:

> Translate OWNERS:
> I added you as owners of //ios/chrome/browser/translate
> "for simple changes".
>
> The "for simple changes" is here because a lot of code there is in
> Objective-C,
> which you may not be familiar with.
>
> If you don't want to be listed here, or if you want to be listed as a main
> owner
> (outside of the "for simple changes" section), just let me know and I'll
> make a
> follow up CL to adjust.
>
> Thanks
>
> https://codereview.chromium.org/809333003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[droger, 2015-01-08 16:09:48]

droger@chromium.org changed reviewers:
+ asvitkine@chromium.org

[droger, 2015-01-08 16:10:25]

TBR=asvitkine for the new dependency on components/web_resource in
//ios/chrome/DEPS.

[droger, 2015-01-08 16:10:39]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-01-08 16:12:01]

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

[commit-bot: I haz the power, 2015-01-08 16:26:03]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2015-01-08 16:26:42]

Patchset 8 (id:??) landed as
https://crrev.com/4b2901c16880fbab6d20beefd8a07e2633340dd8
Cr-Commit-Position: refs/heads/master@{#310513}

[Alexei Svitkine (slow), 2015-01-08 16:52:14]

lgtm with a couple comments for a follow-up cl

https://codereview.chromium.org/809333003/diff/260001/ios/chrome/browser/tran...
File ios/chrome/browser/translate/translate_service_ios.h (right):

https://codereview.chromium.org/809333003/diff/260001/ios/chrome/browser/tran...
ios/chrome/browser/translate/translate_service_ios.h:35: };
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/809333003/diff/260001/ios/public/test/test_ch...
File ios/public/test/test_chrome_provider_initializer.cc (right):

https://codereview.chromium.org/809333003/diff/260001/ios/public/test/test_ch...
ios/public/test/test_chrome_provider_initializer.cc:20: delete
chrome_browser_provider_;
Nit: Maybe should be a scoped_ptr instead?

[droger, 2015-01-08 18:04:44]

On 2015/01/08 16:52:14, Alexei Svitkine wrote:
> lgtm with a couple comments for a follow-up cl

Thanks. The follow up CL is:
https://codereview.chromium.org/826803003