Bug 49563 – [chromium] Fix negative letter spacing in complex text on linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Simonsen]

[chromium] Fix negative letter spacing in complex text on linux

[James Simonsen]

Created attachment 74048 [details]
Patch

[James Simonsen]

Enabling letter spacing on linux revealed that we had made some false assumptions that spacing would always be non-negative. That's not the case and this patch will correctly handle negative spacing.

Since there's a new layout test. I'll have somewhere here land it and then we'll rebaseline the necessary platforms once the bots fail. Let me know if anyone knows an easier way to add layout tests.

[James Simonsen]

(In reply to comment #2)
> Enabling letter spacing on linux revealed that we had made some false assumptions that spacing would always be non-negative. That's not the case and this patch will correctly handle negative spacing.
> 
> Since there's a new layout test. I'll have somewhere here land it and then we'll rebaseline the necessary platforms once the bots fail. Let me know if anyone knows an easier way to add layout tests.

Oops, that should be "...someone here land it..."

[Tony Chang]

Comment on attachment 74048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74048&action=review

One option for chromium win/mac is to mark the test as expected to fail, which gives you a chance to rebaseline it without turning the tree red.  gtk, qt, and win, you'll just have to break the tree and grab the results from the waterfall.

> WebCore/platform/graphics/chromium/FontLinux.cpp:54
> +using namespace std;

Can we do without this?

> WebCore/platform/graphics/chromium/FontLinux.cpp:561
> +    m_pixelWidth = max(position, 0.0);

std::max seems safer to avoid ambiguity.

[James Simonsen]

Created attachment 74052 [details]
Patch

[Tony Chang]

I can land this and the updated baselines for you tomorrow.

[James Simonsen]

(In reply to comment #6)
> I can land this and the updated baselines for you tomorrow.

Thanks! That would really help a lot.

[Tony Chang]

Committed r72239: <http://trac.webkit.org/changeset/72239>

[James Simonsen]

(In reply to comment #8)
> Committed r72239: <http://trac.webkit.org/changeset/72239>

Thanks so much!

[Tony Chang]

Win results: http://trac.webkit.org/changeset/72244
Skip on qt/gtk: http://trac.webkit.org/changeset/72254
Chromium-win results: http://trac.webkit.org/changeset/72255

[David Levin]

Please be kind to gardeners :) 

Add new tests to test_expectations.txt in your patch until you get the chance to land rebaselines.

[Tony Chang]

(In reply to comment #11)
> Please be kind to gardeners :) 
> 
> Add new tests to test_expectations.txt in your patch until you get the chance to land rebaselines.

I thought we did?  I didn't see any of the chromium bots go red.

http://trac.webkit.org/changeset/72239/trunk/LayoutTests/platform/chromium/test_expectations.txt

[David Levin]

(In reply to comment #12)
> (In reply to comment #11)
> > Please be kind to gardeners :) 
> > 
> > Add new tests to test_expectations.txt in your patch until you get the chance to land rebaselines.
> 
> I thought we did?  I didn't see any of the chromium bots go red.
> 
> http://trac.webkit.org/changeset/72239/trunk/LayoutTests/platform/chromium/test_expectations.txt

webkit.org bot for chromium don't run layout tests. You need to watch the canary.

Fixed here: http://trac.webkit.org/changeset/72277

Why is chromium mac different from the mac baseline checked in.... I don't know.

[Tony Chang]

(In reply to comment #13)
> webkit.org bot for chromium don't run layout tests. You need to watch the canary.
> 
> Fixed here: http://trac.webkit.org/changeset/72277
> 
> Why is chromium mac different from the mac baseline checked in.... I don't know.

Ah, you're right.  Sorry about that, I missed it when I was watching the canary bots.

[David Levin]

(In reply to comment #14)
> (In reply to comment #13)
> > webkit.org bot for chromium don't run layout tests. You need to watch the canary.
> > 
> > Fixed here: http://trac.webkit.org/changeset/72277
> > 
> > Why is chromium mac different from the mac baseline checked in.... I don't know.
> 
> Ah, you're right.  Sorry about that, I missed it when I was watching the canary bots.

No worries. I was just attempting a friendly reminder.