[webkit-reviews] review granted: [Bug 176123] Layering violation in Editor::createFragment : [Attachment 319467] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 176123: Layering violation in Editor::createFragment
https://bugs.webkit.org/show_bug.cgi?id=176123

Attachment 319467: proposed patch

https://bugs.webkit.org/attachment.cgi?id=319467&action=review




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 319467
  --> https://bugs.webkit.org/attachment.cgi?id=319467
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
> + at interface WebArchiveResourceAsWebResourceHandler : NSObject {
> + at package
> +    RefPtr<WebCore::ArchiveResource> resource;
> +}
> + at 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?