Added browser tests to test the BudgetAPI. Also added BudgetAPI to ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. Also added BudgetAPI to ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

BUG=617971
Committed: https://crrev.com/9f633be4d10a0a77f21186cdd514ad883ee07656
Cr-Commit-Position: refs/heads/master@{#422431}

[harkness, 2016-09-27 13:55:42]

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

[harkness, 2016-09-27 14:29:08]

Description was changed from

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

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

to

==========
Added browser tests to test the BudgetAPI. These test the getBudget() and
reserve() methods from Blink into the chrome layer. The correct functioning is
then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

[harkness, 2016-09-27 14:29:32]

Description was changed from

==========
Added browser tests to test the BudgetAPI. These test the getBudget() and
reserve() methods from Blink into the chrome layer. The correct functioning is
then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

to

==========
Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

[Peter Beverloo, 2016-09-27 15:57:48]

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

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:25: // const char
kManifestSenderId[] = "1234567890";
nit: delete commented out code

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:27: }  // namespace
Since you don't need `friends` declarations, can we hoist this entire file in
the anonymous namespace?

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:32:
~BudgetManagerBrowserTest() override {}
nit: {}  -->  " = default;"

for both the constructor and destructor

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:54:
command_line->AppendSwitch(
nit: add a TODO w/ a bug reference to remove this once we ship

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:91: virtual Browser*
GetBrowser() const { return browser(); }
These two don't have to be virtual? Why do they exist at all?

I think GetTestURL can just be a constant, and use browser() instead of
GetBrowser().

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:94: BudgetManager*
budget_manager_;
nit: Document why it's OK to hold a weak reference.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:95: bool success_;
Please initialise scalars with a value.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/budget_test.js (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/budget_test.js:25: function ResultQueue() {
Could we share this code with the push messaging test by just including it
through a <script> tag?

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/service_worker.js (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:10: self.workerReserve();
nit: drop the |self| everywhere. That's the global, so it's bound to be
available.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:18: function workerGetBudget()
{
Since we shipped async/await, you can use a much fancier way of writing this
function now:

async function workerGetBudget() {
  try {
    const budget = await navigator.budget.getBudget();
    sendMessageToClients(
        'message', 'ok - budget returned value of ' + budget[0].budgetAt);
  } catch (e) {
    sendMessageToClients('message', 'failed - unable to get budget values');
  }
}

(Also mixing up single and double quotes in the same function call is pushing it
a bit.)

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:41: var message =
JSON.stringify({
nit: use destructoring, don't use var.

const message = JSON.stringify({ type, data });

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:51: });
If this were an async function, you could replace lines 45 - 51 with:

for (const client of await clients.matchAll())
  client.postMessage(message);

:)

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/test.html (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/test.html:8: <h1>Budget API Test</h1>
Why one space indent? Why does this line then have two space indent- or three
spaces in total?

We should use two spaces in Chromium, consistently.

[harkness, 2016-09-28 12:29:21]

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

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:25: // const char
kManifestSenderId[] = "1234567890";
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: delete commented out code

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:27: }  // namespace
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> Since you don't need `friends` declarations, can we hoist this entire file in
> the anonymous namespace?

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:32:
~BudgetManagerBrowserTest() override {}
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: {}  -->  " = default;"
> 
> for both the constructor and destructor

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:54:
command_line->AppendSwitch(
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: add a TODO w/ a bug reference to remove this once we ship

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:91: virtual Browser*
GetBrowser() const { return browser(); }
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> These two don't have to be virtual? Why do they exist at all?
> 
> I think GetTestURL can just be a constant, and use browser() instead of
> GetBrowser().

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:94: BudgetManager*
budget_manager_;
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: Document why it's OK to hold a weak reference.

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:95: bool success_;
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> Please initialise scalars with a value.

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/budget_test.js (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/budget_test.js:25: function ResultQueue() {
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> Could we share this code with the push messaging test by just including it
> through a <script> tag?

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/service_worker.js (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:10: self.workerReserve();
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: drop the |self| everywhere. That's the global, so it's bound to be
> available.

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:18: function workerGetBudget()
{
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> Since we shipped async/await, you can use a much fancier way of writing this
> function now:
> 
> async function workerGetBudget() {
>   try {
>     const budget = await navigator.budget.getBudget();
>     sendMessageToClients(
>         'message', 'ok - budget returned value of ' + budget[0].budgetAt);
>   } catch (e) {
>     sendMessageToClients('message', 'failed - unable to get budget values');
>   }
> }
> 
> (Also mixing up single and double quotes in the same function call is pushing
it
> a bit.)

Actually, when I tried to switch to async, the service worker parsing fails. I
went ahead and fixed the quotes though.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:41: var message =
JSON.stringify({
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> nit: use destructoring, don't use var.
> 
> const message = JSON.stringify({ type, data });

Done.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:51: });
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> If this were an async function, you could replace lines 45 - 51 with:
> 
> for (const client of await clients.matchAll())
>   client.postMessage(message);
> 
> :)

Same issues as above.

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/test.html (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/test.html:8: <h1>Budget API Test</h1>
On 2016/09/27 15:57:47, Peter Beverloo wrote:
> Why one space indent? Why does this line then have two space indent- or three
> spaces in total?
> 
> We should use two spaces in Chromium, consistently.

Done.

[Peter Beverloo, 2016-09-28 13:04:28]

lgtm % comments

Please also update the CL description so that the first line is the CL's title.

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

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:95: bool success_;
On 2016/09/28 12:29:20, harkness wrote:
> On 2016/09/27 15:57:47, Peter Beverloo wrote:
> > Please initialise scalars with a value.
> 
> Done.

Sorry - |budget_manager_|, as a pointer, is also a scalar, and so needs to be
initialized as a nullptr. MSAN will complain about this since it's uninitialised
memory otherwise (i.e. garbage).

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
File chrome/test/data/budget_service/service_worker.js (right):

https://codereview.chromium.org/2370103003/diff/20001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:41: var message =
JSON.stringify({
On 2016/09/28 12:29:21, harkness wrote:
> On 2016/09/27 15:57:47, Peter Beverloo wrote:
> > nit: use destructoring, don't use var.
> > 
> > const message = JSON.stringify({ type, data });
> 
> Done.

This hasn't been done.

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:25: const
std::string kTestURL = "/budget_service/test.html";
This is a static initializer, which are bad. Instead, use a const char[]:

const char kTestURL[] = "...";


For more background -

Basically, the memory backing an std::string lives on the heap as opposed to the
stack. That means it has to be allocated at some point.

For regular code, i.e. functions and methods and members (through constructors),
this happens either when the code is being executed, or when the class is being
allocated.

For global code, i.e. this, this happens when the program starts. (Before the
main() function, even.) That means that a simple statement like this will affect
start-up performance of the entire browser, or, in this case, test suite.

Early in on Chrome for Android development, the sum of static initializers
actually penalized start up performance by ~350ms. Since then we banned them.

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:86: protected:
If the members are protected, the tests should access them directly (without the
getters on line 83/84). If the members are private, the tests should use getters
instead.

Production code doesn't allow protected member variables. We allow both for test
suites. Which one you pick is up to you.

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
File chrome/test/data/budget_service/budget_test.js (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
chrome/test/data/budget_service/budget_test.js:9: var resultQueue = new
ResultQueue();
nit: const, on line 57 too

(In general, you should never use "var" anymore.)

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
File chrome/test/data/budget_service/service_worker.js (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:6:
self.addEventListener('message', function handler (event) {
nit: why do you name this function?

[harkness, 2016-09-28 14:34:14]

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

[harkness, 2016-09-28 14:34:20]

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

https://codereview.chromium.org/2370103003/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:95: bool success_;
On 2016/09/28 13:04:26, Peter Beverloo wrote:
> On 2016/09/28 12:29:20, harkness wrote:
> > On 2016/09/27 15:57:47, Peter Beverloo wrote:
> > > Please initialise scalars with a value.
> > 
> > Done.
> 
> Sorry - |budget_manager_|, as a pointer, is also a scalar, and so needs to be
> initialized as a nullptr. MSAN will complain about this since it's
uninitialised
> memory otherwise (i.e. garbage).

Done.

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:25: const
std::string kTestURL = "/budget_service/test.html";
On 2016/09/28 13:04:27, Peter Beverloo wrote:
> This is a static initializer, which are bad. Instead, use a const char[]:
> 
> const char kTestURL[] = "...";
> 
> 
> For more background -
> 
> Basically, the memory backing an std::string lives on the heap as opposed to
the
> stack. That means it has to be allocated at some point.
> 
> For regular code, i.e. functions and methods and members (through
constructors),
> this happens either when the code is being executed, or when the class is
being
> allocated.
> 
> For global code, i.e. this, this happens when the program starts. (Before the
> main() function, even.) That means that a simple statement like this will
affect
> start-up performance of the entire browser, or, in this case, test suite.
> 
> Early in on Chrome for Android development, the sum of static initializers
> actually penalized start up performance by ~350ms. Since then we banned them.

Good catch, I was just being careless when I moved it up here from inside the
class.

https://codereview.chromium.org/2370103003/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:86: protected:
On 2016/09/28 13:04:27, Peter Beverloo wrote:
> If the members are protected, the tests should access them directly (without
the
> getters on line 83/84). If the members are private, the tests should use
getters
> instead.
> 
> Production code doesn't allow protected member variables. We allow both for
test
> suites. Which one you pick is up to you.

Moved to private.

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
File chrome/test/data/budget_service/budget_test.js (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
chrome/test/data/budget_service/budget_test.js:9: var resultQueue = new
ResultQueue();
On 2016/09/28 13:04:27, Peter Beverloo wrote:
> nit: const, on line 57 too
> 
> (In general, you should never use "var" anymore.)

Done.

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
File chrome/test/data/budget_service/service_worker.js (right):

https://codereview.chromium.org/2370103003/diff/40001/chrome/test/data/budget...
chrome/test/data/budget_service/service_worker.js:6:
self.addEventListener('message', function handler (event) {
On 2016/09/28 13:04:27, Peter Beverloo wrote:
> nit: why do you name this function?

Removed.

[commit-bot: I haz the power, 2016-09-28 14:34:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[harkness, 2016-09-28 14:39:57]

Description was changed from

==========
Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

to

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

[harkness, 2016-09-28 14:57:07]

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

[harkness, 2016-09-28 14:57:08]

haraken@chromium.org: Please review changes in 

third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

[haraken, 2016-09-28 15:00:05]

On 2016/09/28 14:57:08, harkness wrote:
> mailto:haraken@chromium.org: Please review changes in 
> 
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

LGTM

[commit-bot: I haz the power, 2016-09-28 16:35:29]

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

[commit-bot: I haz the power, 2016-09-28 16:35:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[harkness, 2016-10-03 12:47:16]

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 12:47:22]

Dry run: 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-10-03 15:44:19]

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

[commit-bot: I haz the power, 2016-10-03 15:44:20]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-10-03 15:52:08]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-10-03 15:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2370103003/#ps80001
(title: "Rebase and add script tag for result_queue.js to subscope tests.")

[commit-bot: I haz the power, 2016-10-03 15:52:20]

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-10-03 15:57:45]

Description was changed from

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

to

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

[commit-bot: I haz the power, 2016-10-03 15:57:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 15:59:48]

Description was changed from

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

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

to

==========
Added browser tests to test the BudgetAPI. Also added BudgetAPI to
ExperimentalWebPlatformFeatures.

Added browser tests to test the BudgetAPI. These test the getBudget()
and reserve() methods from Blink into the chrome layer. The correct
functioning is then tested by a direct call to BudgetManager::Consume.

Also added BudgetAPI to ExperimentalWebPlatformFeatures.

BUG=617971

Committed: https://crrev.com/9f633be4d10a0a77f21186cdd514ad883ee07656
Cr-Commit-Position: refs/heads/master@{#422431}
==========

[commit-bot: I haz the power, 2016-10-03 15:59:49]

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