Bug 195636 - Add new NSAttributedString API for converting HTML
Summary: Add new NSAttributedString API for converting HTML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-12 12:46 PDT by Timothy Hatcher
Modified: 2019-03-21 05:42 PDT (History)
13 users (show)

See Also:


Attachments
Patch 1 / 4 (5.35 KB, patch)
2019-03-12 12:59 PDT, Timothy Hatcher
thorton: review+
Details | Formatted Diff | Diff
Patch 2 / 4 (55.68 KB, patch)
2019-03-12 13:01 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 3 / 4 (13.24 KB, patch)
2019-03-12 13:02 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 4 / 4 (37.45 KB, patch)
2019-03-12 13:03 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 1 / 4 (7.05 KB, patch)
2019-03-12 13:45 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 2 / 4 (55.48 KB, patch)
2019-03-12 17:04 PDT, Timothy Hatcher
rniwa: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Patch 3 / 4 (13.79 KB, patch)
2019-03-12 17:05 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (572.02 KB, application/zip)
2019-03-12 17:38 PDT, Build Bot
no flags Details
Patch 2 / 4 (56.17 KB, patch)
2019-03-12 20:29 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 2 / 4 (56.34 KB, patch)
2019-03-12 20:43 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 2 / 4 (56.06 KB, patch)
2019-03-12 20:49 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 2 / 4 (56.63 KB, patch)
2019-03-12 21:00 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 2 / 4 (57.11 KB, patch)
2019-03-13 13:27 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Build Fix (1.74 KB, patch)
2019-03-13 16:39 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Build Fix (1.07 KB, patch)
2019-03-14 11:06 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 3 / 4 (16.90 KB, patch)
2019-03-14 14:59 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 3 / 4 (14.65 KB, patch)
2019-03-15 11:39 PDT, Timothy Hatcher
thorton: review+
Details | Formatted Diff | Diff
Patch 3 / 4 (13.68 KB, patch)
2019-03-15 13:58 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 4 / 4 (38.35 KB, patch)
2019-03-15 15:46 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Build Fix (2.79 KB, patch)
2019-03-15 16:12 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 4 / 4 (39.15 KB, patch)
2019-03-15 16:55 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 4 / 4 (40.01 KB, patch)
2019-03-15 21:17 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch 4 / 4 (40.35 KB, patch)
2019-03-16 11:45 PDT, Timothy Hatcher
thorton: review+
thorton: commit-queue-
Details | Formatted Diff | Diff
Patch 4 / 4 (40.39 KB, patch)
2019-03-18 15:52 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2019-03-12 12:46:25 PDT
This moves us into the future and off deprecated WebView.

<rdar://problem/45055697>
Comment 1 Timothy Hatcher 2019-03-12 12:59:12 PDT Comment hidden (obsolete)
Comment 2 Timothy Hatcher 2019-03-12 13:01:40 PDT Comment hidden (obsolete)
Comment 3 Timothy Hatcher 2019-03-12 13:02:39 PDT Comment hidden (obsolete)
Comment 4 Timothy Hatcher 2019-03-12 13:03:23 PDT Comment hidden (obsolete)
Comment 5 Simon Fraser (smfr) 2019-03-12 13:25:22 PDT
Comment on attachment 364434 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:74
> +        return WEB_UI_STRING("Content failed to load as expected", "WKErrorContentFailedToLoad description");

The "as expected" seems unnecessary (and actually adds confusion).

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:77
> +        return WEB_UI_STRING("An action ended without completing", "WKErrorActionTimedOut description");

Can you be more specfic than "an action"?
Comment 6 Timothy Hatcher 2019-03-12 13:45:09 PDT
Created attachment 364438 [details]
Patch 1 / 4
Comment 7 WebKit Commit Bot 2019-03-12 15:42:55 PDT
Comment on attachment 364438 [details]
Patch 1 / 4

Clearing flags on attachment: 364438

