see if we can devirtualize any of nsICSSDeclaration

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: heycam, Assigned: bzbarsky)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(8 attachments)

Followup from bug 1427512.  It's possible this interface could shrink a bit too.
Depends on: 1427512
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.
Actually, I guess we can do removals here too.
MozReview-Commit-ID: 7OGRkYTjTSQ
Attachment #8946684 - Flags: review?(emilio)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: B25qvxKj9CW
Attachment #8946688 - Flags: review?(emilio)
MozReview-Commit-ID: 4H2zADwdo5L
Attachment #8946689 - Flags: review?(emilio)
MozReview-Commit-ID: 2gs8npBJFJY
Attachment #8946690 - Flags: review?(emilio)
MozReview-Commit-ID: 6MQXVA0toiG
Attachment #8946691 - Flags: review?(emilio)
Comment on attachment 8946684 [details] [diff] [review]
part 1.  Devirtualize nsICSSDeclaration::GetDocGroup

sweet, r=me
Attachment #8946684 - Flags: review?(emilio) → review+
Attachment #8946685 - Flags: review?(emilio) → review+
Attachment #8946686 - Flags: review?(emilio) → review+
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.
Attachment #8946687 - Flags: review?(emilio) → review+
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.
Attachment #8946688 - Flags: review?(emilio) → review+
Attachment #8946689 - Flags: review?(emilio) → review-
Comment on attachment 8946689 [details] [diff] [review]
part 6.  Give nsICSSDeclaration::GetPropertyPriority a nicer signature

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

Err,
Attachment #8946689 - Flags: review- → review+
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...
Attachment #8946690 - Flags: review?(emilio) → review+
Comment on attachment 8946691 [details] [diff] [review]
part 8.  Remove unnecessary Item() method

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

Nice cleanup :)
Attachment #8946691 - Flags: review?(emilio) → review+
> 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.
Blocks: 1434390

Comment 18

a year ago
Pushed by bzbarsky@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.