Bug 176123

Created attachment 319396 [details] proposed patch

Attachment 319396 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/WebView/WebFrame.mm:2295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2579: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2589: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2600: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:441: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 319421 [details] proposed patch Build fix for accidental edit.

Attachment 319421 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/WebView/WebFrame.mm:2295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2579: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2589: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2600: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 319421 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=319421&action=review > Source/WebCore/editing/cocoa/EditorCocoa.mm:57 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) This occurs many times in this patch. Could we replace it with #if HAVE(SOMETHING)?

Seems like the layering violation could be avoided by adding an EditorClient function.

> This occurs many times in this patch. Could we replace it with #if HAVE(SOMETHING)? We could, but why? Checking versions in this manner is the norm in WebKit, and it makes it slightly easier to eventually remove the old code. > Seems like the layering violation could be avoided by adding an EditorClient function. I'm not sure what you mean by this. There is a whole chain of violations here, the worst being that AppKit calls into WebKitLegacy.

Created attachment 319467 [details] proposed patch More build fixing.

Attachment 319467 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/WebView/WebFrame.mm:2295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2579: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2589: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2600: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 319467 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=319467&action=review I don’t completely understand this patch, but I will say r=me I guess. > Source/WebCore/editing/cocoa/EditorCocoa.mm:57 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) Instead of repeating this conditional three times, in its two different forms (old and new version), can we define something at the top of the file and use that three times? Or, better, define it in the header file so it can be used in WebKitLegacy too. > Source/WebCore/editing/cocoa/EditorCocoa.mm:157 > + // This function needs to be kept in sync with identically named one in WebKitLegacy, which is used on older OS versions. Can we change WebKitLegacy to call this instead of keeping two copies around? > Source/WebCore/editing/cocoa/EditorCocoa.mm:179 > + @"WebResourceHandler": [WebArchiveResourceAsWebResourceHandler class], TextKit code expects a WebResourceHandler to be an object, but you chose to implement it as a class instead. This will work, but seems an unusual choice. > Source/WebCore/editing/cocoa/EditorCocoa.mm:191 > + NSString *htmlFragmentString = [string _htmlDocumentFragmentString:NSMakeRange(0, [string length]) documentAttributes:attributesForAttributedStringConversion() subresources:&subresources]; I think the name here could be fragmentString, no need to prefix with "html". > Source/WebCore/editing/cocoa/EditorCocoa.mm:192 > + RefPtr<DocumentFragment> fragment = DocumentFragment::create(document); The result of DocumentFragment::create is a Ref, not a RefPtr. I suggest using auto here and letting this local variable be a Ref. > Source/WebCore/editing/cocoa/EditorCocoa.mm:195 > + result.fragment = fragment; Should use WTFMove here to avoid a tiny bit of reference count churn. > Source/WebCore/editing/cocoa/WebArchiveResourceAsWebResourceHandler.h:38 > +@interface WebArchiveResourceAsWebResourceHandler : NSObject { > +@package > + RefPtr<WebCore::ArchiveResource> resource; > +} > +@end As far as I can tell, this class itself is a WebResourceHandle. Then instances of the class are returned as WebResource objects. Both sort of treated as “informal protocols”. Do we really have to combine both together in one class like that? There is economy in doing it that way, but also subtlety that I find a bit confusing. The name of the class is also misleading, because the instances are not WebResourceHandler but that is what the class name implies. Normally the class name describes the role of the instances, not the role of the class object. I think it would be good to emphasize the WebResourceHandler interface we intend to implement by defining it here in the interface, not just the implementation: + (id)resourceForData:(NSData *)data URL:(NSURL *)url MIMEType:(NSString *)mimeType textEncodingName:(NSString *)textEncodingName frameName:(NSString *)frameName; I think it would be good to emphasize the WebResource interface we intend to implement by defining it here in the interface: @property (nonatomic, readonly, copy) NSData *data; @property (nonatomic, readonly, strong) NSURL *URL; @property (nonatomic, readonly, copy) NSString *MIMEType; @property (nonatomic, readonly, copy) NSString *textEncodingName; @property (nonatomic, readonly, copy) NSString *frameName; Except it seems that we only implement the URL property. Why is it OK for us to not implement the other properties? > Source/WebCore/editing/cocoa/WebArchiveResourceAsWebResourceHandler.mm:45 > + resource = WebCore::ArchiveResource::create(SharedBuffer::create([[data copy] autorelease]), URL, MIMEType, textEncodingName, frameName, nil); Seems unnecessary to use autorelease here. Could use RetainPtr instead, by writing "adoptNS([data copy]).get()". > Source/WebCore/editing/cocoa/WebArchiveResourceAsWebResourceHandler.mm:57 > + if (!resource) > + return nil; I don’t think this is needed, because the init function will self destruct if the resource is nil. > Source/WebKitLegacy/mac/Misc/WebNSURLExtras.h:-77 > -#if TARGET_OS_IPHONE > -// FIXME: This method name needs a prefix. > -+ (NSURL *)uniqueURLWithRelativePart:(NSString *)relativePart; > -#endif You checked that there are no clients outside WebKit depending on this? > Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:440 > +// FIXME: Remove this function once we don't use it on any supported platform. This comment has wording that is confusing. We already don’t use this function. It’s just here to make the "exp" file not cause a build failure. The comment should say something more like this: // FIXME: Remove both this stub and the real version of this function below once we don't need the real version on any supported platform. // This stub is not used outside WebKit, but it's here so we won't get a linker error. > Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1 > -/* > + /* Should undo this change. > Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7582 > -// This is used by AppKit and is included here so that WebDataProtocolScheme is only defined once. > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) > @implementation NSURL (WebDataURL) > > + (NSURL *)_web_uniqueWebDataURL There is a call to this method in -[NSHTMLReader readDocumentFragment:]. Why is it OK to not compile it in newer versions of iOS and macOS?

