Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use SetIntegrityLevel instead of Object.freeze() #125

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use SetIntegrityLevel instead of Object.freeze() #125

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5eb2899
Use SetIntegrityLevel instead of Object.freeze()
annevk Feb 27, 2018
9e4392d
nit
annevk Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions notifications.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,13 +909,13 @@ return the <a>notification</a>'s <a for=notification>require interaction prefere
result of the following steps:

<ol>
<li><p>Let <var>frozenActions</var> be an empty list of type {{NotificationAction}}.
<li><p>Let <var>frozenActions</var> be a new <a for=/>list</a>.

<li><p>For each <var>entry</var> in the <a>notification</a>'s list of
<a for=notification>actions</a>, perform the following steps:

<ol>
<li><p>Let <var>action</var> be a new {{NotificationAction}}.
<li><p>Let <var>action</var> be a new {{NotificationAction}} <a for=/>dictionary</a>.

<li><p>Set <var>action</var>'s {{NotificationAction/action}} to
<var>entry</var>'s <a for=action>name</a>.
Expand All @@ -926,15 +926,15 @@ result of the following steps:
<li><p>Set <var>action</var>'s {{NotificationAction/icon}} to
<var>entry</var>'s <a for=action>icon URL</a>.

<!-- XXX IDL dictionaries are usually returned by value, so don't need to be
immutable. But FrozenArray reifies the dictionaries to mutable JS
objects accessed by reference, so we explicitly freeze them.
It would be better to do this with IDL primitives instead of JS - see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 -->
<li><p>Call <a lt="freeze">Object.freeze</a> on <var>action</var>, to
prevent accidental mutation by scripts.
<li><p>Let <var>jsAction</var> be <var>action</var>, <a>converted to an ECMAScript value</a>.

<li><p>Append <var>action</var> to <var>frozenActions</var>.
<li><p>Perform <a abstract-op>SetIntegrityLevel</a>(<var>jsAction</var>, "frozen").

<li><p>Let <var>idlAction</var> be <var>jsAction</var>, <a>converted to an IDL value</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct, at least as written. The conversion, presumably to an IDL NotificationAction type (although that isn't stated), will re-perform the Get()s of all the properties, which can be observable if Object.prototype is modified. Then we'll convert back to JS in the binding layer.

I don't think there's any way to do this rigorously as long as your return type is FrozenArray<NotificationAction> (instead of FrozenArray<object>). The conversion between JS objects and dictionaries is observable and side effecting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot use FrozenArray then either since it's constructor takes an IDL value. I'm not sure I want to inline all of that. That would be rather bad and I'd likely make mistakes or it would result in mismatches down the line.

There is some mismatch here though with implementations I think where JS values are backed by IDL values and you can manipulate either. That's not conversion, but just grabbing the corresponding value, but IDL doesn't formalize that.

<!-- See https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 about replacing the above 3 steps
with something simpler. -->

<li><p><a for=list>Append</a> <var>idlAction</var> to <var>frozenActions</var>.
</ol>

<li><p><a>Create a frozen array</a> from <var>frozenActions</var>.
Expand Down Expand Up @@ -1224,8 +1224,8 @@ urlPrefix: https://w3c.github.io/vibration/
text: validate and normalize
urlPrefix: #idl-def-; type: interface
text: VibratePattern; url: vibratepattern
urlPrefix: https://tc39.github.io/ecma262/#sec-object.; type: dfn
text: freeze
urlPrefix: https://tc39.github.io/ecma262/#sec-; spec: ECMA-262; type: abstract-op
text: SetIntegrityLevel
</pre>

<pre class="biblio">
Expand Down