We need to add runtime settings for html notification such that it can be deprecated for web page. We still want to enable it for extension.
Created attachment 152372 [details] Proposed Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 152372 [details] Proposed Patch Why not use RuntimeEnabledFeatures? That way we can hide the API entirely instead of throwing an exception.
(In reply to comment #3) > (From update of attachment 152372 [details]) > Why not use RuntimeEnabledFeatures? That way we can hide the API entirely instead of throwing an exception. RuntimeFeatures are global/per-renderer process right? In order to keep HTML notifications enabled for apps/extensions it needs to be controlled per Document. I think following the pattern set by Shadow DOM and scoped stylesheets (http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp) is more appropriate.
Don't apps and extensions get their own separate renderer processes? Or, is the issue that extensions may embed remote iframes, such that within the same process, you'd have the outer document w/ this API enabled and the inner document w/ this API disabled?
(In reply to comment #5) > Or, is the issue that extensions may embed remote iframes, such that within the same process, you'd have the outer document w/ this API enabled and the inner document w/ this API disabled? Yes, that can happen (and vice-versa, where a web_accessible extension page is embedded in a regular page by a content script).
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 152372 [details] [details]) > > Why not use RuntimeEnabledFeatures? That way we can hide the API entirely instead of throwing an exception. > > RuntimeFeatures are global/per-renderer process right? In order to keep HTML notifications enabled for apps/extensions it needs to be controlled per Document. I think following the pattern set by Shadow DOM and scoped stylesheets (http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp) is more appropriate. I tried to apply V8EnabledPerContext IDL attribute to NotificationCenter.createHTMLNotification, like: [V8Custom, V8EnabledPerContext=htmlNotifications] Notification createHTMLNotification(in DOMString url) raises(DOMException); However, it does not work because the V8 code generator only supports V8EnabledPerContext for attributes, not methods. In addition, it does not allow additional V8Custom attribute. Do we want to enhance V8 code generator to support per-context methods?
> Do we want to enhance V8 code generator to support per-context methods? Yes.
Jian also asked: > Do we support creating workers from extension code? If not, we do not have any problem to handle it. Otherwise, do we want to allow createHTMLNotification in extension workers? We do support creating of workers in extensions, but I don't think anyone expects them to be able to create notifications.
(In reply to comment #8) > > Do we want to enhance V8 code generator to support per-context methods? > > Yes. I tried to modify CodeGeneratorV8.pm to generate the code to install per-context functions in installPerContextProperties, but it seems not to work. Any V8 bindings experts know how to install the function callback in installPerContextProperties? I tried something like the following and it did not work: if (ContextFeatures::***Enabled(document)) GetTemplate()->PrototypeTemplate()->Set(v8::String::New("createHTMLNotification"), v8::FunctionTemplate::New(V8NotificationCenter::createHTMLNotificationCallback, v8::Handle<v8::Value>(), defaultSignature)); Another issue is that installPerContextProperties is called manually from V8DOMWindowShell::installDOMWindow. We seem to have no automatic support to install per-context properties for non-DOMWindow cases.
> I tried to modify CodeGeneratorV8.pm to generate the code to install per-context functions in installPerContextProperties, but it seems not to work. Any V8 bindings experts know how to install the function callback in installPerContextProperties? I tried something like the following and it did not work: > if (ContextFeatures::***Enabled(document)) > GetTemplate()->PrototypeTemplate()->Set(v8::String::New("createHTMLNotification"), v8::FunctionTemplate::New(V8NotificationCenter::createHTMLNotificationCallback, v8::Handle<v8::Value>(), defaultSignature)); I'm sorry, I don't quite follow. Can you post a work-in-progress patch that shows the trouble you're running into? > Another issue is that installPerContextProperties is called manually from V8DOMWindowShell::installDOMWindow. We seem to have no automatic support to install per-context properties for non-DOMWindow cases. Yeah, I think we're only using it today for DOMWindow. We'll need to add support for other objects that know their scriptExecutionContext.
Created attachment 153364 [details] Patch in progress (changing V8 code generator to support V8EnabledPerContext for functions)
(In reply to comment #10) > Another issue is that installPerContextProperties is called manually from V8DOMWindowShell::installDOMWindow. We seem to have no automatic support to install per-context properties for non-DOMWindow cases. For this, I think we need to modify generated wrapSlow() method to invoke installPerContextProperties(). DOMWindow might need to handle it manually since its lifetime is a bit strange.
Comment on attachment 153364 [details] Patch in progress (changing V8 code generator to support V8EnabledPerContext for functions) View in context: https://bugs.webkit.org/attachment.cgi?id=153364&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2976 > + v8::Persistent<v8::FunctionTemplate> desc = GetTemplate(); I think we shouldn't do this because returned template is cached and reused across the context. Why not just inject the function into a prototype object like per-context objects are doing? (I might miss the point since I'm not familiar with V8 embedding API...)
(In reply to comment #13) > (In reply to comment #10) > > Another issue is that installPerContextProperties is called manually from V8DOMWindowShell::installDOMWindow. We seem to have no automatic support to install per-context properties for non-DOMWindow cases. > > For this, I think we need to modify generated wrapSlow() method to invoke installPerContextProperties(). > DOMWindow might need to handle it manually since its lifetime is a bit strange. This is also what I thought and tested in my local build. I will modify the V8 generator to add the call.
(In reply to comment #14) > (From update of attachment 153364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153364&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2976 > > + v8::Persistent<v8::FunctionTemplate> desc = GetTemplate(); > > I think we shouldn't do this because returned template is cached and reused across the context. > Why not just inject the function into a prototype object like per-context objects are doing? > (I might miss the point since I'm not familiar with V8 embedding API...) I do not know how this could be done in V8. Could some V8 expert help on this?
Comment on attachment 153364 [details] Patch in progress (changing V8 code generator to support V8EnabledPerContext for functions) View in context: https://bugs.webkit.org/attachment.cgi?id=153364&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2976 >>> + v8::Persistent<v8::FunctionTemplate> desc = GetTemplate(); >> >> I think we shouldn't do this because returned template is cached and reused across the context. >> Why not just inject the function into a prototype object like per-context objects are doing? >> (I might miss the point since I'm not familiar with V8 embedding API...) > > I do not know how this could be done in V8. Could some V8 expert help on this? Please look at batchConfigureCallbacks() in V8Proxy.cpp. This is the way to install functions on a prototype object. proto->Set(v8::String::New("function_name"), v8::FunctionTemplate::New(callback_function_ptr, v8::Handle<v8::Value>(), signature), property_attributes) To see what you should set for 'signature' and 'property_attributes', look at configureTemplate() in V8Binding.cpp, where batchConfigureCallbacks() is called. To see more, look at src/out/{Debug,Release}/obj/gen/webcore/bindings/V8Node.cpp, where configureTemplate() is called.
Created attachment 153928 [details] Proposed Patch
Attachment 153928 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/Modules/notifications/NotificationCenter.cpp:59: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:2279: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:2282: One line control clauses should not use braces. [whitespace/braces] [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 153928 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review Overall looks good. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2238 > + $conditional = "if (${enable_function}(impl->document()))\n "; Better ensure that $conditional is empty before the assignment.
Comment on attachment 153928 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review V8 change LGTM. > Source/WebCore/bindings/scripts/test/TestObj.idl:198 > + [V8EnabledPerContext] void enabledAtContextMethod1(in int intArg); > + [V8EnabledPerContext=FeatureName] void enabledAtContextMethod2(in int intArg); Nit: enabledPerContextMethod would be a better name. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1740 > + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); Nit: 'V8int::' is wrong, but this is not related to this patch. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1751 > + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); Ditto.
Comment on attachment 153928 [details] Proposed Patch Attachment 153928 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13329230 New failing tests: fast/notifications/notifications-with-permission.html
Created attachment 153936 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 153928 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1740 >> + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); > > Nit: 'V8int::' is wrong, but this is not related to this patch. "in int intArg" should be "in long longArg". I don't think int is a WebIDL type.
Comment on attachment 153928 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review If haraken is happy with the V8 pieces, I'm happy with the notification pieces. Let's rock and roll. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2623 > + push(@enabledPerContextFunctions, $function); Four-space indent please.
> fast/notifications/notifications-with-permission.html Although, it does look like you've got one test failure to work through.
(In reply to comment #21) > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1740 > > + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); > > Nit: 'V8int::' is wrong, but this is not related to this patch. > Will file a bug for this issue.
Committed as http://trac.webkit.org/changeset/123535.
(In reply to comment #24) > (From update of attachment 153928 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review > > >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1740 > >> + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); > > > > Nit: 'V8int::' is wrong, but this is not related to this patch. > > "in int intArg" should be "in long longArg". I don't think int is a WebIDL type. Looks like you missed this comment. Would you be willing to fix this in a follow up patch?
(In reply to comment #29) > (In reply to comment #24) > > (From update of attachment 153928 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=153928&action=review > > > > >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1740 > > >> + EXCEPTION_BLOCK(int, intArg, V8int::HasInstance(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)) ? V8int::toNative(v8::Handle<v8::Object>::Cast(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined))) : 0); > > > > > > Nit: 'V8int::' is wrong, but this is not related to this patch. > > > > "in int intArg" should be "in long longArg". I don't think int is a WebIDL type. > > Looks like you missed this comment. Would you be willing to fix this in a follow up patch? I've filed a separate bug for this issue. https://bugs.webkit.org/show_bug.cgi?id=92168 I can fix this in another patch.