[Hotword] Sync Audio History pref every 24 hours, when opening chrome://settings and . . .

[Hotword] Sync Audio History pref every 24 hours, when opening
chrome://settings and upon HotwordService creation.

Note: This also changes HotwordAudioHistoryHandler to maintain the
previous state of the pref if a successful connection could not be made
with the server. A separate CL will be created for reporting an error in
connection to the user.

Adds a new function to browser_options_handler to be used as a callback
to the call GetAudioHistoryEnabled.

Adds a task_runner to HotwordAudioHistoryHandler to enable testing.
Test added: HotwordServiceTest.AudioHistorySyncOccurs.

Changes to hotword_private_apitest reflect the fact that
hotword_audio_history_handler now takes a task_runner and that the
previous state of the pref is maintained.

Issue 436683 is included in this because the times which we sync (every
24 hrs, profile startup, opening chrome://settings) are considered
sufficient enough to maintain state and close enough to launching for
the purpose of opening the audio_verification_app.

BUG=438451, 436683
Committed: https://crrev.com/7c2b7812d442c5bb205fca74353d5f7f4a69bc0d
Cr-Commit-Position: refs/heads/master@{#308277}

[rpetterson, 2014-12-11 23:02:07]

rlp@chromium.org changed reviewers:
+ amistry@chromium.org, dbeam@chromium.org

[rpetterson, 2014-12-11 23:02:28]

rlp@chromium.org changed reviewers:
+ amistry@chromium.org, dbeam@chromium.org

[rpetterson, 2014-12-12 00:00:02]

amistry: all hotword files: */hotword*

dbeam: browser_options_handler.{cc,h}

[rpetterson, 2014-12-12 00:00:28]

rlp@chromium.org changed reviewers:
+ amistry@chromium.org, dbeam@chromium.org

[rpetterson, 2014-12-12 00:01:26]

Sending out to reviewers one more time since there were some hiccups with
codereview.

On 2014/12/12 00:00:02, rpetterson wrote:
> amistry: all hotword files: */hotword*
> 
> dbeam: browser_options_handler.{cc,h}

[Dan Beam, 2014-12-12 20:04:20]

lgtm

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680: if (success &&
logging_enabled) {
if !success or !logging_enabled should this section be hidden?

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1723: if
(hotword_service)
curlies

[rpetterson, 2014-12-12 20:19:36]

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680: if (success &&
logging_enabled) {
On 2014/12/12 20:04:20, Dan Beam wrote:
> if !success or !logging_enabled should this section be hidden?

Ideally, if !success we would show an error. I actually have a CL for that ready
to go once I get the final wording on the error from UX.

if (!logging_enabled && success), then we should not show it. Would you
recommend passing that into the function to ensure it stays hidden?

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1723: if
(hotword_service)
On 2014/12/12 20:04:20, Dan Beam wrote:
> curlies

Done.

[Dan Beam, 2014-12-12 20:25:46]

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680: if (success &&
logging_enabled) {
On 2014/12/12 20:19:36, rpetterson wrote:
> On 2014/12/12 20:04:20, Dan Beam wrote:
> > if !success or !logging_enabled should this section be hidden?
> 
> Ideally, if !success we would show an error. I actually have a CL for that
ready
> to go once I get the final wording on the error from UX.
> 
> if (!logging_enabled && success), then we should not show it. Would you
> recommend passing that into the function to ensure it stays hidden?

i generally ask this type of question whenever I see a show* or hide* function
without a balanced counterpart (e.g. a show without a hide, etc.).

often we optimize for the common case but forget error/uncommon cases (another
reason why I like setVisible instead of show/hide -- because you are likely to
need both if you're "Doing It Right").

[rpetterson, 2014-12-12 21:39:56]

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/800523002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680: if (success &&
logging_enabled) {
On 2014/12/12 20:25:46, Dan Beam wrote:
> On 2014/12/12 20:19:36, rpetterson wrote:
> > On 2014/12/12 20:04:20, Dan Beam wrote:
> > > if !success or !logging_enabled should this section be hidden?
> > 
> > Ideally, if !success we would show an error. I actually have a CL for that
> ready
> > to go once I get the final wording on the error from UX.
> > 
> > if (!logging_enabled && success), then we should not show it. Would you
> > recommend passing that into the function to ensure it stays hidden?
> 
> i generally ask this type of question whenever I see a show* or hide* function
> without a balanced counterpart (e.g. a show without a hide, etc.).
> 
> often we optimize for the common case but forget error/uncommon cases (another
> reason why I like setVisible instead of show/hide -- because you are likely to
> need both if you're "Doing It Right").

Since I prefer doing things the right way, I've updated the audio history
functions (in browser_options_handler and browser_options) to all be
SetAudioHistorySectionVisible.

[Dan Beam, 2014-12-12 22:08:31]

lgtm++

https://codereview.chromium.org/800523002/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/800523002/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1234: * @param {boolean}
visible Whether the audio history section is visible.
nit: I'd arguably make this param first

[rpetterson, 2014-12-12 22:22:28]

https://codereview.chromium.org/800523002/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/800523002/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1234: * @param {boolean}
visible Whether the audio history section is visible.
On 2014/12/12 22:08:31, Dan Beam wrote:
> nit: I'd arguably make this param first

Done.

[Anand Mistry (off Chromium), 2014-12-13 00:29:49]

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
File chrome/browser/search/hotword_audio_history_handler.cc (right):

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:49: // If the call was
successful, set the last_successful time stamp and
What last_successful time stamp?

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:102: // If the setting is
now turned off, always on shoudl also be turned off.
s/shoudl/should

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
File chrome/browser/search/hotword_audio_history_handler.h (right):

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.h:8: #include
"base/memory/scoped_ptr.h"
I think you mean "base/memory/ref_counted.h"

[rpetterson, 2014-12-13 01:12:02]

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
File chrome/browser/search/hotword_audio_history_handler.cc (right):

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:49: // If the call was
successful, set the last_successful time stamp and
On 2014/12/13 00:29:49, Anand Mistry wrote:
> What last_successful time stamp?

Removed. Left over comment from when I was using a clock. Removed since now the
behavior is obvious.

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:102: // If the setting is
now turned off, always on shoudl also be turned off.
On 2014/12/13 00:29:49, Anand Mistry wrote:
> s/shoudl/should

Done.

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
File chrome/browser/search/hotword_audio_history_handler.h (right):

https://codereview.chromium.org/800523002/diff/180001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.h:8: #include
"base/memory/scoped_ptr.h"
On 2014/12/13 00:29:49, Anand Mistry wrote:
> I think you mean "base/memory/ref_counted.h"

Done.

[Anand Mistry (off Chromium), 2014-12-13 01:44:42]

lgtm

[rpetterson, 2014-12-13 16:05:54]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-12-13 16:06:51]

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

[commit-bot: I haz the power, 2014-12-13 17:02:25]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-12-13 17:03:12]

Patchset 12 (id:??) landed as
https://crrev.com/7c2b7812d442c5bb205fca74353d5f7f4a69bc0d
Cr-Commit-Position: refs/heads/master@{#308277}