Bug 134645 – [WK2] Expose a few drawing/compositing settings on WKPreferences(Private)

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

We should expose a few of the really standard settings (layer borders, repaint counters, debug indicator, etc.) on WKPreferencesPrivate.

[Tim Horton]

Created attachment 234433 [details]
patch

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.

[Tim Horton]

(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?

[Darin Adler]

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?

(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.

[Darin Adler]

Thanks, Dan. Sounds good.

[Tim Horton]

(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?

(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;

[Tim Horton]

(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.

[Tim Horton]

Created attachment 234481 [details]
patch with new names

[Tim Horton]

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.

[Tim Horton]

Created attachment 234592 [details]
patch with no verbs

[Tim Horton]

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

[Tim Horton]

http://trac.webkit.org/changeset/170896