Bug 40129 – Geolocation needs LayoutTest to test making callbacks to remote frames

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Steve Block]

Geolocation needs LayoutTest to test making callbacks to remote frames

[Alexey Proskuryakov]

Yay for more tests!

[Steve Block]

Created attachment 57787 [details]
Patch

[Jeremy Orlow]

Comment on attachment 57787 [details]
Patch

LayoutTests/fast/dom/Geolocation/script-tests/callback-to-remote-context.js:3
 +  function onIframeReady() {
Isn't this being called with the iframe's ScriptExecutionContext?  I guess testing this is useful as well, but I'm not sure it's doing what you intend it to.

If so, I think doing a setTimeout(..., 0) on that window with that function should do the trick..but you'll want to verify in a debugger.

[Alexey Proskuryakov]

Comment on attachment 57787 [details]
Patch

I don't think that tests like this should be script-tests. That doesn't improve anything, but has very real costs - for example, I can't just open a test and see what it does, as I need to open related .js file in a subdirectory instead.

Also, it's more difficult to share tests that utilize complicated support libraries with engineers working on other engines, so it's less likely that they will run our tests and be compatible with us.

And you had to resort to DOM manipulation just to add an iframe element.

It would be useful to also have a test that calls getCurrentPosition() directly from iframe, not from a function defined in main frame's document.

r=me

[Alexey Proskuryakov]

> It would be useful to also have a test that calls getCurrentPosition() directly from iframe

Which is the same as Jeremy said, I think.

[Steve Block]

(In reply to comment #3)
> (From update of attachment 57787 [details])
> LayoutTests/fast/dom/Geolocation/script-tests/callback-to-remote-context.js:3
>  +  function onIframeReady() {
> Isn't this being called with the iframe's ScriptExecutionContext?
No, I think the the call to getCurrentPosition() is made with the main frame's ScriptExecutionContext, because the function OnIframeReady was defined in the scope of the main frame.

> It would be useful to also have a test that calls getCurrentPosition() directly 
> from iframe, not from a function defined in main frame's document.
Do you mean that the iframe would directly call a method on its own Geolocation object? So just testing the most basic functionality, but in an iframe? It's true that we don't currently have a test for this, but the purpose of this bug was to test calling back to a context other than that belonging to the Geolocation's frame.

[Jeremy Orlow]

(In reply to comment #4)
> (From update of attachment 57787 [details])
> I don't think that tests like this should be script-tests. That doesn't improve anything, but has very real costs - for example, I can't just open a test and see what it does, as I need to open related .js file in a subdirectory instead.
> 
> Also, it's more difficult to share tests that utilize complicated support libraries with engineers working on other engines, so it's less likely that they will run our tests and be compatible with us.

If you feel strongly about this, we should probably start a webkit-dev thread on the subject.  Because most other reviewers I know prefer doing tests like this with script-tests and factoring out as much code as possible into support libraries.  (I actually didn't realize that anyone was advocating keeping them independent and such.  I definitely agree there are some merits to doing so, but I think there are more benefits to keeping tests compact and easy to read.)

[Alexey Proskuryakov]

> Do you mean that the iframe would directly call a method on its own Geolocation object?

No, I meant making a call to one frame's Geolocation from a function defined in another frame when JS execution starts in the latter. Using setTimeout as Jeremy suggested is a good way to achieve that.

I do not fully understand all details (Sam Weinig, Geoff Garen, or Adam Barth would be the ones to talk to), but that's really a different situation.

> If you feel strongly about this, we should probably start a webkit-dev thread on the subject.

I wouldn't expect any specific conclusion from such a discussion thread. Informing people of both upsides and downsides to using script tests might be useful though. In addition to what I mentioned before, here are some more thing I consider when choosing one or another:
- shouldBe() is great when there are multiple subtests;
- shouldBe() is very smart about argument types, more so than a naive "==" would be;
- results are well structured, so there is no doubt whether a given output is a pass or a failure;
- with a script test, you don't need to type in dumpAsText();
- we still hope to run fast/js tests with only JavaScriptCore built one day, so keeping those tests in .js files is useful;
- async script-tests are inherently harder to follow, since notifyDone() is hidden, and execution flow is not all coded in one place;
- dynamically creating a DOM structure in a script test is ugly;
- a script test is more limited in how results are presented, binary PASS/FAIL is hard to dig into (why exactly did it fail? is it the thing we test for that fails, or something in surrounding machinery?)
- you don't get the kind of accidental testing you get with tests that dump more information;
- script tests are sometimes less compatible with other browsers, because you need to both test for a specific issue, and extract the results in a form that can be easily processed - so, you're relying on things like CSSOM that aren't well supported everywhere.

The below is neither clean nor compact, and it's forced on us by script-tests.

+var iframe = document.createElement('iframe');
+iframe.src = 'resources/callback-to-remote-context-inner.html';
+document.body.appendChild(iframe);
+
+window.jsTestIsAsync = true;
+window.successfullyParsed = true;

[Steve Block]

> No, I meant making a call to one frame's Geolocation from a function defined
> in another frame when JS execution starts in the latter.
Filed Bug 40146 to track this.

> > If you feel strongly about this, we should probably start a webkit-dev thread on the subject.
Please CC me if a thread is started - I'm keen to learn the best practice here.

[Steve Block]

Committed r60645: <http://trac.webkit.org/changeset/60645>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60645 might have broken GTK Linux 32-bit Release

[Eric Seidel (no email)]

Borked the gtk?

[Kent Tamura]

(In reply to comment #12)
> Borked the gtk?

I filed Bug#40153 and add the test to gtk/Skipped.