Bug 195636 – Add new NSAttributedString API for converting HTML

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Timothy Hatcher]

This moves us into the future and off deprecated WebView.

<rdar://problem/45055697>

[Timothy Hatcher]

Created attachment 364431 [details]
Patch 1 / 4

[Timothy Hatcher]

Created attachment 364432 [details]
Patch 2 / 4

[Timothy Hatcher]

Created attachment 364433 [details]
Patch 3 / 4

[Timothy Hatcher]

Created attachment 364434 [details]
Patch 4 / 4

[Simon Fraser (smfr)]

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"?

[Timothy Hatcher]

Created attachment 364438 [details]
Patch 1 / 4

[WebKit Commit Bot]

Comment on attachment 364438 [details]
Patch 1 / 4

Clearing flags on attachment: 364438

Committed r242831: <https://trac.webkit.org/changeset/242831>

[Timothy Hatcher]

Created attachment 364476 [details]
Patch 2 / 4

[Timothy Hatcher]

Created attachment 364477 [details]
Patch 3 / 4

[EWS Watchlist]

Attachment 364476 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:508:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:510:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 364476 [details]
Patch 2 / 4

Attachment 364476 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11482838

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 364482 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Daniel Bates]

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?

[Daniel Bates]

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

[Ryosuke Niwa]

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?

[Timothy Hatcher]

Created attachment 364503 [details]
Patch 2 / 4

[Timothy Hatcher]

(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.

[EWS Watchlist]

Attachment 364503 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:63:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:443:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:445:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

(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.

[Timothy Hatcher]

Created attachment 364505 [details]
Patch 2 / 4

[Timothy Hatcher]

(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.

[EWS Watchlist]

Attachment 364505 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:63:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Created attachment 364507 [details]
Patch 2 / 4

[EWS Watchlist]

Attachment 364507 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:63:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Created attachment 364508 [details]
Patch 2 / 4

[EWS Watchlist]

Attachment 364508 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:63:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

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.

[Timothy Hatcher]

Created attachment 364568 [details]
Patch 2 / 4

[EWS Watchlist]

Attachment 364568 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:66:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 364568 [details]:

legacy-animation-engine/animations/animation-end-event-destroy-renderer.html bug 195699 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 364568 [details]
Patch 2 / 4

Clearing flags on attachment: 364568

Committed r242908: <https://trac.webkit.org/changeset/242908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Not done.

[Timothy Hatcher]

Created attachment 364590 [details]
Build Fix

[WebKit Commit Bot]

Comment on attachment 364590 [details]
Build Fix

Clearing flags on attachment: 364590

Committed r242923: <https://trac.webkit.org/changeset/242923>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Created attachment 364666 [details]
Build Fix

[Timothy Hatcher]

Opened for build fix.

[WebKit Commit Bot]

Comment on attachment 364666 [details]
Build Fix

Clearing flags on attachment: 364666

Committed r242950: <https://trac.webkit.org/changeset/242950>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Reopening to attach new patch.

[Timothy Hatcher]

Created attachment 364693 [details]
Patch 3 / 4

[EWS Watchlist]

Attachment 364693 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebPageProxy.h:975:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebPageProxy.cpp:3606:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:367:  WK_MAC_TBA is neither a version number nor WK_IOS_TBA  [build/wk_api_available] [5]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

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

[Tim Horton]

(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.

[Timothy Hatcher]

Created attachment 364817 [details]
Patch 3 / 4

[Timothy Hatcher]

(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!

[EWS Watchlist]

Attachment 364817 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:367:  WK_MAC_TBA is neither a version number nor WK_IOS_TBA  [build/wk_api_available] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

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?

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 364837 [details]
Patch 3 / 4

[EWS Watchlist]

Attachment 364837 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:367:  WK_MAC_TBA is neither a version number nor WK_IOS_TBA  [build/wk_api_available] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 364837 [details]
Patch 3 / 4

Clearing flags on attachment: 364837

Committed r243012: <https://trac.webkit.org/changeset/243012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Reopening to attach new patch.

[Timothy Hatcher]

Created attachment 364859 [details]
Patch 4 / 4

[EWS Watchlist]

Attachment 364859 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:310:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:317:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:325:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:333:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

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.

[Tim Horton]

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.

[Tim Horton]

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.

[Tim Horton]

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.

[Tim Horton]

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?

[Timothy Hatcher]

Created attachment 364875 [details]
Build Fix

[Ryosuke Niwa]

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.

[WebKit Commit Bot]

Comment on attachment 364875 [details]
Build Fix

Clearing flags on attachment: 364875

Committed r243028: <https://trac.webkit.org/changeset/243028>

[Timothy Hatcher]

Created attachment 364880 [details]
Patch 4 / 4

[Timothy Hatcher]

(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.

[Timothy Hatcher]

(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.

[EWS Watchlist]

Attachment 364880 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:330:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:337:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:345:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:353:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 364911 [details]
Patch 4 / 4

[EWS Watchlist]

Attachment 364911 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:341:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:349:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:357:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Created attachment 364938 [details]
Patch 4 / 4

[EWS Watchlist]

Attachment 364938 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:341:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:349:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.mm:357:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shawn Roberts]

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

[Tim Horton]

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;

🎉

[Timothy Hatcher]

Created attachment 365082 [details]
Patch 4 / 4

[Timothy Hatcher]

(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.

[WebKit Commit Bot]

Comment on attachment 365082 [details]
Patch 4 / 4

Clearing flags on attachment: 365082

Committed r243113: <https://trac.webkit.org/changeset/243113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

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