Fix serviceworker default registration scope.

Fix serviceworker default registration scope.

A follow up fix for spec update: https://github.com/slightlyoff/ServiceWorker/issues/312.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176534

[falken, 2014-06-18 08:16:25]

lgtm, thanks!

[jsbell, 2014-06-18 17:57:04]

https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/registration.html:47: var t =
async_test('Registering default scope');
There's a test for the default scope in serviceworkerglobalscope-scope.html
that's apparently passing even with this change. :(

I think the problem with that test and with this one is that '*' and '/*' are
not distinguishable, since the test is running at the root level of the server,
so String(new URL(document.location, '*')) === String(new URL(document.location,
'/*'))

Does this test fail if the C++ change is not present?

If not, we should add a test where the registration is done not in the root,
e.g. an iframe in 'resources/XXXX.html' registers; that should be able to
distinguish '*' from '/*'

[xiang, 2014-06-19 04:57:45]

https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/registration.html:47: var t =
async_test('Registering default scope');
On 2014/06/18 17:57:03, jsbell wrote:
> There's a test for the default scope in serviceworkerglobalscope-scope.html
> that's apparently passing even with this change. :(
> 
> I think the problem with that test and with this one is that '*' and '/*' are
> not distinguishable, since the test is running at the root level of the
server,
> so String(new URL(document.location, '*')) === String(new
URL(document.location,
> '/*'))
> 
> Does this test fail if the C++ change is not present?
> 
> If not, we should add a test where the registration is done not in the root,
> e.g. an iframe in 'resources/XXXX.html' registers; that should be able to
> distinguish '*' from '/*'
> 

The default scope tests in serviceworkerglobalscope-scope.html and
serviceworkerobject-scope.html will fail after this change:
FAIL Verify the scope property: default assert_equals: Worker should see the
scope on eval. expected "http://127.0.0.1:8000/serviceworker/*" but got
"http://127.0.0.1:8000/*"

Think the "relative_path = test_name[len(self.HTTP_DIR):]" in port/driver.py can
make these tests running in 'serviceworker/xxx.html' pattern.

I will remove this test and use those two instead.

[xiang, 2014-06-19 05:06:32]

On 2014/06/19 04:57:45, xiang wrote:
>
https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
> File LayoutTests/http/tests/serviceworker/registration.html (right):
> 
>
https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
> LayoutTests/http/tests/serviceworker/registration.html:47: var t =
> async_test('Registering default scope');
> On 2014/06/18 17:57:03, jsbell wrote:
> > There's a test for the default scope in serviceworkerglobalscope-scope.html
> > that's apparently passing even with this change. :(
> > 
> > I think the problem with that test and with this one is that '*' and '/*'
are
> > not distinguishable, since the test is running at the root level of the
> server,
> > so String(new URL(document.location, '*')) === String(new
> URL(document.location,
> > '/*'))
> > 
> > Does this test fail if the C++ change is not present?
> > 
> > If not, we should add a test where the registration is done not in the root,
> > e.g. an iframe in 'resources/XXXX.html' registers; that should be able to
> > distinguish '*' from '/*'
> > 
> 
> The default scope tests in serviceworkerglobalscope-scope.html and
> serviceworkerobject-scope.html will fail after this change:
> FAIL Verify the scope property: default assert_equals: Worker should see the
> scope on eval. expected "http://127.0.0.1:8000/serviceworker/*" but got
> "http://127.0.0.1:8000/*"
> 
> Think the "relative_path = test_name[len(self.HTTP_DIR):]" in port/driver.py
can
> make these tests running in 'serviceworker/xxx.html' pattern.
> 
> I will remove this test and use those two instead.

Done. PTAL, thanks.

[falken, 2014-06-19 05:27:50]

On 2014/06/19 05:06:32, xiang wrote:
> On 2014/06/19 04:57:45, xiang wrote:
> >
>
https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
> > File LayoutTests/http/tests/serviceworker/registration.html (right):
> > 
> >
>
https://codereview.chromium.org/340763002/diff/1/LayoutTests/http/tests/servi...
> > LayoutTests/http/tests/serviceworker/registration.html:47: var t =
> > async_test('Registering default scope');
> > On 2014/06/18 17:57:03, jsbell wrote:
> > > There's a test for the default scope in
serviceworkerglobalscope-scope.html
> > > that's apparently passing even with this change. :(
> > > 
> > > I think the problem with that test and with this one is that '*' and '/*'
> are
> > > not distinguishable, since the test is running at the root level of the
> > server,
> > > so String(new URL(document.location, '*')) === String(new
> > URL(document.location,
> > > '/*'))
> > > 
> > > Does this test fail if the C++ change is not present?
> > > 
> > > If not, we should add a test where the registration is done not in the
root,
> > > e.g. an iframe in 'resources/XXXX.html' registers; that should be able to
> > > distinguish '*' from '/*'
> > > 
> > 
> > The default scope tests in serviceworkerglobalscope-scope.html and
> > serviceworkerobject-scope.html will fail after this change:
> > FAIL Verify the scope property: default assert_equals: Worker should see the
> > scope on eval. expected "http://127.0.0.1:8000/serviceworker/*" but got
> > "http://127.0.0.1:8000/*"
> > 
> > Think the "relative_path = test_name[len(self.HTTP_DIR):]" in port/driver.py
> can
> > make these tests running in 'serviceworker/xxx.html' pattern.
> > 
> > I will remove this test and use those two instead.
> 
> Done. PTAL, thanks.

Yep, I also verified locally that our existing tests would fail in the way Xiang
described. still lgtm

[jsbell, 2014-06-19 16:17:49]

Awesome! 

lgtm2

[jsbell, 2014-06-19 16:17:54]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:19:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiang.long@intel.com/340763002/20001

[commit-bot: I haz the power, 2014-06-19 18:01:15]

Change committed as 176534