Bug 78322 - SVG should be supported in Dashboard compatibility mode
Summary: SVG should be supported in Dashboard compatibility mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-09 20:22 PST by Tim Horton
Modified: 2012-02-24 11:41 PST (History)
3 users (show)

See Also:


Attachments
patch (34.76 KB, patch)
2012-02-09 20:33 PST, Tim Horton
zimmermann: review-
Details | Formatted Diff | Diff
patch (38.49 KB, patch)
2012-02-10 12:22 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (36.09 KB, patch)
2012-02-20 16:18 PST, Tim Horton
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-02-09 20:22:09 PST
We should revert http://trac.webkit.org/changeset/21418

<rdar://problem/5861278>

I have a patch.
Comment 1 Tim Horton 2012-02-09 20:33:46 PST
Created attachment 126443 [details]
patch
Comment 2 Nikolas Zimmermann 2012-02-10 00:02:47 PST
Comment on attachment 126443 [details]
patch

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

Resetting Olivers r+, the patch doesn't apply so we can't get ews results - I'd like to see that cr-linux ews is green, aka the expectations were updated, if needed.
Also one test is flaky, hence setting to r-:

> LayoutTests/http/tests/xmlhttprequest/svg-created-by-xhr-allowed-in-dashboard.html:68
> +<body onLoad="loadSVG();">

s/onLoad/onload/

> LayoutTests/svg/custom/embedded-svg-allowed-in-dashboard-expected.txt:3
> + PASS: Successfully embedded SVG in document

Where does the leading space come from?

> LayoutTests/svg/custom/svg-allowed-in-dashboard-object.html:16
> +    setTimeout("timeOut()", 500);

Ouch, this is flaky, and needs to be avoided. Try listening to <body onload="timeOut()"> instead -- you need to make sure all subresources loaded, before calling timeOut(), though we shouldn't pray it's done in 500ms, but instead listen to the right events.
Comment 3 Tim Horton 2012-02-10 00:04:27 PST
(In reply to comment #2)
> (From update of attachment 126443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126443&action=review
> 
> Resetting Olivers r+, the patch doesn't apply so we can't get ews results - I'd like to see that cr-linux ews is green, aka the expectations were updated, if needed.

The patch doesn't apply, but not for any discernible reason.

> Also one test is flaky, hence setting to r-:
> 
> > LayoutTests/http/tests/xmlhttprequest/svg-created-by-xhr-allowed-in-dashboard.html:68
> > +<body onLoad="loadSVG();">
> 
> s/onLoad/onload/
> 
> > LayoutTests/svg/custom/embedded-svg-allowed-in-dashboard-expected.txt:3
> > + PASS: Successfully embedded SVG in document
> 
> Where does the leading space come from?
> 
> > LayoutTests/svg/custom/svg-allowed-in-dashboard-object.html:16
> > +    setTimeout("timeOut()", 500);
> 
> Ouch, this is flaky, and needs to be avoided. Try listening to <body onload="timeOut()"> instead -- you need to make sure all subresources loaded, before calling timeOut(), though we shouldn't pray it's done in 500ms, but instead listen to the right events.

Hmm! I didn't look at the tests, I just swapped all the "PASS" and "FAIL"s :-D
Comment 4 Tim Horton 2012-02-10 12:22:02 PST
Created attachment 126553 [details]
patch

Fixed Niko's concerns; I'm sure the patch still won't apply (I bet it's the git mv that is broken), we can make any changes for other platforms later, but I don't expect there will be any.
Comment 5 Dean Jackson 2012-02-20 15:21:28 PST
Comment on attachment 126553 [details]
patch

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

Looks ok to me, but I was concentrating on code changes not tests. Maybe Niko wants another look?

It would be nice to resubmit and verify that the bots compile, etc.

> LayoutTests/http/tests/xmlhttprequest/svg-created-by-xhr-allowed-in-dashboard.html:6
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
> +<html>
> +<head>
> +<title>Test to ensure SVG is enabled in Dashboard compatibility mode</title>
> +<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
> +<script language="JavaScript" type="text/JavaScript">

Just do:

<!doctype html>
<html>
<head>
<title>...
<script>  <- no attributes

I realise you're just moving (and slightly editing) the file, but might as well clean it up.
Comment 6 Dean Jackson 2012-02-20 15:23:14 PST
Comment on attachment 126553 [details]
patch

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

> LayoutTests/svg/custom/svg-allowed-in-dashboard-object.html:2
> +<body onload="timeOut()">

See below.

> LayoutTests/svg/custom/svg-allowed-in-dashboard-object.html:16
> +    function timeOut() {

This function was named timeOut because it was being called from a setTimeout. I think we should rename it runTest or something.
Comment 7 Tim Horton 2012-02-20 16:18:29 PST
Created attachment 127866 [details]
Patch
Comment 8 Tim Horton 2012-02-20 16:19:23 PST
Just tried webkit-patch for the first time, let's see if that makes a patch the bots can deal with.

Other than that, I think I've addressed your changes.
Comment 9 Tim Horton 2012-02-24 11:41:47 PST
Landed in http://trac.webkit.org/changeset/108832