Tidy a few things in legacy WebHTMLView
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
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.
Created attachment 319795 [details] Patch
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.
Created attachment 319802 [details] Patch
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.
Created attachment 319807 [details] Patch
Committed r221559: <http://trac.webkit.org/changeset/221559>
<rdar://problem/34693921>