[Hotword] Remove unneeded preferences. Cleaning up look of settings.

[Hotword] Remove unneeded preferences. Cleaning up look of settings.

Removed preferences include:
- flag for incognito mode -- hotwording is now always off in incognito
- flag for 5 minute timeout -- hotwording now always times out after 5 minutes

Cleanup includes:
- Moved the pref from Search to Privacy
- Added confirm bubble for clicking the pref which adds hotword_confirm_overlay.{html,css}
- Changed the wording for the description (also added strings for the confirm/cancel buttons)


BUG=325439, 289023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247945

[rpetterson, 2014-01-24 19:12:47]

UX made some changes to the preferences. This is mostly removal.

samarth@chromium.org: Please review changes in search/

jhawkins@chromium.org: Please review changes in resources/, webui/, common/

[James Hawkins, 2014-01-24 22:50:49]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
Is this right?  It seems like the IsOffTheRecord shouldn't be in the check
anymore.

[rpetterson, 2014-01-25 00:29:21]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
On 2014/01/24 22:50:49, James Hawkins wrote:
> Is this right?  It seems like the IsOffTheRecord shouldn't be in the check
> anymore.

Technically speaking GetServiceForBrowserContext should return NULL for OTR
profiles. I was mostly keeping it in there for logic sanity to make sure I'd
covered all my cases. I can remove it if you feel strongly.

[James Hawkins, 2014-01-27 18:14:36]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
On 2014/01/25 00:29:22, rpetterson wrote:
> On 2014/01/24 22:50:49, James Hawkins wrote:
> > Is this right?  It seems like the IsOffTheRecord shouldn't be in the check
> > anymore.
> 
> Technically speaking GetServiceForBrowserContext should return NULL for OTR
> profiles. I was mostly keeping it in there for logic sanity to make sure I'd
> covered all my cases. I can remove it if you feel strongly.

Yeah, I think it may be safer to let GetServiceForBrowserContext handle that
duty.  Hypothetically that behavior could change in the future, then this logic
wouldn't be doing the right thing.

[samarth, 2014-01-27 18:24:53]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
On 2014/01/27 18:14:37, James Hawkins wrote:
> On 2014/01/25 00:29:22, rpetterson wrote:
> > On 2014/01/24 22:50:49, James Hawkins wrote:
> > > Is this right?  It seems like the IsOffTheRecord shouldn't be in the check
> > > anymore.
> > 
> > Technically speaking GetServiceForBrowserContext should return NULL for OTR
> > profiles. I was mostly keeping it in there for logic sanity to make sure I'd
> > covered all my cases. I can remove it if you feel strongly.
> 
> Yeah, I think it may be safer to let GetServiceForBrowserContext handle that
> duty.  Hypothetically that behavior could change in the future, then this
logic
> wouldn't be doing the right thing.

Just so I understand correctly, this change is intended to make it so incognito
does not have a separate setting for hotwording and just follows the default? 
If so, I agree that this check is confusing.

If not, then I'm even more confused and perhaps some more detail in the CL
description would be useful.

[rpetterson, 2014-01-27 19:28:13]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
On 2014/01/27 18:24:53, samarth wrote:
> On 2014/01/27 18:14:37, James Hawkins wrote:
> > On 2014/01/25 00:29:22, rpetterson wrote:
> > > On 2014/01/24 22:50:49, James Hawkins wrote:
> > > > Is this right?  It seems like the IsOffTheRecord shouldn't be in the
check
> > > > anymore.
> > > 
> > > Technically speaking GetServiceForBrowserContext should return NULL for
OTR
> > > profiles. I was mostly keeping it in there for logic sanity to make sure
I'd
> > > covered all my cases. I can remove it if you feel strongly.
> > 
> > Yeah, I think it may be safer to let GetServiceForBrowserContext handle that
> > duty.  Hypothetically that behavior could change in the future, then this
> logic
> > wouldn't be doing the right thing.
> 
> Just so I understand correctly, this change is intended to make it so
incognito
> does not have a separate setting for hotwording and just follows the default? 
> If so, I agree that this check is confusing.
> 
> If not, then I'm even more confused and perhaps some more detail in the CL
> description would be useful.

I've pulled out this check.

