1428610 - see if we can devirtualize any of nsICSSDeclaration

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, enhancement)

Description

[Cameron McCormack (:heycam)]

Followup from bug 1427512.  It's possible this interface could shrink a bit too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks to me like we can't obviously devirtualize anything other than GetDocGroup as things stand.

I will file a followup on maybe removing some things from the interface.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, I guess we can do removals here too.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Devirtualize nsICSSDeclaration::GetDocGroup

MozReview-Commit-ID: 7OGRkYTjTSQ

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change Element::GetSMILOverrideStyle to return nsDOMCSSAttributeDeclaration

MozReview-Commit-ID: 9R6ywUEKagK

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the nsCSSPropertyID overload of nsICSSDeclaration::GetPropertyValue

MozReview-Commit-ID: FHrkptqa2hZ

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove the nsCSSPropertyID overload of nsICSSDeclaration::SetPropertyValue

MozReview-Commit-ID: 8BzCHahJjwv

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Make nsICSSDeclaration::Get/SetCSSText have nicer signatures

MozReview-Commit-ID: B25qvxKj9CW

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Give nsICSSDeclaration::GetPropertyPriority a nicer signature

MozReview-Commit-ID: 4H2zADwdo5L

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Make the nsICSSDeclaration length API nicer

MozReview-Commit-ID: 2gs8npBJFJY

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove unnecessary Item() method

MozReview-Commit-ID: 6MQXVA0toiG

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946684 [details] [diff] [review]
part 1.  Devirtualize nsICSSDeclaration::GetDocGroup

sweet, r=me

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946687 [details] [diff] [review]
part 4.  Remove the nsCSSPropertyID overload of nsICSSDeclaration::SetPropertyValue

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

Maybe this and the previous commit should say "Move to nsDOMCSSDeclaration" instead of just remove? Anyway looks fine.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946688 [details] [diff] [review]
part 5.  Make nsICSSDeclaration::Get/SetCSSText have nicer signatures

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

::: layout/style/nsCSSFontFaceRule.cpp
@@ +138,3 @@
>  nsCSSFontFaceStyleDecl::GetCssText(nsAString & aCssText)
>  {
>    GetCssTextImpl(aCssText);

Hmm, after this GetCssText and GetCssTextImpl have the same signature, perhaps it could be simplified further.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946689 [details] [diff] [review]
part 6.  Give nsICSSDeclaration::GetPropertyPriority a nicer signature

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

Err,

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946690 [details] [diff] [review]
part 7.  Make the nsICSSDeclaration length API nicer

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

::: layout/style/nsCSSFontFaceRule.cpp
@@ +231,5 @@
>    uint32_t len = 0;
>    for (nsCSSFontDesc id = nsCSSFontDesc(eCSSFontDesc_UNKNOWN + 1);
>         id < eCSSFontDesc_COUNT;
>         id = nsCSSFontDesc(id + 1))
>      if (mDescriptors.Get(id).GetUnit() != eCSSUnit_Null)

If while you're around you want to add braces, you can do it with r=me. Though you're not touching this code technically so...

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946691 [details] [diff] [review]
part 8.  Remove unnecessary Item() method

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

Nice cleanup :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Maybe this and the previous commit should say "Move to nsDOMCSSDeclaration"

Done.

> Hmm, after this GetCssText and GetCssTextImpl have the same signature, perhaps it could be simplified further.

One's const and one is not.  We could fix that, though.  Filed bug 1434390.

> If while you're around you want to add braces

Done.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d077c5c190
part 1.  Devirtualize nsICSSDeclaration::GetDocGroup.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b5b4868ead
part 2.  Change Element::GetSMILOverrideStyle to return nsDOMCSSAttributeDeclaration.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc0c83e2a1c
part 3.  Move the nsCSSPropertyID overload of nsICSSDeclaration::GetPropertyValue down to nsDOMCSSDeclaration.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/877af6778d58
part 4.  Move the nsCSSPropertyID overload of nsICSSDeclaration::SetPropertyValue to nsDOMCSSDeclaration.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41191a9224d
part 5.  Make nsICSSDeclaration::Get/SetCSSText have nicer signatures.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b83d5f5743a
part 6.  Give nsICSSDeclaration::GetPropertyPriority a nicer signature.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85197e70430
part 7.  Make the nsICSSDeclaration length API nicer.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b01c35b207
part 8.  Remove unnecessary Item() method.  r=emilio

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/34d077c5c190
https://hg.mozilla.org/mozilla-central/rev/a6b5b4868ead
https://hg.mozilla.org/mozilla-central/rev/ecc0c83e2a1c
https://hg.mozilla.org/mozilla-central/rev/877af6778d58
https://hg.mozilla.org/mozilla-central/rev/a41191a9224d
https://hg.mozilla.org/mozilla-central/rev/4b83d5f5743a
https://hg.mozilla.org/mozilla-central/rev/e85197e70430
https://hg.mozilla.org/mozilla-central/rev/03b01c35b207