Bug 91295 – Add per-context setting for html notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jian Li]

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.

[Jian Li]

Created attachment 152372 [details]
Proposed Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 152372 [details]
Proposed Patch

Why not use RuntimeEnabledFeatures?  That way we can hide the API entirely instead of throwing an exception.

[Mihai Parparita]

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

[Darin Fisher (:fishd, Google)]

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?

[Mihai Parparita]

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

[Jian Li]

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

[Adam Barth]

> Do we want to enhance V8 code generator to support per-context methods?

Yes.

[Mihai Parparita]

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.

[Jian Li]

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

[Adam Barth]

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

[Jian Li]

Created attachment 153364 [details]
Patch in progress (changing V8 code generator to support V8EnabledPerContext for functions)

[Hajime Morrita]

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

[Hajime Morrita]

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

[Jian Li]

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

[Jian Li]

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

[Kentaro Hara]

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.

[Jian Li]

Created attachment 153928 [details]
Proposed Patch

[WebKit Review Bot]

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.

[Hajime Morrita]

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.

[Kentaro Hara]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

> fast/notifications/notifications-with-permission.html

Although, it does look like you've got one test failure to work through.

[Jian Li]

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

[Jian Li]

Committed as http://trac.webkit.org/changeset/123535.

[Adam Barth]

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

[Jian Li]

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