Comment on attachment 319467 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=319467&action=review >> Source/WebKitLegacy/mac/Misc/WebNSURLExtras.h:-77 >> -#endif > > You checked that there are no clients outside WebKit depending on this? As far as I can tell, there's no use of this function outside WebKit at least in OS itself. >> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7582 >> + (NSURL *)_web_uniqueWebDataURL > > There is a call to this method in -[NSHTMLReader readDocumentFragment:]. Why is it OK to not compile it in newer versions of iOS and macOS? Indeed this appears to be still used.

Comment on attachment 319467 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=319467&action=review > Source/WebCore/platform/URL.h:78 > - static URL fakeURLWithRelativePart(const String&); > + WEBCORE_EXPORT static URL fakeURLWithRelativePart(const String&); We should either wrap this in if-def PLATFORM(COCOA) or rename it to something scary like deprecatedFakeURLWithRelativePart so that people don't start using it elsewhere.

Comment on attachment 319467 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=319467&action=review >> Source/WebCore/editing/cocoa/WebArchiveResourceAsWebResourceHandler.h:38 >> +@end > > As far as I can tell, this class itself is a WebResourceHandle. Then instances of the class are returned as WebResource objects. Both sort of treated as “informal protocols”. Do we really have to combine both together in one class like that? There is economy in doing it that way, but also subtlety that I find a bit confusing. The name of the class is also misleading, because the instances are not WebResourceHandler but that is what the class name implies. Normally the class name describes the role of the instances, not the role of the class object. > > I think it would be good to emphasize the WebResourceHandler interface we intend to implement by defining it here in the interface, not just the implementation: > > + (id)resourceForData:(NSData *)data URL:(NSURL *)url MIMEType:(NSString *)mimeType textEncodingName:(NSString *)textEncodingName frameName:(NSString *)frameName; > > I think it would be good to emphasize the WebResource interface we intend to implement by defining it here in the interface: > > @property (nonatomic, readonly, copy) NSData *data; > @property (nonatomic, readonly, strong) NSURL *URL; > @property (nonatomic, readonly, copy) NSString *MIMEType; > @property (nonatomic, readonly, copy) NSString *textEncodingName; > @property (nonatomic, readonly, copy) NSString *frameName; > > Except it seems that we only implement the URL property. Why is it OK for us to not implement the other properties? I'll split this into two classes. The archive class is not meant to mimic WebResource though - the only accessor called by TextKit is -URL, and it's arguably a bug that it relies on a method that's not documented to be required. Naming is a challenge for me. Here is what I have for now: - WebArchiveResourceWebResourceHandler - objects of this class are passed as values for the "WebResourceHandler" key. - WebArchiveResourceFromNSAttributedString - the handler creates those, and they are returned from the NSAttributedString call. >>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7582 >>> + (NSURL *)_web_uniqueWebDataURL >> >> There is a call to this method in -[NSHTMLReader readDocumentFragment:]. Why is it OK to not compile it in newer versions of iOS and macOS? > > Indeed this appears to be still used. It is only used in implementation of an SPI call that is no longer used after this patch. But I agree, it is not right to remove it earlier than the SPI.

Created attachment 321245 [details] patch for landing In the interest of getting this landed finally, I didn't implement some of the improvement suggestions (unifying compile time checks, de-duplicating the code, renaming fakeURLWithRelativePart). Also fixed issues with attribute dictionary - one object was leaking, and another was getting over-released.

Attachment 321245 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/WebView/WebFrame.mm:2298: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed http://trac.webkit.org/r222239

<rdar://problem/34693918>

Comment on attachment 321245 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=321245&action=review > Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7293 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) -_web_uniqueWebDataURL is still being used in shipping macOS. This change caused Mail to crash when pasting rich text into a compose window.

I'm not sure how I landed it with the #if, definitely intended to keep _web_uniqueWebDataURL intact. The other cause for crashing is that I overlooked NSAttributedString SPI still being used in -[WebHTMLView(WebPrivate) _documentFragmentFromPasteboard:forType:inContext:subresources:].

Reverted this unintended change in http://trac.webkit.org/r222676