Bug 40129 - Geolocation needs LayoutTest to test making callbacks to remote frames
Summary: Geolocation needs LayoutTest to test making callbacks to remote frames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 39879
  Show dependency treegraph
 
Reported: 2010-06-03 10:25 PDT by Steve Block
Modified: 2010-06-03 21:30 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.98 KB, patch)
2010-06-03 10:32 PDT, Steve Block
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-06-03 10:25:43 PDT
Geolocation needs LayoutTest to test making callbacks to remote frames
Comment 1 Alexey Proskuryakov 2010-06-03 10:30:10 PDT
Yay for more tests!
Comment 2 Steve Block 2010-06-03 10:32:17 PDT
Created attachment 57787 [details]
Patch
Comment 3 Jeremy Orlow 2010-06-03 10:39:07 PDT
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.
Comment 4 Alexey Proskuryakov 2010-06-03 10:48:16 PDT
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
Comment 5 Alexey Proskuryakov 2010-06-03 10:49:18 PDT
> It would be useful to also have a test that calls getCurrentPosition() directly from iframe

Which is the same as Jeremy said, I think.
Comment 6 Steve Block 2010-06-03 10:59:08 PDT
(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.
Comment 7 Jeremy Orlow 2010-06-03 11:12:47 PDT
(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.)
Comment 8 Alexey Proskuryakov 2010-06-03 12:56:43 PDT
> 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;
Comment 9 Steve Block 2010-06-03 15:41:09 PDT
> 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.
Comment 10 Steve Block 2010-06-03 16:47:42 PDT
Committed r60645: <http://trac.webkit.org/changeset/60645>
Comment 11 WebKit Review Bot 2010-06-03 18:59:44 PDT
http://trac.webkit.org/changeset/60645 might have broken GTK Linux 32-bit Release
Comment 12 Eric Seidel (no email) 2010-06-03 21:06:48 PDT
Borked the gtk?
Comment 13 Kent Tamura 2010-06-03 21:30:14 PDT
(In reply to comment #12)
> Borked the gtk?

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