1815229 - Remove nsImageBoxFrame.

Status: RESOLVED FIXED

(Core :: XUL, task)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Remove nsImageBoxFrame. r=tnikkel,dholbert,#layout

nsImageFrame has support for displaying style URIs / an owned image
request, so use it.

The main behavior difference is that we don't fire load / error
events for those images anymore, but I don't see any event listener for
those around, so I think they can go.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Rename nsImageFrame::Kind::ImageElement to ImageLoadingContent. r=dholbert,tnikkel,#layout

We are about to introduce a kind for <xul:image> elements, so this makes
the naming less confusing.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Rename nsImageFrame::mContentURLRequest and related members. r=dholbert,tnikkel,#layout

It's not for content: url() only anymore (we also use it for
list-style-image, and soon for XUL images too). The difference is that
the frame owns that request (vs. the image content).

Depends on D169981

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Implement nsImageFrame::GetImageFromStyle with a switch statement. r=dholbert,tnikkel,#layout

We're about to add a new Kind, so this makes it a bit easier to follow
once that's done, IMO. Also prevents people from forgetting to update
that method in the future.

Depends on D169982

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Factor out a few PresContext() calls in nsImageFrame::Init. r=dholbert,tnikkel,#layout

Depends on D169983

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Factor out owned request clean-up into its own method. r=dholbert,tnikkel,#layout

We're about to use it a bit more.

Depends on D169984

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Use member initializers a bit more in nsImageFrame. r=dholbert,tnikkel,#layout

Depends on D169985

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7de34d8b5e4
Rename nsImageFrame::Kind::ImageElement to ImageLoadingContent. r=tnikkel,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/c6ad17d05cf3
Rename nsImageFrame::mContentURLRequest and related members. r=tnikkel,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/b2a134c60491
Implement nsImageFrame::GetImageFromStyle with a switch statement. r=tnikkel,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/d5853101eec5
Factor out a few PresContext() calls in nsImageFrame::Init. r=tnikkel,layout-reviewers

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/a7de34d8b5e4
https://hg.mozilla.org/mozilla-central/rev/c6ad17d05cf3
https://hg.mozilla.org/mozilla-central/rev/b2a134c60491
https://hg.mozilla.org/mozilla-central/rev/d5853101eec5

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60ebe844b93b
Use member initializers a bit more in nsImageFrame. r=tnikkel,layout-reviewers

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1815229 - Fix a test failure due to async-decoding of XUL images. r=tnikkel!,Gijs!

UL doesn't have visibility tracking nor discarding of images, and they
were always eagerly decoded. Switching to the <img> model causes
browser_startup_flicker.js to fail due to flickering on the FF view
button, which is a lazily-async-decoded chrome:// PNG image.

Given the other benefits that async-decoding has, and the other benefits
that we get from the switch to nsImageFrame, this seems worth
annotating.

We could do some of these alternatives instead:

• Always sync-decode <xul:image> (what my initial version of D168958
  did). The easiest to implement, maybe more behavior-preserving
  option.

• Always sync-decode chrome:// or resource:// images. Maybe worth it?
  Though some of them could be rather big.

• Add an heuristic to sync-decode small-ish images (maybe only for
  <xul:image>). I think if we needed to work around this, this would be
  slightly preferable, but it's a bit unclear what the heuristic should
  be and whether it's worth it.

• Add a chrome-only CSS property to opt-in to image sync-decoding of
  content/etc images. I'd rather not, seems too complicated for the
  rather small benefit that we'd get.

Given that, I tend to think I'd rather land the patch as is and address
this later if it becomes a problem for other images or if it's a visible
regression in practice. But happy to do something else instead.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbea219ac81c
Factor out owned request clean-up into its own method. r=tnikkel,layout-reviewers

Comment 13

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/60ebe844b93b
https://hg.mozilla.org/mozilla-central/rev/fbea219ac81c

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c7ff0a3cc4e
Remove nsImageBoxFrame. r=tnikkel,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/07438d7c2372
Fix a test failure due to async-decoding of XUL images. r=Gijs

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7c7ff0a3cc4e
https://hg.mozilla.org/mozilla-central/rev/07438d7c2372