Committed r242831: <https://trac.webkit.org/changeset/242831>
Comment 8 Timothy Hatcher 2019-03-12 17:04:35 PDT Comment hidden (obsolete)
Comment 9 Timothy Hatcher 2019-03-12 17:05:33 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-03-12 17:07:20 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-03-12 17:38:06 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-03-12 17:38:08 PDT Comment hidden (obsolete)
Comment 13 Daniel Bates 2019-03-12 18:51:57 PDT
Comment on attachment 364434 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:44
> +static const NSRect webViewRect = {{0, 0}, {800, 600}};

Static not necessary. Use constexpr. For this and all const below.

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:266
> +    if ([NSThread isMainThread])

RunLoop class?

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:269
> +        dispatch_async(dispatch_get_main_queue(), runConversion);

RunLoop class?
Comment 14 Daniel Bates 2019-03-12 18:53:28 PDT
Comment on attachment 364438 [details]
Patch 1 / 4

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

> Source/WebCore/editing/cocoa/HTMLConverter.mm:411
> +    Document& document = commonAncestorContainer->document();

Auto
Comment 15 Ryosuke Niwa 2019-03-12 19:08:50 PDT
Comment on attachment 364476 [details]
Patch 2 / 4

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

r=me assuming the issue that the encoder sometimes does not encode the right number of items is fixed.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:89
> +    // Secure coding check is after specific cases.

This just repeats what the code says. We should explain why we need to do this instead.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:105
> +    auto fontClass = [NSFont class];

Why don't we define PlatformFont somewhere above this line as we do in HTMLConverter
so that we don't have to have if-defs everywhere.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:126
> +        ASSERT(isSerializableValue(value));
> +        if (!isSerializableValue(value))
> +            continue;

This would make the length not match the number of the encoded items.
That would result in an encoding mismatch down the line and would result in a mysterious crash, etc...
We should probably add a boolean or something to indicate when this happens.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:213
> +        if (!isSerializableValue(key) || !isSerializableValue(value))
> +            continue;

