40129 2010-06-03 10:25:43 -0700 Geolocation needs LayoutTest to test making callbacks to remote frames 2010-06-03 21:30:14 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 39879 1 steveblock webkit-unassigned abarth andreip ap eric jorlow steveblock webkit.review.bot xan.lopez oldest_to_newest 233812 0 steveblock 2010-06-03 10:25:43 -0700 Geolocation needs LayoutTest to test making callbacks to remote frames 233817 1 ap 2010-06-03 10:30:10 -0700 Yay for more tests! 233819 2 57787 steveblock 2010-06-03 10:32:17 -0700 Created attachment 57787 Patch 233824 3 57787 jorlow 2010-06-03 10:39:07 -0700 Comment on attachment 57787 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. 233827 4 57787 ap 2010-06-03 10:48:16 -0700 Comment on attachment 57787 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 233828 5 ap 2010-06-03 10:49:18 -0700 > It would be useful to also have a test that calls getCurrentPosition() directly from iframe Which is the same as Jeremy said, I think. 233831 6 steveblock 2010-06-03 10:59:08 -0700 (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. 233838 7 jorlow 2010-06-03 11:12:47 -0700 (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.) 233879 8 ap 2010-06-03 12:56:43 -0700 > 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; 233959 9 steveblock 2010-06-03 15:41:09 -0700 > 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. 233993 10 steveblock 2010-06-03 16:47:42 -0700 Committed r60645: <http://trac.webkit.org/changeset/60645> 234029 11 webkit.review.bot 2010-06-03 18:59:44 -0700 http://trac.webkit.org/changeset/60645 might have broken GTK Linux 32-bit Release 234044 12 eric 2010-06-03 21:06:48 -0700 Borked the gtk? 234051 13 tkent 2010-06-03 21:30:14 -0700 (In reply to comment #12) > Borked the gtk? I filed Bug#40153 and add the test to gtk/Skipped. 57787 2010-06-03 10:32:17 -0700 2010-06-03 10:48:15 -0700 Patch bug-40129-20100603183215.patch text/plain 4074 steveblock SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDYwNjI3KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDYtMDMgIFN0ZXZlIEJsb2NrICA8c3Rl dmVibG9ja0Bnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIEdlb2xvY2F0aW9uIG5lZWRzIExheW91dFRlc3QgdG8gdGVzdCBtYWtpbmcg Y2FsbGJhY2tzIHRvIHJlbW90ZSBmcmFtZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTQwMTI5CisKKyAgICAgICAgKiBmYXN0L2RvbS9HZW9sb2NhdGlv bi9jYWxsYmFjay10by1yZW1vdGUtY29udGV4dC1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAg ICAqIGZhc3QvZG9tL0dlb2xvY2F0aW9uL2NhbGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0Lmh0bWw6 IEFkZGVkLgorICAgICAgICAqIGZhc3QvZG9tL0dlb2xvY2F0aW9uL3Jlc291cmNlczogQWRkZWQu CisgICAgICAgICogZmFzdC9kb20vR2VvbG9jYXRpb24vcmVzb3VyY2VzL2NhbGxiYWNrLXRvLXJl bW90ZS1jb250ZXh0LWlubmVyLmh0bWw6IEFkZGVkLgorICAgICAgICAqIGZhc3QvZG9tL0dlb2xv Y2F0aW9uL3NjcmlwdC10ZXN0cy9jYWxsYmFjay10by1yZW1vdGUtY29udGV4dC5qczogQWRkZWQu CisKIDIwMTAtMDYtMDMgIENzYWJhIE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4KIAog ICAgICAgICBSdWJiZXItc3RhbXBlZCBieSBTaW1vbiBIYXVzbWFubi4KSW5kZXg6IExheW91dFRl c3RzL2Zhc3QvZG9tL0dlb2xvY2F0aW9uL2NhbGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0LWV4cGVj dGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L2RvbS9HZW9sb2NhdGlvbi9j YWxsYmFjay10by1yZW1vdGUtY29udGV4dC1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBM YXlvdXRUZXN0cy9mYXN0L2RvbS9HZW9sb2NhdGlvbi9jYWxsYmFjay10by1yZW1vdGUtY29udGV4 dC1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTAgQEAKK1Rlc3RzIHRoYXQg d2hlbiBhIEdlb2xvY2F0aW9uIHJlcXVlc3QgaXMgbWFkZSBmcm9tIGEgcmVtb3RlIGZyYW1lLCBj YWxsYmFja3MgYXJlIG1hZGUgYXMgdXN1YWwuCisKK09uIHN1Y2Nlc3MsIHlvdSB3aWxsIHNlZSBh IHNlcmllcyBvZiAiUEFTUyIgbWVzc2FnZXMsIGZvbGxvd2VkIGJ5ICJURVNUIENPTVBMRVRFIi4K KworCitQQVNTIFN1Y2Nlc3MgY2FsbGJhY2sgaW52b2tlZAorUEFTUyBzdWNjZXNzZnVsbHlQYXJz ZWQgaXMgdHJ1ZQorCitURVNUIENPTVBMRVRFCisKSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZG9t L0dlb2xvY2F0aW9uL2NhbGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0Lmh0bWwKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gTGF5b3V0VGVzdHMvZmFzdC9kb20vR2VvbG9jYXRpb24vY2FsbGJhY2stdG8tcmVtb3RlLWNv bnRleHQuaHRtbAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zhc3QvZG9tL0dlb2xvY2F0 aW9uL2NhbGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0Lmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAg KzEsMTMgQEAKKzwhRE9DVFlQRSBIVE1MIFBVQkxJQyAiLS8vSUVURi8vRFREIEhUTUwvL0VOIj4K KzxodG1sPgorPGhlYWQ+Cis8bGluayByZWw9InN0eWxlc2hlZXQiIGhyZWY9Ii4uLy4uL2pzL3Jl c291cmNlcy9qcy10ZXN0LXN0eWxlLmNzcyI+Cis8c2NyaXB0IHNyYz0iLi4vLi4vanMvcmVzb3Vy Y2VzL2pzLXRlc3QtcHJlLmpzIj48L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPHAgaWQ9ImRl c2NyaXB0aW9uIj48L3A+Cis8ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKzxzY3JpcHQgc3JjPSJz Y3JpcHQtdGVzdHMvY2FsbGJhY2stdG8tcmVtb3RlLWNvbnRleHQuanMiPjwvc2NyaXB0PgorPHNj cmlwdCBzcmM9Ii4uLy4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXBvc3QuanMiPjwvc2NyaXB0Pgor PC9ib2R5PgorPC9odG1sPgpJbmRleDogTGF5b3V0VGVzdHMvZmFzdC9kb20vR2VvbG9jYXRpb24v cmVzb3VyY2VzL2NhbGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0LWlubmVyLmh0bWwKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9kb20vR2VvbG9jYXRpb24vcmVzb3VyY2VzL2NhbGxiYWNr LXRvLXJlbW90ZS1jb250ZXh0LWlubmVyLmh0bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0 cy9mYXN0L2RvbS9HZW9sb2NhdGlvbi9yZXNvdXJjZXMvY2FsbGJhY2stdG8tcmVtb3RlLWNvbnRl eHQtaW5uZXIuaHRtbAkocmV2aXNpb24gMCkKQEAgLTAsMCArMSwxNiBAQAorPCFET0NUWVBFIEhU TUwgUFVCTElDICItLy9JRVRGLy9EVEQgSFRNTC8vRU4iPgorPGh0bWw+CisgIDxoZWFkPgorICAg IDxzY3JpcHQ+CisgICAgICBmdW5jdGlvbiBpbml0KCkgeworICAgICAgICAgIGlmICh3aW5kb3cu bGF5b3V0VGVzdENvbnRyb2xsZXIpIHsKKyAgICAgICAgICAgICAgbGF5b3V0VGVzdENvbnRyb2xs ZXIuc2V0R2VvbG9jYXRpb25QZXJtaXNzaW9uKHRydWUpOworICAgICAgICAgICAgICBsYXlvdXRU ZXN0Q29udHJvbGxlci5zZXRNb2NrR2VvbG9jYXRpb25Qb3NpdGlvbig1MS40NzgsIC0wLjE2Niwg MTAwKTsKKyAgICAgICAgICB9CisgICAgICAgICAgd2luZG93LnBhcmVudC5vbklmcmFtZVJlYWR5 KCkKKyAgICAgIH0KKyAgICA8L3NjcmlwdD4KKyAgPC9oZWFkPgorICA8Ym9keSBvbmxvYWQ9Imlu aXQoKSI+CisgIDwvYm9keT4KKzwvaHRtbD4KSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZG9tL0dl b2xvY2F0aW9uL3NjcmlwdC10ZXN0cy9jYWxsYmFjay10by1yZW1vdGUtY29udGV4dC5qcwo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L2RvbS9HZW9sb2NhdGlvbi9zY3JpcHQtdGVzdHMv Y2FsbGJhY2stdG8tcmVtb3RlLWNvbnRleHQuanMJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0 cy9mYXN0L2RvbS9HZW9sb2NhdGlvbi9zY3JpcHQtdGVzdHMvY2FsbGJhY2stdG8tcmVtb3RlLWNv bnRleHQuanMJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTkgQEAKK2Rlc2NyaXB0aW9uKCJUZXN0 cyB0aGF0IHdoZW4gYSBHZW9sb2NhdGlvbiByZXF1ZXN0IGlzIG1hZGUgZnJvbSBhIHJlbW90ZSBm cmFtZSwgY2FsbGJhY2tzIGFyZSBtYWRlIGFzIHVzdWFsLiIpOworCitmdW5jdGlvbiBvbklmcmFt ZVJlYWR5KCkgeworICAgIC8vIE1ha2UgcmVxdWVzdCBmcm9tIHJlbW90ZSBmcmFtZQorICAgIGlm cmFtZS5jb250ZW50V2luZG93Lm5hdmlnYXRvci5nZW9sb2NhdGlvbi5nZXRDdXJyZW50UG9zaXRp b24oZnVuY3Rpb24oKSB7CisgICAgICAgIHRlc3RQYXNzZWQoJ1N1Y2Nlc3MgY2FsbGJhY2sgaW52 b2tlZCcpOworICAgICAgICBmaW5pc2hKU1Rlc3QoKTsKKyAgICB9LCBmdW5jdGlvbigpIHsKKyAg ICAgICAgdGVzdEZhaWxlZCgnRXJyb3IgY2FsbGJhY2sgaW52b2tlZCB1bmV4cGVjdGVkbHknKTsK KyAgICAgICAgZmluaXNoSlNUZXN0KCk7CisgICAgfSk7Cit9CisKK3ZhciBpZnJhbWUgPSBkb2N1 bWVudC5jcmVhdGVFbGVtZW50KCdpZnJhbWUnKTsKK2lmcmFtZS5zcmMgPSAncmVzb3VyY2VzL2Nh bGxiYWNrLXRvLXJlbW90ZS1jb250ZXh0LWlubmVyLmh0bWwnOworZG9jdW1lbnQuYm9keS5hcHBl bmRDaGlsZChpZnJhbWUpOworCit3aW5kb3cuanNUZXN0SXNBc3luYyA9IHRydWU7Cit3aW5kb3cu c3VjY2Vzc2Z1bGx5UGFyc2VkID0gdHJ1ZTsK