Bug 136849 – [GTK] Add a helper class for display system deduction

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

[Zan Dobersek]

[GTK] Add a helper class for display system deduction

[Zan Dobersek]

Created attachment 238188 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/gtk/DisplaySystemType.h:35:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 4 files


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

[Martin Robinson]

Comment on attachment 238188 [details]
Patch

You can probably just tuck this into GtkUtilities.

[Zan Dobersek]

Created attachment 238195 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/gtk/GtkUtilities.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 3 files


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

[Martin Robinson]

Comment on attachment 238195 [details]
Patch

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

Looks like the style error is a false positive. Apologies for my annoying review habits. Here are my final thoughts before landing.

> Source/WebCore/platform/gtk/GtkUtilities.cpp:75
> +        ASSERT(type != Unknown);

Here you can simply do ASSERT_NOT_REACHED and return X11. By having an unknown type, you are forcing switch statements to handle Unknown which is a fatal error anyway and should be caught earlier.

> Source/WebCore/platform/gtk/GtkUtilities.h:29
> +class DisplaySystemType {

I'm not sure this method needs its own class. Why not just define DisplaySystemType and a method called getDisplaySystemType?

> Source/WebCore/platform/gtk/GtkUtilities.h:32
> +        Unknown,

I think you can omit the Unknown type.

[Zan Dobersek]

Created attachment 238211 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/gtk/GtkUtilities.h:30:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 3 files


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

[Zan Dobersek]

Comment on attachment 238211 [details]
Patch

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

You don't have to apologize. I appreciate you taking the time and going through patches thoroughly.

> Source/WebCore/platform/gtk/GtkUtilities.cpp:67
> +    static DisplaySystemType type = [] {

This version simplifies ('simplifies') the initialization by using a lambda. This enables a properly applied ASSERT_NOT_REACHED() at the bottom of the labmda.

[Zan Dobersek]

Comment on attachment 238211 [details]
Patch

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

>> Source/WebCore/platform/gtk/GtkUtilities.cpp:67
>> +    static DisplaySystemType type = [] {
> 
> This version simplifies ('simplifies') the initialization by using a lambda. This enables a properly applied ASSERT_NOT_REACHED() at the bottom of the labmda.

Plus there's no need for the is-this-static-var-initialized-yet check.

[Martin Robinson]

Comment on attachment 238211 [details]
Patch

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

>>> Source/WebCore/platform/gtk/GtkUtilities.cpp:67
>>> +    static DisplaySystemType type = [] {
>> 
>> This version simplifies ('simplifies') the initialization by using a lambda. This enables a properly applied ASSERT_NOT_REACHED() at the bottom of the labmda.
> 
> Plus there's no need for the is-this-static-var-initialized-yet check.

Does that actually delay the execution of the code until the first call? If so, that's pretty awesome.

[Zan Dobersek]

Comment on attachment 238211 [details]
Patch

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

>>>> Source/WebCore/platform/gtk/GtkUtilities.cpp:67
>>>> +    static DisplaySystemType type = [] {
>>> 
>>> This version simplifies ('simplifies') the initialization by using a lambda. This enables a properly applied ASSERT_NOT_REACHED() at the bottom of the labmda.
>> 
>> Plus there's no need for the is-this-static-var-initialized-yet check.
> 
> Does that actually delay the execution of the code until the first call? If so, that's pretty awesome.

That's exactly what happens. The lambda is also completely optimized away when building with optimizations, so there's no code being generated under a separate symbol.

[Zan Dobersek]

Comment on attachment 238211 [details]
Patch

Clearing flags on attachment: 238211

Committed r173690: <http://trac.webkit.org/changeset/173690>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.