Closed Bug 868411 Opened 7 years ago Closed 7 years ago

Handlify js::GetObjectProto

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(1 file)

Attached patch Patch v1Splinter Review
Because a compilation error is better than a static analysis failure.
Attachment #745141 - Flags: review?(terrence)
Comment on attachment 745141 [details] [diff] [review]
Patch v1

Review of attachment 745141 [details] [diff] [review]:
-----------------------------------------------------------------

Generally agreed, however, this is one of the rare cases where we don't actually need to root proto. Since this adds a bunch of new, non-hazard Rooted to generated code, I'm sending the review over to bz.

::: js/xpconnect/src/XPCJSID.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

This seems to be a different header from the ones in JS. Please put these sort of header changes in a different bug, or at least in a different patch.
Attachment #745141 - Flags: review?(terrence) → review?(bzbarsky)
The codegen part of this is fine; it's a case when someone is doing an explicity getElementIfPresent() on something that's a non-array-like proxy.  That's a slow path anyway.

I'm fine reviewing this as long as long as the API change is fine.
Yes, the API change is fine. My worry was that this friend API is there for performance. Ideally, we'd like to fill the API with Handles with the sole exception being places where we need to do something special for performance. Given that this is not true here, r=me for these changes, with the exception of the comment change, which should be handed to njn, sstangle, or someone else who knows what the right form is.
Comment on attachment 745141 [details] [diff] [review]
Patch v1

Watch out for the merge conflicts in dom/bindings.

>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Leave the tab-width: 8, please: the point is for tabs to be visibly different from normal indentation.

r=me with that
Attachment #745141 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 745141 [details] [diff] [review]
> Patch v1
> 
> Watch out for the merge conflicts in dom/bindings.
> 
> >+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> 
> Leave the tab-width: 8, please: the point is for tabs to be visibly
> different from normal indentation.

The issue is that that makes my tab key insert 8 spaces... I'll see if I can fix that with a vim modeline.
Hrm.  Tab should be doing indent (so using c-basic-offset), not inserting the number of spaces that tabs should be expanded to....

But I guess I can live with it if needed.
https://hg.mozilla.org/mozilla-central/rev/1734d1a36463
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.