Bug 27716

Summary: Add Geolocation layout tests.
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, bolsinga, darin, eric, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch for bug 27716
none
Patch for bug 27716
darin: review-
Patch 3 for bug 27716
eric: review-
Patch 4 for bug 27716
eric: review+, eric: commit-queue-
Patch 5 for bug 27716
eric: review+, eric: commit-queue-
Disables Geolocation tests for GTK
levin: review-
Disables Geolocation tests for GTK, Patch 2
none
Adds a mock Geolocation service, Patch 1
none
Adds a mock Geolocation service, Patch 2 none

Description Steve Block 2009-07-27 10:17:17 PDT
Geolocation was implemented in changeset http://trac.webkit.org/changeset/37854 (bug https://bugs.webkit.org/show_bug.cgi?id=21475). This initial changeset added a layout test to check that navigator.geolocation is not present, but this was later removed in changeset http://trac.webkit.org/changeset/41798.

Bug https://bugs.webkit.org/show_bug.cgi?id=21717 was created to track the need to test Geolocation when the platform does not provide an implementation. This is achieved using a mock Geolocation implementation, but is complicated as navigator.geolocation needs to be present for testing, but should not be present otherwise.

As an intermediate solution, I suggest that Geolocation is tested only on platforms where the platform provides an implementation and hence navigator.geolocation is present. On other platforms, we simply test that navigator.geolocation is absent.

What do people think?
Comment 1 Steve Block 2009-07-28 07:04:53 PDT
Created attachment 33625 [details]
Patch for bug 27716

A patch to add the first couple of unit tests, which simply test that navigator.geolocation is present only when is is supposed to be.
Comment 2 Eric Seidel (no email) 2009-07-28 10:50:57 PDT
I think these tests are OK, but not really necessary.  I do think we should provide some sort of mock-geolocation system via DumpRenderTree and a geolocationController.
Comment 3 Steve Block 2009-07-28 11:06:34 PDT
> I do think we should
> provide some sort of mock-geolocation system via DumpRenderTree and a
> geolocationController.
Agreed. I'm currently working on exactly this and will send patches.

What I wanted to get agreement on now, is that on platforms where Geolocation is not implemented, we should not test it via a mock implementation, but simply test that navigator.geolocation is not present.
Comment 4 Eric Seidel (no email) 2009-07-28 11:11:22 PDT
It's the individual platforms responsibility to manage their skipped lists and turn on/off tests accordingly.  Platforms which want to test for missing geolocation can simply turn on the geolocation tests and check in failing results. :)

I thikn it's also OK to have a "is geolocation enabled" test, expect enabled, and check in "PASS" results on the platforms which have it, and skip it on all other platforms.  But that test is also implied by your more complicated tests you're about to add/write.
Comment 5 Steve Block 2009-07-28 15:20:56 PDT
> It's the individual platforms responsibility to manage their skipped lists and
> turn on/off tests accordingly.  Platforms which want to test for missing
> geolocation can simply turn on the geolocation tests and check in failing
> results. :)
OK, I'll dispense with the 'not-enabled' test then.

> I thikn it's also OK to have a "is geolocation enabled" test, expect enabled,
> and check in "PASS" results on the platforms which have it, and skip it on all
> other platforms.
I'll update the patch to add only the 'enabled' test and not add it to any skip lists.

>  But that test is also implied by your more complicated tests
> you're about to add/write.
Sure, but I think there's value in testing that the geolocation object has the correct type and that the navigator object's properties enumerate correctly.
Comment 6 Eric Seidel (no email) 2009-07-28 15:27:11 PDT
(In reply to comment #5)
> >  But that test is also implied by your more complicated tests
> > you're about to add/write.
> Sure, but I think there's value in testing that the geolocation object has the
> correct type and that the navigator object's properties enumerate correctly.

I certainly agree!  And I wish we had more tests of our DOM bindings.
Comment 7 Steve Block 2009-07-29 08:58:40 PDT
Created attachment 33716 [details]
Patch for bug 27716
Comment 8 Steve Block 2009-08-07 05:07:48 PDT
Comment on attachment 33716 [details]
Patch for bug 27716

It seems I forgot to set the review flag for this patch when I uploaded it.
Comment 9 Steve Block 2009-08-07 05:08:50 PDT
Adding Greg to CC.
Comment 10 Darin Adler 2009-08-07 07:35:49 PDT
Comment on attachment 33716 [details]
Patch for bug 27716

Please put these tests into fast/dom/Geolocation instead of a new top-level directory.
Comment 11 Steve Block 2009-08-07 08:52:12 PDT
Created attachment 34280 [details]
Patch 3 for bug 27716

Moved test to fast/dom/Geolocation as requested.
Comment 12 Greg Bolsinga 2009-08-07 09:45:04 PDT
I'm not a reviewer, but looks good to me.
Comment 13 Eric Seidel (no email) 2009-08-07 10:25:40 PDT
Comment on attachment 34280 [details]
Patch 3 for bug 27716

This really should be a one of the new-style JS tests.  see fast/js/resources and make-js-test-wrappers.

That would give you shouldBe(), and debug(), etc. for free.

It looks like someone has duplicated at least part of that functionality with the "utils.js" file.
Comment 14 Eric Seidel (no email) 2009-08-07 10:26:12 PDT
I think this patch is OK as is, but I'd really rather see it using the same framework as most of the other new tests use.
Comment 15 Steve Block 2009-08-08 15:13:58 PDT
Created attachment 34402 [details]
Patch 4 for bug 27716

Now using JS test framework from fast/js/resources.
Comment 16 Eric Seidel (no email) 2009-08-08 15:29:05 PDT
Comment on attachment 34402 [details]
Patch 4 for bug 27716

Hot hot hot! :)

