Only expose Notification.requestPermission() on the Window global.

Only expose Notification.requestPermission() on the Window global.

BUG=442139
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201555

[Peter Beverloo, 2015-08-28 15:58:58]

peter@chromium.org changed reviewers:
+ haraken@chromium.org

[Peter Beverloo, 2015-08-28 15:58:58]

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:343: ExecutionContext* context =
currentExecutionContext(isolate);
This will only be needed for *one* use of |custom_registration_methods| right
now - the one I'm introducing.

I don't know how performance sensitive this is, and would like your guidance on
whether this is OK, or we need more elaborate detection by iterating over
|custom_registration_methods| to determine any needs the context in the bindings
generator.

(Although presumably compilers will optimize this out?)

[haraken, 2015-08-29 08:04:57]

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:343: ExecutionContext* context =
currentExecutionContext(isolate);
On 2015/08/28 15:58:58, Peter Beverloo wrote:
> This will only be needed for *one* use of |custom_registration_methods| right
> now - the one I'm introducing.
> 
> I don't know how performance sensitive this is, and would like your guidance
on
> whether this is OK, or we need more elaborate detection by iterating over
> |custom_registration_methods| to determine any needs the context in the
bindings
> generator.
> 
> (Although presumably compilers will optimize this out?)

Would you help me understand where the context is used?

In terms of performance overhead, this would be no problem.

[Peter Beverloo, 2015-08-29 11:39:39]

Certainly!

The [Exposed] annotation allows the user to select the global on which a member
should be exposed, e.g. Window or Worker, information which is available on the
ExecutionContext for which the bindings are being installed.

When installing static methods such as requestPermission(), the |context| gets
rightfully used, but the variable is never made available to the scope of, in
this case, installV8NotificationTemplate.

The use of |context| gets introduced by the function exposed_if in
code_generator_v8.py, and ExposureSet._code in v8_utilities.py.

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/tests/r...
File Source/bindings/tests/results/modules/V8TestInterface5.cpp (right):

https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/tests/r...
Source/bindings/tests/results/modules/V8TestInterface5.cpp:996: if (context &&
(context->isWorkerGlobalScope())) {
This line illustrates the usage, and would also result in the compile error I
saw then using [Exposed] on requestPermission().

[haraken, 2015-08-30 01:23:58]

On 2015/08/29 11:39:39, Peter Beverloo wrote:
> Certainly!
> 
> The [Exposed] annotation allows the user to select the global on which a
member
> should be exposed, e.g. Window or Worker, information which is available on
the
> ExecutionContext for which the bindings are being installed.
> 
> When installing static methods such as requestPermission(), the |context| gets
> rightfully used, but the variable is never made available to the scope of, in
> this case, installV8NotificationTemplate.
> 
> The use of |context| gets introduced by the function exposed_if in
> code_generator_v8.py, and ExposureSet._code in v8_utilities.py.
> 
>
https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/tests/r...
> File Source/bindings/tests/results/modules/V8TestInterface5.cpp (right):
> 
>
https://codereview.chromium.org/1318293002/diff/40001/Source/bindings/tests/r...
> Source/bindings/tests/results/modules/V8TestInterface5.cpp:996: if (context &&
> (context->isWorkerGlobalScope())) {
> This line illustrates the usage, and would also result in the compile error I
> saw then using [Exposed] on requestPermission().

Thanks for the clarification; makes sense.

Given that we already do similar hacks for instanceTemplate and
prototypeTemplate, this would be fine. LGTM.

[Peter Beverloo, 2015-09-01 12:30:32]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-09-01 12:30:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318293002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318293002/40001

[commit-bot: I haz the power, 2015-09-01 13:46:52]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201555