Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes instead of just the primary frame. This could be a -[WKWebViewPrivate _getContentsAsStringInAllFrames:withCompletionHandler:].
<rdar://59115798>
Created attachment 389994 [details] Patch
Did I do this right :X ?
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());
Created attachment 390035 [details] Patch
Created attachment 390049 [details] Patch
(attempting to build)
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.
Created attachment 390052 [details] Patch
Comment on attachment 390052 [details] Patch r=me
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 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 };
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.
Oh I understand now.
@Alex How do you recommend I expose enum class InAllFrames : bool { No, Yes }; to WKWebView.mm, WebPageProxy.h, and WebPage.h?
In _getContentsAsStringInAllFrames:withCompletionHandler: call _page->getContentsAsString(allFrames ? InAllFrames::Yes : InAllFrames::No, ...); In _getContentsAsStringWithCompletionHandler: call _page->getContentsAsString(InAllFrames::No, ...);
WKWebView doesn't have a concept of InAllFrames so this won't build. Am I missing something?
You'll have to put the definition of InAllFrames in a place that it can access.
Created attachment 390154 [details] Patch
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 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.
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.
(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.
Created attachment 390298 [details] Patch
Created attachment 390299 [details] Patch
I updated the patch making sure to do two line breaks per Darin's comment above.
Created attachment 390303 [details] Patch
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?
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.
Created attachment 390316 [details] Patch
Created attachment 390318 [details] Patch
Whoops.. don't mind me... amending.
Created attachment 390320 [details] Patch
Comment on attachment 390320 [details] Patch Management-designed software is the best software :/
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 on attachment 390320 [details] Patch Clearing flags on attachment: 390320 Committed r256236: <https://trac.webkit.org/changeset/256236>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59334314>
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 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.
Reopening to attach new patch.
Created attachment 391410 [details] Address comments
Comment on attachment 391410 [details] Address comments Clearing flags on attachment: 391410 Committed r257155: <https://trac.webkit.org/changeset/257155>