Summary: | Add new NSAttributedString API for converting HTML | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit API | Assignee: | Timothy Hatcher <timothy> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, commit-queue, dbates, ews-watchlist, fred.wang, ggaren, rniwa, simon.fraser, sroberts, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=201812 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Timothy Hatcher
2019-03-12 12:46:25 PDT
Created attachment 364431 [details]
Patch 1 / 4
Created attachment 364432 [details]
Patch 2 / 4
Created attachment 364433 [details]
Patch 3 / 4
Created attachment 364434 [details]
Patch 4 / 4
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"? Created attachment 364438 [details]
Patch 1 / 4
Comment on attachment 364438 [details] Patch 1 / 4 Clearing flags on attachment: 364438 Committed r242831: <https://trac.webkit.org/changeset/242831> Created attachment 364476 [details]
Patch 2 / 4
Created attachment 364477 [details]
Patch 3 / 4
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.
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. 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
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 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 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? Created attachment 364503 [details]
Patch 2 / 4
(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. 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.
(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. Created attachment 364505 [details]
Patch 2 / 4
(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. 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.
Created attachment 364507 [details]
Patch 2 / 4
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.
Created attachment 364508 [details]
Patch 2 / 4
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.
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. Created attachment 364568 [details]
Patch 2 / 4
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.
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: graouts@apple.com) The commit-queue is continuing to process your patch. Comment on attachment 364568 [details] Patch 2 / 4 Clearing flags on attachment: 364568 Committed r242908: <https://trac.webkit.org/changeset/242908> All reviewed patches have been landed. Closing bug. Not done. Created attachment 364590 [details]
Build Fix
Comment on attachment 364590 [details] Build Fix Clearing flags on attachment: 364590 Committed r242923: <https://trac.webkit.org/changeset/242923> All reviewed patches have been landed. Closing bug. Created attachment 364666 [details]
Build Fix
Opened for build fix. Comment on attachment 364666 [details] Build Fix Clearing flags on attachment: 364666 Committed r242950: <https://trac.webkit.org/changeset/242950> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 364693 [details]
Patch 3 / 4
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.
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 (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. Created attachment 364817 [details]
Patch 3 / 4
(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! 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.
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 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. Created attachment 364837 [details]
Patch 3 / 4
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.
Comment on attachment 364837 [details] Patch 3 / 4 Clearing flags on attachment: 364837 Committed r243012: <https://trac.webkit.org/changeset/243012> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 364859 [details]
Patch 4 / 4
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.
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 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 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 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 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? Created attachment 364875 [details]
Build Fix
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 on attachment 364875 [details] Build Fix Clearing flags on attachment: 364875 Committed r243028: <https://trac.webkit.org/changeset/243028> Created attachment 364880 [details]
Patch 4 / 4
(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. (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. 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.
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. Created attachment 364911 [details]
Patch 4 / 4
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.
Created attachment 364938 [details]
Patch 4 / 4
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.
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 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; 🎉 Created attachment 365082 [details]
Patch 4 / 4
(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 on attachment 365082 [details] Patch 4 / 4 Clearing flags on attachment: 365082 Committed r243113: <https://trac.webkit.org/changeset/243113> All reviewed patches have been landed. Closing bug. 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 |