Convert most geolocation layout tests to use a JS mock implementation.

Convert most geolocation layout tests to use a JS mock implementation.

The one unchanged test is maximum-age. This relies on particular timing
between the implementation and the mock which is not currently
satisfied, but will be once the implementation moves into blink.

BUG=561879
Committed: https://crrev.com/e668780e8c87f9cb5f40ddb6127d01124676f016
Cr-Commit-Position: refs/heads/master@{#393444}

[Sam McNally, 2016-05-05 08:07:19]

Description was changed from

==========
Convert most geolocation layout tests to use a JS mock implementation.

The one unchanged test is maximum-age. This relies on particular timing
between the implementation and the mock which is not currently
satisfied, but will be once the implementation moves into blink.
==========

to

==========
Convert most geolocation layout tests to use a JS mock implementation.

The one unchanged test is maximum-age. This relies on particular timing
between the implementation and the mock which is not currently
satisfied, but will be once the implementation moves into blink.

BUG=561879
==========

[Sam McNally, 2016-05-05 11:39:26]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-05-10 01:19:22]

sammc@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-05-10 17:48:32]

Thanks Sam!

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/TEMPLATE.html
(right):

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/TEMPLATE.html:9:
<script src="YOUR_JS_FILE_HERE"></script>
What does this file even do? Let's delete it.

[Michael van Ouwerkerk, 2016-05-10 17:49:24]

lgtm

[Dirk Pranke, 2016-05-10 20:51:48]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-05-10 20:51:50]

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py
(right):

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:79:
'-c', 'AliasMatch /(.*/)?js-test-resources/(.+) "%s/$1resources/$2"' % test_dir,
why was this change needed, and what did it do?

[Sam McNally, 2016-05-11 00:40:41]

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/TEMPLATE.html
(right):

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/TEMPLATE.html:9:
<script src="YOUR_JS_FILE_HERE"></script>
On 2016/05/10 17:48:32, Michael van Ouwerkerk wrote:
> What does this file even do? Let's delete it.

Done.

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py
(right):

https://codereview.chromium.org/1948033003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:79:
'-c', 'AliasMatch /(.*/)?js-test-resources/(.+) "%s/$1resources/$2"' % test_dir,
On 2016/05/10 20:51:49, Dirk Pranke wrote:
> why was this change needed, and what did it do?

The existing /js-test-resources allows http layout tests to access
LayoutTests/resources. The problem is the geolocation-specific resources are in
LayoutTests/fast/dom/Geolocation/resources. This maps
/some/path/js-test-resources/another/path to
LayoutTests/some/path/resources/another/path so the geolocation-specific
resources are accessible.

[Sam McNally, 2016-05-12 01:25:53]

sammc@chromium.org changed reviewers:
+ jam@chromium.org

[Sam McNally, 2016-05-12 01:25:53]

+jam for content/

[jam, 2016-05-12 02:04:56]

On 2016/05/12 01:25:53, Sam McNally wrote:
> +jam for content/

can you explain why this change is needed?

[Sam McNally, 2016-05-12 04:20:01]

On 2016/05/12 02:04:56, jam wrote:
> On 2016/05/12 01:25:53, Sam McNally wrote:
> > +jam for content/
> 
> can you explain why this change is needed?

This was originally part of https://codereview.chromium.org/1367853002/ so the
mocking in these tests is structured with some assumptions about mojo connection
lifetime. In particular, they expect that a new PermissionService connection is
established for each test. In that CL, the PermissionService connection is
dropped after completing a request. In the current GeolocationDispatcher, the
permission connection is established on demand and retained for the life of the
frame, which can span multiple layout tests. This changes GeolocationDispatcher
to only keep the PermissionService connection alive while requests are in
progress so permission requests will always reach the mock PermissionService for
the current test.

[jam, 2016-05-12 16:29:36]

On 2016/05/12 04:20:01, Sam McNally wrote:
> On 2016/05/12 02:04:56, jam wrote:
> > On 2016/05/12 01:25:53, Sam McNally wrote:
> > > +jam for content/
> > 
> > can you explain why this change is needed?
> 
> This was originally part of https://codereview.chromium.org/1367853002/ so the
> mocking in these tests is structured with some assumptions about mojo
connection
> lifetime. In particular, they expect that a new PermissionService connection
is
> established for each test. In that CL, the PermissionService connection is
> dropped after completing a request. In the current GeolocationDispatcher, the
> permission connection is established on demand and retained for the life of
the
> frame, which can span multiple layout tests. This changes
GeolocationDispatcher
> to only keep the PermissionService connection alive while requests are in
> progress so permission requests will always reach the mock PermissionService
for
> the current test.