hasGeolocationProperty() is really testing "isGeolocationPropertyEnumerable" :)

 13 shouldBeTrue("typeof navigator.geolocation == 'object'");

could be written as:
shouldBeEqualToString("typeof navigator.geolocation", "object");

But it looks great as is too! :)

Thanks!
Comment 17 Eric Seidel (no email) 2009-08-11 15:18:07 PDT
Comment on attachment 34402 [details]
Patch 4 for bug 27716

This is missing new results and cannot be landed by the commit-queue.
Comment 18 Steve Block 2009-08-12 07:33:52 PDT
> This is missing new results and cannot be landed by the commit-queue.
The patch includes expected results for GTK at LayoutTests/platform/gtk/fast/dom/Geolocation/enabled-expected.txt.

Perhaps I misinterpreted your earlier comment ...
> It's the individual platforms responsibility to manage their skipped lists and
> turn on/off tests accordingly.
>
> I thikn it's also OK to have a "is geolocation enabled" test, expect enabled,
> and check in "PASS" results on the platforms which have it, and skip it on all
> other platforms.

Did you mean that it's the platform's responsibility for all platforms other than mac? Should I add a 'fail' result for mac or add this test to the mac skipped list?
Comment 19 Eric Seidel (no email) 2009-08-12 09:08:44 PDT
The commit-queue runs from a mac.  Should these results be the same for all platforms?  If so, then they should be placed right next to the original test and platforms which want to check in platform-specific failing results can do so.
Comment 20 Steve Block 2009-08-12 09:15:51 PDT
> The commit-queue runs from a mac.  Should these results be the same for all
> platforms?
The 'pass' results should be the same for all platforms that support Geolocation. Currently, this is only GTK. On platforms that don't support Geolocation (which includes mac), all tests will fail.

Should I add 'failed' results to LayoutTests/fast/dom/ and 'pass' results to LayoutTests/platform/gtk/fast/dom/Geolocation/? Or should I add the 'pass' results to LayoutTests/fast/dom/ and add 'failed' results to LayoutTests/platform/mac/fast/dom/Geolocation/ to keep the build happy?
Comment 21 Eric Seidel (no email) 2009-08-12 09:18:36 PDT
The failure case seems uninteresting.  I would checkin the results next to the tests, and edit the skipped lists of all platforms you expect to fail.
Comment 22 Steve Block 2009-08-12 10:34:38 PDT
Created attachment 34671 [details]
Patch 5 for bug 27716

> The failure case seems uninteresting.  I would checkin the results next to the
> tests, and edit the skipped lists of all platforms you expect to fail.
Done. I was confused by your earlier comments about maintenance of the skipped lists being the responsibility of the individual platforms.
Comment 23 Eric Seidel (no email) 2009-08-12 10:46:46 PDT
Comment on attachment 34671 [details]
Patch 5 for bug 27716

LGTM.
Comment 24 Eric Seidel (no email) 2009-08-12 12:29:28 PDT
Comment on attachment 34671 [details]
Patch 5 for bug 27716

Rejecting patch 34671 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34671 from bug 27716 failed to download and apply.
Comment 25 Eric Seidel (no email) 2009-08-12 12:44:49 PDT
Applying 34671 from bug 27716.
[snip]
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 89.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej
patch -p0 "LayoutTests/platform/mac/Skipped" returned 1.  Pass --force to ignore patch failures.
Rejecting patch 34671 from commit-queue.  This patch will require manual commit.

