Bug 35495 – [chromium] inverted logic in per-strike renderer patch

• Component: New Bugs
• Status: RESOLVED FIXED

[Evan Martin]

[chromium] inverted logic in per-strike renderer patch

[Evan Martin]

Created attachment 49699 [details]
Patch

[Darin Fisher (:fishd, Google)]

I'd like Adam Langley to review this patch.  If he's happy, then LGTM.

[Darin Fisher (:fishd, Google)]

Also, is there a way to write a layout test for this?

[David Levin]

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]

Am working on Chrome side changes to enable a layout test of this change.

[Adam Langley]

Created attachment 50223 [details]
patch

[Darin Fisher (:fishd, Google)]

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]

(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]

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]

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]

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 on attachment 50506 [details]
patch

Clearing flags on attachment: 50506

Committed r56162: <http://trac.webkit.org/changeset/56162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.