Bug 134645

Summary: [WK2] Expose a few drawing/compositing settings on WKPreferences(Private)
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, darin, mitz, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch with new names
none
patch with no verbs mitz: review+

Description Tim Horton 2014-07-04 23:46:31 PDT
We should expose a few of the really standard settings (layer borders, repaint counters, debug indicator, etc.) on WKPreferencesPrivate.
Comment 1 Tim Horton 2014-07-05 01:22:53 PDT
Created attachment 234433 [details]
patch
Comment 2 mitz 2014-07-05 09:36:25 PDT
Comment on attachment 234433 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:42
> +// FIXME: This should be setter=_setTelephoneNumberDetectionEnabled:, getter=_isTelephoneNumberDetectionEnabled, property=telephoneNumberDetectionEnabled

Why? Getter names shouldn’t be question phrases. They should fit naturally into conditions like this:
if ([object isHot] && [object isLarge] || [object cornersAreRound] && [object hasSharpEdges] || [object nightVisionIsEnabled])

> Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:48
> +@property (nonatomic, setter=_setCompositingBordersVisible:, getter=_areCompositingBordersVisible) BOOL _compositingBordersVisible;
> +@property (nonatomic, setter=_setCompositingRepaintCountersVisible:, getter=_areCompositingRepaintCountersVisible) BOOL _compositingRepaintCountersVisible;
> +@property (nonatomic, setter=_setTiledScrollingIndicatorVisible:, getter=_isTiledScrollingIndicatorVisible) BOOL _tiledScrollingIndicatorVisible;

