Bug 176280 – [Cocoa] Tidy a few things in legacy WebHTMLView

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Tidy a few things in legacy WebHTMLView

[Darin Adler]

Created attachment 319731 [details]
Patch

Comment on attachment 319731 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:15
> +        (-[WebHTMLView _web_makePluginSubviewsPerformSelector:withObject:]): Moved up in the
> +        file so it can be shared by two plug-in methods below.

I thought modern Objective-C compilers didn’t care about the order methods were defined in the file!

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1622
> +    [subviews release];

Use RetainPtr instead?

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2593
> +        NSArray *localSubresources;

You seem to have left the definition of s on line 2587. Is it still used?

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4180
> +        // Don't send didDrawRect on iOS, it is not used by anyone and it is causing thread synchronization problems.

This comment is now more confusing than before.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4396
>  - (BOOL)_isSelectionEvent:(NSEvent *)event

Did you mean to keep the WebEvent version instead?

Comment on attachment 319731 [details]
Patch

r+ for a version that builds

[Darin Adler]

Comment on attachment 319731 [details]
Patch

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

>> Source/WebKitLegacy/mac/ChangeLog:15
>> +        file so it can be shared by two plug-in methods below.
> 
> I thought modern Objective-C compilers didn’t care about the order methods were defined in the file!

I think this was really about moving it into the appropriate category? Let me check.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1622
>> +    [subviews release];
> 
> Use RetainPtr instead?

Yes. Turns out really nice. Can just adoptNS/get inside the for loop instead of using local variable.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2593
>> +        NSArray *localSubresources;
> 
> You seem to have left the definition of s on line 2587. Is it still used?

It’s not. Merge mistake. Guess I did not recompile after merging!

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4180
>> +        // Don't send didDrawRect on iOS, it is not used by anyone and it is causing thread synchronization problems.
> 
> This comment is now more confusing than before.

I could delete the comment, or I could use this new one I wrote:

        // Note that the didDrawRect implemention here is entirely in Mac-specific code. That's OK because we
        // don't need support for didDrawRect on iOS; not required and a long while back it caused a threading problem.

I think I will include that new comment, but then some day someone will probably delete it without it ever doing anyone any good!

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4396
>>  - (BOOL)_isSelectionEvent:(NSEvent *)event
> 
> Did you mean to keep the WebEvent version instead?

I did. But, strangely, this incorrect code compiled fine on in the iOS EWS server. Makes me wonder what is going on.

[Darin Adler]

Created attachment 319795 [details]
Patch

[Darin Adler]

New patch has more changes in it, so needs review again, but I am guessing/hoping it will be Dan again.

Also not yet compiled on iOS so needs EWS love.

Comment on attachment 319795 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:36
> +        (-[WebHTMLView _shouldDeleteRange:]): Deleted.

Why? Presumably because it’s unused.

> Source/WebKitLegacy/mac/ChangeLog:40
> +        (-[WebHTMLView _web_setPrintingModeRecursive:adjustViewSize:]): Added. Helper method so

I’m not sure you need “web_” in these internal methods in a private class. Maybe worth doing because it’s an NSView subclass.

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:909
> +#if !PLATFORM(MAC)

Should this say #if PLATFORM(IOS)? Or perhaps start with the #if PLATFORM(MAC) case?

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:777
> +#define AUTOSCROLL_INTERVAL 0.1

Could turn this into a static const NSTimeInterval, or just inline it in the one place it’s used.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1008
> +static NSString *createUUIDString()
> +{
> +    return (NSString *)adoptCF(CFUUIDCreateString(kCFAllocatorDefault, adoptCF(CFUUIDCreate(kCFAllocatorDefault)).get())).autorelease();
> +}

Simpler to use all-Objective-C API these days: [NSUUID UUID].UUIDString, and probably not worth wrapping in a static function.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1406
> +        const unichar nonBreakingSpaceCharacter = 0xA0;
> +        NSString *nonBreakingSpaceString = [NSString stringWithCharacters:&nonBreakingSpaceCharacter length:1];

