Bug 136849 - [GTK] Add a helper class for display system deduction
Summary: [GTK] Add a helper class for display system deduction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 136829 136831 136832 136833
  Show dependency treegraph
 
Reported: 2014-09-16 02:54 PDT by Zan Dobersek
Modified: 2014-09-17 05:36 PDT (History)
2 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2014-09-16 11:14 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (2.85 KB, patch)
2014-09-16 13:12 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (3.01 KB, patch)
2014-09-16 14:32 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-09-16 02:54:57 PDT
[GTK] Add a helper class for display system deduction
Comment 1 Zan Dobersek 2014-09-16 11:14:38 PDT
Created attachment 238188 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-16 11:17:58 PDT
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.
Comment 3 Martin Robinson 2014-09-16 11:24:54 PDT
Comment on attachment 238188 [details]
Patch

You can probably just tuck this into GtkUtilities.
Comment 4 Zan Dobersek 2014-09-16 13:12:29 PDT
Created attachment 238195 [details]
Patch
Comment 5 WebKit Commit Bot 2014-09-16 13:14:58 PDT
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.
Comment 6 Martin Robinson 2014-09-16 13:59:08 PDT
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.
Comment 7 Zan Dobersek 2014-09-16 14:32:23 PDT
Created attachment 238211 [details]
Patch
Comment 8 WebKit Commit Bot 2014-09-16 14:33:46 PDT
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.
Comment 9 Zan Dobersek 2014-09-16 14:35:29 PDT
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.
Comment 10 Zan Dobersek 2014-09-16 14:36:19 PDT
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.
Comment 11 Martin Robinson 2014-09-16 14:39:09 PDT
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.
Comment 12 Zan Dobersek 2014-09-17 04:52:56 PDT
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.
Comment 13 Zan Dobersek 2014-09-17 05:35:59 PDT
Comment on attachment 238211 [details]
Patch

Clearing flags on attachment: 238211

Committed r173690: <http://trac.webkit.org/changeset/173690>
Comment 14 Zan Dobersek 2014-09-17 05:36:08 PDT
All reviewed patches have been landed.  Closing bug.