Bug 180340 – Improve FontSelectionAlgorithm, including moving from IntegerHasher to Hasher

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Darin Adler]

Improve FontSelectionAlgorithm, including moving from IntegerHasher to Hasher

[Darin Adler]

Created attachment 328674 [details]
Patch

[EWS Watchlist]

Attachment 328674 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 328821 [details]
Patch

[EWS Watchlist]

Comment on attachment 328821 [details]
Patch

Attachment 328821 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5546666

New failing tests:
fast/text/vertical-quotation-marks.html

[EWS Watchlist]

Created attachment 328830 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

The mac-wk2 test failure looks like service worker and storage process problems unrelated to this patch.

[Darin Adler]

Created attachment 328917 [details]
Patch

[Darin Adler]

OK, this time tests passed on all bots. Ready for review.

[Daniel Bates]

Comment on attachment 328917 [details]
Patch

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

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:46
> +    FontSelectionValue() { }

This is minor. I prefer using = default. Although there is not much benefit here, I prefer using = default for consistency and because the defaulted default ccnstructor is considered a trivial constructor and allows the type to be aggregate initialized.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:98
> +    static const FontSelectionValue result(std::numeric_limits<BackingType>::max(), RawTag::RawTag);

No love for uniform initializer syntax?

[Darin Adler]

Comment on attachment 328917 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:46
>> +    FontSelectionValue() { }
> 
> This is minor. I prefer using = default. Although there is not much benefit here, I prefer using = default for consistency and because the defaulted default ccnstructor is considered a trivial constructor and allows the type to be aggregate initialized.

I used brace here to work around a bug in some compilers; otherwise I get a warning below in the normalItalicValue function. It warns that I am defining a constant object with no initialization in a class using the default constructor. I could try to find a workaround in the normalItalicValue function instead, perhaps passing a zero will make it work better.

>> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:98
>> +    static const FontSelectionValue result(std::numeric_limits<BackingType>::max(), RawTag::RawTag);
> 
> No love for uniform initializer syntax?

I’ll try switching to that. In this case there might be a problem because RawTag takes an int and the value of expression is a short.

[Daniel Bates]

Comment on attachment 328917 [details]
Patch

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

I like this patch, including the improved naming, comments and use of “using “ to make this code more readable and hackable.

>>> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:98
>>> +    static const FontSelectionValue result(std::numeric_limits<BackingType>::max(), RawTag::RawTag);
>> 
>> No love for uniform initializer syntax?
> 
> I’ll try switching to that. In this case there might be a problem because RawTag takes an int and the value of expression is a short.

If we made RawTag a regular enum as opposed to an enum class then we could drop the enum class name qualifier :/

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:334
> +    // FIXME: This name is no so great. Move this into the add function below

no  => not

[Darin Adler]

Created attachment 328953 [details]
Patch

[Darin Adler]

Created attachment 328954 [details]
Patch

[Darin Adler]

Created attachment 328982 [details]
Patch

[Darin Adler]

Created attachment 328983 [details]
Patch

[Darin Adler]

Created attachment 329077 [details]
Patch

[Darin Adler]

Committed r225769: <https://trac.webkit.org/changeset/225769>

[Radar WebKit Bug Importer]

<rdar://problem/35985791>

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Broke older gcc builds: https://bugs.webkit.org/show_bug.cgi?id=180692