test for https://codereview.chromium.org/419693002/

A browsertest for for blink r179868.

> Bust the MemoryCache to ensure script requests reach the browser-side
> and get added to and retrieved from the ServiceWorker's script cache.

BUG=387589
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288887

[michaeln1, 2014-07-31 03:10:41]

have to wait the blink change for this test

[michaeln, 2014-07-31 20:07:58]

This is weird and i don't understand why?

Without the blink patch, when i run this test page in chrome or content_shell,
there is only 1 resource in the scriptcache when the 2nd time registration is
committed, failing behavior. However in the context of the browser test runner
(based on content_shell) there are 2 resources in the scriptcache which is
expected, so that test passes.

With the patch applied test passes in all runtimes.

Something is curiously different about the environment provided by the
browser_test runner. I'll look into whether the layout_test runner acts like
browser_tests or real browsers? If the test envrironment is different from
actual browsers in this regard, that's not good for our tests.

[dominicc (has gone to gerrit), 2014-08-04 01:27:10]

Comments inline.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:707:
*num_resource_out = -1;
I think you want a plural instead of "resource"?

Is there a convention to call the parameter _out? Isn't the pointer enough?

num_resources?
num_script_resources?

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:709:
std::vector<ServiceWorkerRegistrationInfo> infos =
Maybe "registrations" reads more easily than "infos"; information doesn't have a
plural like that.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:716: if
(!infos[index].installing_version.is_null)
Don't vectors index from 0?

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:731: const
std::string kScopeUrl = "/service_worker/imports_bust_memcache_scope/*";
I think a change has landed that updates the handling of *

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
File content/test/data/service_worker/empty_script.js (right):

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/empty_script.js:5: // Is emptiness is
copyrightable?
This is ungrammatical.