And yes, this change is intended to make it so there is not a separate setting
for incognito (it's always off for incognito). I've updated the CL description
to better describe the changes.

[samarth, 2014-01-27 19:50:16]

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
profile->IsOffTheRecord() ||
On 2014/01/27 19:28:13, rpetterson wrote:
> On 2014/01/27 18:24:53, samarth wrote:
> > On 2014/01/27 18:14:37, James Hawkins wrote:
> > > On 2014/01/25 00:29:22, rpetterson wrote:
> > > > On 2014/01/24 22:50:49, James Hawkins wrote:
> > > > > Is this right?  It seems like the IsOffTheRecord shouldn't be in the
> check
> > > > > anymore.
> > > > 
> > > > Technically speaking GetServiceForBrowserContext should return NULL for
> OTR
> > > > profiles. I was mostly keeping it in there for logic sanity to make sure
> I'd
> > > > covered all my cases. I can remove it if you feel strongly.
> > > 
> > > Yeah, I think it may be safer to let GetServiceForBrowserContext handle
that
> > > duty.  Hypothetically that behavior could change in the future, then this
> > logic
> > > wouldn't be doing the right thing.
> > 
> > Just so I understand correctly, this change is intended to make it so
> incognito
> > does not have a separate setting for hotwording and just follows the
default? 
> > If so, I agree that this check is confusing.
> > 
> > If not, then I'm even more confused and perhaps some more detail in the CL
> > description would be useful.
> 
> I've pulled out this check.
> 
> And yes, this change is intended to make it so there is not a separate setting
> for incognito (it's always off for incognito). I've updated the CL description
> to better describe the changes.

Oh, that's different from what I thought.

I thought the default behavior was for services to share instances between the
main and incognito profiles?

[rpetterson, 2014-01-27 20:15:26]

On 2014/01/27 19:50:16, samarth wrote:
>
https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
> File chrome/browser/search/hotword_service_factory.cc (right):
> 
>
https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
> chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
> profile->IsOffTheRecord() ||
> On 2014/01/27 19:28:13, rpetterson wrote:
> > On 2014/01/27 18:24:53, samarth wrote:
> > > On 2014/01/27 18:14:37, James Hawkins wrote:
> > > > On 2014/01/25 00:29:22, rpetterson wrote:
> > > > > On 2014/01/24 22:50:49, James Hawkins wrote:
> > > > > > Is this right?  It seems like the IsOffTheRecord shouldn't be in the
> > check
> > > > > > anymore.
> > > > > 
> > > > > Technically speaking GetServiceForBrowserContext should return NULL
for
> > OTR
> > > > > profiles. I was mostly keeping it in there for logic sanity to make
sure
> > I'd
> > > > > covered all my cases. I can remove it if you feel strongly.
> > > > 
> > > > Yeah, I think it may be safer to let GetServiceForBrowserContext handle
> that
> > > > duty.  Hypothetically that behavior could change in the future, then
this
> > > logic
> > > > wouldn't be doing the right thing.
> > > 
> > > Just so I understand correctly, this change is intended to make it so
> > incognito
> > > does not have a separate setting for hotwording and just follows the
> default? 
> > > If so, I agree that this check is confusing.
> > > 
> > > If not, then I'm even more confused and perhaps some more detail in the CL
> > > description would be useful.
> > 
> > I've pulled out this check.
> > 
> > And yes, this change is intended to make it so there is not a separate
setting
> > for incognito (it's always off for incognito). I've updated the CL
description
> > to better describe the changes.
> 
> Oh, that's different from what I thought.
> 
> I thought the default behavior was for services to share instances between the
> main and incognito profiles?

UX has decided they want it always off for incognito.

[samarth, 2014-01-27 21:51:53]

On 2014/01/27 20:15:26, rpetterson wrote:
> On 2014/01/27 19:50:16, samarth wrote:
> >
>
https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
> > File chrome/browser/search/hotword_service_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/136693013/diff/30002/chrome/browser/search/ho...
> > chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
> > profile->IsOffTheRecord() ||
> > On 2014/01/27 19:28:13, rpetterson wrote:
> > > On 2014/01/27 18:24:53, samarth wrote:
> > > > On 2014/01/27 18:14:37, James Hawkins wrote:
> > > > > On 2014/01/25 00:29:22, rpetterson wrote:
> > > > > > On 2014/01/24 22:50:49, James Hawkins wrote:
> > > > > > > Is this right?  It seems like the IsOffTheRecord shouldn't be in
the
> > > check
> > > > > > > anymore.
> > > > > > 
> > > > > > Technically speaking GetServiceForBrowserContext should return NULL
> for
> > > OTR
> > > > > > profiles. I was mostly keeping it in there for logic sanity to make
> sure
> > > I'd
> > > > > > covered all my cases. I can remove it if you feel strongly.
> > > > > 
> > > > > Yeah, I think it may be safer to let GetServiceForBrowserContext
handle
> > that
> > > > > duty.  Hypothetically that behavior could change in the future, then
> this
> > > > logic
> > > > > wouldn't be doing the right thing.
> > > > 
> > > > Just so I understand correctly, this change is intended to make it so
> > > incognito
> > > > does not have a separate setting for hotwording and just follows the
> > default? 
> > > > If so, I agree that this check is confusing.
> > > > 
> > > > If not, then I'm even more confused and perhaps some more detail in the
CL
> > > > description would be useful.
> > > 
> > > I've pulled out this check.
> > > 
> > > And yes, this change is intended to make it so there is not a separate
> setting
> > > for incognito (it's always off for incognito). I've updated the CL
> description
> > > to better describe the changes.
> > 
> > Oh, that's different from what I thought.
> > 
> > I thought the default behavior was for services to share instances between
the
> > main and incognito profiles?
> 
> UX has decided they want it always off for incognito.

Sorry, what I meant was that I thought the default for services was to be shared
with incognito.  Just double checked and indeed the default is to return NULL. 
So this lgtm.

[rpetterson, 2014-01-28 07:09:02]

Ah, got it :)

So UX has decided they want this moved to privacy so rather than make
another CL for this since we're trying to get this in for M34, I'm just
going to update this one tomorrow. It will mostly affect browser_options.*


On Mon, Jan 27, 2014 at 1:51 PM, <samarth@chromium.org> wrote:

> On 2014/01/27 20:15:26, rpetterson wrote:
>
>> On 2014/01/27 19:50:16, samarth wrote:
>> >
>>
>
> https://codereview.chromium.org/136693013/diff/30002/
> chrome/browser/search/hotword_service_factory.cc
>
>> > File chrome/browser/search/hotword_service_factory.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/136693013/diff/30002/
> chrome/browser/search/hotword_service_factory.cc#newcode17
>
>> > chrome/browser/search/hotword_service_factory.cc:17: if (!profile ||
>> > profile->IsOffTheRecord() ||
>> > On 2014/01/27 19:28:13, rpetterson wrote:
>> > > On 2014/01/27 18:24:53, samarth wrote:
>> > > > On 2014/01/27 18:14:37, James Hawkins wrote:
>> > > > > On 2014/01/25 00:29:22, rpetterson wrote:
>> > > > > > On 2014/01/24 22:50:49, James Hawkins wrote:
>> > > > > > > Is this right?  It seems like the IsOffTheRecord shouldn't be
>> in
>>
> the
>
>> > > check
>> > > > > > > anymore.
>> > > > > >
>> > > > > > Technically speaking GetServiceForBrowserContext should return
>> NULL
>> for
>> > > OTR
>> > > > > > profiles. I was mostly keeping it in there for logic sanity to
>> make
>> sure
>> > > I'd
>> > > > > > covered all my cases. I can remove it if you feel strongly.
>> > > > >
>> > > > > Yeah, I think it may be safer to let GetServiceForBrowserContext
>>
> handle
>
>> > that
>> > > > > duty.  Hypothetically that behavior could change in the future,
>> then
>> this
>> > > > logic
>> > > > > wouldn't be doing the right thing.
>> > > >
>> > > > Just so I understand correctly, this change is intended to make it
>> so
>> > > incognito
>> > > > does not have a separate setting for hotwording and just follows the
>> > default?
>> > > > If so, I agree that this check is confusing.
>> > > >
>> > > > If not, then I'm even more confused and perhaps some more detail in
>> the
>>
> CL
>
>> > > > description would be useful.
>> > >
>> > > I've pulled out this check.
>> > >
>> > > And yes, this change is intended to make it so there is not a separate
>> setting
>> > > for incognito (it's always off for incognito). I've updated the CL
>> description
>> > > to better describe the changes.
>> >
>> > Oh, that's different from what I thought.
>> >
>> > I thought the default behavior was for services to share instances
>> between
>>
> the
>
>> > main and incognito profiles?
>>
>
>  UX has decided they want it always off for incognito.
>>
>
> Sorry, what I meant was that I thought the default for services was to be
> shared
> with incognito.  Just double checked and indeed the default is to return
> NULL.
> So this lgtm.
>
> https://codereview.chromium.org/136693013/
>

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

[rpetterson, 2014-01-29 02:20:10]

Hi, Samarth and James,

Here's the updated CL with the changes UX requested. Summary of the changes
since you last looked at it:

- Moved the pref from Search to Privacy 
- Added confirm bubble for clicking the pref which adds
hotword_confirm_overlay.{html,css} 
- Changed the wording for the description (also added strings for the
confirm/cancel buttons)

Thanks for taking another look!

[samarth, 2014-01-29 02:27:31]

slgtm for search

[James Hawkins, 2014-01-29 19:06:16]

LGTM with nits.

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
File chrome/browser/resources/options/hotword_confirm_overlay.html (right):

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/hotword_confirm_overlay.html:8:
i18n-values="href:hotwordLearnMoreURL"></a>
nit: Closing tag must be on a new line since the first line is wrapped.

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/hotword_confirm_overlay.html:13:
class="cancel-button"></button>
Same here.

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

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1: 
nit: Remove blank line.

[rpetterson, 2014-01-29 22:31:58]

Thanks!

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
File chrome/browser/resources/options/hotword_confirm_overlay.html (right):

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/hotword_confirm_overlay.html:8:
i18n-values="href:hotwordLearnMoreURL"></a>
On 2014/01/29 19:06:16, James Hawkins wrote:
> nit: Closing tag must be on a new line since the first line is wrapped.

Done.

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/hotword_confirm_overlay.html:13:
class="cancel-button"></button>
On 2014/01/29 19:06:16, James Hawkins wrote:
> Same here.

Done.

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

https://codereview.chromium.org/136693013/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1: 
On 2014/01/29 19:06:16, James Hawkins wrote:
> nit: Remove blank line.

Done.

[commit-bot: I haz the power, 2014-01-30 07:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/136693013/170001

[commit-bot: I haz the power, 2014-01-30 09:03:42]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-30 09:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/136693013/190001

[commit-bot: I haz the power, 2014-01-30 13:37:17]

Change committed as 247945