Supervised user SafeSites: Expose & use API key from src-internal

Supervised user SafeSites: Expose & use API key from src-internal

Key was added to src-internal in https://chromereviews.googleplex.com/113767014/

BUG=417722
Committed: https://crrev.com/2ee5f146d57d07bbad3804715e06590b850711f9
Cr-Commit-Position: refs/heads/master@{#305412}

[Marc Treib, 2014-11-13 17:45:40]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org, rogerta@chromium.org

[Marc Treib, 2014-11-13 17:45:41]

Roger: Please review google_apis/
Bernhard: Please review c/b/supervised_user/
Thanks!

https://codereview.chromium.org/690423003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/690423003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:95: virtual std::string
GetSafeSitesApiKey() const;
I'll remove this after removing the downstream implementations, so I don't break
the build.

[Bernhard Bauer, 2014-11-13 18:01:50]

LGTM

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:93: virtual std::string
GetSafeSitesCx() const;
Are we keeping the CX in the delegate?

[Marc Treib, 2014-11-13 18:04:15]

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:93: virtual std::string
GetSafeSitesCx() const;
On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> Are we keeping the CX in the delegate?

That was the plan, yes. We could also move it directly into the AsyncChecker,
but then we'd need a new "bool EnableSafeSites()" in the delegate.

[Bernhard Bauer, 2014-11-14 09:04:58]

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:93: virtual std::string
GetSafeSitesCx() const;
On 2014/11/13 18:04:15, Marc Treib wrote:
> On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > Are we keeping the CX in the delegate?
> 
> That was the plan, yes. We could also move it directly into the AsyncChecker,
> but then we'd need a new "bool EnableSafeSites()" in the delegate.

I would have put it together with the secret, just like we do for the OAuth
client ID and secret. (And yes, then also added a flag in the delegate, because
that's really what the delegate does: it configures the SUService).

[Marc Treib, 2014-11-14 09:54:41]

On 2014/11/14 09:04:58, Bernhard Bauer wrote:
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> File chrome/browser/supervised_user/supervised_user_service.h (right):
> 
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> chrome/browser/supervised_user/supervised_user_service.h:93: virtual
std::string
> GetSafeSitesCx() const;
> On 2014/11/13 18:04:15, Marc Treib wrote:
> > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > Are we keeping the CX in the delegate?
> > 
> > That was the plan, yes. We could also move it directly into the
AsyncChecker,
> > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> 
> I would have put it together with the secret, just like we do for the OAuth
> client ID and secret. (And yes, then also added a flag in the delegate,
because
> that's really what the delegate does: it configures the SUService).

It's really independent from the key, and also not secret, so I see no reason to
put it next to the key. If you want it removed from the delegate, then I'll just
bake it into the AsyncFetcher.

[Bernhard Bauer, 2014-11-14 10:07:52]

On 2014/11/14 09:54:41, Marc Treib wrote:
> On 2014/11/14 09:04:58, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > File chrome/browser/supervised_user/supervised_user_service.h (right):
> > 
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > chrome/browser/supervised_user/supervised_user_service.h:93: virtual
> std::string
> > GetSafeSitesCx() const;
> > On 2014/11/13 18:04:15, Marc Treib wrote:
> > > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > > Are we keeping the CX in the delegate?
> > > 
> > > That was the plan, yes. We could also move it directly into the
> AsyncChecker,
> > > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> > 
> > I would have put it together with the secret, just like we do for the OAuth
> > client ID and secret. (And yes, then also added a flag in the delegate,
> because
> > that's really what the delegate does: it configures the SUService).
> 
> It's really independent from the key, and also not secret, so I see no reason
to
> put it next to the key. If you want it removed from the delegate, then I'll
just
> bake it into the AsyncFetcher.

Is it really independent from the key? Could you use it with a different key?
It's more about that for me, not so much about removing it from the delegate.

[Marc Treib, 2014-11-14 10:52:33]

On 2014/11/14 10:07:52, Bernhard Bauer wrote:
> On 2014/11/14 09:54:41, Marc Treib wrote:
> > On 2014/11/14 09:04:58, Bernhard Bauer wrote:
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > File chrome/browser/supervised_user/supervised_user_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > chrome/browser/supervised_user/supervised_user_service.h:93: virtual
> > std::string
> > > GetSafeSitesCx() const;
> > > On 2014/11/13 18:04:15, Marc Treib wrote:
> > > > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > > > Are we keeping the CX in the delegate?
> > > > 
> > > > That was the plan, yes. We could also move it directly into the
> > AsyncChecker,
> > > > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> > > 
> > > I would have put it together with the secret, just like we do for the
OAuth
> > > client ID and secret. (And yes, then also added a flag in the delegate,
> > because
> > > that's really what the delegate does: it configures the SUService).
> > 
> > It's really independent from the key, and also not secret, so I see no
reason
> to
> > put it next to the key. If you want it removed from the delegate, then I'll
> just
> > bake it into the AsyncFetcher.
> 
> Is it really independent from the key? Could you use it with a different key?
> It's more about that for me, not so much about removing it from the delegate.

I think so, yes - afaik there's nothing that links the key to the CSE. If you
have another key for which the CSE API is enabled, that should work as well (on
that key's quota).

[Bernhard Bauer, 2014-11-14 11:15:20]

On 2014/11/14 10:52:33, Marc Treib wrote:
> On 2014/11/14 10:07:52, Bernhard Bauer wrote:
> > On 2014/11/14 09:54:41, Marc Treib wrote:
> > > On 2014/11/14 09:04:58, Bernhard Bauer wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > File chrome/browser/supervised_user/supervised_user_service.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > chrome/browser/supervised_user/supervised_user_service.h:93: virtual
> > > std::string
> > > > GetSafeSitesCx() const;
> > > > On 2014/11/13 18:04:15, Marc Treib wrote:
> > > > > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > > > > Are we keeping the CX in the delegate?
> > > > > 
> > > > > That was the plan, yes. We could also move it directly into the
> > > AsyncChecker,
> > > > > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> > > > 
> > > > I would have put it together with the secret, just like we do for the
> OAuth
> > > > client ID and secret. (And yes, then also added a flag in the delegate,
> > > because
> > > > that's really what the delegate does: it configures the SUService).
> > > 
> > > It's really independent from the key, and also not secret, so I see no
> reason
> > to
> > > put it next to the key. If you want it removed from the delegate, then
I'll
> > just
> > > bake it into the AsyncFetcher.
> > 
> > Is it really independent from the key? Could you use it with a different
key?
> > It's more about that for me, not so much about removing it from the
delegate.
> 
> I think so, yes - afaik there's nothing that links the key to the CSE. If you
> have another key for which the CSE API is enabled, that should work as well
(on
> that key's quota).

Huh. So does that mean that if I could use this particular CSE by getting my own
key for it?

[Bernhard Bauer, 2014-11-14 11:15:39]

On 2014/11/14 11:15:20, Bernhard Bauer wrote:
> On 2014/11/14 10:52:33, Marc Treib wrote:
> > On 2014/11/14 10:07:52, Bernhard Bauer wrote:
> > > On 2014/11/14 09:54:41, Marc Treib wrote:
> > > > On 2014/11/14 09:04:58, Bernhard Bauer wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > > File chrome/browser/supervised_user/supervised_user_service.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > > chrome/browser/supervised_user/supervised_user_service.h:93: virtual
> > > > std::string
> > > > > GetSafeSitesCx() const;
> > > > > On 2014/11/13 18:04:15, Marc Treib wrote:
> > > > > > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > > > > > Are we keeping the CX in the delegate?
> > > > > > 
> > > > > > That was the plan, yes. We could also move it directly into the
> > > > AsyncChecker,
> > > > > > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> > > > > 
> > > > > I would have put it together with the secret, just like we do for the
> > OAuth
> > > > > client ID and secret. (And yes, then also added a flag in the
delegate,
> > > > because
> > > > > that's really what the delegate does: it configures the SUService).
> > > > 
> > > > It's really independent from the key, and also not secret, so I see no
> > reason
> > > to
> > > > put it next to the key. If you want it removed from the delegate, then
> I'll
> > > just
> > > > bake it into the AsyncFetcher.
> > > 
> > > Is it really independent from the key? Could you use it with a different
> key?
> > > It's more about that for me, not so much about removing it from the
> delegate.
> > 
> > I think so, yes - afaik there's nothing that links the key to the CSE. If
you
> > have another key for which the CSE API is enabled, that should work as well
> (on
> > that key's quota).
> 
> Huh. So does that mean that if I could use this particular CSE by getting my
own
> key for it?

s/if //

[Marc Treib, 2014-11-14 11:35:23]

On 2014/11/14 11:15:39, Bernhard Bauer wrote:
> On 2014/11/14 11:15:20, Bernhard Bauer wrote:
> > On 2014/11/14 10:52:33, Marc Treib wrote:
> > > On 2014/11/14 10:07:52, Bernhard Bauer wrote:
> > > > On 2014/11/14 09:54:41, Marc Treib wrote:
> > > > > On 2014/11/14 09:04:58, Bernhard Bauer wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > > > File chrome/browser/supervised_user/supervised_user_service.h
(right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/690423003/diff/60001/chrome/browser/supervise...
> > > > > > chrome/browser/supervised_user/supervised_user_service.h:93: virtual
> > > > > std::string
> > > > > > GetSafeSitesCx() const;
> > > > > > On 2014/11/13 18:04:15, Marc Treib wrote:
> > > > > > > On 2014/11/13 18:01:49, Bernhard Bauer wrote:
> > > > > > > > Are we keeping the CX in the delegate?
> > > > > > > 
> > > > > > > That was the plan, yes. We could also move it directly into the
> > > > > AsyncChecker,
> > > > > > > but then we'd need a new "bool EnableSafeSites()" in the delegate.
> > > > > > 
> > > > > > I would have put it together with the secret, just like we do for
the
> > > OAuth
> > > > > > client ID and secret. (And yes, then also added a flag in the
> delegate,
> > > > > because
> > > > > > that's really what the delegate does: it configures the SUService).
> > > > > 
> > > > > It's really independent from the key, and also not secret, so I see no
> > > reason
> > > > to
> > > > > put it next to the key. If you want it removed from the delegate, then
> > I'll
> > > > just
> > > > > bake it into the AsyncFetcher.
> > > > 
> > > > Is it really independent from the key? Could you use it with a different
> > key?
> > > > It's more about that for me, not so much about removing it from the
> > delegate.
> > > 
> > > I think so, yes - afaik there's nothing that links the key to the CSE. If
> you
> > > have another key for which the CSE API is enabled, that should work as
well
> > (on
> > > that key's quota).
> > 
> > Huh. So does that mean that if I could use this particular CSE by getting my
> own
> > key for it?
> 
> s/if //

As far as I understand it, yes. The CSE doesn't do anything special though, so
there wouldn't be much point.

[Roger Tawa OOO till Jul 10th, 2014-11-14 19:27:04]

lgtm google_apis

[Marc Treib, 2014-11-24 10:00:36]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-11-24 10:01:20]

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

[commit-bot: I haz the power, 2014-11-24 10:53:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-24 10:54:21]

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

[Thiemo Nagel, 2014-11-24 11:32:33]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/753683004/ by tnagel@chromium.org.

The reason for reverting is: Build breakage on Mac:

https://build.chromium.org/p/chromium.mac/builders/Mac%20Builder/builds/14318....

[Marc Treib, 2014-11-24 15:33:32]

On 2014/11/24 11:32:33, Thiemo Nagel wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/753683004/ by mailto:tnagel@chromium.org.
> 
> The reason for reverting is: Build breakage on Mac:
> 
>
https://build.chromium.org/p/chromium.mac/builders/Mac%20Builder/builds/14318....

Relanded as https://codereview.chromium.org/756593002/ (build breakage was a
flake)