Same comments about naming.
Comment 3 Tim Horton 2014-07-05 14:25:56 PDT
(In reply to comment #2)
> (From update of attachment 234433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234433&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:42
> > +// FIXME: This should be setter=_setTelephoneNumberDetectionEnabled:, getter=_isTelephoneNumberDetectionEnabled, property=telephoneNumberDetectionEnabled
> 
> Why? Getter names shouldn’t be question phrases. They should fit naturally into conditions like this:
> if ([object isHot] && [object isLarge] || [object cornersAreRound] && [object hasSharpEdges] || [object nightVisionIsEnabled])
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:48
> > +@property (nonatomic, setter=_setCompositingBordersVisible:, getter=_areCompositingBordersVisible) BOOL _compositingBordersVisible;
> > +@property (nonatomic, setter=_setCompositingRepaintCountersVisible:, getter=_areCompositingRepaintCountersVisible) BOOL _compositingRepaintCountersVisible;
> > +@property (nonatomic, setter=_setTiledScrollingIndicatorVisible:, getter=_isTiledScrollingIndicatorVisible) BOOL _tiledScrollingIndicatorVisible;
> 
> Same comments about naming.

I guess I got very confused by WKPreferences.h, which has getters like isJavaScriptEnabled and arePlugInsEnabled?
Comment 4 Darin Adler 2014-07-06 17:23:25 PDT
Comment on attachment 234433 [details]
patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:48
>> +@property (nonatomic, setter=_setTiledScrollingIndicatorVisible:, getter=_isTiledScrollingIndicatorVisible) BOOL _tiledScrollingIndicatorVisible;
> 
> Same comments about naming.

Dan, I think the “preferences” context is a bit more difficult. Here’s what we obviously don’t want:

    if ([preferences areConfiguredForThoseWhoPreferVisibleCompositingBorders])

What exactly is the good alternative? Is there a good example of this to model this class on?
Comment 5 mitz 2014-07-06 17:35:26 PDT
(In reply to comment #4)
> (From update of attachment 234433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234433&action=review
> 
> >> Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:48
> >> +@property (nonatomic, setter=_setTiledScrollingIndicatorVisible:, getter=_isTiledScrollingIndicatorVisible) BOOL _tiledScrollingIndicatorVisible;
> > 
> > Same comments about naming.
> 
> Dan, I think the “preferences” context is a bit more difficult. Here’s what we obviously don’t want:
> 
>     if ([preferences areConfiguredForThoseWhoPreferVisibleCompositingBorders])
> 
> What exactly is the good alternative?

Approximately
    if ([preferences compositingBordersVisible])
or
    if ([preferences compositingBordersEnabled])
or with the verb “are” after “borders”

> Is there a good example of this to model this class on?

I don’t know of any similar class that consistently avoid the question-phrase style. In the legacy WebPreferences class, for example, there is both a (good) example like -javaScriptCanOpenWindowsAutomatically and a (bad) example like -isJavaScriptEnabled.
Comment 6 Darin Adler 2014-07-06 17:53:35 PDT
Thanks, Dan. Sounds good.
Comment 7 Tim Horton 2014-07-06 17:57:45 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 234433 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=234433&action=review
> > Is there a good example of this to model this class on?
> 
> I don’t know of any similar class that consistently avoid the question-phrase style. In the legacy WebPreferences class, for example, there is both a (good) example like -javaScriptCanOpenWindowsAutomatically and a (bad) example like -isJavaScriptEnabled.

Sooooo, will we fix the 3 out of 5 actually-API prefs that are poorly-named? Or do they get a pass because they match the legacy WebKit names? Or some other reason?
Comment 8 mitz 2014-07-06 18:06:05 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 234433 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=234433&action=review
> > > Is there a good example of this to model this class on?
> > 
> > I don’t know of any similar class that consistently avoid the question-phrase style. In the legacy WebPreferences class, for example, there is both a (good) example like -javaScriptCanOpenWindowsAutomatically and a (bad) example like -isJavaScriptEnabled.
> 
> Sooooo, will we fix the 3 out of 5 actually-API prefs that are poorly-named? Or do they get a pass because they match the legacy WebKit names? Or some other reason?

Yes, I think these should be changed:

@property (nonatomic, getter=isJavaScriptEnabled) BOOL javaScriptEnabled;
@property (nonatomic, getter=isJavaEnabled) BOOL javaEnabled;
@property (nonatomic, getter=arePlugInsEnabled) BOOL plugInsEnabled;
Comment 9 Tim Horton 2014-07-06 23:21:01 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 234433 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=234433&action=review
> > > > Is there a good example of this to model this class on?
> > > 
> > > I don’t know of any similar class that consistently avoid the question-phrase style. In the legacy WebPreferences class, for example, there is both a (good) example like -javaScriptCanOpenWindowsAutomatically and a (bad) example like -isJavaScriptEnabled.
> > 
> > Sooooo, will we fix the 3 out of 5 actually-API prefs that are poorly-named? Or do they get a pass because they match the legacy WebKit names? Or some other reason?
> 
> Yes, I think these should be changed:
> 
> @property (nonatomic, getter=isJavaScriptEnabled) BOOL javaScriptEnabled;
> @property (nonatomic, getter=isJavaEnabled) BOOL javaEnabled;
> @property (nonatomic, getter=arePlugInsEnabled) BOOL plugInsEnabled;

I filed https://bugs.webkit.org/show_bug.cgi?id=134678 about that. I will post a new version of this patch with the names adjusted in a bit.
Comment 10 Tim Horton 2014-07-06 23:28:36 PDT
Created attachment 234481 [details]
patch with new names
Comment 11 Tim Horton 2014-07-07 12:50:07 PDT
We decided on no verbs in https://bugs.webkit.org/show_bug.cgi?id=134678, so I'll post another-other-other patch to match.
Comment 12 Tim Horton 2014-07-08 13:46:45 PDT
Created attachment 234592 [details]
patch with no verbs
Comment 13 Tim Horton 2014-07-08 14:41:39 PDT
Comment on attachment 234592 [details]
patch with no verbs

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

> Tools/MiniBrowser/mac/WK1BrowserWindowController.m:140
> +        [menuItem setState:[self layerBordersAreVisible] ? NSOnState : NSOffState];

remove the "Are"

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:150
> +        [menuItem setState:[self layerBordersAreVisible] ? NSOnState : NSOffState];

remove the Ares and Iss
Comment 14 Tim Horton 2014-07-08 15:25:25 PDT
http://trac.webkit.org/changeset/170896