[ios] Adds support for parsing Text-to-Speech search results.

[ios] Adds support for parsing Text-to-Speech search results.

BUG=656617
Committed: https://crrev.com/5fb238c24cae7ec3e97a09226a26d292f73851dc
Cr-Commit-Position: refs/heads/master@{#427423}

[rohitrao (ping after 24h), 2016-10-24 20:17:29]

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

[commit-bot: I haz the power, 2016-10-24 20:18:37]

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-24 20:20:04]

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

[rohitrao (ping after 24h), 2016-10-24 20:44:13]

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

[commit-bot: I haz the power, 2016-10-24 20:45:27]

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-24 20:55:03]

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

[commit-bot: I haz the power, 2016-10-24 20:55:04]

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-24 21:48:18]

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

[commit-bot: I haz the power, 2016-10-24 21:49:00]

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-24 22:26:02]

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

[commit-bot: I haz the power, 2016-10-24 22:26:03]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[sdefresne, 2016-10-25 13:20:30]

lgtm

The comments can be addressed in followup CLs if you want. Some of them are more
questions than prescription, so just feel free to nack them.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/BUILD.gn (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/BUILD.gn:5: source_set("tts") {
I would call this "voice" or change the directory name to "tts", unless you plan
to also have non "tts" related code in that directory (as there is in
//ios/chrome/browser/voice, but then maybe the code should be moved to
//ios/chrome/browser/tts too).

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/BUILD.gn:20: source_set("tts_unit_tests") {
We usually just call those targets "unit_tests" as the full name will be
//ios/chrome/browser/ui/voice:unit_tests which make it easy to understand what
this is testing.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player.h (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player.h:27: //
|-prepareToPlayAudioData:|.  no-op if called when |readyForPlayback| is NO
Should this be spelled "No-op" as it is starting a sentence?

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player.mm:13: 
Should we convert the file to ARC?

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm:23: @interface
TTSPlayerObserver : NSObject {
nit: move the ivar declaration to @implementation

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener.h (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.h:37:
didReceieveResult:(NSData*)result;
nit: s/didReceieveResult/didReceiveResult/ (or should it be receivedResult?)

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.h:39: // Called by |listener|
when its WebState was destroyed.
nit: "Called by |listener| after its WebState was destroyed." or "Called by
|listener| when its WebState is destroyed."

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:23: @interface
TextToSpeechListener () {
nit: move the ivar declaration to @implementation

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:27:
std::unique_ptr<TextToSpeechWebStateObserver> _webStateObserver;
I think it could be just std::unique_ptr<web::WebStateObserver> as only method
of the sub-class are called, which would allow removing the forward-declaration
of the class and moving it to an anonymous namespace (unless it is friend with
some class).

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:67: GURL url =
web_state()->GetLastCommittedURL();
nit: const GURL& url = ...

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener_unittest.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener_unittest.mm:93: NSData*
expected_audio_data = [encoder decode:@"dGVzdGF1ZG8zMm9pbw=="];
Why not put decoded string in the test?

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_parser.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_parser.mm:33: // Escaped encoding for
GWS Voice Search serice's trailing '='.
s/serice/service/?

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_parser.mm:37: }  // namespace
nit: blank line before closing the namespace or remove the one after opening it

[rohitrao (ping after 24h), 2016-10-25 15:06:40]

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

[commit-bot: I haz the power, 2016-10-25 15:07:07]

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-25 15:23:12]

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/BUILD.gn (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/BUILD.gn:5: source_set("tts") {
On 2016/10/25 13:20:30, sdefresne wrote:
> I would call this "voice" or change the directory name to "tts", unless you
plan
> to also have non "tts" related code in that directory (as there is in
> //ios/chrome/browser/voice, but then maybe the code should be moved to
> //ios/chrome/browser/tts too).

Done.

In general, the "tts" code is going into separate targets to avoid breaking the
downstream build.  But for this specific case, the target should just be named
"voice" to start with.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/BUILD.gn:20: source_set("tts_unit_tests") {
On 2016/10/25 13:20:30, sdefresne wrote:
> We usually just call those targets "unit_tests" as the full name will be
> //ios/chrome/browser/ui/voice:unit_tests which make it easy to understand what
> this is testing.

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player.h (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player.h:27: //
|-prepareToPlayAudioData:|.  no-op if called when |readyForPlayback| is NO
On 2016/10/25 13:20:30, sdefresne wrote:
> Should this be spelled "No-op" as it is starting a sentence?

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player.mm:13: 
On 2016/10/25 13:20:30, sdefresne wrote:
> Should we convert the file to ARC?

I'll leave that to stk to decide =)

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
File ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/ui/v...
ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm:23: @interface
TTSPlayerObserver : NSObject {
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: move the ivar declaration to @implementation

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener.h (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.h:37:
didReceieveResult:(NSData*)result;
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: s/didReceieveResult/didReceiveResult/ (or should it be receivedResult?)

Good catch, done!

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.h:39: // Called by |listener|
when its WebState was destroyed.
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: "Called by |listener| after its WebState was destroyed." or "Called by
> |listener| when its WebState is destroyed."

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:23: @interface
TextToSpeechListener () {
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: move the ivar declaration to @implementation

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:27:
std::unique_ptr<TextToSpeechWebStateObserver> _webStateObserver;
On 2016/10/25 13:20:30, sdefresne wrote:
> I think it could be just std::unique_ptr<web::WebStateObserver> as only method
> of the sub-class are called, which would allow removing the
forward-declaration
> of the class and moving it to an anonymous namespace (unless it is friend with
> some class).

Protected destructor on web::WebStateObserver prevents this.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener.mm:67: GURL url =
web_state()->GetLastCommittedURL();
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: const GURL& url = ...

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_listener_unittest.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_listener_unittest.mm:93: NSData*
expected_audio_data = [encoder decode:@"dGVzdGF1ZG8zMm9pbw=="];
On 2016/10/25 13:20:30, sdefresne wrote:
> Why not put decoded string in the test?

Not sure, will follow up with Kurt.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
File ios/chrome/browser/voice/text_to_speech_parser.mm (right):

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_parser.mm:33: // Escaped encoding for
GWS Voice Search serice's trailing '='.
On 2016/10/25 13:20:30, sdefresne wrote:
> s/serice/service/?

Done.

https://codereview.chromium.org/2449593002/diff/20001/ios/chrome/browser/voic...
ios/chrome/browser/voice/text_to_speech_parser.mm:37: }  // namespace
On 2016/10/25 13:20:30, sdefresne wrote:
> nit: blank line before closing the namespace or remove the one after opening
it

Done.

[commit-bot: I haz the power, 2016-10-25 16:08:49]

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

[commit-bot: I haz the power, 2016-10-25 16:08:49]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2016-10-25 16:25:43]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-10-25 16:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2449593002/#ps40001
(title: "Review.")

[commit-bot: I haz the power, 2016-10-25 16:26:11]

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-25 16:32:37]

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

[commit-bot: I haz the power, 2016-10-25 16:32:38]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sdefresne, 2016-10-25 16:32:44]

lgtm

[rohitrao (ping after 24h), 2016-10-25 16:39:07]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-10-25 16:39:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2449593002/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-10-25 16:39:29]

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-25 17:07:15]

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

[commit-bot: I haz the power, 2016-10-25 17:07:16]

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-25 17:37:10]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-10-25 17:37:10]

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

[commit-bot: I haz the power, 2016-10-25 17:38:03]

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-25 18:42:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 19:09:09]

Description was changed from

==========
[ios] Adds support for parsing Text-to-Speech search results.

BUG=656617
==========

to

==========
[ios] Adds support for parsing Text-to-Speech search results.

BUG=656617

Committed: https://crrev.com/5fb238c24cae7ec3e97a09226a26d292f73851dc
Cr-Commit-Position: refs/heads/master@{#427423}
==========

[commit-bot: I haz the power, 2016-10-25 19:09:10]

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