WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
207352
Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler:] that includes contents from subframes.
https://bugs.webkit.org/show_bug.cgi?id=207352
Summary
Add a variant of -[WKWebViewPrivate _getContentsAsStringWithCompletionHandler...
Alan Sien Wei Hshieh
Reported
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:].
Attachments
Patch
(11.12 KB, patch)
2020-02-06 14:10 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(12.50 KB, patch)
2020-02-06 17:54 PST
,
Alan Sien Wei Hshieh
wenson_hsieh
: review+
wenson_hsieh
: commit-queue-
Details
Formatted Diff
Diff
Patch
(12.51 KB, patch)
2020-02-06 19:46 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(10.45 KB, patch)
2020-02-06 20:38 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.65 KB, patch)
2020-02-07 17:48 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.65 KB, patch)
2020-02-10 14:51 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.66 KB, patch)
2020-02-10 14:54 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.61 KB, patch)
2020-02-10 15:39 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.58 KB, patch)
2020-02-10 16:48 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(20.62 KB, patch)
2020-02-10 16:53 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Patch
(21.46 KB, patch)
2020-02-10 17:05 PST
,
Alan Sien Wei Hshieh
no flags
Details
Formatted Diff
Diff
Address comments
(1.92 KB, patch)
2020-02-21 10:49 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Alan Sien Wei Hshieh
Comment 1
2020-02-06 14:06:15 PST
<
rdar://59115798
>
Alan Sien Wei Hshieh
Comment 2
2020-02-06 14:10:30 PST
Created
attachment 389994
[details]
Patch
Alan Sien Wei Hshieh
Comment 3
2020-02-06 14:13:00 PST
Did I do this right :X ?
Wenson Hsieh
Comment 4
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());
Alan Sien Wei Hshieh
Comment 5
2020-02-06 17:54:38 PST
Created
attachment 390035
[details]
Patch
Alan Sien Wei Hshieh
Comment 6
2020-02-06 19:46:13 PST
Created
attachment 390049
[details]
Patch
Alan Sien Wei Hshieh
Comment 7
2020-02-06 19:52:52 PST
(attempting to build)
Wenson Hsieh
Comment 8
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.
Alan Sien Wei Hshieh
Comment 9
2020-02-06 20:38:33 PST
Created
attachment 390052
[details]
Patch
Wenson Hsieh
Comment 10
2020-02-06 20:47:00 PST
Comment on
attachment 390052
[details]
Patch r=me
WebKit Commit Bot
Comment 11
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
Alex Christensen
Comment 12
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 };
Alan Sien Wei Hshieh
Comment 13
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.
Alan Sien Wei Hshieh
Comment 14
2020-02-07 11:13:24 PST
Oh I understand now.
Alan Sien Wei Hshieh
Comment 15
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?
Alex Christensen
Comment 16
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, ...);
Alan Sien Wei Hshieh
Comment 17
2020-02-07 12:59:55 PST
WKWebView doesn't have a concept of InAllFrames so this won't build. Am I missing something?
Alex Christensen
Comment 18
2020-02-07 15:12:19 PST
You'll have to put the definition of InAllFrames in a place that it can access.
Alan Sien Wei Hshieh
Comment 19
2020-02-07 17:48:42 PST
Created
attachment 390154
[details]
Patch
Darin Adler
Comment 20
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.
Alex Christensen
Comment 21
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.
Alan Sien Wei Hshieh
Comment 22
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.
Darin Adler
Comment 23
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.
Alan Sien Wei Hshieh
Comment 24
2020-02-10 14:51:21 PST
Created
attachment 390298
[details]
Patch
Alan Sien Wei Hshieh
Comment 25
2020-02-10 14:54:36 PST
Created
attachment 390299
[details]
Patch
Alan Sien Wei Hshieh
Comment 26
2020-02-10 14:55:23 PST
I updated the patch making sure to do two line breaks per Darin's comment above.
Alan Sien Wei Hshieh
Comment 27
2020-02-10 15:39:19 PST
Created
attachment 390303
[details]
Patch
Alex Christensen
Comment 28
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?
Alan Sien Wei Hshieh
Comment 29
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.
Alan Sien Wei Hshieh
Comment 30
2020-02-10 16:48:04 PST
Created
attachment 390316
[details]
Patch
Alan Sien Wei Hshieh
Comment 31
2020-02-10 16:53:14 PST
Created
attachment 390318
[details]
Patch
Alan Sien Wei Hshieh
Comment 32
2020-02-10 16:55:15 PST
Whoops.. don't mind me... amending.
Alan Sien Wei Hshieh
Comment 33
2020-02-10 17:05:23 PST
Created
attachment 390320
[details]
Patch
Alex Christensen
Comment 34
2020-02-10 17:43:15 PST
Comment on
attachment 390320
[details]
Patch Management-designed software is the best software :/
WebKit Commit Bot
Comment 35
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.
WebKit Commit Bot
Comment 36
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
>
WebKit Commit Bot
Comment 37
2020-02-10 18:18:13 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 38
2020-02-10 18:19:17 PST
Comment hidden (obsolete)
<
rdar://problem/59334314
>
Darin Adler
Comment 39
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");
Darin Adler
Comment 40
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.
Wenson Hsieh
Comment 41
2020-02-21 10:49:36 PST
Reopening to attach new patch.
Wenson Hsieh
Comment 42
2020-02-21 10:49:37 PST
Created
attachment 391410
[details]
Address comments
WebKit Commit Bot
Comment 43
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
>
WebKit Commit Bot
Comment 44
2020-02-21 12:23:50 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug