477708 - expose nsIRegion::getRects() to scripts

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(dormant account)]

Attachment: stuff

Comment 1

[(dormant account)]

Attachment: much simpler ver

Comment 2

[Stuart Parmenter]

Comment on attachment 361412 [details] [diff] [review]
much simpler ver

looks good. please add some comments to GetRect around the lifetime of mRectSet

Comment 3

[(dormant account)]

Attachment: now with an extra comment

Comment 4

[(dormant account)]

Attachment: also updated uuid

Comment 5

[(dormant account)]

Attachment: also updated uuid(for real)

Comment 6

[(dormant account)]

Attachment: another comment was needed

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Landed as db65e0a217ef on trunk

Comment 8

[Damon Sicore (:damons)]

What's the risk to 1.9.1 if we land it there?  Does this have tests?

Comment 9

[(dormant account)]

we need a higher perf api for this

Comment 10

[(dormant account)]

Attachment: fast getRects()

This is meant to be applied after backing out https://hg.mozilla.org/mozilla-central/rev/db65e0a217ef

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 367929 [details] [diff] [review]
fast getRects()

Looks fine, but:

>+  for(PRUint32 i = 0;i<mRectSet->mNumRects;i++) {

Add some spaces here, at least after the ; and around the <.. spaces don't cost nothin'!

>+    JS_DefineElement(cx, destArray, n, INT_TO_JSVAL(rect.x), NULL, NULL, JSPROP_ENUMERATE);
>+    JS_DefineElement(cx, destArray, n+1, INT_TO_JSVAL(rect.y), NULL, NULL, JSPROP_ENUMERATE);
>+    JS_DefineElement(cx, destArray, n+2, INT_TO_JSVAL(rect.width), NULL, NULL, JSPROP_ENUMERATE);
>+    JS_DefineElement(cx, destArray, n+3, INT_TO_JSVAL(rect.height), NULL, NULL, JSPROP_ENUMERATE);

Add a comment here (and maybe in the docs in the idl) saying that you'll get bogus data if the coords don't fit in a jsval.

Comment 12

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/89d1fb16456b
http://hg.mozilla.org/mozilla-central/rev/9d34172f942f

Comment 13

[(dormant account)]

pushed http://hg.mozilla.org/mozilla-central/rev/cb3129b2d9f2 and discovered a memory leak while writing a testcase

Comment 14

[(dormant account)]

Attachment: leak fix

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 368115 [details] [diff] [review]
leak fix

Use xpConnect = do_GetService(nsIXPConnect::GetCID(), &rv) instead of CallGetService.. but fine with that

Comment 16

[(dormant account)]

Attachment: testcase + memleak fix

Comment 17

[(dormant account)]

pushed http://hg.mozilla.org/mozilla-central/rev/d2cddfaf106c

Comment 18

[Damon Sicore (:damons)]

What's the impact of this API change?  Anything we need to do to make sure we don't break people? Just want to make sure we're thinking about this before we approve.  Once we have an answer, will approve.

Comment 19

[(dormant account)]

(In reply to comment #18)
> What's the impact of this API change?  Anything we need to do to make sure we
> don't break people? Just want to make sure we're thinking about this before we
> approve.  Once we have an answer, will approve.

it's a revised api addition, so no impact on existing users.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We've promised no scriptable interface changes on 1.9.1 (especially after b3), so you can't change nsIScriptableRegion there at this point. You'll have to add nsIScriptableRegion_MOZILLA_1_9_1, I guess.

Comment 21

[(dormant account)]

(In reply to comment #20)
> We've promised no scriptable interface changes on 1.9.1 (especially after b3),
> so you can't change nsIScriptableRegion there at this point. You'll have to add
> nsIScriptableRegion_MOZILLA_1_9_1, I guess.

That's a bit of an overkill, there is a very good chance that nobody uses that api other than us.

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think it's _certain_ that nobody other than us implements it, and it doesn't affect scripted consumers.  If the entry appears at the end of the vtable, I think it's fine.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I agree with shaver -- we're adding functionality and we're doing it at the end of the vtable.  If we break someone I will eat my tie.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

I made the same argument in bug 483634, and bz disagreed. Different situation, perhaps, but I think we should at least try to be consistent in making these decisions.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(and for what it's worth, attachment 367929 [details] [diff] [review] *is* changing the vtable, though that's easy enough to fix)

Comment 26

[Boris Zbarsky [:bzbarsky]]

I doubt non-script code uses nsIScriptableRegion; it'd just use nsIRegion, no?

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368298 [details] [diff] [review]
testcase + memleak fix

a191=beltzner

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1

Comment 29

[Samuel Sidler (old account; do not CC)]

Taras: Is this still wanted on 1.9.1? How safe is it now that we're a bit into the cycle?

Comment 30

[(dormant account)]

(In reply to comment #29)
> Taras: Is this still wanted on 1.9.1? How safe is it now that we're a bit into
> the cycle?

We decided to not ship fennec on 1.9.1, so this is no longer needed on 1.9.1.