stylo: ServoElementSnapshot is somewhat dumb.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

Attachments

(4 attachments)

No description provided.
Comment on attachment 8933945 [details]
Bug 1422538: Inline ServoElementSnapshot destructor.

https://reviewboard.mozilla.org/r/204880/#review211984
Attachment #8933945 - Flags: review?(bzbarsky) → review+
Comment on attachment 8933946 [details]
Bug 1422538: Inline ServoElementSnapshot::AddAttrs.

https://reviewboard.mozilla.org/r/204882/#review211988

OK, inlining this makes sense esp. given there is only one callsite (though you'd think LTO would take care of that....)

But having the code in the class decl like this is pretty hard on readability, given how long this function body is.  So I'd prefer that you just mark it inline here and put the function definition at the end of this file, after the class declaration.  r=me with that.

::: layout/style/ServoElementSnapshot.h:23
(Diff revision 1)
>  #include "nsAtom.h"
>  
>  namespace mozilla {
>  
>  namespace dom {
>  class Element;

You can take this bit out now.
Attachment #8933946 - Flags: review?(bzbarsky) → review+
Comment on attachment 8933947 [details]
Bug 1422538: Be smarter when snapshotting attributes.

https://reviewboard.mozilla.org/r/204884/#review211990

Nice!  I wonder how many other uses of GetAttrNameAt could do this instead...  r=me
Attachment #8933947 - Flags: review?(bzbarsky) → review+
Comment on attachment 8933948 [details]
Bug 1422538: Rename ServoElementSnapshotFlags::MaybeClass to just Class.

https://reviewboard.mozilla.org/r/204886/#review211994
Attachment #8933948 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db63a1ad3e7d
Inline ServoElementSnapshot destructor. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb6ddd964b1
Inline ServoElementSnapshot::AddAttrs. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ff136cf8d2
Move ServoElementSnapshot::AddAttrs outside the class declaration. r=bz
(In reply to Pulsebot from comment #9)
> Pushed by ecoal95@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/db63a1ad3e7d
> Inline ServoElementSnapshot destructor. r=bz
> https://hg.mozilla.org/integration/mozilla-inbound/rev/adb6ddd964b1
> Inline ServoElementSnapshot::AddAttrs. r=bz
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ff136cf8d2
> Move ServoElementSnapshot::AddAttrs outside the class declaration. r=bz

Hmm, the "be smarter when..." commit apparently didn't land as an individual commit, I bet I messed up when splitting the last bit (since that required servo changes and I'll land it tomorrow).

Anyway...
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44f01cff875
Use a plain for loop instead of IntegerRange because it busts OSX builds who knows why. r=me
You need to log in before you can comment on or make changes to this bug.