Bug 78322

Summary:

SVG should be supported in Dashboard compatibility mode

Product:

WebKit

Reporter:

Tim Horton <thorton>

Component:

SVG

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adele, oliver, zimmermann

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	zimmermann: review-
patch	none
Patch	dino: review+

Tim Horton

Reported 2012-02-09 20:22:09 PST

We should revert http://trac.webkit.org/changeset/21418 <rdar://problem/5861278> I have a patch.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Tim Horton

Comment 1 2012-02-09 20:33:46 PST

Created attachment 126443 [details] patch

Nikolas Zimmermann

Comment 2 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.

Tim Horton

Comment 3 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

Tim Horton

Comment 4 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.

Dean Jackson

Comment 5 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.

Dean Jackson

Comment 6 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.

Tim Horton

Comment 7 2012-02-20 16:18:29 PST

Created attachment 127866 [details] Patch

Tim Horton

Comment 8 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.

Tim Horton

Comment 9 2012-02-24 11:41:47 PST

Landed in http://trac.webkit.org/changeset/108832

Note You need to log in before you can comment on or make changes to this bug.