Bug 143808 – _WKUserContentExtensionStore should report errors

• Component: WebKit API
• Status: RESOLVED FIXED

[Alex Christensen]

There is a FIXME in _WKUserContentExtensionStore.mm that needs fixing.

[Alex Christensen]

Created attachment 250887 [details]
Patch

[Alex Christensen]

rdar://problem/19667718

[Alex Christensen]

If I used WEB_UI_STRING here, then would I have to localize all the strings in ContentExtensionError.cpp?  What is done in cases like this?

[Brady Eidson]

(In reply to comment #3)
> If I used WEB_UI_STRING here, then would I have to localize all the strings
> in ContentExtensionError.cpp?  What is done in cases like this?

We mark them as needing localization.

[Alex Christensen]

Created attachment 250987 [details]
Patch

[Build Bot]

Comment on attachment 250987 [details]
Patch

Attachment 250987 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6446108033155072

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 250990 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250987 [details]
Patch

Attachment 250987 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4798753127006208

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 250996 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

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

[Darin Adler]

Comment on attachment 250987 [details]
Patch

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

Build failing on iOS, tests failing on Mac.

It seems that WEB_UI_STRING is a poor match for the code that is being done with the C++ library and Mac platform stuff and and not using WTF at all. It’s really inelegant to use a localized string API that returns an NSString, then convert it to a WTF::String and then to UTF-8 to put it in a WTF::CString and then to a const char* and then make a std::string out of that.

The only things that make WebCore::localizedString non-thread-safe are:

1) The use of a global variable with no locking to store the result of CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebCore")). This could easily be fixed using std::call_once.
2) The conversion of the string from NSString to WTF::String for the return type of the function.

If you’d like to use this in code that’s Cocoa-specific, then I suggest you make a Cocoa-specific version that returns an NSString *; you’d need both a function that returns the localized string and one or more appropriate macros that’s picked up by the update-webkit-localizable-strings script. We should not convert from NSString to WTF::String and then back to WTF::String.

Similarly, if you’d like to use this in code that’s using std::string, then I suggest you make a version that returns a std::string. We should not use the WTF::String version and then convert to std::string. And it should use -[NSString UTF8String], not convert from NSString to WTF::String to WTF::CString just to make it UTF-8.

> Source/WebCore/English.lproj/Localizable.strings:382
> +// I know, this needs alphabetizing before landing.  r-, but see my question in Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm.

This file should be automatically generated by update-webkit-localizable-strings, not edited by hand. Have we been editing it by hand? Maybe we never did this for WebCore. If so, we should fix that.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:59
> +            // What should I do about this? (OOPS!)

See my comment about how to solve that problem.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:61
> +            auto userInfo = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:[NSString stringWithFormat:@"%s: %s", message.utf8().data(), error.message().c_str()], NSLocalizedDescriptionKey, nil]);

This should be using %@ for the localized string, not %s. It’s OK to use %s for a std::string, although I am not sure that %s in this NSString method will handle UTF-8 characters correctly; is that the encoding it assumes for %s? It doesn’t seem smart to convert an NSString to UTF-8 just so we can turn around and pass it back to an API that handles UTF-16.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:80
> +            auto userInfo = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:[NSString stringWithFormat:@"%s: %s", message.utf8().data(), error.message().c_str()], NSLocalizedDescriptionKey, nil]);

Ditto.

[Alex Christensen]

(In reply to comment #10)
Thanks Darin!  I learned a lot about strings in WebCore.  I agree all the string conversion is inelegant, but since we are using std::error_code to get a std::string with the error, I see two options:
1) Make a lot of code platform dependent and use NSStrings and NSErrors everywhere in content extensions.
2) Don't localize the compiler error strings (which shouldn't be seen by end users anyway) and use a key like NSHelpAnchorErrorKey that does not imply that the string is localized.
> The only things that make WebCore::localizedString non-thread-safe are:
> 
> 1) The use of a global variable with no locking to store the result of
> CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebCore")). This could
> easily be fixed using std::call_once.
> 2) The conversion of the string from NSString to WTF::String for the return
> type of the function.
This might be worth looking into later if we find more places we want to use localized strings in callbacks.
> This file should be automatically generated by
> update-webkit-localizable-strings, not edited by hand. Have we been editing
> it by hand? Maybe we never did this for WebCore. If so, we should fix that.
Everything that a script can do, I can do by hand :) 
I didn't know about this script, but Localizable.strings looked generated.  I'll use this in the future.

[Alex Christensen]

Created attachment 251160 [details]
Patch

[Darin Adler]

Comment on attachment 251160 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:58
> +            auto userInfo = @{NSHelpAnchorErrorKey: [NSString stringWithFormat:@"Failed to compile extension with error: %s", error.message().c_str()]};

"Failed to compile" is not good grammar. Maybe "Extension compilation failed: "?

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:76
> +            auto userInfo = @{NSHelpAnchorErrorKey: [NSString stringWithFormat:@"Failed to lookup extension with error: %s", error.message().c_str()]};

lookup is a noun, not a verb. Maybe "Extension lookup failed: "?

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:94
> +            auto userInfo = @{NSHelpAnchorErrorKey: [NSString stringWithFormat:@"Failed to remove extension with error: %s", error.message().c_str()]};

Maybe "Extension removal failed: "?

[Alex Christensen]

http://trac.webkit.org/changeset/183013