Closed Bug 821593 Opened 7 years ago Closed 7 years ago

convert RGBColor to webidl

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(11 files, 2 obsolete files)

884 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.26 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
1.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.73 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.81 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.26 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.60 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.83 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #692179 - Flags: review?(bzbarsky)
not really needed, but it'll mean we can avoid xpcom in some places in later patches
Attachment #692181 - Flags: review?(bzbarsky)
this still works with the external interface definition of RGBColor, but will now also when work when its native type is nsDOMRGBColor.
Attachment #692182 - Flags: review?(bzbarsky)
Attachment #692182 - Attachment is obsolete: true
Attachment #692182 - Flags: review?(bzbarsky)
Attachment #692181 - Attachment is obsolete: true
Attachment #692181 - Flags: review?(bzbarsky)
Comment on attachment 692179 [details] [diff] [review]
part 1 add RGBColor webidl

r=me
Attachment #692179 - Flags: review?(bzbarsky) → review+
Comment on attachment 692180 [details] [diff] [review]
part 2 add webidl api to RGBColor and store its members as nsROCSSPrimitiveValue* not nsIDOMCSSPrimitiveValue*

r=me
Attachment #692180 - Flags: review?(bzbarsky) → review+
Comment on attachment 692180 [details] [diff] [review]
part 2 add webidl api to RGBColor and store its members as nsROCSSPrimitiveValue* not nsIDOMCSSPrimitiveValue*

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

::: layout/style/nsDOMCSSRGBColor.h
@@ +30,5 @@
>  
>    bool HasAlpha() const { return mHasAlpha; }
>  
> +  // RGBColor webidl interface
> +  nsROCSSPrimitiveValue *Red() const

* to the left, please
Attachment #692180 - Flags: review+ → review?(bzbarsky)
Comment on attachment 692180 [details] [diff] [review]
part 2 add webidl api to RGBColor and store its members as nsROCSSPrimitiveValue* not nsIDOMCSSPrimitiveValue*

Silly bugzilla... Resetting r+
Attachment #692180 - Flags: review?(bzbarsky) → review+
(In reply to :Ms2ger from comment #7)
> Comment on attachment 692180 [details] [diff] [review]
> part 2 add webidl api to RGBColor and store its members as
> nsROCSSPrimitiveValue* not nsIDOMCSSPrimitiveValue*
> 
> Review of attachment 692180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsDOMCSSRGBColor.h
> @@ +30,5 @@
> >  
> >    bool HasAlpha() const { return mHasAlpha; }
> >  
> > +  // RGBColor webidl interface
> > +  nsROCSSPrimitiveValue *Red() const
> 
> * to the left, please

ok fine ;\ (I'll "fix" all these patches at once)
Attachment #692386 - Flags: review?(bzbarsky)
Attachment #692387 - Flags: review?(bzbarsky)
Attachment #692389 - Flags: review?(bzbarsky)
nsDOMCSSRGBColor can loose the nsISupports too, but I just asume do that after bug 819215
Attachment #692397 - Flags: review?(bzbarsky)
Comment on attachment 692382 [details] [diff] [review]
part 3 v2 add downcasting from CSSValue to nsROCSSPrimitiveValue

I would rather the implementation did the test of CssValueType() and returned null if not, and we documented that, instead of relying on all callers checking + assert.

r=me if you do that.
Attachment #692382 - Flags: review?(bzbarsky) → review+
Comment on attachment 692383 [details] [diff] [review]
part 4 v2 GetRGBColorValue() should return nsDOMCSSRGBColor*

>+++ b/dom/bindings/Bindings.conf
>+  "nativeType": "nsROCSSPrimitiveValue",
>+    "resultNotAddRefed": ["GetRGBColorValue"]

Please fix the indent.  And it should be "getRGBColorValue".

r=me with that.
Attachment #692383 - Flags: review?(bzbarsky) → review+
Comment on attachment 692386 [details] [diff] [review]
part 5 cycle collect and wrapper cache

r=me
Attachment #692386 - Flags: review?(bzbarsky) → review+
Comment on attachment 692387 [details] [diff] [review]
part 5 using webidl for RGBColor

>+++ b/dom/bindings/Bindings.conf
>+    'resultNotAddRefed': [ "Alpha", "Blue", "Green", "Red" ]

These should use the IDL (lowercase) names, not the C++ ones.

>+++ b/layout/style/nsDOMCSSRGBColor.cpp
>+  JSObject*
>+  nsDOMCSSRGBColor::WrapObject(JSContext *aCx, JSObject *aScope, bool *aTried)

Please fix the indent.

r=me with that.
Attachment #692387 - Flags: review?(bzbarsky) → review+
Comment on attachment 692389 [details] [diff] [review]
part 6 remove dom classinfo stuff

r=me
Attachment #692389 - Flags: review?(bzbarsky) → review+
Comment on attachment 692390 [details] [diff] [review]
part 7 stop using nsIDOMCSSValue / nsIDOMRGBColor in editor

r=me with the changes to AsPrimitiveValue I asked for.
Attachment #692390 - Flags: review?(bzbarsky) → review+
Comment on attachment 692393 [details] [diff] [review]
part 8 remove nsIDOMCSSPrimitiveValue:::GetRGBColor()

r=me
Attachment #692393 - Flags: review?(bzbarsky) → review+
Comment on attachment 692395 [details] [diff] [review]
part 9 don't use nsIDOMRGBColor xpcom methods in nsROCSSPrimitiveValue

r=me
Attachment #692395 - Flags: review?(bzbarsky) → review+
Comment on attachment 692397 [details] [diff] [review]
part 10 remove the nsIDOMRGBColor xpidl now that nothing uses it

r=me.  Thank you for breaking this up into pieces!
Attachment #692397 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #24)
> Comment on attachment 692390 [details] [diff] [review]
> part 7 stop using nsIDOMCSSValue / nsIDOMRGBColor in editor
> 
> r=me with the changes to AsPrimitiveValue I asked for.

sure, I did AsPrimitiveValue() the way I doid to be similar to AsElement() / AsContent() but that doesn't seem hugely important.

should this editor stuff still check the type before trying to cast?

btw thanks for quick reviews :)
> should this editor stuff still check the type before trying to cast?

If AsPrimitiveValue() returns null on incorrect type, then no.  I guess you're right that what you had was similar to AsElement(); the other parent is used for the various element FromContent() methods...
Whiteboard: [leave open]
Whiteboard: [leave open]
Depends on: 840906
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.