Bug 207352 – Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alan Sien Wei Hshieh]

Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes instead of just the primary frame. This could be a -[WKWebViewPrivate _getContentsAsStringInAllFrames:withCompletionHandler:].

[Alan Sien Wei Hshieh]

<rdar://59115798>

[Alan Sien Wei Hshieh]

Created attachment 389994 [details]
Patch

[Alan Sien Wei Hshieh]

Did I do this right :X ?

[Wenson Hsieh]

Comment on attachment 389994 [details]
Patch

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

We'd ideally want an API test for this, as well.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:270
> +- (void)_getContentsAsStringInAllFrames:(BOOL)allFrames withCompletionHandler:(void (^)(NSString *, NSError *))completionHandler WK_API_AVAILABLE(macos(10.15), ios(13.0));

The availability should be `WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))`.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3459
> +        WebFrame* webFrame = WebFrame::fromCoreFrame(*frame);

Nit - this would be ever so slightly cleaner as:

if (auto* webFrame = WebFrame::fromCoreFrame(*frame))
    resultString.append(webFrame->contentsAsString());

[Alan Sien Wei Hshieh]

Created attachment 390035 [details]
Patch

[Alan Sien Wei Hshieh]

Created attachment 390049 [details]
Patch

[Alan Sien Wei Hshieh]

(attempting to build)

[Wenson Hsieh]

Comment on attachment 390035 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKPage.h:250
> +typedef void (*WKPageGetContentsAsStringInAllFramesFunction)(WKStringRef, WKErrorRef, void*);
> +WK_EXPORT void WKPageGetContentsAsStringInAllFrames(WKPageRef page, void* context, WKPageGetContentsAsStringInAllFramesFunction function);
> +

If there aren't any clients that need C API, I think we should avoid adding it (we've been pushing SPI clients towards using the more modern, Objective-C API when possible).

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3457
> +    String resultString = "";

I /think/ using StringBuilder and toString() here would be more efficient than calling String::append here, which seems to copy the contents of the string every time.

[Alan Sien Wei Hshieh]

Created attachment 390052 [details]
Patch

[Wenson Hsieh]

Comment on attachment 390052 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 390052 [details]
Patch

Rejecting attachment 390052 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 390052, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/13318632

[Alex Christensen]

Comment on attachment 390052 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.h:1081
>      void getContentsAsString(WTF::Function<void (const String&, CallbackBase::Error)>&&);
> +    void getContentsAsStringInAllFrames(WTF::Function<void(const String&, CallbackBase::Error)>&&);

Instead of making a new function doing basically the same thing, could you pass a parameter?
enum class InAllFrames : bool { No, Yes };

[Alan Sien Wei Hshieh]

There are existing clients which I don't want to break by augmenting a method. My plan was to add this and then move all clients over and then ideally, remove the original method.

[Alan Sien Wei Hshieh]

Oh I understand now.

[Alan Sien Wei Hshieh]

@Alex How do you recommend I expose enum class InAllFrames : bool { No, Yes }; to WKWebView.mm, WebPageProxy.h, and WebPage.h?

[Alex Christensen]

In _getContentsAsStringInAllFrames:withCompletionHandler: call _page->getContentsAsString(allFrames ? InAllFrames::Yes : InAllFrames::No, ...);
In _getContentsAsStringWithCompletionHandler: call _page->getContentsAsString(InAllFrames::No, ...);

[Alan Sien Wei Hshieh]

WKWebView doesn't have a concept of InAllFrames so this won't build. Am I missing something?

[Alex Christensen]

You'll have to put the definition of InAllFrames in a place that it can access.

[Alan Sien Wei Hshieh]

Created attachment 390154 [details]
Patch

[Darin Adler]

Comment on attachment 390154 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:269
>  - (void)_getContentsAsStringWithCompletionHandler:(void (^)(NSString *, NSError *))completionHandler WK_API_AVAILABLE(macos(10.13), ios(11.0));
> +- (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:

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3461
> +                builder.append(webFrame->contentsAsString());

Seems risky to concatenate frames without separating them with something like a newline.

[Alex Christensen]

Comment on attachment 390154 [details]
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.

[Alan Sien Wei Hshieh]

I should be able to get rid of the bool at the interface level. Books I believe will never want subframe content. I’ll add in a line break after each concatenation and remove the error also.

[Darin Adler]

(In reply to Alan Sien Wei Hshieh from comment #22)
> I’ll add in a line break after
> each concatenation and remove the error also.

On reflection maybe two line breaks would be even better.

[Alan Sien Wei Hshieh]

Created attachment 390298 [details]
Patch

[Alan Sien Wei Hshieh]

Created attachment 390299 [details]
Patch

[Alan Sien Wei Hshieh]

I updated the patch making sure to do two line breaks per Darin's comment above.

[Alan Sien Wei Hshieh]

Created attachment 390303 [details]
Patch

[Alex Christensen]

Comment on attachment 390303 [details]
Patch

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

r=me

> Source/WebKit/Shared/ContentAsStringIncludesChildFrames.h:28
> +#include <wtf/EnumTraits.h>

Not needed.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3463
> +                    builder.append('\n');
> +                    builder.append('\n');

Are you sure you want two newlines?

[Alan Sien Wei Hshieh]

Will remove the include.

I double checked with my management and it seems they are pro-double-new-lines, so I think that is what we'll go with.

[Alan Sien Wei Hshieh]

Created attachment 390316 [details]
Patch

[Alan Sien Wei Hshieh]

Created attachment 390318 [details]
Patch

[Alan Sien Wei Hshieh]

Whoops.. don't mind me... amending.

[Alan Sien Wei Hshieh]

Created attachment 390320 [details]
Patch

[Alex Christensen]

Comment on attachment 390320 [details]
Patch

Management-designed software is the best software :/

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390320 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390320 [details]
Patch

Clearing flags on attachment: 390320

Committed r256236: <https://trac.webkit.org/changeset/256236>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59334314>

[Darin Adler]

Comment on attachment 390320 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3464
> +                if (!builder.isEmpty()) {
> +                    builder.append('\n');
> +                    builder.append('\n');
> +                }

Someone could come back here and tweak this. It’s slightly more efficient like this:

    if (!builder.isEmpty())
        builder.appendLiteral("\n\n");

[Darin Adler]

Comment on attachment 390320 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3459
> +        for (RefPtr<Frame> frame = &corePage()->mainFrame(); frame; frame = frame->tree().traverseNextRendered()) {

Just noticed that the code above using m_mainFrame, but we use corePage()->mainFrame() here. I think we can use m_mainFrame too.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 391410 [details]
Address comments

[WebKit Commit Bot]

Comment on attachment 391410 [details]
Address comments

Clearing flags on attachment: 391410

Committed r257155: <https://trac.webkit.org/changeset/257155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.