Bug 149732 – Cleaning up after revision 190339

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Receiving new comments regarding to the revision.

[Jiewen Tan]

Created attachment 262309 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262309 [details]
Patch

Rejecting attachment 262309 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Myles C. Maxfield]

Comment on attachment 262309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262309&action=review

> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
> -layer at (0,0) size 800x600

Please replace this file instead of deleting it.

[Jiewen Tan]

Comment on attachment 262309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262309&action=review

>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>> -layer at (0,0) size 800x600
> 
> Please replace this file instead of deleting it.

Got you. I wonder under what condition should I use the TestExpectations file instead of generating a *-expected file?

[Jiewen Tan]

Comment on attachment 262309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262309&action=review

>>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>>> -layer at (0,0) size 800x600
>> 
>> Please replace this file instead of deleting it.
> 
> Got you. I wonder under what condition should I use the TestExpectations file instead of generating a *-expected file?

What other kinds of test-expected files can I generate for this case except for the render tree dump one? Do you have any suggestion?

[Jiewen Tan]

Created attachment 262455 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 262455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262455&action=review

> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
> -layer at (0,0) size 800x600

You still should replace this file instead of deleting it.

[Myles C. Maxfield]

Comment on attachment 262455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262455&action=review

>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>> -layer at (0,0) size 800x600
> 
> You still should replace this file instead of deleting it.

The file is replaced. With nothing.

[WebKit Commit Bot]

Comment on attachment 262455 [details]
Patch

Clearing flags on attachment: 262455

Committed r190579: <http://trac.webkit.org/changeset/190579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 262455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262455&action=review

>>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>>> -layer at (0,0) size 800x600
>> 
>> You still should replace this file instead of deleting it.
> 
> The file is replaced. With nothing.

How did this get r+'ed and landed despite this comment? The test result is now missing.

> LayoutTests/svg/custom/invalid-xslt-crash.svg:7
> +        if (window.testRunner)
> +            testRunner.dumpAsText();

This didn't work, the test still dumps a render tree.

[Alexey Proskuryakov]

Reverted these test changes in http://trac.webkit.org/r190586

[Jiewen Tan]

Comment on attachment 262455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262455&action=review

>>>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>>>> -layer at (0,0) size 800x600
>>> 
>>> You still should replace this file instead of deleting it.
>> 
>> The file is replaced. With nothing.
> 
> How did this get r+'ed and landed despite this comment? The test result is now missing.

In my local commit, the status of this file is marked as changed not deleted.

>> LayoutTests/svg/custom/invalid-xslt-crash.svg:7
>> +            testRunner.dumpAsText();
> 
> This didn't work, the test still dumps a render tree.

Why? What should I do then to make it a text dump?

[Said Abou-Hallawa]

Comment on attachment 262455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262455&action=review

>>>>> LayoutTests/svg/custom/invalid-xslt-crash-expected.txt:-1
>>>>> -layer at (0,0) size 800x600
>>>> 
>>>> You still should replace this file instead of deleting it.
>>> 
>>> The file is replaced. With nothing.
>> 
>> How did this get r+'ed and landed despite this comment? The test result is now missing.
> 
> In my local commit, the status of this file is marked as changed not deleted.

Your expected file should include some text like PASS, Passed, Passed if it does not crash, or something like that instead of having an empty result file.

> LayoutTests/svg/custom/invalid-xslt-crash.svg:2
>  <svg xmlns="http://www.w3.org/2000/svg"

What is the use of <?xml-stylesheet type="application/xml" href=""?> at the beginning of the SVG? It looks like this is preventing any script from running.

>>> LayoutTests/svg/custom/invalid-xslt-crash.svg:7
>>> +            testRunner.dumpAsText();
>> 
>> This didn't work, the test still dumps a render tree.
> 
> Why? What should I do then to make it a text dump?

Remove <?xml-stylesheet type="application/xml" href=""?> and your test should be dumped as text.

> LayoutTests/svg/custom/invalid-xslt-crash.svg:11
> +    <xslt:attribute nnnnnnnnnnname="fill">lime</xslt:attribute>

Your test should include something like 
<text>PASS</text>
or <foreignObject><p  xmlns="http://www.w3.org/1999/xhtml">PASS</p></foreignObject>
or you can create it from the script also.

[Jiewen Tan]

Reopening to attach new patch.

[Jiewen Tan]

Created attachment 262548 [details]
Patch

[Myles C. Maxfield]

Please wait to cq+ this until EWS comes back up and says everything is green.

[Myles C. Maxfield]

Please re-upload the patch.

[Jiewen Tan]

Created attachment 262701 [details]
Patch

[Jiewen Tan]

Created attachment 262710 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262710 [details]
Patch

Clearing flags on attachment: 262710

Committed r190757: <http://trac.webkit.org/changeset/190757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.