Bug 79527 – Add more tests for web intents

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Billock]

Add more tests for web intents

[Greg Billock]

Created attachment 128796 [details]
Patch

[Greg Billock]

Should I pull some of this JS fixture into a shared file? Or keep each test pretty self-contained?

[Adam Barth]

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]

Created attachment 129067 [details]
Patch

[Greg Billock]

(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 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]

Created attachment 129072 [details]
Patch

[Greg Billock]

(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]

The commit-queue encountered the following flaky tests while processing attachment 129072 [details]:

css3/filters/effect-invert-hw.html bug 79639 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129072 [details]
Patch

Clearing flags on attachment: 129072

Committed r109041: <http://trac.webkit.org/changeset/109041>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

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]

Reopening to attach new patch.

[Greg Billock]

Created attachment 129363 [details]
Patch

[Greg Billock]

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]

The commit-queue encountered the following flaky tests while processing attachment 129363 [details]:

css3/filters/effect-hue-rotate-hw.html bug 79845 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129363 [details]
Patch

Clearing flags on attachment: 129363

Committed r109236: <http://trac.webkit.org/changeset/109236>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Committed r109242: <http://trac.webkit.org/changeset/109242>

[Philippe Normand]

(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]

(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]

Thanks.