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
Alan Sien Wei Hshieh
2020-02-06 14:05:50 PST
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. 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> All reviewed patches have been landed. Closing bug. |