459623 - Clear all image notifiers from PresShell::Destroy()

Status: RESOLVED WONTFIX

(Core :: Layout, enhancement)

Description

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Clear all image notifiers in PresShell::Destroy(), for two reasons:

1. we can skip telling the pres context to tear it down for each frame
2. we can assert that new notifiers aren't added while destroying the
   frame tree

Not sure if it's worth it, your call.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 342833 [details] [diff] [review]
Patch rev. 1

Did this show up in a profile somewhere?  Otherwise, it doesn't seem worth it.

>+    //XXX Why is this done in nsFrame instead of some frame class
>+    // that actually loads images?

This comment should go, though; nearly all frames can have background images.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 342833 [details] [diff] [review]
Patch rev. 1

Marking review- until questions are answered; feel free to re-request.

Comment 3

[Mats Palmgren (inactive)]

(In reply to comment #1)
> Did this show up in a profile somewhere?

No, not really.  I just printf'ed how many there were and saw that it
typically was 100 - 250.  Even for pages were I didn't expect it, like
a Bugzilla bug page has 141.  But you're right, it's most likely not a
significant part of shell destruction time.  I do see some value in
the asserts but I guess we can live without them.

-> WONTFIX