| Summary: | CSS 2.1 failure: outline-color-* tests fail | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||
| Component: | CSS | Assignee: | Robert Hogan <robert> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | jchaffraix, robert | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 72648 | ||||||||||
| Bug Blocks: | 47141 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Robert Hogan
2011-11-09 10:31:16 PST
Created attachment 114315 [details]
Patch
Created attachment 114322 [details]
Patch
Comment on attachment 114322 [details] Patch Looks great. Too bad the EWS was borked the day you submitted the patch. 2 comments: * it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests? * the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific. (In reply to comment #3) > (From update of attachment 114322 [details]) > Looks great. Too bad the EWS was borked the day you submitted the patch. > > 2 comments: > * it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests? They're from the CSS 2.1 test suite so although I agree they would be better as reftests the place to do that is in the public css test suite I think. > * the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific. Yes, unfortunately the text in the tests makes their results platform specific. If you look at CSS2.1/20110323 all the results sit in platform folders. On a positive note there *are* some ref tests in the CSS2.1 test suite so we will be able to adopt them as such once 60605 is landed. (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 114322 [details] [details]) > > Looks great. Too bad the EWS was borked the day you submitted the patch. > > > > 2 comments: > > * it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests? > > They're from the CSS 2.1 test suite so although I agree they would be better as reftests the place to do that is in the public css test suite I think. I was thinking something along those line would happen. Could you make sure to post a follow-up on the public mailing list pointing this short-coming. For most browsers, this would be a good change as this would avoid the need of many different baselines. > > * the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific. > > Yes, unfortunately the text in the tests makes their results platform specific. If you look at CSS2.1/20110323 all the results sit in platform folders. Fair enough, I should have known that the explanations for each tests would get us... Committed r100652: <http://trac.webkit.org/changeset/100652> Created attachment 115843 [details]
Patch
Comment on attachment 115843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115843&action=review > Source/WebCore/rendering/RenderObject.cpp:1155 > + if (outer.isEmpty()) It took me some time to figure out why this was needed. Please add a comment about the need to bail out when offset (could you rename it to outlineOffset for coherency) is negative and bigger than outlineWidth (mentioning fast/borders/outline-offset-min-assert.html and/or bug 12042). Also this is a FIXME as this does not match FireFox anymore! Committed r102526: <http://trac.webkit.org/changeset/102526> |