1422538 - stylo: ServoElementSnapshot is somewhat dumb.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1422538: Inline ServoElementSnapshot destructor.

No point in doing a function call for a default destructor.

Review commit: https://reviewboard.mozilla.org/r/204880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204880/

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1422538: Inline ServoElementSnapshot::AddAttrs.

Review commit: https://reviewboard.mozilla.org/r/204882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204882/

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1422538: Be smarter when snapshotting attributes.

Review commit: https://reviewboard.mozilla.org/r/204884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204884/

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1422538: Rename ServoElementSnapshotFlags::MaybeClass to just Class.

Since it's exact...

Review commit: https://reviewboard.mozilla.org/r/204886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204886/

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

https://reviewboard.mozilla.org/r/204880/#review211984

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933948 [details]
Bug 1422538: Rename ServoElementSnapshotFlags::MaybeClass to just Class.

https://reviewboard.mozilla.org/r/204886/#review211994

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Emilio Cobos Álvarez [:emilio]]

(In reply to Pulsebot from comment #9)
> Pushed by [e-mail]:
> 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...

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/db63a1ad3e7d
https://hg.mozilla.org/mozilla-central/rev/adb6ddd964b1
https://hg.mozilla.org/mozilla-central/rev/b9ff136cf8d2
https://hg.mozilla.org/mozilla-central/rev/f44f01cff875