see if we can devirtualize any of nsICSSDeclaration

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: heycam, Assigned: bzbarsky)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(8 attachments)

(Reporter)

Description

10 months ago
Followup from bug 1427512.  It's possible this interface could shrink a bit too.
(Assignee)

Updated

10 months ago
Depends on: 1427512
(Assignee)

Comment 1

10 months ago
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.
(Assignee)

Comment 2

10 months ago
Actually, I guess we can do removals here too.
(Assignee)

Comment 3

10 months ago
Created attachment 8946684 [details] [diff] [review]
part 1.  Devirtualize nsICSSDeclaration::GetDocGroup

MozReview-Commit-ID: 7OGRkYTjTSQ
Attachment #8946684 - Flags: review?(emilio)
(Assignee)

Updated

10 months ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 months ago
Created attachment 8946685 [details] [diff] [review]
part 2.  Change Element::GetSMILOverrideStyle to return nsDOMCSSAttributeDeclaration

MozReview-Commit-ID: 9R6ywUEKagK
Attachment #8946685 - Flags: review?(emilio)
(Assignee)

Comment 5

10 months ago
Created attachment 8946686 [details] [diff] [review]
part 3.  Remove the nsCSSPropertyID overload of nsICSSDeclaration::GetPropertyValue

MozReview-Commit-ID: FHrkptqa2hZ
Attachment #8946686 - Flags: review?(emilio)
(Assignee)

Comment 6

10 months ago
Created attachment 8946687 [details] [diff] [review]
part 4.  Remove the nsCSSPropertyID overload of nsICSSDeclaration::SetPropertyValue

MozReview-Commit-ID: 8BzCHahJjwv
Attachment #8946687 - Flags: review?(emilio)
(Assignee)

Comment 7

10 months ago
Created attachment 8946688 [details] [diff] [review]
part 5.  Make nsICSSDeclaration::Get/SetCSSText have nicer signatures

MozReview-Commit-ID: B25qvxKj9CW
Attachment #8946688 - Flags: review?(emilio)
(Assignee)

Comment 8

10 months ago
Created attachment 8946689 [details] [diff] [review]
part 6.  Give nsICSSDeclaration::GetPropertyPriority a nicer signature

MozReview-Commit-ID: 4H2zADwdo5L
Attachment #8946689 - Flags: review?(emilio)
(Assignee)

Comment 9

10 months ago
Created attachment 8946690 [details] [diff] [review]
part 7.  Make the nsICSSDeclaration length API nicer

MozReview-Commit-ID: 2gs8npBJFJY
Attachment #8946690 - Flags: review?(emilio)
(Assignee)

Comment 10

10 months ago
Created attachment 8946691 [details] [diff] [review]
part 8.  Remove unnecessary Item() method

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+
(Assignee)

Comment 17

10 months ago
> 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

10 months 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.