Ditto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:504
> +namespace WTF {
> +template<> struct EnumTraits<IPC::NSType> {

Why not define this at the beginning of the file instead?
Comment 16 Timothy Hatcher 2019-03-12 20:29:58 PDT Comment hidden (obsolete)
Comment 17 Timothy Hatcher 2019-03-12 20:32:49 PDT
(In reply to Daniel Bates from comment #13)
> Comment on attachment 364434 [details]
> Patch 4 / 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364434&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:44
> > +static const NSRect webViewRect = {{0, 0}, {800, 600}};
> 
> Static not necessary. Use constexpr. For this and all const below.

Thanks!

> > Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:266
> > +    if ([NSThread isMainThread])
> 
> RunLoop class?
> 
> > Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:269
> > +        dispatch_async(dispatch_get_main_queue(), runConversion);
> 
> RunLoop class?

I stuck with Cocoa concepts, since this is a Cocoa API.
Comment 18 Build Bot 2019-03-12 20:33:31 PDT Comment hidden (obsolete)
Comment 19 Timothy Hatcher 2019-03-12 20:38:12 PDT
(In reply to Ryosuke Niwa from comment #15)
> Comment on attachment 364476 [details]
> Patch 2 / 4
> 
> > Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:105
> > +    auto fontClass = [NSFont class];
> 
> Why don't we define PlatformFont somewhere above this line as we do in
> HTMLConverter
> so that we don't have to have if-defs everywhere.

Makes sense, done!

> > Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:126
> > +        ASSERT(isSerializableValue(value));
> > +        if (!isSerializableValue(value))
> > +            continue;
> 
> This would make the length not match the number of the encoded items.
> That would result in an encoding mismatch down the line and would result in
> a mysterious crash, etc...
> We should probably add a boolean or something to indicate when this happens.

Good catch! I fixed these two cases by looping first and encoding a correct size.

> > Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:504
> > +namespace WTF {
> > +template<> struct EnumTraits<IPC::NSType> {
> 
> Why not define this at the beginning of the file instead?

It looks like we define these traits at the end in most cases.
Comment 20 Timothy Hatcher 2019-03-12 20:43:38 PDT Comment hidden (obsolete)
Comment 21 Timothy Hatcher 2019-03-12 20:44:59 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 364434 [details]
> Patch 4 / 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364434&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKError.mm:74
> > +        return WEB_UI_STRING("Content failed to load as expected", "WKErrorContentFailedToLoad description");
> 
> The "as expected" seems unnecessary (and actually adds confusion).
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKError.mm:77
> > +        return WEB_UI_STRING("An action ended without completing", "WKErrorActionTimedOut description");
> 
> Can you be more specfic than "an action"?

Thanks, I fixed these.
Comment 22 Build Bot 2019-03-12 20:46:01 PDT Comment hidden (obsolete)
Comment 23 Timothy Hatcher 2019-03-12 20:49:46 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-03-12 20:53:19 PDT Comment hidden (obsolete)
Comment 25 Timothy Hatcher 2019-03-12 21:00:33 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2019-03-12 21:02:49 PDT Comment hidden (obsolete)
Comment 27 Ryosuke Niwa 2019-03-12 22:02:02 PDT
Comment on attachment 364508 [details]
Patch 2 / 4

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

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:125
> +    RetainPtr<NSMutableIndexSet> validIndicies = adoptNS([[NSMutableIndexSet alloc] init]);

Why don't we use our own HashSet? It's quite a bit faster.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
> +        [validIndicies addIndex:i];

It's more efficient to store the invalid ones since we generally expect invalid set to be empty.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:152
> +        Optional<RetainPtr<id>> value = decodeObject(decoder, nil);

auto?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:212
> +    RetainPtr<NSMutableOrderedSet> validKeys = adoptNS([[NSMutableOrderedSet alloc] initWithCapacity:dictionary.count]);

Ditto about using HashSet and storing the invalid keys.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:242
> +        Optional<RetainPtr<id>> key = decodeObject(decoder, nil);

auto?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:246
> +        Optional<RetainPtr<id>> value = decodeObject(decoder, nil);

Ditto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:353
> +        encodeInternal(encoder, static_cast<NSArray *>(object));

I'd prefer calling these functions encode*Internal as done for decoding instead of
having a bunch of function overloading. But it's not a big deal even if we landed as is.
Comment 28 Timothy Hatcher 2019-03-13 13:27:04 PDT
Created attachment 364568 [details]
Patch 2 / 4
Comment 29 Build Bot 2019-03-13 13:29:33 PDT Comment hidden (obsolete)
Comment 30 WebKit Commit Bot 2019-03-13 14:17:46 PDT Comment hidden (obsolete)
Comment 31 WebKit Commit Bot 2019-03-13 14:18:40 PDT
Comment on attachment 364568 [details]
Patch 2 / 4

Clearing flags on attachment: 364568

Committed r242908: <https://trac.webkit.org/changeset/242908>
Comment 32 WebKit Commit Bot 2019-03-13 14:18:42 PDT Comment hidden (obsolete)
Comment 33 Timothy Hatcher 2019-03-13 15:35:48 PDT Comment hidden (obsolete)
Comment 34 Timothy Hatcher 2019-03-13 16:39:47 PDT
Created attachment 364590 [details]
Build Fix
Comment 35 WebKit Commit Bot 2019-03-13 17:17:29 PDT
Comment on attachment 364590 [details]
Build Fix

Clearing flags on attachment: 364590

Committed r242923: <https://trac.webkit.org/changeset/242923>
Comment 36 WebKit Commit Bot 2019-03-13 17:17:31 PDT Comment hidden (obsolete)
Comment 37 Timothy Hatcher 2019-03-14 11:06:41 PDT
Created attachment 364666 [details]
Build Fix
Comment 38 Timothy Hatcher 2019-03-14 11:06:56 PDT Comment hidden (obsolete)
Comment 39 WebKit Commit Bot 2019-03-14 11:40:56 PDT
Comment on attachment 364666 [details]
Build Fix

Clearing flags on attachment: 364666

Committed r242950: <https://trac.webkit.org/changeset/242950>
Comment 40 WebKit Commit Bot 2019-03-14 11:40:58 PDT Comment hidden (obsolete)
Comment 41 Timothy Hatcher 2019-03-14 14:59:19 PDT Comment hidden (obsolete)
Comment 42 Timothy Hatcher 2019-03-14 14:59:20 PDT Comment hidden (obsolete)
Comment 43 Build Bot 2019-03-14 15:01:53 PDT Comment hidden (obsolete)
Comment 44 Ryosuke Niwa 2019-03-14 22:28:35 PDT
Comment on attachment 364693 [details]
Patch 3 / 4

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

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:187
> +#if PLATFORM(COCOA)
> +    GetContentsAsAttributedString(WebKit::CallbackID callbackID)
> +#endif

Instead of manually managing a callback, can we use a completion handler?
e.g. GetContentsAsAttributedString() ->(struct WebKit::AttributedString result) Async
Comment 45 Tim Horton 2019-03-14 22:35:32 PDT
(In reply to Ryosuke Niwa from comment #44)
> Comment on attachment 364693 [details]
> Patch 3 / 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364693&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:187
> > +#if PLATFORM(COCOA)
> > +    GetContentsAsAttributedString(WebKit::CallbackID callbackID)
> > +#endif
> 
> Instead of manually managing a callback, can we use a completion handler?
> e.g. GetContentsAsAttributedString() ->(struct WebKit::AttributedString
> result) Async

Yes! Please use the new thing, it's so much better.
Comment 46 Timothy Hatcher 2019-03-15 11:39:36 PDT Comment hidden (obsolete)
Comment 47 Timothy Hatcher 2019-03-15 11:40:39 PDT
(In reply to Tim Horton from comment #45)
> (In reply to Ryosuke Niwa from comment #44)
> > Comment on attachment 364693 [details]
> > Patch 3 / 4
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=364693&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:187
> > > +#if PLATFORM(COCOA)
> > > +    GetContentsAsAttributedString(WebKit::CallbackID callbackID)
> > > +#endif
> > 
> > Instead of manually managing a callback, can we use a completion handler?
> > e.g. GetContentsAsAttributedString() ->(struct WebKit::AttributedString
> > result) Async
> 
> Yes! Please use the new thing, it's so much better.

Awesome, done!
Comment 48 Build Bot 2019-03-15 11:42:12 PDT Comment hidden (obsolete)
Comment 49 Tim Horton 2019-03-15 13:21:50 PDT
Comment on attachment 364817 [details]
Patch 3 / 4

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

> Source/WebKit/UIProcess/WebPageProxy.h:299
> +#if PLATFORM(COCOA)

Do we have to #ifdef all these things, I wonder? AttributedString is cross-platform, just empty

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm:89
> +        EXPECT_WK_STREQ(@"sRGB IEC61966-2.1 colorspace 1 0 0 1", dynamic_objc_cast<PlatformColor>(documentAttributes[NSBackgroundColorDocumentAttribute]).description);

I wonder if this is actually a safe thing to check. Maybe convert to sRGB and test the components?
Comment 50 Timothy Hatcher 2019-03-15 13:51:50 PDT
Comment on attachment 364817 [details]
Patch 3 / 4

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

>> Source/WebKit/UIProcess/WebPageProxy.h:299
>> +#if PLATFORM(COCOA)
> 
> Do we have to #ifdef all these things, I wonder? AttributedString is cross-platform, just empty

This line can be removed, it is for the old callback code!

AttributedString is cross-platform, but only WebPageCocoa implements getContentsAsAttributedString(). We would need stubs for the other platforms.
Comment 51 Timothy Hatcher 2019-03-15 13:58:06 PDT
Created attachment 364837 [details]
Patch 3 / 4
Comment 52 Build Bot 2019-03-15 14:01:52 PDT Comment hidden (obsolete)
Comment 53 WebKit Commit Bot 2019-03-15 14:37:01 PDT
Comment on attachment 364837 [details]
Patch 3 / 4

Clearing flags on attachment: 364837

Committed r243012: <https://trac.webkit.org/changeset/243012>
Comment 54 WebKit Commit Bot 2019-03-15 14:37:04 PDT Comment hidden (obsolete)
Comment 55 Timothy Hatcher 2019-03-15 15:46:32 PDT Comment hidden (obsolete)
Comment 56 Timothy Hatcher 2019-03-15 15:46:32 PDT
Created attachment 364859 [details]
Patch 4 / 4
Comment 57 Build Bot 2019-03-15 15:49:51 PDT Comment hidden (obsolete)
Comment 58 Timothy Hatcher 2019-03-15 15:50:26 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:211
> +    if (!range) {

Drive-by null de-deref crash fix I forgot to include in my last change.
Comment 59 Tim Horton 2019-03-15 16:00:39 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/WKError.h:60
> +    WKErrorContentFailedToLoad WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)),
> +    WKErrorContentLoadTimedOut WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)),

Because of the shared prefix these feel a lot like the ContentRuleList ones :) but are totally unrelated.
Also it's funny that you won't get these via normal WebKit loading, despite their relevance.
Comment 60 Tim Horton 2019-03-15 16:01:35 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/SourcesCocoa.txt:239
> +UIProcess/API/Cocoa/NSAttributedString.mm

This... should be further down in this file.
Comment 61 Tim Horton 2019-03-15 16:08:34 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:46
> + @abstract Type definition for the completion handler block used to get asynchronous results.

"asynchronous results". The abstract alone is pretty vague

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:61
> + @abstract Loads an HTML URL request and converts the contents into an attributed string.

Is it true to say HTML in all these places? What if we load XML+XSLT? Or an SVG with text in it? (it looks like you'll bail) What happened in those cases with the old API?

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:106
> +    static auto* cache = [[NSMutableArray<WKWebView *> alloc] initWithCapacity:maximumWebViewCacheSize];

I don't think I've ever seen anyone specify generics on the type in the alloc message send, that is pretty weird. And unnecessary.
Comment 62 Tim Horton 2019-03-15 16:10:53 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:250
> +            if ([action._mainFrameNavigation isEqual:contentNavigation.get()])

This is only going to save you from main frame navigation. What do we want to do with subframes? What did the old API do for other subresources?
Comment 63 Timothy Hatcher 2019-03-15 16:12:20 PDT
Created attachment 364875 [details]
Build Fix
Comment 64 Ryosuke Niwa 2019-03-15 16:34:38 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:95
> +@interface _WKAttributedStringWebViewCache : NSObject

It appears to me that we should be clearing WKWebView's upon receiving a memory pressure.
Comment 65 WebKit Commit Bot 2019-03-15 16:50:48 PDT
Comment on attachment 364875 [details]
Build Fix

Clearing flags on attachment: 364875

Committed r243028: <https://trac.webkit.org/changeset/243028>
Comment 66 Timothy Hatcher 2019-03-15 16:55:34 PDT
Created attachment 364880 [details]
Patch 4 / 4
Comment 67 Timothy Hatcher 2019-03-15 16:56:04 PDT
(In reply to Ryosuke Niwa from comment #64)
> Comment on attachment 364859 [details]
> Patch 4 / 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364859&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:95
> > +@interface _WKAttributedStringWebViewCache : NSObject
> 
> It appears to me that we should be clearing WKWebView's upon receiving a
> memory pressure.

Added.
Comment 68 Timothy Hatcher 2019-03-15 16:56:31 PDT
(In reply to Tim Horton from comment #62)
> Comment on attachment 364859 [details]
> Patch 4 / 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364859&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:250
> > +            if ([action._mainFrameNavigation isEqual:contentNavigation.get()])
> 
> This is only going to save you from main frame navigation. What do we want
> to do with subframes? What did the old API do for other subresources?

We discussed this. This will indeed block sub-frames.
Comment 69 Build Bot 2019-03-15 16:58:19 PDT Comment hidden (obsolete)
Comment 70 Timothy Hatcher 2019-03-15 16:58:20 PDT
Comment on attachment 364859 [details]
Patch 4 / 4

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

>> Source/WebKit/SourcesCocoa.txt:239
>> +UIProcess/API/Cocoa/NSAttributedString.mm
> 
> This... should be further down in this file.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:46
>> + @abstract Type definition for the completion handler block used to get asynchronous results.
> 
> "asynchronous results". The abstract alone is pretty vague

Reworded a bit.

>> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:61
>> + @abstract Loads an HTML URL request and converts the contents into an attributed string.
> 
> Is it true to say HTML in all these places? What if we load XML+XSLT? Or an SVG with text in it? (it looks like you'll bail) What happened in those cases with the old API?

Yeah, the backend only really works with HTML elements. Undefined if you try anything else I guess.

>> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:106
>> +    static auto* cache = [[NSMutableArray<WKWebView *> alloc] initWithCapacity:maximumWebViewCacheSize];
> 
> I don't think I've ever seen anyone specify generics on the type in the alloc message send, that is pretty weird. And unnecessary.

Removed.

>> Source/WebKit/UIProcess/API/Cocoa/WKError.h:60
>> +    WKErrorContentLoadTimedOut WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)),
> 
> Because of the shared prefix these feel a lot like the ContentRuleList ones :) but are totally unrelated.
> Also it's funny that you won't get these via normal WebKit loading, despite their relevance.

Fixed the names to be about attributed strings.
Comment 71 Timothy Hatcher 2019-03-15 21:17:43 PDT Comment hidden (obsolete)
Comment 72 Build Bot 2019-03-15 21:21:09 PDT Comment hidden (obsolete)
Comment 73 Timothy Hatcher 2019-03-16 11:45:20 PDT
Created attachment 364938 [details]
Patch 4 / 4
Comment 74 Build Bot 2019-03-16 11:47:39 PDT Comment hidden (obsolete)
Comment 75 Shawn Roberts 2019-03-18 09:53:19 PDT
TestWebKitAPI.WKWebView.GetContentsShouldReturnAttributedString is crashing on iOS Simulator

Will upload full crash log to Radar.

Reproduced with : 

run-api-tests TestWebKitAPI.WKWebView.GetContentsShouldReturnAttributedString --debug --ios-simulator

Crash Log:

0   JavaScriptCore                	0x000000010bc96f90 WTFCrash + 16
1   com.apple.WebKit              	0x000000010fa9f6cb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebKit              	0x0000000110c0d406 Messages::WebPage::GetContentsAsAttributedString::callReply(IPC::Decoder&, WTF::CompletionHandler<void (WebKit::AttributedString&&)>&&) + 134
3   com.apple.WebKit              	0x000000011035e991 void IPC::Connection::sendWithAsyncReply<Messages::WebPage::GetContentsAsAttributedString, WebKit::AttributedString const&>(Messages::WebPage::GetContentsAsAttributedString&&, WTF::CompletionHandler<void (WebKit::AttributedString const&)>&&, unsigned long long)::'lambda'(IPC::Decoder*)::operator()(IPC::Decoder*) + 97
4   com.apple.WebKit              	0x000000011035e8a2 WTF::Function<void (IPC::Decoder*)>::CallableWrapper<void IPC::Connection::sendWithAsyncReply<Messages::WebPage::GetContentsAsAttributedString, WebKit::AttributedString const&>(Messages::WebPage::GetContentsAsAttributedString&&, WTF::CompletionHandler<void (WebKit::AttributedString const&)>&&, unsigned long long)::'lambda'(IPC::Decoder*)>::call(IPC::Decoder*) + 50
Comment 76 Tim Horton 2019-03-18 10:55:10 PDT
Comment on attachment 364938 [details]
Patch 4 / 4

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

> Source/WebCore/en.lproj/Localizable.strings:184
> +/* WKErrorContentFailedToLoad description */

The names here don't match the implementation anymore

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:49
> + of this block type must expect to be called asynchronously when passed to HTML methods.

"HTML methods" is a bit odd.

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:110
> +    static auto* cache = [[NSMutableArray alloc] initWithCapacity:maximumWebViewCacheSize];

I have a feeling the star's on the wrong side, but auto is weird :D

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:123
> +        configuration._allowsJavaScriptMarkup = NO;

🎉
Comment 77 Timothy Hatcher 2019-03-18 15:52:13 PDT
Created attachment 365082 [details]
Patch 4 / 4
Comment 78 Timothy Hatcher 2019-03-18 15:53:15 PDT
(In reply to Shawn Roberts from comment #75)
> TestWebKitAPI.WKWebView.GetContentsShouldReturnAttributedString is crashing
> on iOS Simulator
> 
> Will upload full crash log to Radar.
> 
> Reproduced with : 
> 
> run-api-tests
> TestWebKitAPI.WKWebView.GetContentsShouldReturnAttributedString --debug
> --ios-simulator
> 
> Crash Log:
> 
> 0   JavaScriptCore                	0x000000010bc96f90 WTFCrash + 16
> 1   com.apple.WebKit              	0x000000010fa9f6cb WTFCrashWithInfo(int,
> char const*, char const*, int) + 27
> 2   com.apple.WebKit              	0x0000000110c0d406
> Messages::WebPage::GetContentsAsAttributedString::callReply(IPC::Decoder&,
> WTF::CompletionHandler<void (WebKit::AttributedString&&)>&&) + 134
> 3   com.apple.WebKit              	0x000000011035e991 void
> IPC::Connection::sendWithAsyncReply<Messages::WebPage::
> GetContentsAsAttributedString, WebKit::AttributedString
> const&>(Messages::WebPage::GetContentsAsAttributedString&&,
> WTF::CompletionHandler<void (WebKit::AttributedString const&)>&&, unsigned
> long long)::'lambda'(IPC::Decoder*)::operator()(IPC::Decoder*) + 97
> 4   com.apple.WebKit              	0x000000011035e8a2 WTF::Function<void
> (IPC::Decoder*)>::CallableWrapper<void
> IPC::Connection::sendWithAsyncReply<Messages::WebPage::
> GetContentsAsAttributedString, WebKit::AttributedString
> const&>(Messages::WebPage::GetContentsAsAttributedString&&,
> WTF::CompletionHandler<void (WebKit::AttributedString const&)>&&, unsigned
> long long)::'lambda'(IPC::Decoder*)>::call(IPC::Decoder*) + 50

Thanks! I filed bug 195916 with a fix.
Comment 79 WebKit Commit Bot 2019-03-18 16:32:26 PDT
Comment on attachment 365082 [details]
Patch 4 / 4

Clearing flags on attachment: 365082

Committed r243113: <https://trac.webkit.org/changeset/243113>
Comment 80 WebKit Commit Bot 2019-03-18 16:32:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 81 Frédéric Wang (:fredw) 2019-03-21 05:42:28 PDT
Comment on attachment 364837 [details]
Patch 3 / 4

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:367
> +- (void)_getContentsAsAttributedStringWithCompletionHandler:(void (^)(NSAttributedString *, NSDictionary<NSAttributedStringDocumentAttributeKey, id> *, NSError *))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_MAC_TBA));

Just for the record, this should have been ios(WK_IOS_TBA) as reported by the EWS style check. Note that this causes build error for people using the public SDK, so it would be nice to avoid it in the future... Thanks!

Fixed in https://trac.webkit.org/changeset/243212