RESOLVED FIXED 79527
Add more tests for web intents 
https://bugs.webkit.org/show_bug.cgi?id=79527
Summary Add more tests for web intents
Greg Billock
Reported 2012-02-24 13:26:51 PST
Add more tests for web intents
Attachments
Patch (11.89 KB, patch)
2012-02-24 13:30 PST, Greg Billock 		no flags
DetailsFormatted DiffDiff
Patch (12.37 KB, patch)
2012-02-27 10:38 PST, Greg Billock 		no flags
DetailsFormatted DiffDiff
Patch (12.66 KB, patch)
2012-02-27 11:10 PST, Greg Billock 		no flags
DetailsFormatted DiffDiff
Patch (12.79 KB, patch)
2012-02-28 17:33 PST, Greg Billock 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Greg Billock
Comment 1 2012-02-24 13:30:46 PST
Created attachment 128796 [details] Patch
Greg Billock
Comment 2 2012-02-24 13:31:49 PST
Should I pull some of this JS fixture into a shared file? Or keep each test pretty self-contained?
Adam Barth
Comment 3 2012-02-24 14:28:21 PST
Comment on attachment 128796 [details] Patch I would share the code in a JS file. We generally try to keep each directory fairly self-contained (with the exception of depending on js-test), but tests in a given directory often share resources, like JS files.
Greg Billock
Comment 4 2012-02-27 10:38:31 PST
Created attachment 129067 [details] Patch
Greg Billock
Comment 5 2012-02-27 10:39:26 PST
(In reply to comment #3) > (From update of attachment 128796 [details]) > I would share the code in a JS file. We generally try to keep each directory fairly self-contained (with the exception of depending on js-test), but tests in a given directory often share resources, like JS files. Done. (There wasn't as much refactorable as I hoped with existing tests, but I did do a little.)
Adam Barth
Comment 6 2012-02-27 10:58:17 PST
Comment on attachment 129067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129067&action=review > LayoutTests/webintents/web-intents-reply.html:11 > + if (window.layoutTestController) { > + window.layoutTestController.sendWebIntentResponse("reply"); > + } By the way, when a test doesn't work at all without layoutTestController, we usually include a message to that effect so that folks looking at the test in a normal browser don't get too confused. Certainly not a big deal.
Greg Billock
Comment 7 2012-02-27 11:10:48 PST
Created attachment 129072 [details] Patch
Greg Billock
Comment 8 2012-02-27 11:12:04 PST
(In reply to comment #6) > (From update of attachment 129067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129067&action=review > > > LayoutTests/webintents/web-intents-reply.html:11 > > + if (window.layoutTestController) { > > + window.layoutTestController.sendWebIntentResponse("reply"); > > + } > > By the way, when a test doesn't work at all without layoutTestController, we usually include a message to that effect so that folks looking at the test in a normal browser don't get too confused. Certainly not a big deal. I can do that. I saw a couple ways we're doing this. I stuck them in alerts.
WebKit Review Bot
Comment 9 2012-02-27 16:46:46 PST
The commit-queue encountered the following flaky tests while processing attachment 129072 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.
WebKit Review Bot
Comment 10 2012-02-27 16:49:08 PST
Comment on attachment 129072 [details] Patch Clearing flags on attachment: 129072 Committed r109041: <http://trac.webkit.org/changeset/109041>
WebKit Review Bot
Comment 11 2012-02-27 16:49:15 PST
All reviewed patches have been landed. Closing bug.
Kenichi Ishibashi
Comment 12 2012-02-27 20:51:54 PST
The patch broke Chromium win build. http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/21085 I rolled out r109041. http://trac.webkit.org/changeset/109069
Greg Billock
Comment 13 2012-02-28 17:33:51 PST
Reopening to attach new patch.
Greg Billock
Comment 14 2012-02-28 17:33:53 PST
Created attachment 129363 [details] Patch
Greg Billock
Comment 15 2012-02-28 18:18:17 PST
OK, I think I've fixed the problem. Windows was concerned about the call of the v8::String::New function with WebString.data(). I think it was right to be -- looking more carefully this function wants UTF8. I think the types match right now, but the chromium win_layout hasn't confirmed quite yet. (It's been timing out.)
WebKit Review Bot
Comment 16 2012-02-29 11:47:09 PST
The commit-queue encountered the following flaky tests while processing attachment 129363 [details]: css3/filters/effect-hue-rotate-hw.html bug 79845 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.
WebKit Review Bot
Comment 17 2012-02-29 11:51:17 PST
Comment on attachment 129363 [details] Patch Clearing flags on attachment: 129363 Committed r109236: <http://trac.webkit.org/changeset/109236>
WebKit Review Bot
Comment 18 2012-02-29 11:51:25 PST
All reviewed patches have been landed. Closing bug.
Dirk Pranke
Comment 19 2012-02-29 12:04:10 PST
Committed r109242: <http://trac.webkit.org/changeset/109242>
Philippe Normand
Comment 20 2012-03-01 04:05:39 PST
(In reply to comment #19) > Committed r109242: <http://trac.webkit.org/changeset/109242> It seems that patch broke NRWT on GTK: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/21901/steps/layout-test/logs/stdio
Philippe Normand
Comment 21 2012-03-01 05:18:25 PST
(In reply to comment #20) > (In reply to comment #19) > > Committed r109242: <http://trac.webkit.org/changeset/109242> > > It seems that patch broke NRWT on GTK: > > http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/21901/steps/layout-test/logs/stdio I landed a potential fix in r109337
Greg Billock
Comment 22 2012-03-01 10:04:58 PST
Thanks.
Note You need to log in before you can comment on or make changes to this bug.