WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
134645
[WK2] Expose a few drawing/compositing settings on WKPreferences(Private)
https://bugs.webkit.org/show_bug.cgi?id=134645
Summary
[WK2] Expose a few drawing/compositing settings on WKPreferences(Private)
Tim Horton
Reported
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.
Attachments
patch
(15.32 KB, patch)
2014-07-05 01:22 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
patch with new names
(15.14 KB, patch)
2014-07-06 23:28 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
patch with no verbs
(15.36 KB, patch)
2014-07-08 13:46 PDT
,
Tim Horton
mitz: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2014-07-05 01:22:53 PDT
Created
attachment 234433
[details]
patch
mitz
Comment 2
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.
Tim Horton
Comment 3
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?
Darin Adler
Comment 4
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?
mitz
Comment 5
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.
Darin Adler
Comment 6
2014-07-06 17:53:35 PDT
Thanks, Dan. Sounds good.
Tim Horton
Comment 7
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?
mitz
Comment 8
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;
Tim Horton
Comment 9
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.
Tim Horton
Comment 10
2014-07-06 23:28:36 PDT
Created
attachment 234481
[details]
patch with new names
Tim Horton
Comment 11
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.
Tim Horton
Comment 12
2014-07-08 13:46:45 PDT
Created
attachment 234592
[details]
patch with no verbs
Tim Horton
Comment 13
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
Tim Horton
Comment 14
2014-07-08 15:25:25 PDT
http://trac.webkit.org/changeset/170896
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug