Closed Bug 477708 Opened 11 years ago Closed 11 years ago

expose nsIRegion::getRects() to scripts

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.1 --- wontfix

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(2 files, 7 obsolete files)

Attached patch stuff (obsolete) — Splinter Review
No description provided.
Blocks: 477105
Attached patch much simpler ver (obsolete) — Splinter Review
Assignee: nobody → tglek
Attachment #361393 - Attachment is obsolete: true
Attachment #361412 - Flags: review?(pavlov)
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+
Attached patch now with an extra comment (obsolete) — Splinter Review
Attachment #361412 - Attachment is obsolete: true
Attached patch also updated uuid (obsolete) — Splinter Review
Attachment #361584 - Attachment is obsolete: true
Attached patch also updated uuid(for real) (obsolete) — Splinter Review
Attachment #361587 - Attachment is obsolete: true
Attached patch another comment was needed (obsolete) — Splinter Review
Attachment #361588 - Attachment is obsolete: true
Attachment #361590 - Flags: review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.1?
Attachment #361590 - Flags: approval1.9.1?
What's the risk to 1.9.1 if we land it there?  Does this have tests?
we need a higher perf api for this
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fast getRects()Splinter Review
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+
http://hg.mozilla.org/mozilla-central/rev/89d1fb16456b
http://hg.mozilla.org/mozilla-central/rev/9d34172f942f
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
pushed http://hg.mozilla.org/mozilla-central/rev/cb3129b2d9f2 and discovered a memory leak while writing a testcase
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch leak fix (obsolete) — Splinter Review
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+
Attachment #368115 - Attachment is obsolete: true
Attachment #368298 - Flags: review?(vladimir)
pushed http://hg.mozilla.org/mozilla-central/rev/d2cddfaf106c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #367929 - Flags: approval1.9.1?
Attachment #368298 - Flags: approval1.9.1?
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.
(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.
(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
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?
(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]
You need to log in before you can comment on or make changes to this bug.