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: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Tim Horton
2012-02-09 20:22:09 PST
Created attachment 126443 [details]
patch
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. (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 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 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 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. Created attachment 127866 [details]
Patch
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. Landed in http://trac.webkit.org/changeset/108832 |