Add more tests for web intents
Created attachment 128796 [details] Patch
Should I pull some of this JS fixture into a shared file? Or keep each test pretty self-contained?
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.
Created attachment 129067 [details] Patch
(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.)
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.
Created attachment 129072 [details] Patch
(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.
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.
Comment on attachment 129072 [details] Patch Clearing flags on attachment: 129072 Committed r109041: <http://trac.webkit.org/changeset/109041>
All reviewed patches have been landed. Closing bug.
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
Reopening to attach new patch.
Created attachment 129363 [details] Patch
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.)
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.
Comment on attachment 129363 [details] Patch Clearing flags on attachment: 129363 Committed r109236: <http://trac.webkit.org/changeset/109236>
Committed r109242: <http://trac.webkit.org/changeset/109242>
(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
(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
Thanks.