[ios] Adds configuration helper files for voice search.

[ios] Adds configuration helper files for voice search.

Also adds a provider API in ChromeBrowserProvider to return the list of
available voice search languages.

BUG=656617
TEST=No visible impact.  Voice search languages should continue to appear in Settings.

Committed: https://crrev.com/976869e5ad80f517cc8d7515aec8c7916275d745
Cr-Commit-Position: refs/heads/master@{#425760}

[rohitrao (ping after 24h), 2016-10-17 13:50:14]

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

[commit-bot: I haz the power, 2016-10-17 13:50:23]

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, 2016-10-17 14:19:14]

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

[commit-bot: I haz the power, 2016-10-17 14:19:15]

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

[rohitrao (ping after 24h), 2016-10-17 14:55:44]

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

[commit-bot: I haz the power, 2016-10-17 14:56:00]

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

[rohitrao (ping after 24h), 2016-10-17 15:12:16]

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

[commit-bot: I haz the power, 2016-10-17 15:12:39]

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

[rohitrao (ping after 24h), 2016-10-17 15:12:58]

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

[sdefresne, 2016-10-17 16:12:40]

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_config.h (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:25: virtual
SpeechInputLocale GetDefaultLocale() = 0;
Can this method be const?

virtual SpeechInputLocale GetDefaultLocale() const = 0;

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:29: virtual const
std::vector<SpeechInputLocale>& GetAvailableLocales() = 0;
ditto, can this method be const?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:35: virtual
SpeechInputLocale GetLocaleForCode(
ditto, can this method be const?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:40: virtual const
std::vector<std::string>& GetTextToSpeechLanguages() = 0;
ditto, can this method be const?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:44: virtual bool
IsTextToSpeechEnabledForCode(const std::string& locale_code) = 0;
ditto, can this method be const?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:47:
SpeechInputLocaleConfig();
Can you use "= default" here and for the destructor?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:49: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:24: if
(tokens.size())
Can you change this to use tokens.empty()?

  if (tokens.empty())
    return std::string();
  return tokens[0];

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:44:
SpeechInputLocaleConfig* SpeechInputLocaleConfig::GetInstance() {
I really dislike having the GetInstance in the subclass implementation file, can
you move it to speech_input_locale_config.cc (renaming the file to
speech_input_locale_input_config.mm I guess).

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:70:
SpeechInputLocale SpeechInputLocaleConfigImpl::GetLocaleForCode(
This method implementation does not match the comment in the super class.

   // Returns the SpeechInputLocale to use for |locale_code|. If |locale_code|
is
   // not contained in GetAvailableLocales()'s return value, then the
   // SpeechInputLocaleConfig will attempt to match with an appropriate
subsitute
   // (e.g. "en-NZ" => "en-AU").
   virtual SpeechInputLocale GetLocaleForCode(
       const std::string& locale_code) = 0;
 
It looks like the method that has the implementation described in the comment is
GetMatchingLocale().

Should GetLocaleForCode be changed to have the same implementation as
GetMatchingLocale() (and then the code migrated to call the method from the
public interface instead of the better method from the implementation class).

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:195: }
nit: blank line before closing brace of the namespace and // namespace voice
comment

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm:18: }
nit: blank line before closing the namespace and then // namespace comment

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm:57:
_matches.reset([[NSArray alloc] initWithArray:matches]);
nit: would be better to use -copy here

  _matches.reset([matches copy]);

https://codereview.chromium.org/2425713002/diff/40001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/voice/voice_search_language.h (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/public/provider/chr...
ios/public/provider/chrome/browser/voice/voice_search_language.h:10: @interface
VoiceSearchLanguage : NSObject
nit: can you add a initializer that initializes the properties

- (instancetype)initWithIdentifier:(NSString*)identifier
                       displayName:(NSString*)displayName
            localizationPreference:(NSString*)localizationPreference
    NS_DESIGNATED_INITIALIZER;

[commit-bot: I haz the power, 2016-10-17 16:21:11]

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

[commit-bot: I haz the power, 2016-10-17 16:21:12]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2016-10-17 17:42:01]

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

[rohitrao (ping after 24h), 2016-10-17 17:42:27]

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_config.h (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:25: virtual
SpeechInputLocale GetDefaultLocale() = 0;
On 2016/10/17 16:12:39, sdefresne wrote:
> Can this method be const?
> 
> virtual SpeechInputLocale GetDefaultLocale() const = 0;

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:29: virtual const
std::vector<SpeechInputLocale>& GetAvailableLocales() = 0;
On 2016/10/17 16:12:39, sdefresne wrote:
> ditto, can this method be const?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:35: virtual
SpeechInputLocale GetLocaleForCode(
On 2016/10/17 16:12:39, sdefresne wrote:
> ditto, can this method be const?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:40: virtual const
std::vector<std::string>& GetTextToSpeechLanguages() = 0;
On 2016/10/17 16:12:39, sdefresne wrote:
> ditto, can this method be const?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:44: virtual bool
IsTextToSpeechEnabledForCode(const std::string& locale_code) = 0;
On 2016/10/17 16:12:39, sdefresne wrote:
> ditto, can this method be const?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:47:
SpeechInputLocaleConfig();
On 2016/10/17 16:12:39, sdefresne wrote:
> Can you use "= default" here and for the destructor?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config.h:49: };
On 2016/10/17 16:12:39, sdefresne wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:24: if
(tokens.size())
On 2016/10/17 16:12:39, sdefresne wrote:
> Can you change this to use tokens.empty()?
> 
>   if (tokens.empty())
>     return std::string();
>   return tokens[0];

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:44:
SpeechInputLocaleConfig* SpeechInputLocaleConfig::GetInstance() {
On 2016/10/17 16:12:40, sdefresne wrote:
> I really dislike having the GetInstance in the subclass implementation file,
can
> you move it to speech_input_locale_config.cc (renaming the file to
> speech_input_locale_input_config.mm I guess).

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:70:
SpeechInputLocale SpeechInputLocaleConfigImpl::GetLocaleForCode(
On 2016/10/17 16:12:40, sdefresne wrote:
> This method implementation does not match the comment in the super class.
> 
>    // Returns the SpeechInputLocale to use for |locale_code|. If |locale_code|
> is
>    // not contained in GetAvailableLocales()'s return value, then the
>    // SpeechInputLocaleConfig will attempt to match with an appropriate
> subsitute
>    // (e.g. "en-NZ" => "en-AU").
>    virtual SpeechInputLocale GetLocaleForCode(
>        const std::string& locale_code) = 0;
>  
> It looks like the method that has the implementation described in the comment
is
> GetMatchingLocale().
> 
> Should GetLocaleForCode be changed to have the same implementation as
> GetMatchingLocale() (and then the code migrated to call the method from the
> public interface instead of the better method from the implementation class).

Will follow up with Kurt on this.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_config_impl.mm:195: }
On 2016/10/17 16:12:39, sdefresne wrote:
> nit: blank line before closing brace of the namespace and // namespace voice
> comment

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm (right):

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm:18: }
On 2016/10/17 16:12:40, sdefresne wrote:
> nit: blank line before closing the namespace and then // namespace comment

Done.

https://codereview.chromium.org/2425713002/diff/40001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/speech_input_locale_match_config.mm:57:
_matches.reset([[NSArray alloc] initWithArray:matches]);
On 2016/10/17 16:12:40, sdefresne wrote:
> nit: would be better to use -copy here
> 
>   _matches.reset([matches copy]);

Done.

[commit-bot: I haz the power, 2016-10-17 17:42:29]

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, 2016-10-17 18:36:35]

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

[commit-bot: I haz the power, 2016-10-17 18:36:37]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2016-10-17 19:02:07]

lgtm

[rohitrao (ping after 24h), 2016-10-17 19:19:42]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2016-10-17 19:20:21]

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, 2016-10-17 20:06:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-17 20:09:13]

Description was changed from

==========
[ios] Adds configuration helper files for voice search.

Also adds a provider API in ChromeBrowserProvider to return the list of
available voice search languages.

BUG=656617
TEST=No visible impact.  Voice search languages should continue to appear in
Settings.
==========

to

==========
[ios] Adds configuration helper files for voice search.

Also adds a provider API in ChromeBrowserProvider to return the list of
available voice search languages.

BUG=656617
TEST=No visible impact.  Voice search languages should continue to appear in
Settings.

Committed: https://crrev.com/976869e5ad80f517cc8d7515aec8c7916275d745
Cr-Commit-Position: refs/heads/master@{#425760}
==========

[commit-bot: I haz the power, 2016-10-17 20:09:14]

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