I don't think this helps people understand what this file is for, so I would
change or remove this.

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
File content/test/data/service_worker/imports_bust_memcache.html (right):

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:4: // Content-shell
data persists, so unreg first to clear old data.
Content-shell -> Content Shell
unreg - avoid abbreviations.

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:5: // Register,
unregister, then reregister, the browsertest .cc code examines
then/the seems odd

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:24:
.then(function(worker) {
This will become a registration object. Since it is not used you might name it _
to avoid the need to update it. Ditto above.

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:29:
console.log('error will robinson!');
I think Will Robinson was warned of danger, not errors; maybe omit this. Since
the test failure by setting the title to FAILED will point to this place, the
log probably doesn't help with debugging the test.

[dominicc (has gone to gerrit), 2014-08-04 01:28:05]

On 2014/07/31 20:07:58, michaeln wrote:
> This is weird and i don't understand why?
> 
> Without the blink patch, when i run this test page in chrome or content_shell,
> there is only 1 resource in the scriptcache when the 2nd time registration is
> committed, failing behavior. However in the context of the browser test runner
> (based on content_shell) there are 2 resources in the scriptcache which is
> expected, so that test passes.
> 
> With the patch applied test passes in all runtimes.
> 
> Something is curiously different about the environment provided by the
> browser_test runner. I'll look into whether the layout_test runner acts like
> browser_tests or real browsers? If the test envrironment is different from
> actual browsers in this regard, that's not good for our tests.

This is worth investigating! I'm interested in what you find.

[dominicc (has gone to gerrit), 2014-08-04 01:32:07]

Another idea on testing: window.internals can poke at the MemoryCache, another
approach would be to add 'worker internals' or something and check that the
memory cache is affected. However the test you have here is more appealing
because it can potentially be end-to-end, so hopefully you can get this one to
work in browser_tests.

[michaeln, 2014-08-08 02:32:37]

> This is worth investigating! I'm interested in what you find.

The difference is the webserver, layout tests use apache whereas the browser
test use some other embedded server.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:707:
*num_resource_out = -1;
On 2014/08/04 01:27:10, dominicc wrote:
> I think you want a plural instead of "resource"?
> 
> Is there a convention to call the parameter _out? Isn't the pointer enough?
> 
> num_resources?
> num_script_resources?
> 

Done.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:709:
std::vector<ServiceWorkerRegistrationInfo> infos =
On 2014/08/04 01:27:09, dominicc wrote:
> Maybe "registrations" reads more easily than "infos"; information doesn't have
a
> plural like that.

there is a good convention for calling structs like this an 'info' and a
collection of them 'infos'. we've avoided using the name of the first class
object so instances of these structs aren't confused with the first class
objects. you can see this in many place within the storage apis and also in a
number of other places as well.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:716: if
(!infos[index].installing_version.is_null)
On 2014/08/04 01:27:09, dominicc wrote:
> Don't vectors index from 0?

I googled it, yes they do.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:731: const
std::string kScopeUrl = "/service_worker/imports_bust_memcache_scope/*";
On 2014/08/04 01:27:10, dominicc wrote:
> I think a change has landed that updates the handling of *

Done.

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
File content/test/data/service_worker/imports_bust_memcache.html (right):

https://codereview.chromium.org/434453005/diff/20001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:29:
console.log('error will robinson!');
On 2014/08/04 01:27:10, dominicc wrote:
> I think Will Robinson was warned of danger, not errors; maybe omit this. Since
> the test failure by setting the title to FAILED will point to this place, the
> log probably doesn't help with debugging the test.

levity removed

[dominicc (has gone to gerrit), 2014-08-08 05:23:38]

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:709:
std::vector<ServiceWorkerRegistrationInfo> infos =
On 2014/08/08 02:32:37, michaeln wrote:
> On 2014/08/04 01:27:09, dominicc wrote:
> > Maybe "registrations" reads more easily than "infos"; information doesn't
have
> a
> > plural like that.
> 
> there is a good convention for calling structs like this an 'info' and a
> collection of them 'infos'. we've avoided using the name of the first class
> object so instances of these structs aren't confused with the first class
> objects. you can see this in many place within the storage apis and also in a
> number of other places as well.
> 

Acknowledged.

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:716: if
(!infos[index].installing_version.is_null)
On 2014/08/08 02:32:37, michaeln wrote:
> On 2014/08/04 01:27:09, dominicc wrote:
> > Don't vectors index from 0?
> 
> I googled it, yes they do.

So isn't infos[infos.size()] out of bounds?

[dominicc (has gone to gerrit), 2014-08-08 05:24:45]

https://codereview.chromium.org/434453005/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_browsertest.cc:716: if
> (!infos[index].installing_version.is_null)
> On 2014/08/08 02:32:37, michaeln wrote:
> > On 2014/08/04 01:27:09, dominicc wrote:
> > > Don't vectors index from 0?
> > 
> > I googled it, yes they do.
> 
> So isn't infos[infos.size()] out of bounds?

Hmm, somehow I missed the - 1 there. LGTM.

[michaeln, 2014-08-08 19:46:03]

https://codereview.chromium.org/434453005/diff/40001/content/test/data/servic...
File content/test/data/service_worker/imports_bust_memcache.html (right):

https://codereview.chromium.org/434453005/diff/40001/content/test/data/servic...
content/test/data/service_worker/imports_bust_memcache.html:6: // the state of
the scriptcache to ensure the script and import are cached.
ooops... my edits to this file didn't make it into patchset 3?

[dominicc (has gone to gerrit), 2014-08-11 01:43:21]

On 2014/08/08 02:32:37, michaeln wrote:
> > This is worth investigating! I'm interested in what you find.
> 
> The difference is the webserver, layout tests use apache whereas the browser
> test use some other embedded server.

Just to clarify... is this test effective? If I revert the code change part, the
test will fail?

[michaeln, 2014-08-11 21:21:08]

> Just to clarify... is this test effective? If I revert the code change part,
the
> test will fail?

yup, they'll fail in the way that tryjobs of for patchset 6 failed.

[michaeln, 2014-08-11 22:24:15]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-08-11 22:28:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/434453005/120001

[commit-bot: I haz the power, 2014-08-12 03:07:50]

Change committed as 288887