it seems a bit unfortunate to change how the renderer grabs services just for
tests. could this be changed so that there's a testing-only method to reset the
service pointer, and tests call that at the beginning?

[esprehn, 2016-05-12 19:26:06]

Yeah I think normally we have the TestRunner itself do the reset logic? It
already handles reset of many other things.

[Sam McNally, 2016-05-13 01:00:13]

On 2016/05/12 16:29:36, jam wrote:
> On 2016/05/12 04:20:01, Sam McNally wrote:
> > On 2016/05/12 02:04:56, jam wrote:
> > > On 2016/05/12 01:25:53, Sam McNally wrote:
> > > > +jam for content/
> > > 
> > > can you explain why this change is needed?
> > 
> > This was originally part of https://codereview.chromium.org/1367853002/ so
the
> > mocking in these tests is structured with some assumptions about mojo
> connection
> > lifetime. In particular, they expect that a new PermissionService connection
> is
> > established for each test. In that CL, the PermissionService connection is
> > dropped after completing a request. In the current GeolocationDispatcher,
the
> > permission connection is established on demand and retained for the life of
> the
> > frame, which can span multiple layout tests. This changes
> GeolocationDispatcher
> > to only keep the PermissionService connection alive while requests are in
> > progress so permission requests will always reach the mock PermissionService
> for
> > the current test.
> 
> it seems a bit unfortunate to change how the renderer grabs services just for
> tests. could this be changed so that there's a testing-only method to reset
the
> service pointer, and tests call that at the beginning?

It could be, but this is just a temporary measure until
https://codereview.chromium.org/1367853002/ deletes GeolocationDispatcher.

[jam, 2016-05-13 01:11:28]

On 2016/05/13 01:00:13, Sam McNally wrote:
> On 2016/05/12 16:29:36, jam wrote:
> > On 2016/05/12 04:20:01, Sam McNally wrote:
> > > On 2016/05/12 02:04:56, jam wrote:
> > > > On 2016/05/12 01:25:53, Sam McNally wrote:
> > > > > +jam for content/
> > > > 
> > > > can you explain why this change is needed?
> > > 
> > > This was originally part of https://codereview.chromium.org/1367853002/ so
> the
> > > mocking in these tests is structured with some assumptions about mojo
> > connection
> > > lifetime. In particular, they expect that a new PermissionService
connection
> > is
> > > established for each test. In that CL, the PermissionService connection is
> > > dropped after completing a request. In the current GeolocationDispatcher,
> the
> > > permission connection is established on demand and retained for the life
of
> > the
> > > frame, which can span multiple layout tests. This changes
> > GeolocationDispatcher
> > > to only keep the PermissionService connection alive while requests are in
> > > progress so permission requests will always reach the mock
PermissionService
> > for
> > > the current test.
> > 
> > it seems a bit unfortunate to change how the renderer grabs services just
for
> > tests. could this be changed so that there's a testing-only method to reset
> the
> > service pointer, and tests call that at the beginning?
> 
> It could be, but this is just a temporary measure until
> https://codereview.chromium.org/1367853002/ deletes GeolocationDispatcher.

lgtm

[Sam McNally, 2016-05-13 03:02:30]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-05-13 03:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1948033003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-13 03:02:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948033003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948033003/60001

[commit-bot: I haz the power, 2016-05-13 04:07:12]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-13 04:08:38]

Description was changed from

==========
Convert most geolocation layout tests to use a JS mock implementation.

The one unchanged test is maximum-age. This relies on particular timing
between the implementation and the mock which is not currently
satisfied, but will be once the implementation moves into blink.

BUG=561879
==========

to

==========
Convert most geolocation layout tests to use a JS mock implementation.

The one unchanged test is maximum-age. This relies on particular timing
between the implementation and the mock which is not currently
satisfied, but will be once the implementation moves into blink.

BUG=561879

Committed: https://crrev.com/e668780e8c87f9cb5f40ddb6127d01124676f016
Cr-Commit-Position: refs/heads/master@{#393444}
==========

[commit-bot: I haz the power, 2016-05-13 04:08:40]

Patchset 3 (id:??) landed as
https://crrev.com/e668780e8c87f9cb5f40ddb6127d01124676f016
Cr-Commit-Position: refs/heads/master@{#393444}

[Łukasz Anforowicz, 2016-05-13 19:45:51]

On 2016/05/13 04:08:40, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/e668780e8c87f9cb5f40ddb6127d01124676f016
> Cr-Commit-Position: refs/heads/master@{#393444}

This CL has made the linux_site_isolation bot red - please see
https://crbug.com/611838