Bug 177551 – Build fix for High Sierra 32 bit Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jonathan Bedard]

Some assertions have been removed form the SDK.  These assertions are used in some old carbon code, we need to replace them with equivalent WTF variants.  Additionally, since this carbon code is so old, it's still using tabs.  We need to do a bit of modernization so that this patch can land.

[Radar WebKit Bug Importer]

<rdar://problem/34690283>

[Jonathan Bedard]

Created attachment 321976 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:658:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:672:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/Carbon/HIWebView.mm:417:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1267:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 3 files


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

[Alexey Proskuryakov]

Comment on attachment 321976 [details]
Patch

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

There are lots of other opportunities for modernizing the code here. That's probably not worth it, as we may well survive another 15 years without updating it.

> Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:250
> +    

There are quite a few lines with trailing spaces, would be nice to get rid of them.

> Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:501
> +    if (osStatus==noErr && defaultButton)

Missing spaces around ==

> Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:656
> +    view = [HIViewAdapter getHIViewForNSView:aView];\

Spurious backslash here.

Please take a closer look at the other style checker warnings - it's usually not wrong about braces in multiline line control clauses.

> Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:669
> +    HIViewRef view = NULL;

We don't use NULL, should be nullptr or nil for Objective-C.

> Source/WebKitLegacy/mac/Carbon/CarbonWindowAdapter.mm:762
> +    if (eventType==NSAppKitDefined)
> +{

There should be indentation here. Missing spaces around ==.

[Jonathan Bedard]

Created attachment 321983 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321983 [details]
Patch for landing

Clearing flags on attachment: 321983

Committed r222567: <http://trac.webkit.org/changeset/222567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r222567 for reason:

This broke and internal build.

Committed r222571: <http://trac.webkit.org/changeset/222571>

[Jonathan Bedard]

(In reply to Matt Lewis from comment #8)
> Reverted r222567 for reason:
> 
> This broke and internal build.
> 
> Committed r222571: <http://trac.webkit.org/changeset/222571>

Actually, this would break an WebKit 32 bit debug build too.  The issue is surprising, the original code in two of the check(...) asserts was actually bad, but apparently WebKit's build was optimizing out the assert even on debug builds.

[Daniel Bates]

Comment on attachment 321983 [details]
Patch for landing

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

I would have preferred that the code style changes be be done in a separate patch from the actual build failure fix so as to make it easy to understand what the more important build fixes are. Not sure how long we need to keep this code. We could have done more clean up like making comments complete sentence and more.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:930
> +            /* Despite our efforts to record what view we made the first responder

Wrong comment style. The original code used the correct comment style.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:958
> +        /* Try to move on to the next or previous key view.  We use the laboriously

Ditto.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:982
> +        /* No view in this window has the keyboard focus.  This view should

Ditto.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:998
> +                /* The user has tabbed into this control view backwards.  Find

Ditto.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1047
> +        /*     If this control is not allowed to do autodisplay, don't let

Ditto.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1059
> +        /*  The Cocoa first responder does not correspond to the Carbon

Ditto.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1105
> +                SEL selector = _NSSelectorForHICommand(inCommand);

Although we are unlikely to ever modify this code and add new switch cases, the old style which put braces around the contents of this case was better as it scoped the variables.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1167
> +    case kHICommandUndo: return @selector(undo:);

Not the correct code style.

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:1465
> +//    printf("Update observer called\n");

Not the correct code style. Same issue throughout this function.

[Jonathan Bedard]

Created attachment 322085 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322085 [details]
Patch

Clearing flags on attachment: 322085

Committed r222614: <http://trac.webkit.org/changeset/222614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.