Loading a content extension can fail in many places: 1) Parsing JSON 2) Parsing the structure of the JSON object (like if there are invalid actions or triggers) 3) Compiling the content extension (like if there is a .* anywhere but the beginning). Ignoring a rule that has an invalid trigger or action seems ok, but everything else should be a complete failure so developers notice.
Created attachment 249427 [details] Patch
Comment on attachment 249427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249427&action=review > Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:136 > + dataLogF("Triggers matching everything must be at beginning."); This (and the other errors) need to go to the web inspector.
(In reply to comment #2) > Comment on attachment 249427 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249427&action=review > > > Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:136 > > + dataLogF("Triggers matching everything must be at beginning."); > > This (and the other errors) need to go to the web inspector. In order to go to the web inspector, we need a page. ContentExtensionCompiler should not need or know about a page. I'm going to use WTFLogAlways.
Nevermind. I'm going to wait for https://bugs.webkit.org/show_bug.cgi?id=143059 and use that.
(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 249427 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=249427&action=review > > > > > Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:136 > > > + dataLogF("Triggers matching everything must be at beginning."); > > > > This (and the other errors) need to go to the web inspector. > > In order to go to the web inspector, we need a page. > ContentExtensionCompiler should not need or know about a page. I'm going to > use WTFLogAlways. I think you're getting caught up on some sort of separation of responsibilities that is not important, or is somehow misguided. My in-person request was that when compiling a content extension in a WebKit app and the extension fails to compile, the *web* developer be told in a place where they expect to see it. The web developer is not a WebKit developer. Logging is definitely not that place. AFAIK, the Web Inspector is the only such place that exists. --- The content extension API operates on page groups. Therefore when a content extension fails to compile, it is doing so within the context of a page group. It should be trivial to walk the Pages in that PageGroup and announce the error to their WebInspectors. Please consider my r- to stand on any form of this patch that doesn't put the failure logging in a place where web developers will see it using a standard release build of WebKit launched as normal (i.e. not running it from the command line)
Created attachment 249457 [details] Patch
Comment on attachment 249457 [details] Patch r- until we resolve the error reporting issue.
> The content extension API operates on page groups. Therefore when a content > extension fails to compile, it is doing so within the context of a page > group. If this is associated with a PageGroup like Brady says, then walking over the Pages in the group and output doesn't sound as crazy to me as when I first heard this. Does the developer have control over when the compile happens? - Can the compile and warning generate happen before any page is loaded? - Web Inspector clears its console messages on page navigations, so a navigation could clear these messages (in that page).
Comment on attachment 249457 [details] Patch The problem is that the api using content extensions operates on page groups, but the api making content extensions does not. See WKUserContentFilterCreate (which also needs to be renamed, but that's another issue). That is the point at which we get the error and could get a string. No pages. No page groups. That api is being worked on to get better error reporting. See the declaration of LegacyContentExtensionCompilationClient. But that work is not directly related to this patch. I propose this patch go in, then we worry about what to do with the error messages.
(In reply to comment #9) > Comment on attachment 249457 [details] > Patch > > The problem is that the api using content extensions operates on page > groups, but the api making content extensions does not. See > WKUserContentFilterCreate (which also needs to be renamed, but that's > another issue). That is the point at which we get the error and could get a > string. No pages. No page groups. > > That api is being worked on to get better error reporting. See the > declaration of LegacyContentExtensionCompilationClient. But that work is > not directly related to this patch. I propose this patch go in, then we > worry about what to do with the error messages. Oy vey. Okay, so we synchronously compile the extension at creation time, even though it can't possibly be used until it is installed in a page group. I assumed we compiled when installing into a page group (since it cannot possibly be used before then) Can we rethink this?
The motivation of the API design was startup performance. An application should already have a compiled content extension to use so it does not need to compile or even check if anything has changed every time it starts. The API needs to be modified to report errors while compiling, rename things like WKUserContentFilterCreate, and possibly fail to accept a compiled content extension to inform the application that it needs to recompile (with a future version of WebKit when if change the byte code, for example). Should all that be added to this patch?
Comment on attachment 249457 [details] Patch I'm going to put an error code and a version number in the CompiledContentExtension, then we can put errors in the inspector when using an invalid extension.
The useful parts of this patch were included in https://bugs.webkit.org/show_bug.cgi?id=143235