Closed Bug 707564 Opened 8 years ago Closed 6 years ago

force the properties onto navigator in the enumerate hook

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: fabrice, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

navigator properties that are added with the JavaScript-navigator-property category should be enumerated on the navigator object
Version: unspecified → Trunk
Assignee: nobody → bzbarsky
Attachment #783663 - Flags: review?(fabrice) → review+
Blocks: 900034
Attachment #783665 - Flags: review?(bugs) → review+
Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

I don't understand this.
Why do we want those properties to be own properties?
Shouldn't they be getters(/setters?) in prototype and that way enumerable?

Clearing r? for now.
Attachment #783666 - Flags: review?(bugs)
Attachment #783667 - Flags: review?(bugs)
Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

We just want to be consistent.  The resolve hook resolves these as own properties, so the enumerate hook should enumerate them.

Long-term we do want the navigator category stuff to be on the proto somehow, but we just haven't figured out how.  At the point at which we manage to do that, we'll almost certainly no longer need this code or a resolve hook at all: setting up Navigator.prototype would just enumerate the prop names and set up accessors.
Attachment #783666 - Flags: review?(bugs)
Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.

Again, this is just being consistent with the resolve hook's behavior.
Attachment #783667 - Flags: review?(bugs)
Thanks for explaining. I'll try to review these still today.
Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

nsScriptNameSpaceManager *nameSpaceManager ->
nsScriptNameSpaceManager* nameSpaceManager

In the test could you try also for-in.
Attachment #783666 - Flags: review?(bugs) → review+
Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.


>+class CGEnumerateOwnPropertiesViaGetOwnPropertyNames(CGAbstractBindingMethod):
>+    """
>+    An implementation of Xray EnumerateOwnProperties stuff for things
>+    that have a newresolve hook.
>+    """
>+    def __init__(self, descriptor):
>+        args = [Argument('JSContext*', 'cx'),
>+                Argument('JS::Handle<JSObject*>', 'wrapper'),
>+                Argument('JS::Handle<JSObject*>', 'obj'),
>+                Argument('JS::AutoIdVector&', 'props')]
Not about this bug, but the generated code, and I believe even the codegen itself would be
easier to read if aFoo style was used for parameters.

>+        CGAbstractBindingMethod.__init__(self, descriptor,
>+                                         "EnumerateOwnPropertiesViaGetOwnPropertyNames",
>+                                         args, getThisObj="",
>+                                         callArgs="", returnType="bool")
>+    def generate_code(self):
>+        return CGIndenter(CGGeneric(
>+                "nsAutoTArray<nsString, 8> names;\n"
>+                "ErrorResult rv;\n"
>+                "self->GetOwnPropertyNames(cx, names, rv);\n"
>+                "rv.WouldReportJSException();\n"
>+                "if (rv.Failed()) {\n"
>+                '  return ThrowMethodFailedWithDetails<true>(cx, rv, "%s", "enumerate");\n'
>+                "}\n"
>+                '// OK to pass "obj" as "proxy" because it\'s ignored if\n'
>+                "// shadowPrototypeProperties is true\n"
>+                "return DOMProxyHandler::AppendNamedPropertyIds(cx, JS::NullPtr(), names, true, nullptr, props);"))

well, you don't pass obj, and nullptr makes more sense anyway. Remove the comment.

>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=707564">Mozilla Bug 707564</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+<iframe id="t" src="http://example.org/tests/dom/bindings/test/file_bug707564.html"></iframe>
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for Bug 775543 **/
>+function test()
>+{
>+  var props = Object.getOwnPropertyNames(XPCNativeWrapper(document.getElementById("t").contentWindow.navigator));
>+  isnot(props.indexOf("mozApps"), -1,
>+        "Should enumerate a mozApps property on navigator xray");
Could you test for-in, and perhaps also some expando property.



I need to still understand how CGEnumerateOwnPropertiesViaGetOwnPropertyNames is actually about XRay
Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.

Please file a bug about making Navigator stuff to use prototype.
Attachment #783667 - Flags: review?(bugs) → review+
> In the test could you try also for-in.

Will do.

> the generated code, and I believe even the codegen itself would be
> easier to read if aFoo style was used for parameters.

Let's take that up with peterv when he gets back.  ;)

> well, you don't pass obj, and nullptr makes more sense anyway. Remove the comment.

Done; it was left over from a previous patch version.

> Could you test for-in, and perhaps also some expando property.

I can test for/in.  For the expando bit you mean test that it's not present?
And https://hg.mozilla.org/integration/mozilla-inbound/rev/0868beaeab71 to deal with push notifications code not following the JS-implemented webidl contract.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.