Created Mojo BudgetService implementation in chrome/browser

Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
Committed: https://crrev.com/4108cd014d785a83c19ade128fc15c72b1c672c4
Cr-Commit-Position: refs/heads/master@{#414382}

[harkness, 2016-08-22 17:27:09]

harkness@chromium.org changed reviewers:
+ peter@chromium.org

[harkness, 2016-08-23 09:40:33]

This has been updated with storing the RenderProcessHost ID instead of the
BrowserContext*.

[Peter Beverloo, 2016-08-23 10:13:31]

Thanks :)

Given our current iteration speed, have you considered writing up a basic
browser test for these things?

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:100:
"+third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom",
File extension's missing? You're including the C header file (.h), not the mojom
file.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_service_impl.cc (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:16: // Silent push is the
only operation type supported currently.
Now that we have the blink::mojom::BudgetOperationType type, can we drop
BudgetManager::CostType altogether to avoid duplication?

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:35: //
blink::mojom::BudgetService implementation.
nit: remove? Line 48 too. They don't really add value.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:41: ->GetBrowserContext();
Mmm. DCHECK(context)?

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_service_impl.h (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.h:13: class BrowserContext;
nit: unused

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.h:28: //
blink::mojom::BudgetService implementation.
nit: remove lines 27 and 28 (we usually have a "block" of the overrides)

[Peter Beverloo, 2016-08-23 10:14:20]

Also— please use a mode descriptive subject for your CL. The one-liner is:

    "Created Mojo Service implementation in chrome/browser"

Which lacks a lot of necessary context.

[harkness, 2016-08-23 10:15:23]

Description was changed from

==========
Created Mojo Service implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
==========

to

==========
Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
==========

[harkness, 2016-08-23 10:48:33]

re: BrowserTest - I'll do it once I get the WorkerBudget done in Blink.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:100:
"+third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom",
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> File extension's missing? You're including the C header file (.h), not the
mojom
> file.

Done.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_service_impl.cc (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:16: // Silent push is the
only operation type supported currently.
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> Now that we have the blink::mojom::BudgetOperationType type, can we drop
> BudgetManager::CostType altogether to avoid duplication?

Good idea. Done.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:35: //
blink::mojom::BudgetService implementation.
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> nit: remove? Line 48 too. They don't really add value.

Done.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.cc:41: ->GetBrowserContext();
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> Mmm. DCHECK(context)?

Done.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_service_impl.h (right):

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.h:13: class BrowserContext;
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> nit: unused

Done.

https://codereview.chromium.org/2265173002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_service_impl.h:28: //
blink::mojom::BudgetService implementation.
On 2016/08/23 10:13:30, Peter Beverloo wrote:
> nit: remove lines 27 and 28 (we usually have a "block" of the overrides)

Done.

[Peter Beverloo, 2016-08-23 15:12:56]

lgtm

[harkness, 2016-08-23 16:38:58]

harkness@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[harkness, 2016-08-23 16:38:59]

+tsepez Please review this and https://codereview.chromium.org/2231873002/
(Added you as a reviewer there also) for Mojo sanity.

+jam Please review for browser side Mojo.

[jam, 2016-08-23 17:19:44]

First question is: why is this in chrome/browser instead of content/browser?

chrome/ is generally for Google Chrome specific features. It sounds like this is
a web platform API, so we would want it for any content-embedder and not just
Google Chrome. The browser side of web platform implementations lives in
content/ for that reason.

[Peter Beverloo, 2016-08-23 17:23:28]

On 2016/08/23 17:19:44, jam wrote:
> First question is: why is this in chrome/browser instead of content/browser?
> 
> chrome/ is generally for Google Chrome specific features. It sounds like this
is
> a web platform API, so we would want it for any content-embedder and not just
> Google Chrome. The browser side of web platform implementations lives in
> content/ for that reason.

The Budget API basically exposes the Site Engagement Score, which lives in
//chrome/browser/engagement/ and relies on a collection of //chrome-level
signals. We decided to skip //content because it literally wouldn't do anything
useful, beyond forwarding the calls through the Content API towards an identical
interface in //chrome.

[jam, 2016-08-23 19:45:31]

On 2016/08/23 17:23:28, Peter Beverloo wrote:
> On 2016/08/23 17:19:44, jam wrote:
> > First question is: why is this in chrome/browser instead of content/browser?
> > 
> > chrome/ is generally for Google Chrome specific features. It sounds like
this
> is
> > a web platform API, so we would want it for any content-embedder and not
just
> > Google Chrome. The browser side of web platform implementations lives in
> > content/ for that reason.
> 
> The Budget API basically exposes the Site Engagement Score, which lives in
> //chrome/browser/engagement/ and relies on a collection of //chrome-level
> signals. We decided to skip //content because it literally wouldn't do
anything
> useful, beyond forwarding the calls through the Content API towards an
identical
> interface in //chrome.

What metrics impact site engagement? I would suspect that content/ would know
about most of these? (i.e. how long a person is on a site, input, media...).

AFAIK, site engagement started in chrome/ because it was used to influence code
in chrome that granted extra quota (right?). But now that you want to use it
directly from the web platform, it does seem to go against what we've worked on
for the last few years of having web platform implementations be part of what
embedders of content get. So we should reconsider where the source of the data
lives, and whether it should stay in chrome/ or move elsewhere.

Can we sync up over VC about this? I think it'll be more efficient than email.

[jam, 2016-08-24 18:10:54]

lgtm

(for others reading: Jen & Peter explained how the budget service implementation
is similar to Chrome making UI decisions about permissions)

[harkness, 2016-08-25 07:57:14]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2016-08-25 07:57:42]

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-08-25 08:34:43]

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

[commit-bot: I haz the power, 2016-08-25 08:34:45]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[harkness, 2016-08-25 09:16:03]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-08-25 09:16:03]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2265173002/#ps80001
(title: "Remove use of local variable and call GetCost directly.")

[commit-bot: I haz the power, 2016-08-25 09:16:26]

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

Description was changed from

==========
Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
==========

to

==========
Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
==========

[commit-bot: I haz the power, 2016-08-25 10:26:38]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971
==========

to

==========
Created Mojo BudgetService implementation in chrome/browser

This implementation will be invoked from Blink via Mojo and will return the
budget information specified in the BudgetAPI.

Current implementation only support the getCost call, although the GetBudget
call is also defined.

BUG=617971

Committed: https://crrev.com/4108cd014d785a83c19ade128fc15c72b1c672c4
Cr-Commit-Position: refs/heads/master@{#414382}
==========

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

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

[Tom Sepez, 2016-08-25 21:27:05]

lgtm