1473856 - MediaQueryList could use ProbablyShortLivingWrapper + LastRelease

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

See discussion in bug 1473759

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: media_query_short_living.diff

This may not do quite enough what you want. Perhaps we should explicitly trigger snow white killer once we've done deferred release.

remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=14283887ec437b770e47893da89510e312d3f1d4
remote: recorded changegroup in replication log in 0.118s

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8990275 [details] [diff] [review]
media_query_short_living.diff

>+    LinkedListElement<MediaQueryList>* listElement =
>+      static_cast<LinkedListElement<MediaQueryList>*>(this);

This actually seems like a good use case for "auto", fwiw.

r=me

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

auto doesn't really improve readability even in that case, IMO.
Often one wants to know the type of some variable, and scroll up the code o see were the variable is defined. If there is auto anywhere, one needs to read what kind of value is assigned to the variable. Without auto it is always clear.

But I can change this, at least static_cast makes it clear.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Right, the static_cast already has the type very clearly, and imo typing it twice just adds noise here.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: media_query_short_living_2.diff

with auto

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8990481 [details] [diff] [review]
media_query_short_living_2.diff

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

::: layout/style/MediaQueryList.h
@@ +65,5 @@
>  
>  private:
> +  void LastRelease() final
> +  {
> +    auto listElement = static_cast<LinkedListElement<MediaQueryList>*>(this);

Perhaps writing `auto*` could improve readability by indicating the auto variable it's a pointer.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e047937cda
MediaQueryList could use ProbablyShortLivingWrapper + LastRelease, r=bz

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/67e047937cda