Bug 207352

Summary: Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes.
Product: WebKit Reporter: Alan Sien Wei Hshieh <hshieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bweinstein, commit-queue, darin, hshieh, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
wenson_hsieh: review+, wenson_hsieh: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Address comments none

Description Alan Sien Wei Hshieh 2020-02-06 14:05:50 PST
Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes instead of just the primary frame. This could be a -[WKWebViewPrivate _getContentsAsStringInAllFrames:withCompletionHandler:].
Comment 1 Alan Sien Wei Hshieh 2020-02-06 14:06:15 PST
<rdar://59115798>
Comment 2 Alan Sien Wei Hshieh 2020-02-06 14:10:30 PST
Created attachment 389994 [details]
Patch
Comment 3 Alan Sien Wei Hshieh 2020-02-06 14:13:00 PST
Did I do this right :X ?
Comment 4 Wenson Hsieh 2020-02-06 16:01:22 PST
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());
Comment 5 Alan Sien Wei Hshieh 2020-02-06 17:54:38 PST
Created attachment 390035 [details]
Patch
Comment 6 Alan Sien Wei Hshieh 2020-02-06 19:46:13 PST
Created attachment 390049 [details]
Patch
Comment 7 Alan Sien Wei Hshieh 2020-02-06 19:52:52 PST
(attempting to build)
Comment 8 Wenson Hsieh 2020-02-06 20:10:55 PST
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.
Comment 9 Alan Sien Wei Hshieh 2020-02-06 20:38:33 PST
Created attachment 390052 [details]
Patch
Comment 10 Wenson Hsieh 2020-02-06 20:47:00 PST
Comment on attachment 390052 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2020-02-07 10:26:51 PST
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
Comment 12 Alex Christensen 2020-02-07 10:38:20 PST
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 };
Comment 13 Alan Sien Wei Hshieh 2020-02-07 11:12:12 PST
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.
Comment 14 Alan Sien Wei Hshieh 2020-02-07 11:13:24 PST
Oh I understand now.
Comment 15 Alan Sien Wei Hshieh 2020-02-07 12:38:46 PST
@Alex How do you recommend I expose enum class InAllFrames : bool { No, Yes }; to WKWebView.mm, WebPageProxy.h, and WebPage.h?
Comment 16 Alex Christensen 2020-02-07 12:47:08 PST
In _getContentsAsStringInAllFrames:withCompletionHandler: call _page->getContentsAsString(allFrames ? InAllFrames::Yes : InAllFrames::No, ...);
In _getContentsAsStringWithCompletionHandler: call _page->getContentsAsString(InAllFrames::No, ...);
Comment 17 Alan Sien Wei Hshieh 2020-02-07 12:59:55 PST
WKWebView doesn't have a concept of InAllFrames so this won't build. Am I missing something?
Comment 18 Alex Christensen 2020-02-07 15:12:19 PST
You'll have to put the definition of InAllFrames in a place that it can access.
Comment 19 Alan Sien Wei Hshieh 2020-02-07 17:48:42 PST
Created attachment 390154 [details]
Patch
Comment 20 Darin Adler 2020-02-09 21:02:42 PST
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.
Comment 21 Alex Christensen 2020-02-10 08:07:41 PST
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.
Comment 22 Alan Sien Wei Hshieh 2020-02-10 08:32:27 PST
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.
Comment 23 Darin Adler 2020-02-10 14:22:58 PST
(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.
Comment 24 Alan Sien Wei Hshieh 2020-02-10 14:51:21 PST
Created attachment 390298 [details]
Patch
Comment 25 Alan Sien Wei Hshieh 2020-02-10 14:54:36 PST
Created attachment 390299 [details]
Patch
Comment 26 Alan Sien Wei Hshieh 2020-02-10 14:55:23 PST
I updated the patch making sure to do two line breaks per Darin's comment above.
Comment 27 Alan Sien Wei Hshieh 2020-02-10 15:39:19 PST
Created attachment 390303 [details]
Patch
Comment 28 Alex Christensen 2020-02-10 15:41:50 PST
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?
Comment 29 Alan Sien Wei Hshieh 2020-02-10 16:33:15 PST
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.
Comment 30 Alan Sien Wei Hshieh 2020-02-10 16:48:04 PST
Created attachment 390316 [details]
Patch
Comment 31 Alan Sien Wei Hshieh 2020-02-10 16:53:14 PST
Created attachment 390318 [details]
Patch
Comment 32 Alan Sien Wei Hshieh 2020-02-10 16:55:15 PST
Whoops.. don't mind me... amending.
Comment 33 Alan Sien Wei Hshieh 2020-02-10 17:05:23 PST
Created attachment 390320 [details]
Patch
Comment 34 Alex Christensen 2020-02-10 17:43:15 PST
Comment on attachment 390320 [details]
Patch

Management-designed software is the best software :/
Comment 35 WebKit Commit Bot 2020-02-10 18:17:32 PST
The commit-queue encountered the following flaky tests while processing attachment 390320 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 36 WebKit Commit Bot 2020-02-10 18:18:11 PST
Comment on attachment 390320 [details]
Patch

Clearing flags on attachment: 390320

Committed r256236: <https://trac.webkit.org/changeset/256236>
Comment 37 WebKit Commit Bot 2020-02-10 18:18:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Radar WebKit Bug Importer 2020-02-10 18:19:17 PST Comment hidden (obsolete)
Comment 39 Darin Adler 2020-02-11 12:25:58 PST
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");
Comment 40 Darin Adler 2020-02-11 12:52:48 PST
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.
Comment 41 Wenson Hsieh 2020-02-21 10:49:36 PST
Reopening to attach new patch.
Comment 42 Wenson Hsieh 2020-02-21 10:49:37 PST
Created attachment 391410 [details]
Address comments
Comment 43 WebKit Commit Bot 2020-02-21 12:23:48 PST
Comment on attachment 391410 [details]
Address comments

Clearing flags on attachment: 391410

Committed r257155: <https://trac.webkit.org/changeset/257155>
Comment 44 WebKit Commit Bot 2020-02-21 12:23:50 PST
All reviewed patches have been landed.  Closing bug.