Summary: | Add more tests for web intents | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Billock <gbillock> | ||||||||||
Component: | New Bugs | Assignee: | Greg Billock <gbillock> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, bashi, dpranke, pnormand, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 79741 | ||||||||||||
Bug Blocks: | 75123 | ||||||||||||
Attachments: |
|
Description
Greg Billock
2012-02-24 13:26:51 PST
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> All reviewed patches have been landed. Closing bug. 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. |