RESOLVED FIXED 35495
[chromium] inverted logic in per-strike renderer patch
https://bugs.webkit.org/show_bug.cgi?id=35495
Summary [chromium] inverted logic in per-strike renderer patch
Evan Martin
Reported 2010-02-28 09:10:50 PST
[chromium] inverted logic in per-strike renderer patch
Attachments
Patch (1.29 KB, patch)
2010-02-28 09:11 PST, Evan Martin
levin: review-
patch (2.76 KB, patch)
2010-03-08 08:17 PST, Adam Langley
fishd: review-
patch (2.81 KB, patch)
2010-03-11 08:26 PST, Adam Langley
no flags
Evan Martin
Comment 1 2010-02-28 09:11:34 PST
Darin Fisher (:fishd, Google)
Comment 2 2010-03-01 11:38:57 PST
I'd like Adam Langley to review this patch. If he's happy, then LGTM.
Darin Fisher (:fishd, Google)
Comment 3 2010-03-01 11:39:26 PST
Also, is there a way to write a layout test for this?
David Levin
Comment 4 2010-03-01 15:31:05 PST
Comment on attachment 49699 [details] Patch Needs a layout test, or explanation of why a layout test isn't needed or isn't possible.
Adam Langley
Comment 5 2010-03-02 10:26:45 PST
Am working on Chrome side changes to enable a layout test of this change.
Adam Langley
Comment 6 2010-03-08 08:17:13 PST
Darin Fisher (:fishd, Google)
Comment 7 2010-03-09 16:24:51 PST
Comment on attachment 50223 [details] patch > +++ b/LayoutTests/fast/text/chromium-linux-fontconfig-renderstyle.html shouldn't there be some expected results for this test for other platforms? or should the test be skipped on other platforms? if not, then perhaps the test should not be named "chromium-linux-*"?
Adam Langley
Comment 8 2010-03-10 07:35:07 PST
(In reply to comment #7) > shouldn't there be some expected results for this test for other platforms? > or should the test be skipped on other platforms? if not, then perhaps the > test should not be named "chromium-linux-*"? The Chrome special layout tests seem to have disappeared, and I obviously can't include expected results until the patch is committed. If there's a test-expectations.txt like way to tell the webkit.org builders not to run a given test I'm afraid I don't know it. (I guess I mostly don't understand your comment, sorry!)
Evan Martin
Comment 9 2010-03-10 08:27:31 PST
It's suboptimal, but there it is: $ git ls-files | grep Skipped LayoutTests/platform/gtk/Skipped LayoutTests/platform/mac-leopard/Skipped LayoutTests/platform/mac-snowleopard/Skipped LayoutTests/platform/mac-tiger/Skipped LayoutTests/platform/mac/Skipped LayoutTests/platform/qt-linux/Skipped LayoutTests/platform/qt-mac/Skipped LayoutTests/platform/qt-win/Skipped LayoutTests/platform/qt/Skipped LayoutTests/platform/win/Skipped I'm not sure how a port-specific layout test is supposed to work.
Tony Chang
Comment 10 2010-03-10 17:04:25 PST
It looks like LayoutTests/platform/chromium already exists. Can you move the test there and put the expected png and txt file there? It should then be automatically skipped by the other webkit ports.
Adam Langley
Comment 11 2010-03-11 08:26:18 PST
Created attachment 50506 [details] patch Have moved the test into platform/chromium. I haven't included any expected results partly because --new-baseline is broken (that's a different issue) but mostly because it doesn't make any sense: the results will be different for each platform, so we can't put one set in platform/chromium. Also, our baselines are still carried in our Chromium tree I think.
WebKit Commit Bot
Comment 12 2010-03-18 08:41:57 PDT
Comment on attachment 50506 [details] patch Clearing flags on attachment: 50506 Committed r56162: <http://trac.webkit.org/changeset/56162>
WebKit Commit Bot
Comment 13 2010-03-18 08:42:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.