mac/Skipped has changed.
Comment 26 Eric Seidel (no email) 2009-08-12 17:23:51 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/Geolocation/enabled-expected.txt
	A	LayoutTests/fast/dom/Geolocation/enabled.html
	A	LayoutTests/fast/dom/Geolocation/resources/TEMPLATE.html
	A	LayoutTests/fast/dom/Geolocation/resources/enabled.js
	M	LayoutTests/platform/mac/Skipped
Committed r47168
	M	LayoutTests/platform/mac/Skipped
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/Geolocation/enabled-expected.txt
	A	LayoutTests/fast/dom/Geolocation/resources/enabled.js
	A	LayoutTests/fast/dom/Geolocation/resources/TEMPLATE.html
	A	LayoutTests/fast/dom/Geolocation/enabled.html
r47168 = 4c2f9fcb3b4be28142507c551e59d825778c701a (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47168
Comment 27 Mark Rowe (bdash) 2009-08-12 17:31:12 PDT
<http://trac.webkit.org/changeset/47168> doesn't include the changes to qt/Skipped or win/Skipped.  Presumably this should also be skipped for GTK too.
Comment 28 Mark Rowe (bdash) 2009-08-12 17:32:47 PDT
Comment on attachment 34671 [details]
Patch 5 for bug 27716

> +    for (var property in navigator) {
> +        if (property == "geolocation") {
> +            return true;
> +        }
> +    }

We don't include braces around the body of single-line if statements.
Comment 29 Eric Seidel (no email) 2009-08-12 17:35:56 PDT
The patch has the changes, so I must have just applied it wrong somehow.
Comment 30 Steve Block 2009-08-13 01:55:06 PDT
> <http://trac.webkit.org/changeset/47168> doesn't include the changes to
> qt/Skipped or win/Skipped.
> We don't include braces around the body of single-line if statements.
Both of these are fixed in http://trac.webkit.org/changeset/47169. Thanks Eric!
Comment 31 Steve Block 2009-08-13 02:35:22 PDT
Created attachment 34725 [details]
Disables Geolocation tests for GTK

Disables Geolocation tests for GTK, at the request of zecke@webkit.org.
Comment 32 David Levin 2009-08-13 02:49:59 PDT
Comment on attachment 34725 [details]
Disables Geolocation tests for GTK

> Index: LayoutTests/platform/gtk/Skipped
> @@ -5979,3 +5979,6 @@ editing/selection/find-in-text-control.h
>  fast/harness/override-preferences.html
>  fast/harness/override-preferences-2.html
>  fast/harness/override-zzz-reset.html
> +
> +# This port doesn't support Geolocation.
> +fast/dom/Geolocation

Two issues:
* According to comment at the top of the file: "no grouping should occur. Every test for itself."

* This appears to be doing the addition in the section which is "# Tests with bugs attached : Enable these tests again once these bugs are fixed." but this failure doesn't fit in that category.
Comment 33 Steve Block 2009-08-13 03:08:04 PDT
Created attachment 34727 [details]
Disables Geolocation tests for GTK, Patch 2

Sorry, didn't see those rules.
Comment 34 Steve Block 2009-08-13 07:30:21 PDT
Created attachment 34740 [details]
Adds a mock Geolocation service, Patch 1

Adds a mock Geolocation service for use in LayoutTests.
Comment 35 Steve Block 2009-08-13 08:03:14 PDT
Created attachment 34741 [details]
Adds a mock Geolocation service, Patch 2

Patch now includes additions to makefiles, xcode project etc.
Comment 36 Eric Seidel (no email) 2009-08-13 09:01:01 PDT
In general we should use new bugs for new patches.  Twill be less confusing to reviewers and our tools. :)
Comment 37 Eric Seidel (no email) 2009-08-13 09:43:28 PDT
Comment on attachment 34727 [details]
Disables Geolocation tests for GTK, Patch 2

Clearing flags on attachment: 34727

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/gtk/Skipped
Committed r47204
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
r47204 = 634ae99c08d1c4c5665a1e7608861c350d25570c (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47204
Comment 38 Eric Seidel (no email) 2009-08-13 09:43:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Eric Seidel (no email) 2009-08-13 11:46:14 PDT
Case in point.  We were just bitten by bug 28230.
Comment 40 Steve Block 2009-08-13 12:30:40 PDT
I've moved the patch to add the mock Geolocation service to a new bug - Bug 28264.