868411 - Handlify js::GetObjectProto

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Because a compilation error is better than a static analysis failure.

Comment 1

[Terrence Cole [: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.

Comment 2

[Boris Zbarsky [: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.

Comment 3

[Terrence Cole [:terrence]]

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 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1734d1a36463