Bug 164437 – Support TouchBar in WebKit

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Beth Dakin]

This bug tracks adding TouchBar support to WebKit.

rdar://problem/28876524

[Beth Dakin]

Created attachment 293959 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:531:  Missing space before ( in switch(  [whitespace/parens] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1067:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:41:  Should have space between @property and attributes.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:42:  Should have space between @property and attributes.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:43:  Should have space between @property and attributes.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:44:  Should have space between @property and attributes.  [whitespace/property] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:765:  Missing space before ( in switch(  [whitespace/parens] [5]
ERROR: Source/WebCore/platform/spi/cocoa/AVKitSPI.h:150:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
Total errors found: 10 in 21 files


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

[Tim Horton]

Comment on attachment 293959 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:176
> +#if HAVE(TOUCH_BAR)

I think this would look less ugly as its own class extension (you can have as many as you want!) but either way is fine.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4573
> +    return NO;

If HAVE(TOUCH_BAR) is true, you return twice?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1505
> +    return NO;

And here.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:971
> +            NSTextAlignment textAlignment;
> +            switch (editorState.postLayoutData().textAlignment) {
> +            case NoAlignment:
> +                textAlignment = NSTextAlignmentNatural;
> +                break;

Should factor this out. Maybe we already have one?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1111
> +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, (__bridge CFStringRef)NSTouchBarDidExitCustomization, 0);

__bridge? In non-ARC code?

> Source/WebKit/mac/WebView/WebView.mm:5136
> +#if HAVE(TOUCH_BAR)

Why not around the whole thing?

> Source/WebKit/mac/WebView/WebView.mm:6865
> +    NSRange resultRange = [nsResult range];

Dots.

> Source/WebKit/mac/WebView/WebView.mm:6868
> +    result.replacement = [nsResult replacementString];

Dots.

> Source/WebKit/mac/WebView/WebView.mm:9465
> +                NSTextAlignment textAlignment;
> +                switch (style->textAlign()) {
> +                case RIGHT:
> +                case WEBKIT_RIGHT:
> +                    textAlignment = NSTextAlignmentRight;

And again.

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 293959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293959&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:176
> > +#if HAVE(TOUCH_BAR)
> 
> I think this would look less ugly as its own class extension (you can have
> as many as you want!) but either way is fine.
> 

Good point. Fixed!

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4573
> > +    return NO;
> 
> If HAVE(TOUCH_BAR) is true, you return twice?
> 

Yeah, the compiler didn't complain, but I fixed it.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1505
> > +    return NO;
> 
> And here.
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:971
> > +            NSTextAlignment textAlignment;
> > +            switch (editorState.postLayoutData().textAlignment) {
> > +            case NoAlignment:
> > +                textAlignment = NSTextAlignmentNatural;
> > +                break;
> 
> Should factor this out. Maybe we already have one?
> 

I don't believe it exists anywhere already. This will be a good follow-up.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1111
> > +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, (__bridge CFStringRef)NSTouchBarDidExitCustomization, 0);
> 
> __bridge? In non-ARC code?
> 

Good point. It's been bothering me that this uses CFNotificationCenter instead of NSNotificationCenter, so I have changed it to use NSNotificationCenter.

> > Source/WebKit/mac/WebView/WebView.mm:5136
> > +#if HAVE(TOUCH_BAR)
> 
> Why not around the whole thing?
> 

Fixed.

> > Source/WebKit/mac/WebView/WebView.mm:6865
> > +    NSRange resultRange = [nsResult range];
> 
> Dots.
> 

Fixed.

> > Source/WebKit/mac/WebView/WebView.mm:6868
> > +    result.replacement = [nsResult replacementString];
> 
> Dots.
> 

Fixed.

> > Source/WebKit/mac/WebView/WebView.mm:9465
> > +                NSTextAlignment textAlignment;
> > +                switch (style->textAlign()) {
> > +                case RIGHT:
> > +                case WEBKIT_RIGHT:
> > +                    textAlignment = NSTextAlignmentRight;
> 
> And again.

Will do in follow-up.

[Beth Dakin]

Created attachment 293983 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

Created attachment 293985 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

iOS errors are pasted below. Why do we always get these nullability errors on lines of code that were not modified? All of the code I added to this header is even compiled out on iOS except for the gullibility specifiers that I added to get Mac to build.

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:71:47: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
@property (nonatomic, readonly) AVPlayerLayer *playerLayer;
                                              ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:72:67: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
@property (nonatomic, readonly) AVPictureInPicturePlayerLayerView *pictureInPicturePlayerLayerView;
                                                                  ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:86:54: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;
                                                     ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:90:4: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (instancetype)initWithPlayerLayerView:(__AVPlayerLayerView *)playerLayerView;
   ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:90:62: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (instancetype)initWithPlayerLayerView:(__AVPlayerLayerView *)playerLayerView;
                                                             ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:91:98: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (void)enterFullScreenAnimated:(BOOL)animated completionHandler:(void (^)(BOOL success, NSError *))completionHandler;
                                                                                                 ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:91:73: error: block pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (void)enterFullScreenAnimated:(BOOL)animated completionHandler:(void (^)(BOOL success, NSError *))completionHandler;
                                                                        ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:92:97: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (void)exitFullScreenAnimated:(BOOL)animated completionHandler:(void (^)(BOOL success, NSError *))completionHandler;
                                                                                                ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:92:72: error: block pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
- (void)exitFullScreenAnimated:(BOOL)animated completionHandler:(void (^)(BOOL success, NSError *))completionHandler;
                                                                       ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/spi/cocoa/AVKitSPI.h:98:50: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
@property (nonatomic, strong) AVPlayerController *playerController;
                                                 ^
10 errors generated.

(In reply to comment #9)
> iOS errors are pasted below. Why do we always get these nullability errors
> on lines of code that were not modified? All of the code I added to this
> header is even compiled out on iOS except for the gullibility specifiers
> that I added to get Mac to build.

If the compiler sees even one nullability specifier in a file, it enforces the use of nullability specifiers everywhere in the file.

The solution is normally to wrap the *SPI.h header in NS_ASSUME_NONNULL_BEGIN/END, assuming the headers it’s replicating have those wrappers.

[Beth Dakin]

(In reply to comment #10)
> (In reply to comment #9)
> > iOS errors are pasted below. Why do we always get these nullability errors
> > on lines of code that were not modified? All of the code I added to this
> > header is even compiled out on iOS except for the gullibility specifiers
> > that I added to get Mac to build.
> 
> If the compiler sees even one nullability specifier in a file, it enforces
> the use of nullability specifiers everywhere in the file.
> 
> The solution is normally to wrap the *SPI.h header in
> NS_ASSUME_NONNULL_BEGIN/END, assuming the headers it’s replicating have
> those wrappers.

OOooOooOooooO

[Beth Dakin]

Created attachment 293986 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

Created attachment 293987 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

Created attachment 293988 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

Created attachment 293989 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

[Beth Dakin]

Created attachment 293990 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:480:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:717:  Extra space in capture list.  [whitespace/brackets] [4]
Total errors found: 2 in 21 files


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

Comment on attachment 293990 [details]
Patch

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

> Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:27
> +#if USE(APPLE_INTERNAL_SDK) && HAVE(TOUCH_BAR)

Should this entire header be guarded with HAVE(TOUCH_BAR) regardless of whether you’re using the Apple-internal SDK? Seems unnecessary to have these Touch Bar things declared when the feature is not available.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4590
> +    if (!_impl->clientWantsMediaPlaybackControlsView())
> +        return nil;
> +    return _impl->mediaPlaybackControlsView();

Maybe write this as a single return statement using the ternary operator?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4602
> +- (void)_addMediaPlaybackControlsView:(AVFunctionBarScrubber *)mediaPlaybackControlsView
> +{
> +}
> +
> +- (void)_removeMediaPlaybackControlsView
> +{
> +}

Maybe comment in these that they are for subclasses to override.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1522
> +    if (!_data->_impl->clientWantsMediaPlaybackControlsView())
> +        return nil;
> +    return _data->_impl->mediaPlaybackControlsView();

Maybe write this as a single return statement using the ternary operator?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1534
> +- (void)_addMediaPlaybackControlsView:(AVFunctionBarScrubber *)mediaPlaybackControlsView
> +{
> +}
> +
> +- (void)_removeMediaPlaybackControlsView
> +{
> +}

Maybe comment in these that they are for subclasses to override.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:491
> +    void updateTouchBar();

Can this be in the HAVE(TOUCH_BAR) section so that you don’t have to add an empty implementation below?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:107
> +NSString * const WKMediaExitFullScreenItem = @"WKMediaExitFullScreenItem";

Should this be static? Doesn’t seem to need external linkage.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:451
> +    ListType _currentListType;
> +

Can this be @private too? Do you even need to declare it if you @synthesize it?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:467
> +static const double listControlSegmentWidth = 67;
> +static const double exitFullScreenButtonWidth = 64;

These should be CGFloat.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:471
> +static const unsigned noListSegment = 0;
> +static const unsigned unorderedListSegment = 1;
> +static const unsigned orderedListSegment = 2;

And these should be NSUInteger.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:493
> +    [[segments objectAtIndex:noListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeNone() forAttribute:NSAccessibilityDescriptionAttribute];
> +    [[segments objectAtIndex:unorderedListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeBulletedAccessibilityTitle() forAttribute:NSAccessibilityDescriptionAttribute];
> +    [[segments objectAtIndex:orderedListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeNumberedAccessibilityTitle() forAttribute:NSAccessibilityDescriptionAttribute];

You can use array subscripting instead of -objectAtIndex: here:

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:512
> +    if (insertListControl.selectedSegment == noListSegment) {

Maybe use a switch statement here (and thus also avoid calling -selectedSegment up to three times).

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:554
> +    BOOL _textIsBold;
> +    BOOL _textIsItalic;
> +    BOOL _textIsUnderlined;
> +    NSTextAlignment _currentTextAlignment;
> +    RetainPtr<NSColor> _textColor;
> +    RetainPtr<WKTextListTouchBarViewController> _wkTextListTouchBarViewController;

Can these all be @private too? Can you omit the ones that you @synthesize?
Probably don’t need wk in the name of the last one.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:642
> +    _webViewImpl->handleAcceptedCandidate((NSTextCheckingResult *)candidate);

I’d just declare candidate as NSTextCheckingResult * and avoid the cast here.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:688
> +    return (WKTextListTouchBarViewController *)[self textListViewController];

.textListViewController

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:744
> +    NSTextAlignment alignment = (NSTextAlignment)[self.textAlignments.cell tagForSegment:[self.textAlignments selectedSegment]];

.selectedSegment

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:802
> +namespace WebKit {

Newline after?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:844
> +        if ([[item identifier] isEqualToString:NSTouchBarItemIdentifierTextFormat]) {

.identifier

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:845
> +            for (NSTouchBarItem *childItem in [(NSGroupTouchBarItem *)item groupTouchBar].items) {

.groupTouchBar

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:889
> +    [touchBar setDefaultItems:[NSMutableSet setWithObjects:isRichTextTouchBar ? m_richTextCandidateListTouchBarItem.get() : m_plainTextCandidateListTouchBarItem.get(), nil]];

Can use setWithObject: and drop the trailing nil.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:959
> +        if (!m_emptyCandidatesArray)
> +            m_emptyCandidatesArray = adoptNS([[NSArray alloc] init]);
> +        [candidateListTouchBarItem() setCandidates:m_emptyCandidatesArray.get() forSelectedRange:NSMakeRange(0, 0) inString:nil];

Why do we need a member variable for the empty NSArray? Pretty sure @[ ] works just as well, and is always the same object.

[Beth Dakin]

(In reply to comment #22)
> Comment on attachment 293990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293990&action=review
> 
> > Source/WebCore/platform/spi/cocoa/NSTouchBarSPI.h:27
> > +#if USE(APPLE_INTERNAL_SDK) && HAVE(TOUCH_BAR)
> 
> Should this entire header be guarded with HAVE(TOUCH_BAR) regardless of
> whether you’re using the Apple-internal SDK? Seems unnecessary to have these
> Touch Bar things declared when the feature is not available.
> 

Yes, I think so. Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4590
> > +    if (!_impl->clientWantsMediaPlaybackControlsView())
> > +        return nil;
> > +    return _impl->mediaPlaybackControlsView();
> 
> Maybe write this as a single return statement using the ternary operator?
> 

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4602
> > +- (void)_addMediaPlaybackControlsView:(AVFunctionBarScrubber *)mediaPlaybackControlsView
> > +{
> > +}
> > +
> > +- (void)_removeMediaPlaybackControlsView
> > +{
> > +}
> 
> Maybe comment in these that they are for subclasses to override.
> 

Added comments.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1522
> > +    if (!_data->_impl->clientWantsMediaPlaybackControlsView())
> > +        return nil;
> > +    return _data->_impl->mediaPlaybackControlsView();
> 
> Maybe write this as a single return statement using the ternary operator?
> 

Fixed.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1534
> > +- (void)_addMediaPlaybackControlsView:(AVFunctionBarScrubber *)mediaPlaybackControlsView
> > +{
> > +}
> > +
> > +- (void)_removeMediaPlaybackControlsView
> > +{
> > +}
> 
> Maybe comment in these that they are for subclasses to override.
> 

Added comments.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:491
> > +    void updateTouchBar();
> 
> Can this be in the HAVE(TOUCH_BAR) section so that you don’t have to add an
> empty implementation below?
> 

Yes, fixed. 

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:107
> > +NSString * const WKMediaExitFullScreenItem = @"WKMediaExitFullScreenItem";
> 
> Should this be static? Doesn’t seem to need external linkage.
> 

Sure.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:451
> > +    ListType _currentListType;
> > +
> 
> Can this be @private too? Do you even need to declare it if you @synthesize
> it?
> 

Looks like @synthesize takes care of it!

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:467
> > +static const double listControlSegmentWidth = 67;
> > +static const double exitFullScreenButtonWidth = 64;
> 
> These should be CGFloat.

Fixed.

> 
> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:471
> > +static const unsigned noListSegment = 0;
> > +static const unsigned unorderedListSegment = 1;
> > +static const unsigned orderedListSegment = 2;
> 
> And these should be NSUInteger.
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:493
> > +    [[segments objectAtIndex:noListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeNone() forAttribute:NSAccessibilityDescriptionAttribute];
> > +    [[segments objectAtIndex:unorderedListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeBulletedAccessibilityTitle() forAttribute:NSAccessibilityDescriptionAttribute];
> > +    [[segments objectAtIndex:orderedListSegment] accessibilitySetOverrideValue:WebCore::insertListTypeNumberedAccessibilityTitle() forAttribute:NSAccessibilityDescriptionAttribute];
> 
> You can use array subscripting instead of -objectAtIndex: here:
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:512
> > +    if (insertListControl.selectedSegment == noListSegment) {
> 
> Maybe use a switch statement here (and thus also avoid calling
> -selectedSegment up to three times).
> 

Went with a switch.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:554
> > +    BOOL _textIsBold;
> > +    BOOL _textIsItalic;
> > +    BOOL _textIsUnderlined;
> > +    NSTextAlignment _currentTextAlignment;
> > +    RetainPtr<NSColor> _textColor;
> > +    RetainPtr<WKTextListTouchBarViewController> _wkTextListTouchBarViewController;
> 
> Can these all be @private too? Can you omit the ones that you @synthesize?
> Probably don’t need wk in the name of the last one.
> 

I made them @private. The compiler wasn't happy with just @synthesize.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:642
> > +    _webViewImpl->handleAcceptedCandidate((NSTextCheckingResult *)candidate);
> 
> I’d just declare candidate as NSTextCheckingResult * and avoid the cast here.
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:688
> > +    return (WKTextListTouchBarViewController *)[self textListViewController];
> 
> .textListViewController
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:744
> > +    NSTextAlignment alignment = (NSTextAlignment)[self.textAlignments.cell tagForSegment:[self.textAlignments selectedSegment]];
> 
> .selectedSegment
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:802
> > +namespace WebKit {
> 
> Newline after?
> 

Added.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:844
> > +        if ([[item identifier] isEqualToString:NSTouchBarItemIdentifierTextFormat]) {
> 
> .identifier
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:845
> > +            for (NSTouchBarItem *childItem in [(NSGroupTouchBarItem *)item groupTouchBar].items) {
> 
> .groupTouchBar
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:889
> > +    [touchBar setDefaultItems:[NSMutableSet setWithObjects:isRichTextTouchBar ? m_richTextCandidateListTouchBarItem.get() : m_plainTextCandidateListTouchBarItem.get(), nil]];
> 
> Can use setWithObject: and drop the trailing nil.
> 

Fixed.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:959
> > +        if (!m_emptyCandidatesArray)
> > +            m_emptyCandidatesArray = adoptNS([[NSArray alloc] init]);
> > +        [candidateListTouchBarItem() setCandidates:m_emptyCandidatesArray.get() forSelectedRange:NSMakeRange(0, 0) inString:nil];
> 
> Why do we need a member variable for the empty NSArray? Pretty sure @[ ]
> works just as well, and is always the same object.

So, it turns out this is broken! But @[ ] is equally broken. I think I will leave it as-if for now, and fix it with a follow-up, at which time, maybe @[ ] will work.

Patch coming soon.

[Beth Dakin]

Created attachment 294046 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:478:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:510:  Missing space before ( in switch(  [whitespace/parens] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:894:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:715:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:9286:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 21 files


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

[Wenson Hsieh]

Comment on attachment 294046 [details]
Patch

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

> Source/WebKit/mac/WebView/WebView.mm:9577
> +- (void)webViewAdditionsWillDestroyView

Could we rename this to -webViewWillDestroyView? (It looks like the "Additions" terminology is being dropped from other places).
Alternately, maybe we don't need a separate function on the WebView for performing cleanup in -[WebView dealloc] -- I think it makes sense just to move the cleanup logic directly to -dealloc and remove the -webViewWillDestroyView helper.

This also applies to the WebKit2 case (webViewImplAdditionsWillDestroyView)

[Beth Dakin]

(In reply to comment #26)
> Comment on attachment 294046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294046&action=review
> 
> > Source/WebKit/mac/WebView/WebView.mm:9577
> > +- (void)webViewAdditionsWillDestroyView
> 
> Could we rename this to -webViewWillDestroyView? (It looks like the
> "Additions" terminology is being dropped from other places).
> Alternately, maybe we don't need a separate function on the WebView for
> performing cleanup in -[WebView dealloc] -- I think it makes sense just to
> move the cleanup logic directly to -dealloc and remove the
> -webViewWillDestroyView helper.
> 
> This also applies to the WebKit2 case (webViewImplAdditionsWillDestroyView)

Good find. I removed the functions entirely.

[Beth Dakin]

Created attachment 294107 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:478:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:510:  Missing space before ( in switch(  [whitespace/parens] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:894:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:715:  Extra space in capture list.  [whitespace/brackets] [4]
ERROR: Source/WebKit/mac/WebView/WebView.mm:9286:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 21 files


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

[Darin Adler]

Comment on attachment 294107 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:750
> +    case NSTextAlignmentLeft: {

No need for these braces.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:755
> +    case NSTextAlignmentRight: {

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:760
> +    case NSTextAlignmentCenter: {

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:765
> +    case NSTextAlignmentJustified: {

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1043
> +            NSTextAlignment textAlignment;
> +            switch (editorState.postLayoutData().textAlignment) {
> +            case NoAlignment:
> +                textAlignment = NSTextAlignmentNatural;
> +                break;
> +            case LeftAlignment:
> +                textAlignment = NSTextAlignmentLeft;
> +                break;
> +            case RightAlignment:
> +                textAlignment = NSTextAlignmentRight;
> +                break;
> +            case CenterAlignment:
> +                textAlignment = NSTextAlignmentCenter;
> +                break;
> +            case JustifiedAlignment:
> +                textAlignment = NSTextAlignmentJustified;
> +                break;
> +            }

I’d like to see a helper function, inlined probably, for this conversion, so we can do things like ASSERT_NOT_REACHED if it’s an unknown value rather than leaving the textAlignment uninitialized, and also so the large switch statement isn’t so distracting in the middle of the code.

> Source/WebKit/mac/WebView/WebView.mm:901
> +    case NSTextAlignmentLeft: {

No braces needed here.

> Source/WebKit/mac/WebView/WebView.mm:906
> +    case NSTextAlignmentRight: {

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:911
> +    case NSTextAlignmentCenter: {

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:916
> +    case NSTextAlignmentJustified: {

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:6863
> +    switch ([nsResult resultType]) {
> +    case NSTextCheckingTypeSpelling:
> +        result.type = WebCore::TextCheckingTypeSpelling;
> +        break;
> +    case NSTextCheckingTypeReplacement:
> +        result.type = WebCore::TextCheckingTypeReplacement;
> +        break;
> +    case NSTextCheckingTypeCorrection:
> +        result.type = WebCore::TextCheckingTypeCorrection;
> +        break;
> +    default:
> +        result.type = WebCore::TextCheckingTypeNone;
> +    }

Helper function for this conversion?

> Source/WebKit/mac/WebView/WebView.mm:9484
> +                NSTextAlignment textAlignment;
> +                switch (style->textAlign()) {
> +                case RIGHT:
> +                case WEBKIT_RIGHT:
> +                    textAlignment = NSTextAlignmentRight;
> +                    break;
> +                case LEFT:
> +                case WEBKIT_LEFT:
> +                    textAlignment = NSTextAlignmentLeft;
> +                    break;
> +                case CENTER:
> +                case WEBKIT_CENTER:
> +                    textAlignment = NSTextAlignmentCenter;
> +                    break;
> +                case JUSTIFY:
> +                    textAlignment = NSTextAlignmentJustified;
> +                    break;
> +                case TASTART:
> +                    textAlignment = style->isLeftToRightDirection() ? NSTextAlignmentLeft : NSTextAlignmentRight;
> +                    break;
> +                case TAEND:
> +                    textAlignment = style->isLeftToRightDirection() ? NSTextAlignmentRight : NSTextAlignmentLeft;
> +                    break;
> +                }

Helper function for this?

[Beth Dakin]

Thanks Darin! I have incorporated your feedback, and I'm ready to land the patch when possible.

[Beth Dakin]

https://trac.webkit.org/changeset/208452

[Alex Christensen]

Fixed CMake build in http://trac.webkit.org/changeset/208949