expose nsIRegion::getRects() to scripts

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: (dormant account), Assigned: (dormant account))

Tracking

unspecified
x86
Linux
Points:
---
Bug Flags:
wanted1.9.1 ?

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 361393 [details] [diff] [review]
stuff
(Assignee)

Updated

10 years ago
Blocks: 477105
(Assignee)

Comment 1

10 years ago
Created attachment 361412 [details] [diff] [review]
much simpler ver
Assignee: nobody → tglek
Attachment #361393 - Attachment is obsolete: true
Attachment #361412 - Flags: review?(pavlov)

Comment 2

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

looks good. please add some comments to GetRect around the lifetime of mRectSet
Attachment #361412 - Flags: review?(pavlov) → review+
(Assignee)

Comment 3

10 years ago
Created attachment 361584 [details] [diff] [review]
now with an extra comment
Attachment #361412 - Attachment is obsolete: true
(Assignee)

Comment 4

10 years ago
Created attachment 361587 [details] [diff] [review]
also updated uuid
Attachment #361584 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
Created attachment 361588 [details] [diff] [review]
also updated uuid(for real)
Attachment #361587 - Attachment is obsolete: true
(Assignee)

Comment 6

10 years ago
Created attachment 361590 [details] [diff] [review]
another comment was needed
Attachment #361588 - Attachment is obsolete: true

Updated

10 years ago
Attachment #361590 - Flags: review+
Landed as db65e0a217ef on trunk
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?

Updated

10 years ago
Attachment #361590 - Flags: approval1.9.1?
What's the risk to 1.9.1 if we land it there?  Does this have tests?
(Assignee)

Comment 9

10 years ago
we need a higher perf api for this
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

10 years ago
Created attachment 367929 [details] [diff] [review]
fast getRects()

This is meant to be applied after backing out https://hg.mozilla.org/mozilla-central/rev/db65e0a217ef
Attachment #367929 - Flags: review?(vladimir)
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.
Attachment #367929 - Flags: review?(vladimir) → review+
(Assignee)

Comment 12

10 years ago
http://hg.mozilla.org/mozilla-central/rev/89d1fb16456b
http://hg.mozilla.org/mozilla-central/rev/9d34172f942f
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

10 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/cb3129b2d9f2 and discovered a memory leak while writing a testcase
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

10 years ago
Created attachment 368115 [details] [diff] [review]
leak fix
Attachment #361590 - Attachment is obsolete: true
Attachment #368115 - Flags: review?(vladimir)
Attachment #361590 - Flags: approval1.9.1?
Comment on attachment 368115 [details] [diff] [review]
leak fix

Use xpConnect = do_GetService(nsIXPConnect::GetCID(), &rv) instead of CallGetService.. but fine with that
Attachment #368115 - Flags: review?(vladimir) → review+
(Assignee)

Comment 16

10 years ago
Created attachment 368298 [details] [diff] [review]
testcase + memleak fix
Attachment #368115 - Attachment is obsolete: true
Attachment #368298 - Flags: review?(vladimir)
(Assignee)

Comment 17

10 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/d2cddfaf106c
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #367929 - Flags: approval1.9.1?
(Assignee)

Updated

10 years ago
Attachment #368298 - Flags: approval1.9.1?

Updated

10 years ago
Summary: expose nsIRegion::getrects() to scripts → expose nsIRegion::getRects() to scripts
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.
(Assignee)

Comment 19

9 years ago
(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.
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.
(Assignee)

Comment 21

9 years ago
(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.
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.
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.
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.
(and for what it's worth, attachment 367929 [details] [diff] [review] *is* changing the vtable, though that's easy enough to fix)
I doubt non-script code uses nsIScriptableRegion; it'd just use nsIRegion, no?
Attachment #367929 - Flags: approval1.9.1? → approval1.9.1+
Attachment #368298 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 368298 [details] [diff] [review]
testcase + memleak fix

a191=beltzner

Updated

9 years ago
Whiteboard: [needs 191 landing]
Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1
Flags: wanted1.9.1.x?
Attachment #367929 - Flags: approval1.9.1+ → approval1.9.1-
Attachment #368298 - Flags: approval1.9.1+ → approval1.9.1-
Taras: Is this still wanted on 1.9.1? How safe is it now that we're a bit into the cycle?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
(Assignee)

Comment 30

9 years ago
(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.
Whiteboard: [needs 191 landing]
status1.9.1: ? → wontfix
You need to log in before you can comment on or make changes to this bug.