[webkit-reviews] review denied: [Bug 207352] Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes. : [Attachment 390154] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Alan Sien Wei Hshieh
<hshieh at apple.com>'s request for review:
Bug 207352: Add a variant of -[WKWebViewPrivate
_getContentsAsStringWithCompletionHandler:] that includes contents from
subframes.
https://bugs.webkit.org/show_bug.cgi?id=207352

Attachment 390154: Patch

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




--- Comment #21 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 390154
  --> https://bugs.webkit.org/attachment.cgi?id=390154
Patch

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

> Source/WebKit/Shared/ContentAsStringIncludesChildFrames.h:38
> +template<> struct EnumTraits<WebKit::ContentAsStringIncludesChildFrames> {

If you use a bool as the storage type of an enum, you don't need EnumTraits
because we know there are only two valid values: 0 and 1.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:269
>> +- (void)_getContentsAsStringInAllFrames:(BOOL)allFrames
withCompletionHandler:(void (^)(NSString *, NSError *))completionHandler
WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Do we really need the boolean here? Why not just have two separate methods?
I’d suggest the name:
> 
>     _getAllFrameContentsAsStringWithCompletionHandler:
> 
> Or maybe:
> 
>     _getContentsOfAllFramesAsStringWithCompletionHandler:

Do we know existing users of _getContentsAsStringWithCompletionHandler need to
not have subframe content, or do we just want to not change them out of
caution?
If we do need the boolean, should we deprecate
_getContentsAsStringWithCompletionHandler with this as a replacement?
Also, I don't think we need to have the NSError exposed in this SPI, even
though its predecessor had it.