[Hotword] Implement audio history pref accessing and setting.

[Hotword] Implement audio history pref accessing and setting.

This CL adds new functionality to the web history service to access audio history as well since accessing audio history uses the same underlying mechanism as standard web history.

This CL also removes the onChange observer for the pref so that it is entirely handled by the handler. There is no longer a need for it since the only access to the pref is through this handler (i.e., there is no more settings checkbox).

Note: The completion callbacks in the hotword_audio_history_handler will probably be updated once some additional extension apis have been added.

We may eventually shift to a newer technology but it is appearing more likely it might not be ready when we need it. However, we will continue to use this method for the accessing of the audio history pref for quite some time (it's one of the approved methods) even once the setting of the pref switches to the newer model.

BUG=426640
Committed: https://crrev.com/997840adca15d4f278b2b8ad955aeb8bec663b5e
Cr-Commit-Position: refs/heads/master@{#304994}

[rpetterson, 2014-11-04 21:31:49]

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

[rpetterson, 2014-11-04 21:31:50]

sky - web history
anand - hotword

[rpetterson, 2014-11-05 00:49:22]

Reviewers,

Please hold off reviewing this for now. We may be using the newer stuff sooner
and I won't want to make unnecessary review work for you. I'll ping again if it
turns out this is still needed.

Thanks,
Rachel

[rpetterson, 2014-11-11 20:07:37]

Hi, Reviewers,

Thank you for your patience. It looks like we will need this. I've updated the
CL description, but here it is as well:

This CL adds new functionality to the web history service to access audio
history as well since accessing audio history uses the same underlying mechanism
as standard web history.

This CL also removes the onChange observer for the pref so that it is entirely
handled by the handler. There is no longer a need for it since the only access
to the pref is through this handler (i.e., there is no more settings checkbox).

Note: The completion callbacks in the hotword_audio_history_handler will
probably be updated once some additional extension apis have been added.

We may eventually shift to a newer technology but it is appearing more likely it
might not be ready when we need it. However, we will continue to use this method
for the accessing of the audio history pref for quite some time (it's one of the
approved methods) even once the setting of the pref switches to the newer model.

I look forward to your feedback!

Thanks,
Rachel

On 2014/11/05 00:49:22, rpetterson wrote:
> Reviewers,
> 
> Please hold off reviewing this for now. We may be using the newer stuff sooner
> and I won't want to make unnecessary review work for you. I'll ping again if
it
> turns out this is still needed.
> 
> Thanks,
> Rachel

[sky, 2014-11-11 20:30:10]

How about test coverage?

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:471:
pending_audio_history_requests_.erase(request);
Does this leak request?

[rpetterson, 2014-11-11 22:29:16]

Re: test coverage, I couldn't find any tests for web history. Can you point me
to some? I'm not sure how to structure these such that the tests don't actually
submit requests. Adding hotword tests did not seem useful since it would
essentially just be testing setting a boolean pref because the web history would
need to be very mocked.

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:471:
pending_audio_history_requests_.erase(request);
On 2014/11/11 20:30:10, sky wrote:
> Does this leak request?

No, it should not. We use the pending_audio_history_requests set to track each
request. Each request is added to the pending_audio_history_requests set when
it's sent. When the callback returns, we delete it from that set. In the
destructor, we delete any pending requests that are still in the set. This is
based on the way expiry requests are handled.

[Anand Mistry (off Chromium), 2014-11-12 00:00:11]

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

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:45:
web_history->SetAudioHistoryEnabled(
How is the local PrefService setting supposed to be updated when the user calls
this to update the setting?

[rpetterson, 2014-11-12 00:36:04]

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

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:45:
web_history->SetAudioHistoryEnabled(
On 2014/11/12 00:00:10, Anand Mistry wrote:
> How is the local PrefService setting supposed to be updated when the user
calls
> this to update the setting?

Good point. I suppose we should not leave it up to the caller.

Either it can be done in the callback SetAudioHistoryComplete (which makes that
callback essentially identical to GetAudioHistoryComplete) or we can do it
before we make the call.

In reality, with respect to the logging, it should not make a difference since
we will check before logging if it's legit to do so. So as of right now, the
only place the pref will be used is in chrome://settings.

The former is preferable since we don't update unless we know the set call was
successful. The latter is cleaner IMO.

Do you have a preference?

[sky, 2014-11-12 00:46:32]

No idea on the testing. There should be tests for this code. Maybe you get to be
the first to write one;)

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:471:
pending_audio_history_requests_.erase(request);
On 2014/11/11 22:29:16, rpetterson wrote:
> On 2014/11/11 20:30:10, sky wrote:
> > Does this leak request?
> 
> No, it should not. We use the pending_audio_history_requests set to track each
> request. Each request is added to the pending_audio_history_requests set when
> it's sent. When the callback returns, we delete it from that set. In the
> destructor, we delete any pending requests that are still in the set. This is
> based on the way expiry requests are handled.

Where is the delete? From what I see you erase the request right here, which
removes from pending_audio_history_requests_ but doesn't actually delete it.

[Anand Mistry (off Chromium), 2014-11-12 00:56:55]

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

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:45:
web_history->SetAudioHistoryEnabled(
On 2014/11/12 00:36:04, rpetterson wrote:
> On 2014/11/12 00:00:10, Anand Mistry wrote:
> > How is the local PrefService setting supposed to be updated when the user
> calls
> > this to update the setting?
> 
> Good point. I suppose we should not leave it up to the caller.
> 
> Either it can be done in the callback SetAudioHistoryComplete (which makes
that
> callback essentially identical to GetAudioHistoryComplete) or we can do it
> before we make the call.
> 
> In reality, with respect to the logging, it should not make a difference since
> we will check before logging if it's legit to do so. So as of right now, the
> only place the pref will be used is in chrome://settings.
> 
> The former is preferable since we don't update unless we know the set call was
> successful. The latter is cleaner IMO.
> 
> Do you have a preference?

I think the former (on success), since we should aim to be as close to the
current web pref as possible and a successful response indicates what the true
value is.

Speaking of success, I think something should happen on failure. If the request
to "enable" failed, maybe it should be disabled. If a request to "disable"
failed, then... um... dunno.

[rpetterson, 2014-11-12 00:58:42]

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:471:
pending_audio_history_requests_.erase(request);
On 2014/11/12 00:46:32, sky wrote:
> On 2014/11/11 22:29:16, rpetterson wrote:
> > On 2014/11/11 20:30:10, sky wrote:
> > > Does this leak request?
> > 
> > No, it should not. We use the pending_audio_history_requests set to track
each
> > request. Each request is added to the pending_audio_history_requests set
when
> > it's sent. When the callback returns, we delete it from that set. In the
> > destructor, we delete any pending requests that are still in the set. This
is
> > based on the way expiry requests are handled.
> 
> Where is the delete? From what I see you erase the request right here, which
> removes from pending_audio_history_requests_ but doesn't actually delete it.

Whoops. It's back.

[rpetterson, 2014-11-12 01:04:24]

On 2014/11/12 00:46:32, sky wrote:
> No idea on the testing. There should be tests for this code. Maybe you get to
be
> the first to write one;)
> 
Weren't you the one who originally wrote web_history? :)

In all seriousness though, I should be able to do something with a
MockURLFetcher though it will make this CL a bit messier to allow for passing in
a testing fetcher.

[rpetterson, 2014-11-12 01:15:42]

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

https://codereview.chromium.org/687803004/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_audio_history_handler.cc:45:
web_history->SetAudioHistoryEnabled(
On 2014/11/12 00:56:55, Anand Mistry wrote:
> On 2014/11/12 00:36:04, rpetterson wrote:
> > On 2014/11/12 00:00:10, Anand Mistry wrote:
> > > How is the local PrefService setting supposed to be updated when the user
> > calls
> > > this to update the setting?
> > 
> > Good point. I suppose we should not leave it up to the caller.
> > 
> > Either it can be done in the callback SetAudioHistoryComplete (which makes
> that
> > callback essentially identical to GetAudioHistoryComplete) or we can do it
> > before we make the call.
> > 
> > In reality, with respect to the logging, it should not make a difference
since
> > we will check before logging if it's legit to do so. So as of right now, the
> > only place the pref will be used is in chrome://settings.
> > 
> > The former is preferable since we don't update unless we know the set call
was
> > successful. The latter is cleaner IMO.
> > 
> > Do you have a preference?
> 
> I think the former (on success), since we should aim to be as close to the
> current web pref as possible and a successful response indicates what the true
> value is.
> 
> Speaking of success, I think something should happen on failure. If the
request
> to "enable" failed, maybe it should be disabled. If a request to "disable"
> failed, then... um... dunno.

I've combined the two callbacks since they would essentially do the same thing:
set the pref, but make it false if it was unsuccessful.

I think this is the correct behavior because we don't want to be logging unless
we're sure they wanted it enabled. In practice, this will only be set from the
hotword verification, so a cancel (don't enable) and a failure should have the
same result of disabling.

[Anand Mistry (off Chromium), 2014-11-12 01:29:15]

lgtm

[rpetterson, 2014-11-18 22:58:30]

sky@ -- web_history_service_unittest.cc added. PTAL.

Thanks!

https://codereview.chromium.org/687803004/diff/160001/chrome/browser/history/...
File chrome/browser/history/DEPS (right):

https://codereview.chromium.org/687803004/diff/160001/chrome/browser/history/...
chrome/browser/history/DEPS:44:
"!chrome/browser/sync/profile_sync_service_mock.h",
I check with Erik about this and this okay. These temporary rules will be
removed (separate CL for bug 370850 that I will do after this) as history is no
longer being componentized.

[sky, 2014-11-19 01:02:41]

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:462:
scoped_ptr<base::DictionaryValue> response_value;
nit: use a scoped_ptr for request early on.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.h (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:43: virtual int response_code() =
0;
We don't use unix_hacker_style for virtual methods.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:75: virtual Request*
CreateRequest(const GURL& url,
Can this and the next be protected?

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:139: Profile* profile_;
Style guide says no protected fields.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service_unittest.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service_unittest.cc:7: #include
"chrome/browser/history/web_history_service.h"
nit: this should be your first include (unittests have the same order as .cc for
corresponding .h).

[rpetterson, 2014-11-19 02:04:56]

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:462:
scoped_ptr<base::DictionaryValue> response_value;
On 2014/11/19 01:02:41, sky wrote:
> nit: use a scoped_ptr for request early on.

The request actually does start out as a scoped_ptr, but is stored in the
pending_audio_history_requests_ so it can be deleted later on. I was following
them template established by ExpireHistory. Do you know why ExpireHistory
doesn't use a scoped_ptr?

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.h (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:43: virtual int response_code() =
0;
On 2014/11/19 01:02:41, sky wrote:
> We don't use unix_hacker_style for virtual methods.

I was following the pre-existing style (see is_pending()) above. Would you
prefer I change all of these functions throughout the file?

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:75: virtual Request*
CreateRequest(const GURL& url,
On 2014/11/19 01:02:41, sky wrote:
> Can this and the next be protected?

Done.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:139: Profile* profile_;
On 2014/11/19 01:02:41, sky wrote:
> Style guide says no protected fields.

Done.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service_unittest.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service_unittest.cc:7: #include
"chrome/browser/history/web_history_service.h"
On 2014/11/19 01:02:41, sky wrote:
> nit: this should be your first include (unittests have the same order as .cc
for
> corresponding .h).

Done.

[sky, 2014-11-19 16:02:44]

LGTM

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:462:
scoped_ptr<base::DictionaryValue> response_value;
On 2014/11/19 02:04:55, rpetterson wrote:
> On 2014/11/19 01:02:41, sky wrote:
> > nit: use a scoped_ptr for request early on.
> 
> The request actually does start out as a scoped_ptr, but is stored in the
> pending_audio_history_requests_ so it can be deleted later on. I was following
> them template established by ExpireHistory. Do you know why ExpireHistory
> doesn't use a scoped_ptr?

What about erasing and putting in a scoped_ptr first thing, rather than at the
end (which is error prone)? Not sure about ExpireHistory. Brett wrote that.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.h (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.h:43: virtual int response_code() =
0;
On 2014/11/19 02:04:56, rpetterson wrote:
> On 2014/11/19 01:02:41, sky wrote:
> > We don't use unix_hacker_style for virtual methods.
> 
> I was following the pre-existing style (see is_pending()) above. Would you
> prefer I change all of these functions throughout the file?

Yes, but if you want to fix up the other feel free to do it later.

[rpetterson, 2014-11-19 19:48:24]

rlp@chromium.org changed reviewers:
+ benwells@chromium.org

[rpetterson, 2014-11-19 19:48:25]

+benwells for component_loader_unittest.cc

Reasoning: 2 of the component loader unittests load component extensions (and
namely hotwording). This CL adds a transitive dependency on ProfileSyncService
within hotwording via WebHistoryService so I need to turn sync off in those two
tests since they don't do shutdown in such a way that handles shutting down the
sync service properly.

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/687803004/diff/250001/chrome/browser/history/...
chrome/browser/history/web_history_service.cc:462:
scoped_ptr<base::DictionaryValue> response_value;
On 2014/11/19 16:02:43, sky wrote:
> On 2014/11/19 02:04:55, rpetterson wrote:
> > On 2014/11/19 01:02:41, sky wrote:
> > > nit: use a scoped_ptr for request early on.
> > 
> > The request actually does start out as a scoped_ptr, but is stored in the
> > pending_audio_history_requests_ so it can be deleted later on. I was
following
> > them template established by ExpireHistory. Do you know why ExpireHistory
> > doesn't use a scoped_ptr?
> 
> What about erasing and putting in a scoped_ptr first thing, rather than at the
> end (which is error prone)? Not sure about ExpireHistory. Brett wrote that.

Done.

[benwells, 2014-11-20 02:18:14]

lgtm

[rpetterson, 2014-11-20 02:20:07]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-11-20 02:21:41]

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

[commit-bot: I haz the power, 2014-11-20 03:57:44]

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

[commit-bot: I haz the power, 2014-11-20 03:57:46]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rpetterson, 2014-11-20 07:38:18]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-11-20 07:39:25]

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

[commit-bot: I haz the power, 2014-11-20 08:45:11]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2014-11-20 08:45:51]

Patchset 17 (id:??) landed as
https://crrev.com/997840adca15d4f278b2b8ad955aeb8bec663b5e
Cr-Commit-Position: refs/heads/master@{#304994}