This is so awkward, and using the Objective-C string literal @"\u00A0" would also streamline the code.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2050
> +    } else if ([type isEqual:NSTIFFPboardType] && _private->promisedDragTIFFDataSource) {

Can use -isEqualToString:

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:3926
> +        static bool version3OrLaterClient = WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITHOUT_QUICKBOOKS_QUIRK);

The variable type perfectly matched the return type in the declaration of WebKitLinkedOnOrAfter!

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6427
> +    if (!_private->autoscrollTimer) {

Can reverse the condition and return early.

[Darin Adler]

Created attachment 319802 [details]
Patch

[Darin Adler]

Comment on attachment 319795 [details]
Patch

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

>> Source/WebKitLegacy/mac/ChangeLog:36
>> +        (-[WebHTMLView _shouldDeleteRange:]): Deleted.
> 
> Why? Presumably because it’s unused.

Yes, correct. I will specify.

>> Source/WebKitLegacy/mac/ChangeLog:40
>> +        (-[WebHTMLView _web_setPrintingModeRecursive:adjustViewSize:]): Added. Helper method so
> 
> I’m not sure you need “web_” in these internal methods in a private class. Maybe worth doing because it’s an NSView subclass.

I am also not sure. We certainly aren’t consistent about this nor is there a clear guideline that keeps us out of the way of engineering teams at Apple. I agree that when it’s a private class there is no concern about colliding with developers using WebKit.

I used the prefix here primarily because I was factoring code out of methods with names that were already using that prefix.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:909
>> +#if !PLATFORM(MAC)
> 
> Should this say #if PLATFORM(IOS)? Or perhaps start with the #if PLATFORM(MAC) case?

I suppose I could just take advantage of the fact that unused parameter warnings are off in this project and use #if PLATFORM(MAC) and not bother with UNUSED_PARAM.

But if not, then I like start with the “empty” case first because it’s so much smaller. When it’s #if/#else I really don’t know which is better. Here I think of it as “we have here code needed only on Mac, and not on not-Mac”, which is why I wrote it this way. I prefer not to change this one but I am slightly torn because you didn’t immediately understand/agree.

At various other call sites I prefer not to use #else, when it’s just two pieces of code that aren’t truly mutually exclusive. But for a function that just has two different implementations it’s tricky.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:777
>> +#define AUTOSCROLL_INTERVAL 0.1
> 
> Could turn this into a static const NSTimeInterval, or just inline it in the one place it’s used.

I chose to inline it where used since the constant name adds no clarity and there is no comment either.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1008
>> +}
> 
> Simpler to use all-Objective-C API these days: [NSUUID UUID].UUIDString, and probably not worth wrapping in a static function.

Thanks. That is better. And I checked and discovered that the UUID will stringify if we pass it as the value for "%@", obviating the need to invoke UUIDString explicitly.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1406
>> +        NSString *nonBreakingSpaceString = [NSString stringWithCharacters:&nonBreakingSpaceCharacter length:1];
> 
> This is so awkward, and using the Objective-C string literal @"\u00A0" would also streamline the code.

Yes, much better.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2050
>> +    } else if ([type isEqual:NSTIFFPboardType] && _private->promisedDragTIFFDataSource) {
> 
> Can use -isEqualToString:

Yes, fixed.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:3926
>> +        static bool version3OrLaterClient = WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITHOUT_QUICKBOOKS_QUIRK);
> 
> The variable type perfectly matched the return type in the declaration of WebKitLinkedOnOrAfter!

OK, undid this change.

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6427
>> +    if (!_private->autoscrollTimer) {
> 
> Can reverse the condition and return early.

Done.

[Darin Adler]

Created attachment 319807 [details]
Patch

[Darin Adler]

Committed r221559: <http://trac.webkit.org/changeset/221559>

[Radar WebKit Bug Importer]

<rdar://problem/34693921>