SvendTofte Reporter
	Description • 21 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030804
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030804

Using the css property "content", to replace an element
  acronym , abbr { content:attr(title); }
doesn't work. Only when using :before or :after, does the content display. Opera
seems to be able to do it, and the W3 working draft seem pretty clear, on how it
should work.

http://www.w3.org/TR/css3-content/#content

Maybe this shouldn't be filed, since the spec isn't a TR yet? If so, sorry for
wasting your time :)

Reproducible: Always

Steps to Reproduce:
1. Go to url

Actual Results:  
Mozilla didn't replace the element content, with the contents of the responding
elements title attribute.

Expected Results:  
It should replace the element content, with the value of it's title attribute.

	David Baron :dbaron:
	Comment 1 • 21 years ago

This spec is nowhere near Candidate Recommendation and is thus not ready for
implementation.  I think it just clutters up bug lists to have bugs on such
things, so marking as wontfix, for now, anyway.

	SvendTofte Reporter
	Comment 2 • 21 years ago

Cool enough, just wanted to make sure what the policy was on yet-TR specs :)

	Bill Mason
	Comment 3 • 21 years ago

*** Bug 215403 has been marked as a duplicate of this bug. ***

	Anne (:annevk)
	Comment 4 • 21 years ago

Sorry for the duplicate. I searched Bugzilla and I didn't found any bug/feature
request about this.

About the test case:
 The text should be: "The rule is applied", as it is in Opera 7.11.

	David Baron :dbaron:
	Comment 5 • 21 years ago

I'll reopen, but I don't want to encourage this, because it just makes real bugs
harder to find, and I certainly don't want a bug for every feature we haven't
implemented.

	Justin Wood (:Callek)
	Comment 6 • 20 years ago

*** Bug 219417 has been marked as a duplicate of this bug. ***

	David Baron :dbaron:
	Comment 7 • 19 years ago

See also bug 309217, for the css3 content property's fallback lists.

	David Baron :dbaron:
	Comment 8 • 19 years ago

*** Bug 315391 has been marked as a duplicate of this bug. ***

	Martin Suchan
	Comment 11 • 13 years ago

Sorry for my duplicate, but anyway. This issue has been opened 7 years ago(!) and it is still unsolved? The usage of 'label' in 'option' tag is well defined since 1999 in HTML 4.01 Specification and should be working, imho:
http://test.suchan.cz/test.html
http://www.w3.org/TR/html401/interact/forms.html#h-17.6
OPTION Attribute definitions
When rendering a menu choice, user agents should use the value of the label attribute of the OPTION element as the choice. If this attribute is not specified, user agents should use the contents of the OPTION element.

	j.j.
	Comment 15 • 13 years ago

Alexei, (In reply to Alexei from comment #13)
> Well, when you fix it?

Not before there is s specification. 
You re-reported this twice no, please stop it! (bug 689088 comment 5)

	Martin Suchan
	Comment 16 • 13 years ago

At least the 'label' attribute in 'option' tags should be fixed (it's part of basic HTML 4.01 specification) - it works in IE, Chrome, Safari and Opera, so why not in FF?
https://bugzilla.mozilla.org/show_bug.cgi?id=40545
Compare this page in FF and Chrome/IE http://jsfiddle.net/jPKQz/3/

	Lea Verou
	Comment 18 • 13 years ago

(In reply to Alexei from comment #17)
> Firefox is getting worse! Soon come to the point that this browser will be
> ****.

O RLY? Which other browser do you think implements this? Lemme give you a hint: Only Opera.

	Martin Suchan
	Comment 19 • 13 years ago

This bug 215083 blocks(?) bug No. 40545: 'labels' for 'option' tags, which is something implemented in all other browsers except FF ;)

	j.j.
	Comment 20 • 13 years ago

People, please read
  https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
before adding more comments. (Reading this ideally would prompt you to stop commenting)

(And comments about bug 40545 are mostly unrelated here. Fixing bug 215083 is not the one and only option to fix that bug.)

	Alexei
	Comment 21 • 13 years ago

Support all browsers: Safari, Opera, Google Chrome and even IE version 10.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 22 • 13 years ago

(In reply to Alexei from comment #21)
> Support all browsers: Safari, Opera, Google Chrome and even IE version 10.

For ::before and ::after, yes. This bug is explicitly about div { content: "Hi"; }, which is currently only supported in Opera.

	philippe (part-time)
	Comment 23 • 13 years ago

(In reply to Ms2ger from comment #22)
> (In reply to Alexei from comment #21)
> > Support all browsers: Safari, Opera, Google Chrome and even IE version 10.
> 
> For ::before and ::after, yes. This bug is explicitly about div { content:
> "Hi"; }, which is currently only supported in Opera.

WebKit (Safari 5.1, Chrome something) supports div { content: url(path/to/image/png); }; but div { content: 'foo bar'; } is unfortunately not yet supported.

see also this thread: http://lists.w3.org/Archives/Public/www-style/2011Nov/thread.html#msg451

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 24 • 13 years ago

(In reply to philippe from comment #23)
> (In reply to Ms2ger from comment #22)
> > (In reply to Alexei from comment #21)
> > > Support all browsers: Safari, Opera, Google Chrome and even IE version 10.
> > 
> > For ::before and ::after, yes. This bug is explicitly about div { content:
> > "Hi"; }, which is currently only supported in Opera.
> 
> WebKit (Safari 5.1, Chrome something) supports div { content:
> url(path/to/image/png); }; but div { content: 'foo bar'; } is unfortunately
> not yet supported.

I didn't know that; thanks for the correction.

	Alexei
	Comment 26 • 12 years ago

Stable draft: http://www.w3.org/TR/css3-content/

	j.j.
	Comment 27 • 12 years ago

(In reply to Alexei from comment #26)
> Stable draft: http://www.w3.org/TR/css3-content/

This draft is not stable and out of date. Current draft is in URL field.

(Alexei, sorry, some of your comments here and in other bugs aren't helpful at all. You can not speed up the implementation process this way. Please read
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html )

	Jim Michaels
	Comment 29 • 12 years ago

for reference, this is in css 2.1 TR as well. not sure why 2.1 is in TR right now.
http://www.w3.org/TR/CSS21/generate.html#propdef-content
I see no date on this document.

	Jim Michaels
	Comment 30 • 12 years ago

by the way, chrome doesn't implement this either.

	David Baron :dbaron:
	Comment 31 • 12 years ago

(In reply to Jim Michaels from comment #29)
> for reference, this is in css 2.1 TR as well. not sure why 2.1 is in TR
> right now.
> http://www.w3.org/TR/CSS21/generate.html#propdef-content
> I see no date on this document.

No, in CSS 2.1 it's only for the :before and :after pseudo-elements.

	Gordon P. Hemsley [:GPHemsley]
	Comment 35 • 11 years ago

@dbaron: What would it take to implement this? I note the spec is no longer being maintained, and "should not be used as a guide for implementations". Is there a problem with the spec? Would you be available as a mentor?

	David Baron :dbaron:
	Comment 36 • 10 years ago

I think the first step to implementing this would be to figure out what the state of the spec is -- how solid it is, how much working group consensus there is behind it, etc.  I think it was probably reasonably well-specified, but it might not all make sense anymore.

I'd be willing to mentor somebody doing that, I think, though they'd need to be willing to do that without extremely detailed guidance.


Implementation wouldn't be trivial; it would require some to frame construction code, figuring out how it should interact with things like image loading, frame loading, and loading of any other resource types that would be supported (video?).  (I wonder if the feature would work better if, instead of url(), the arguments made it clear whether the resource was an image or frame before it was loaded.  That would certainly be helpful from an implementation perspective, and may well be more efficient.)

	Masaya Iseki[:isk](UTC+9)
	Comment 37 • 10 years ago

I'm interested in solving this bug.
But I don't have enough knowledge to solve it.

If you don't mind, would you please tell me the overview how to implement it?
I have read the document[1] but this document is too old. 

[1]http://dxr.mozilla.org/mozilla-central/source/layout/doc

	Daniel Holbert [:dholbert]
	Comment 38 • 8 years ago

FWIW, fantasai rewrote/updated the (2003-era) css-content-3 spec, and announced it back in June on www-style:
 https://lists.w3.org/Archives/Public/www-style/2016Jun/0137.html

Hopefully the modern spec has fewer of the possible-pitfalls that dbaron was hinting at in comment 36, and might be more directly implementable...

	David Baron :dbaron:
	Comment 39 • 8 years ago

Oh, and since I accidentally cleared the needinfo? when adding the "See Also" in response to dholbert's comment -- I guess the real response to comment 37 is that this isn't a good choice of bug for somebody who hasn't already written some substantial Gecko code before.  Figuring out a reasonable plan for implementing it is at least multiple hours of work if not multiple days.

	David Baron :dbaron:
	Comment 40 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #38)
> Hopefully the modern spec has fewer of the possible-pitfalls that dbaron was
> hinting at in comment 36, and might be more directly implementable...

As I pointed out in https://github.com/w3c/csswg-drafts/issues/821 , I don't think the spec is even clear enough to *tell* whether those pitfalls are still present.  (For example, does the spec expect url() to load images, videos, iframes, etc., or only images?  What does Blink do there?)

	David Baron :dbaron:
	Comment 44 • 6 years ago

Transferring webcompat markings from duplicate bug 1372086.

	Jonathan Watt [:jwatt]
	Comment 45 • 6 years ago

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

	Maire Reavy [:mreavy]
	Comment 46 • 6 years ago

Cameron -- As I mentioned in the Layout standup meeting last week, this is a P1 webcompat bug that I really want to see solved.  Can you assess how you'd approach this?  Would you be available to mentor the assignee of it?  Thanks.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 47 • 6 years ago

I could probably take this, given this is frame constructor work.

However I think google is not being fair requesting us to implement this in order to ship the tier1 page on android when there are more suitable replacements for this, and the way this is implemented in both Blink and WebKit is kind of a hack.

	Xidorn Quan [:xidorn] UTC+11
	Comment 48 • 6 years ago

I've raised several new spec issues after reading the spec and playing with the implementation of Chrome and Safari:
* w3c/csswg-drafts#2656
* w3c/csswg-drafts#2657

It seems to me that Blink and WebKit currently only implement <content-replacement> for element (that a single. That doesn't feel terribly hard to implement in terms of frame constructing. We probably just need a new frame class, or maybe just reuse the frame of <img>, and yield the frame when we meet content property with <content-replacement> value on an element.

It may, though, need some nontrivial work on the style system, due to that we may not be generating frames for elements which are not in a display:none subtree. Maybe it's not that hard, though. We can probably just take <iframe>, <video>, and maybe other replaced element as a reference for how to suppress children.

But it's not clear to me whether we should go this way... Probably need to have some consensus from the working group on the two issues I mentioned above.

If the working group can agree on those issues, ideally we should shrink the feature set of css-content-3 to just include `content: <content-replacement>` for element in addition to what CSS 2 already has, and defer others into the next level...

TBH, I'm not very excited about the current status of the implementation of Blink and WebKit. But if webcompat requires us to do so...

(An alternative approach for the webcompat user story of this bug is to ask Google not to use this experimental feature in their tier-1 UI...)

	Xidorn Quan [:xidorn] UTC+11
	Comment 49 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> It seems to me that Blink and WebKit currently only implement
> <content-replacement> for element (that a single.

(that a single url means a <image> replacing the content).

	Cameron McCormack (:heycam)
	Comment 50 • 6 years ago

I'm happy to mentor someone who wants to look at this.

The first thing to do would be to verify what parts of the spec we actually need to implement.  I suspect it is content:url(...) on certain elements with sizing behavior of some sort.  Bug 1239922 comment 11 mentions this, but it would be good to know what other pages are relying on this feature and to what extent.

Once we've determined what features from the spec we need, we should do some thorough testing to see how Chrome (and other browsers, if any now do support this) behave.  Then, compare this with what the spec requires, make some judgements as to what actually makes sense, and then file issues on the spec accordingly.

Some brief testing in Chrome shows that not all elements can have their contents replaced, for example it works on an <img>, but not on a <button> or an <iframe>:

  http://mcc.id.au/temp/content.html

For the implementation, I think there are two ways we could go.

One approach would be to model this like we do for ::before / ::after generated content, where we create native anonymous content to represent the image.  This would be created via CreateGenConImageContent, which is what we use for `::before { content: url() }`, and which is already set up to create an nsImageFrame and handle image loading due to nsGenConImageContent (the concrete DOM node class we use) implementing nsImageLoadingContent.

We would need to tweak how the sizing works somehow to look at the parent real element's sizing proprerties somehow, which might be tricky.  How do we make `span { content: url(...); width: 200px; }` influence the size of the child gencon element?

We would need to suppress frames for the actual content of the element.

I suspect there would be other issues with using generated content here, e.g. a border doesn't go around an entire generated content image in ::before / ::after, but it seems to work fine in Chrome for content:url() on an element.

(If in the future the spec allowed `span { content: "some text"; }` as well as just an image, then this approach would easily extend to supporting that.  But given the need to make width/height on the element influence the image's size, I don't think the spec will go in that direction.)

The second approach would be to make the element itself get an nsImageFrame, by adjusting nsImageFrame::ShouldCreateImageFrameFor to return true for elements that have a content:url() value.  An nsImageFrame for the element is what we do for <img>.  We wouldn't have any issues with mismatches between the sizes of the real element's frame and the gencon element's frame, since we've just got the one element.

We would need an instance of nsImageLoadingContent somewhere, and I don't think we want to make all HTML elements inherit from this.  Maybe we can create one and stuff it in the DOM element's property table.  nsImageLoadingContent currently makes some assumptions that it is implemented on an nsIContent -- see the do_QueryInterface calls in nsImageLoadingContent.cpp.  But they should probably all be using nsImageLoadingContent::AsContent anyway, which we could override to return our element.  I'm not sure what the best way to manage the lifetime of this nsImageLoadingContent object would be.  Perhaps the nsImageFrame's constructor/destructor could be in charge of calling something on the element to add/remove it.

<img> elements already suppress the frames for any children they might have, but I'm not sure how that happens.  We might still need to do something to make that work here too, or it might work for free.

On balance I think the second approach is going to be easier, just because we don't have to worry about the fact that we have a frame for the real DOM element and one for the gencon element.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 51 • 6 years ago

The chromium implementation is:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_object.cc?l=216&rcl=ae6a05c5388e96c3e430d385c38e53e8bd61760d

Note that they still respect display: none or contents because of Element::ShouldCreateLayoutObject.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> It seems to me that Blink and WebKit currently only implement
> <content-replacement> for element (that a single. That doesn't feel terribly
> hard to implement in terms of frame constructing. We probably just need a
> new frame class, or maybe just reuse the frame of <img>, and yield the frame
> when we meet content property with <content-replacement> value on an element.

That is exactly true.

> It may, though, need some nontrivial work on the style system, due to that
> we may not be generating frames for elements which are not in a display:none
> subtree. Maybe it's not that hard, though. We can probably just take
> <iframe>, <video>, and maybe other replaced element as a reference for how
> to suppress children.

That's not right. We already do that for other replaced elements, it should just work, no need to do anything special there. We just will consider the frame a leaf, and won't find an insertion point if the element is replaced. There's no extra style system hackery required I think.

(In reply to Cameron McCormack (:heycam) from comment #50)
> Once we've determined what features from the spec we need, we should do some
> thorough testing to see how Chrome (and other browsers, if any now do
> support this) behave.  Then, compare this with what the spec requires, make
> some judgements as to what actually makes sense, and then file issues on the
> spec accordingly.

As Xidorn said, WebKit / Blink's impl is pretty minimal.

> Some brief testing in Chrome shows that not all elements can have their
> contents replaced, for example it works on an <img>, but not on a <button>
> or an <iframe>:
> 
>   http://mcc.id.au/temp/content.html

Right, they do it in the equivalent of our FindDisplayData in the frame constructor, that is, once we know an element isn't any special kind of element.

> One approach would be to model this like we do for ::before / ::after
> generated content, where we create native anonymous content to represent the
> image.  This would be created via CreateGenConImageContent, which is what we
> use for `::before { content: url() }`, and which is already set up to create
> an nsImageFrame and handle image loading due to nsGenConImageContent (the
> concrete DOM node class we use) implementing nsImageLoadingContent.
> 
> We would need to tweak how the sizing works somehow to look at the parent
> real element's sizing proprerties somehow, which might be tricky.  How do we
> make `span { content: url(...); width: 200px; }` influence the size of the
> child gencon element?
> 
> We would need to suppress frames for the actual content of the element.

Right, this sounds a no-go, we'd need to treat the content as display: contents, make the generated content all: inherit... Sounds complicated.

> The second approach would be to make the element itself get an nsImageFrame,
> by adjusting nsImageFrame::ShouldCreateImageFrameFor to return true for
> elements that have a content:url() value.  An nsImageFrame for the element
> is what we do for <img>.  We wouldn't have any issues with mismatches
> between the sizes of the real element's frame and the gencon element's
> frame, since we've just got the one element.

Well, we'd need to do it in the equivalent place I pointed up above, but yes, this is basically true.

> We would need an instance of nsImageLoadingContent somewhere, and I don't
> think we want to make all HTML elements inherit from this.  Maybe we can
> create one and stuff it in the DOM element's property table.

Reusing nsImageLoadingContent sounds problematic, given it handles all the DOM events, etc... We should probably just split the relevant bits out of nsImageLoadingContent and use a different kind of abstraction in nsImageFrame. Then nsImageLoadingContent can use that.

> <img> elements already suppress the frames for any children they might have,
> but I'm not sure how that happens.  We might still need to do something to
> make that work here too, or it might work for free.

It works for free. See nsIFrame::IsLeaf.

> On balance I think the second approach is going to be easier, just because
> we don't have to worry about the fact that we have a frame for the real DOM
> element and one for the gencon element.

Agreed. :)

	Maire Reavy [:mreavy]
	Comment 52 • 6 years ago

Thanks for taking this, Emilio!

	Daniel Holbert [:dholbert]
	Comment 53 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #48)
> (An alternative approach for the webcompat user story of this bug is to ask
> Google not to use this experimental feature in their tier-1 UI...)

We tried that for Google Docs in bug 1239922, and they had a possible way forward that they were willing to do, but it wasn't a high-priority thing for them at the time. But they've made a decent amount of progress in that case -- specifically:
- Per bug 1239922 comment 9, the best alternate strategy (to avoid depending on this experimental feature & to be sure the images actually showed up when a11y features were enabled) was to convert these icons to SVG.
- And by inspection, it seems that they've largely done that! They now have a SVG sprite-sheet referenced in the CSS alongside their PNG one, and they use the SVG sprite-sheet for the google-docs main toolbar's icons, as noted in bug 1239922 comment 17.
- And there are a few places where they're still using the PNG sprite sheet, but that might be accidental -- I can add a single CSS class to *make* them use the SVG sprite sheet in those places, and that makes this Just Work as far as I can tell, per bug 1239922 comment 17.

It sounds like maybe google docs wasn't the main site in question here, though? (There are references to "Google Search experience for Gecko on Android" here  -- do we have any bugs/notes covering what's broken in that context due to our lack-of-support for this experimental feature?)

	Xidorn Quan [:xidorn] UTC+11
	Comment 54 • 6 years ago

Yeah, it doesn't seem there is any webcompat issue connected to this bug.

It's probably a question for Mike. (I see he has ni?ed himself in this bug already.)

	Mike Taylor [:miketaylr]
	Comment 55 • 6 years ago

Yeah, sorry about the delay here. The original reason this got lumped in with Google Tier 1 Search bugs is the following issue (hidden in one of the dupes, bug 1372086):

https://github.com/webcompat/web-bugs/issues/7321

(Note, we only see this bug when spoofing as Chrome, which we did during QA to get the Tier 1 search experience).

However, I'm not sure I'm convinced this is a P1 quality/experience breaker. It seems kind of minor, but I have such low expectations on the mobile web these days my judgement might be skewed. ^__^ But I also don't expect Google Search to fix this in the next year or longer.

Emilio, if we do implement this, I think following the Blink/WebKit hack is best. IIRC, we had compat problems around supporting content in Presto, but I don't remember all the details -- presumably following Blink is safe here.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 57 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review250448

::: commit-message-12b22:12
(Diff revision 1)
> +images and that.
> +
> +That being said, this works. Dynamic changes are handled correctly because
> +content property changes already cause a reframe.
> +
> +The mImage change for intrinsic sizing also fixes bug 1149357, since

Actually this is not right, this change isn't the correct one for intrinsic sizing. But I'll submit that separately so please ignore that bit altogether.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 59 • 6 years ago

I uploaded the resulting patch after both dependent bugs. Here's a green try run as well.

There was a very basic test for this in WPT which is now passing, but I should probably spend some more time testing the edge cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d676541f38e7f84db8b9f45fb24df088b60bb4e6

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 60 • 6 years ago

There's also the interesting case of a broken image which would return false from nsImageFrame::ShouldCreateImageFrameFor (and thus display the alt text), but it has content: url(..).

I think in that case Blink / WebKit would still show the content: url URI. But we wouldn't (we'd just treat it as a broken image) since the logic in nsImageFrame is something like: If this is an nsIImageLoadingContent, then fine, otherwise use the content: url() request. So perhaps we should be more explicit about what does nsImageFrame honor and what doesn't, maybe passing a parameter down or what not.

It probably doesn't matter much but worth a test and a spec issue I guess...

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 61 • 6 years ago

Here's a test-case for that.

	David Baron :dbaron:
	Comment 62 • 6 years ago

Is it worth thinking about the fallback mechanism that was in the spec until recently (i.e., still in the most recent Working Draft on TR... which means I'm not sure if the working group has accepted the removal or not)?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 63 • 6 years ago

(In reply to David Baron :dbaron: ⌚UTC-7 from comment #62)
> Is it worth thinking about the fallback mechanism that was in the spec until
> recently (i.e., still in the most recent Working Draft on TR... which means
> I'm not sure if the working group has accepted the removal or not)?

Perhaps, though it's not clear to me how to go about implementing that right now, it probably would add significant complexity to the patch. We have no way to tell whether the image is known broken or not at that point, and we'd have to persist that state somewhere, which sounds annoying.

	Daniel Holbert [:dholbert]
	Comment 64 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review251158

My feedback from a quick skim: this seems generally sane (though I don't know the intricacies of the various nsImageFrame special cases & fallback paths being tweaked here)

::: commit-message-12b22:1
(Diff revision 2)
> +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert

Extreme nit: I noticed you use "url(..)" with 2 dots as a placeholder throughout this patch.  I'm not used to that "two-dot" ellipsis -- is that a standard spelling?  If not, you probably want "url(...)" to be more canonical, I imagine.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 65 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review251158

> Extreme nit: I noticed you use "url(..)" with 2 dots as a placeholder throughout this patch.  I'm not used to that "two-dot" ellipsis -- is that a standard spelling?  If not, you probably want "url(...)" to be more canonical, I imagine.

It's the standard "anything" value in Rust, so I guess I'm more used to that... Can tweak I guess :)

	Timothy Nikkel (:tnikkel)
	Comment 66 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

::: layout/generic/nsImageFrame.cpp:202
(Diff revision 2)
>      PresShell()->CancelReflowCallback(this);
>      mReflowCallbackPosted = false;
>    }
>  
> +  if (mContentURLRequest) {
> +    mContentURLRequest->CancelAndForgetObserver(NS_BINDING_ABORTED);

Can we just do Cancel here? The observer can stick around after the frame goes away.

::: layout/generic/nsImageFrame.cpp:277
(Diff revision 2)
> -
> -  // We have a PresContext now, so we need to notify the image content node that
> +    // We have a PresContext now, so we need to notify the image content node
> +    // that it can register images.
> -  // it can register images.
> -  imageLoader->FrameCreated(this);
> +    imageLoader->FrameCreated(this);
> +  } else if (auto* proxy = StyleContent()->ContentAt(0).GetImage()) {
> +    proxy->SyncClone(mListener,

Why SyncClone? This seems like asking for danger. We are trying to get rid of SyncClone.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 67 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Can we just do Cancel here? The observer can stick around after the frame goes away.

Presumably, yes. Just curious, why would that be preferrable? The observer would be quite useless without anything to notify on.

> Why SyncClone? This seems like asking for danger. We are trying to get rid of SyncClone.

I grabbed this from the imageboxframe / bulletframe / ImageLoader / etc. code:

  https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/xul/nsImageBoxFrame.cpp#277
  https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/style/ImageLoader.cpp#368
  
I thought it was the right thing to do, maybe it's not? I can try to change it to an async clone, but my hintch is that we really want to know the intrinsic size and such immediately and such if the image is already loaded...

	Timothy Nikkel (:tnikkel)
	Comment 68 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255730

::: commit-message-12b22:5
(Diff revision 2)
> +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert
> +
> +Take the review request as a feedback request for now.
> +
> +Pretty sure there's a bunch of image tracking stuff that could (should?) be done

So for image tracking I believe these images will get added to the document image tracker here

https://dxr.mozilla.org/mozilla-central/rev/752465b44c793318cef36df46ca5ff00c3d8854a/layout/style/nsStyleStruct.cpp#2208

like all other CSS images, is that correct? This means that they will remain "locked" whenever their tab is the active tab. This differs from all other nsImageFrames which can be discarded when they are not close to being scrolled in the visible viewport. Which is acceptable for now.

And ImageLoader should handle getting animated images registered with the refresh driver with its nsLayoutUtils::De/RegisterImageRequest calls.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 69 • 6 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #68)
> Comment on attachment 8976241 [details]
> Bug 215083: Implement content: url(..) for elements.
> 
> https://reviewboard.mozilla.org/r/244430/#review255730
> 
> ::: commit-message-12b22:5
> (Diff revision 2)
> > +Bug 215083: Implement content: url(..) for elements. r?tnikkel,dholbert
> > +
> > +Take the review request as a feedback request for now.
> > +
> > +Pretty sure there's a bunch of image tracking stuff that could (should?) be done
> 
> So for image tracking I believe these images will get added to the document
> image tracker here
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 752465b44c793318cef36df46ca5ff00c3d8854a/layout/style/nsStyleStruct.cpp#2208
> 
> like all other CSS images, is that correct? This means that they will remain
> "locked" whenever their tab is the active tab. This differs from all other
> nsImageFrames which can be discarded when they are not close to being
> scrolled in the visible viewport. Which is acceptable for now.
> 
> And ImageLoader should handle getting animated images registered with the
> refresh driver with its nsLayoutUtils::De/RegisterImageRequest calls.

Yeah, that makes sense. I was looking at all the image box frame and such which uses list-style-image, which track stuff manually.

But I forgot that that is pretty much the special-case, and we pass Mode::Track here:

  https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/layout/style/ServoBindings.cpp#1615

So that should be fine indeed.

	Timothy Nikkel (:tnikkel)
	Comment 70 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review259970

	Timothy Nikkel (:tnikkel)
	Comment 71 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Presumably, yes. Just curious, why would that be preferrable? The observer would be quite useless without anything to notify on.

What I meant about the observer is that it still lives so it's okay if (useless) notifications get sent to it. CancelAndForgetObserver removes the request from the load group, which can fire the load event, which can do anything, which we would like to not happen. So for example if we were in the destructor of an observer we would have no other choice and would have to call CancelAndForgetObserver (but we'd like to avoid this case and call).

> I grabbed this from the imageboxframe / bulletframe / ImageLoader / etc. code:
> 
>   https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/xul/nsImageBoxFrame.cpp#277
>   https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/layout/style/ImageLoader.cpp#368
>   
> I thought it was the right thing to do, maybe it's not? I can try to change it to an async clone, but my hintch is that we really want to know the intrinsic size and such immediately and such if the image is already loaded...

SyncClone can send notifications synchronously, which means the observers run, and they can do anything, including getting us into bad situations historically (recursive notifcations for one). So we are trying to get rid of SyncClones. Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 72 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> SyncClone can send notifications synchronously, which means the observers run, and they can do anything, including getting us into bad situations historically (recursive notifcations for one). So we are trying to get rid of SyncClones. Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

Sure... In this case the observer is under control though, but I agree.

> Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.

Does it, though? As far as I can tell, we don't get an `mImage` until the `OnSizeAvailable` notification runs. Doesn't that mean that our intrinsic size and ratio would be the default until then?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 75 • 6 years ago

Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is closer to what happens for normal images: AddNativeObserver does call all the notifications via ReplayImageStatus. I think I'd prefer to keep SyncClone, but Timothy has the last word there I guess.

I added code to handle the edge cases, and filed a couple spec issues for them (WebKit / Blink handle them erratically): https://github.com/w3c/csswg-drafts/issues/2832 / https://github.com/w3c/csswg-drafts/issues/2831.

I think this is ready for review.

	Timothy Nikkel (:tnikkel)
	Comment 76 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review255398

> Sure... In this case the observer is under control though, but I agree.
> 
> > Further, getting the notifications doesn't actually get us anything as we can ask the image for its status at any time we want, and that is what Reflow does. So I don't think it actually helps us show the image any faster.
> 
> Does it, though? As far as I can tell, we don't get an `mImage` until the `OnSizeAvailable` notification runs. Doesn't that mean that our intrinsic size and ratio would be the default until then?

Good point! We could just call OnSizeAvailable or NotifyNewCurrentRequest after cloning if the image request has a size.

	Timothy Nikkel (:tnikkel)
	Comment 77 • 6 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> closer to what happens for normal images: AddNativeObserver does call all
> the notifications via ReplayImageStatus. I think I'd prefer to keep
> SyncClone, but Timothy has the last word there I guess.

Oh yeah, I guess you are right. That was added relatively recently in bug 1103157. So you can view the problem two ways: either you replay all the status whenever a new observer is added or image observers are responsible for checking the status of the image when they add themselves as an observer and acting appropriately.

I prefer the second because it lets us remove SyncClone at some point in the future when we've refactored the code. The issue about the first reflow having the correct intrinsic size and ratio should be fixed by just calling one of the functions that updates that in Init.

	Timothy Nikkel (:tnikkel)
	Comment 78 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260182

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 79 • 6 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #77)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> > Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> > closer to what happens for normal images: AddNativeObserver does call all
> > the notifications via ReplayImageStatus. I think I'd prefer to keep
> > SyncClone, but Timothy has the last word there I guess.
> 
> Oh yeah, I guess you are right. That was added relatively recently in bug
> 1103157. So you can view the problem two ways: either you replay all the
> status whenever a new observer is added or image observers are responsible
> for checking the status of the image when they add themselves as an observer
> and acting appropriately.
> 
> I prefer the second because it lets us remove SyncClone at some point in the
> future when we've refactored the code. The issue about the first reflow
> having the correct intrinsic size and ratio should be fixed by just calling
> one of the functions that updates that in Init.

Timothy, mind if I land this patch with SyncClone, then get rid of that and ReplayImageStatus in a different bug? Without SyncClone it causes some test to fail in a very funny way.

No SyncClone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe5eaab46672c4cb322e213a5378f48a18e411f
SyncClone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad332f09de54c10ab21edb30ffb7b84172c8d28e

The reason for at least the style system one is funny: We have "content: url(..)" tests in a couple tests on the property database (https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/style/test/property_database.js#3349).

Without SyncClone, we don't get the right size, so the first read for `line-height: -moz-block-height` gives the block height at the time which doesn't include the image size. The second read happens after the image has already loaded, so we get a different height.

I'm not opposed to do those changes in this bug, but I think it's worth doing it in a separate one. In any case I promise I won't forget about it, I just want to figure out the best way to share code between the current SyncClone callers.

	Daniel Holbert [:dholbert]
	Comment 80 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

::: layout/base/nsCSSFrameConstructor.cpp:5801
(Diff revision 4)
>      // Now check for XUL display types
>      if (!data) {
>        data = FindXULDisplayData(display, element, computedStyle);
>      }
>  
> +    if (!data && ShouldCreateImageFrameForContent(*computedStyle)) {
> +      static const FrameConstructionData sImgData =
> +        SIMPLE_FCDATA(NS_NewImageFrameForContentProperty);
> +      data = &sImgData;
> +    }
> +
>      // And general display types

Two things:

(1) Please add a comment above this chunk. (Right now it's implicitly under the "Now check for XUL display types" heading, which is awkward/incorrect.)

Maybe:
 // Now check for 'content: <image-url>' (which makes us
 // ignore the 'display' property).


(2) My suggested ^^ comment is not *quite* true right now, because you've got us honoring XUL 'display' values over this feature, vs. ignoring all other display values.  Is that a meaningful distinction?  Maybe we should just insert this a few lines up, so that this has a more coherent/understandable precedence? (i.e. if you insert it a few lines up, then "content" would beat *all* 'display' values, rather than only beating non-XUL display values.)  I don't imagine we intend to use this feature in XUL anyway, so it probably doesn't really matter, other than maintainability/understandability/simplicity.   So, seems better to have this fully override 'display' -- or, if there's a reason it needs to be where you've got it, maybe call that reason out in the comment?

::: layout/generic/nsImageFrame.h:183
(Diff revision 4)
> +  // The kind of frame we are.
> +  enum class Kind : uint8_t
> +  {
> +    // For an nsImageLoadingContent.
> +    ImageElement,
> +    // For css content: url(..).
> +    ContentProperty,
> +  };

Two things:
 (1) s/frame/image frame/ in the documentation.

 (2) Also: right now, the comments & typenames here implies that this is for all "content"-generated image frames.  But I don't think that's actually true -- we take a different codepath for ::before/::after { content: ... } (which we already support).  I don't think those trigger the codepath that produce this "Kind" -- does it?  (If it does, great! I'm just not sure, and I'm guessing it doesn't based on your extra check for NS_FRAME_GENERATED_CONTENT elsewhere in this patch).

Anyway -- please clarify that, and possibly even rename, if this does not cover "content" for pseudo-elements.


(Trivial demo of pseudo-element 'content' usage that Gecko already supports & which I'm wondering about RE these Kinds: https://jsfiddle.net/4vho7y2d/  )

::: layout/generic/nsImageFrame.h:196
(Diff revision 4)
> +  explicit nsImageFrame(ComputedStyle* aStyle, Kind aKind)
> +    : nsImageFrame(aStyle, kClassID, aKind)

You can drop the "explicit" keyword, since this is now a 2-arg constructor, which doesn't need "explicit".

::: layout/generic/nsImageFrame.h:202
(Diff revision 4)
>  protected:
> -  nsImageFrame(ComputedStyle* aStyle, ClassID aID);
> +  nsImageFrame(ComputedStyle* aStyle, ClassID aId)
> +    : nsImageFrame(aStyle, aId, Kind::ImageElement)
> +  { }

nit: this patch is renaming "aID" to "Id" *only* here in this one constructor in the .h file -- but it's still called aID (capital D) in the other constructor that's defined in the c++ file, and in the constructors within the nsIFrame.h.

So let's revert this renaming here, to avoid introducing inconsistency.

If you happen to feel strongly about changing the "d" to lowercase, let's take that to a separate more-thorough bug. :)

::: layout/generic/nsImageFrame.cpp:434
(Diff revision 4)
> -  nsCOMPtr<nsIImageLoadingContent> imageLoader(do_QueryInterface(mContent));
> -  NS_ASSERTION(imageLoader, "No image loading content?");
> +  if (mKind == Kind::ContentProperty) {
> +    MOZ_ASSERT(aRequest == mContentURLRequest);
> +    return false;
> +  }
>  
> +  nsCOMPtr<nsIImageLoadingContent> imageLoader(do_QueryInterface(mContent));
>    int32_t requestType = nsIImageLoadingContent::UNKNOWN_REQUEST;

Looks like you're removing an assertion (enforcing/documenting that imageLoader must be non-null).

Seems like that assertion would be useful to keep around.  We've got this assertion sprinkled throughout this file and it's a nice assumption to have documented/checked before we dereference the pointer.

(I notice you've strengthened some "if (imageLoader)" null-checks to become fatal assertions elsewhere; that seems good too.)

::: layout/generic/nsImageFrame.cpp:864
(Diff revision 4)
> +
> +  // NOTE(emilio, https://github.com/w3c/csswg-drafts/issues/2832): WebKit
> +  // and Blink behave differently here for content: url(..), for now adapt to
> +  // Blink's behavior.
> +  const bool mayDisplayBrokenIcon =
> +    mKind == Kind::ImageElement &&
> +    !(GetStateBits() & NS_FRAME_GENERATED_CONTENT);
> +
> +  if (!mayDisplayBrokenIcon) {
> +    return;
> +  }
> +

I didn't initially understand what this boolean condition was getting at.

After staring at it for a bit, I think this approximately represents "is this image from a real image element, i.e. and not generated/replaced using the content property".  Is that right?

Might be nice to document that (or whatever meaning it's trying to capture), to help people grok what we're really checking here, beyond just "this is what Blink checks". :)  (This is what got me thinking about Kind::ImageElement vs. Kind::ContentProperty and which bucket ::before/::after generated content falls into, BTW.)

::: layout/generic/nsImageFrame.cpp:887
(Diff revision 4)
> -        if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) {
> +  if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) {
> -          uint32_t imageStatus;
> +    uint32_t imageStatus;
> -          imageInvalid =
> +    imageInvalid =
> -            NS_SUCCEEDED(currentRequest->GetImageStatus(&imageStatus)) &&
> +      NS_SUCCEEDED(currentRequest->GetImageStatus(&imageStatus)) &&
> -            (imageStatus & imgIRequest::STATUS_ERROR);
> +      (imageStatus & imgIRequest::STATUS_ERROR);
> -        } else if (nsCOMPtr<nsIImageLoadingContent> loader = do_QueryInterface(mContent)) {
> +  } else if (mKind == Kind::ImageElement) {

Note: this change (and this check) seems unnecessary.  mKind *has to be* ImageElement here, I think.

If mKind were anything other than ImageElement, then we'd have set mayDisplayBrokenIcon = false up above, and we'd have already returned up there as a result.

::: layout/generic/nsImageFrame.cpp
(Diff revision 4)
> -  MOZ_ASSERT(imageLoader);
>  

As above, I'm not sure it makes sense to remove MOZ_ASSERT(imageLoader) assertions like this one; they're useful to document expectations/invariants that aren't otherwise obvious.

e.g. here, the do_QueryInterface() is technically allowed to return null -- and we're implicitly assuming that it does not do so, when we dereference it.  And the MOZ_ASSERT is documenting/enforcing that implicit assumption, & making it more explicit. (In an ideal world, it'd also have a 1-liner explanation of *why* we can make that assumption, but it's still useful even without that IMO.)

So: maybe revert this assertion-removal?

	Daniel Holbert [:dholbert]
	Comment 81 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260728

(I guess this is r- from me in its present state, but likely r+ with feedback addressed.)

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 82 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

> Two things:
> 
> (1) Please add a comment above this chunk. (Right now it's implicitly under the "Now check for XUL display types" heading, which is awkward/incorrect.)
> 
> Maybe:
>  // Now check for 'content: <image-url>' (which makes us
>  // ignore the 'display' property).
> 
> 
> (2) My suggested ^^ comment is not *quite* true right now, because you've got us honoring XUL 'display' values over this feature, vs. ignoring all other display values.  Is that a meaningful distinction?  Maybe we should just insert this a few lines up, so that this has a more coherent/understandable precedence? (i.e. if you insert it a few lines up, then "content" would beat *all* 'display' values, rather than only beating non-XUL display values.)  I don't imagine we intend to use this feature in XUL anyway, so it probably doesn't really matter, other than maintainability/understandability/simplicity.   So, seems better to have this fully override 'display' -- or, if there's a reason it needs to be where you've got it, maybe call that reason out in the comment?

Guess it's not a big deal overriding XUL display types, but generally I wanted to deal as little as possible with XUL :-).

Will do. Also will mention that we do honor display: none and display: contents, just like Blink or WebKit.

> Two things:
>  (1) s/frame/image frame/ in the documentation.
> 
>  (2) Also: right now, the comments & typenames here implies that this is for all "content"-generated image frames.  But I don't think that's actually true -- we take a different codepath for ::before/::after { content: ... } (which we already support).  I don't think those trigger the codepath that produce this "Kind" -- does it?  (If it does, great! I'm just not sure, and I'm guessing it doesn't based on your extra check for NS_FRAME_GENERATED_CONTENT elsewhere in this patch).
> 
> Anyway -- please clarify that, and possibly even rename, if this does not cover "content" for pseudo-elements.
> 
> 
> (Trivial demo of pseudo-element 'content' usage that Gecko already supports & which I'm wondering about RE these Kinds: https://jsfiddle.net/4vho7y2d/  )

Good point. Any suggestions for a new name? I've renamed to NonGeneratedContentProperty, but not in love with it.

> Looks like you're removing an assertion (enforcing/documenting that imageLoader must be non-null).
> 
> Seems like that assertion would be useful to keep around.  We've got this assertion sprinkled throughout this file and it's a nice assumption to have documented/checked before we dereference the pointer.
> 
> (I notice you've strengthened some "if (imageLoader)" null-checks to become fatal assertions elsewhere; that seems good too.)

Yeah, to be honest I'm not sure how useful these are given we assert on init, I'm ambivalent.

I'll keep them for now, but we may want to do something like adding an asserting function instead, for example.

> Note: this change (and this check) seems unnecessary.  mKind *has to be* ImageElement here, I think.
> 
> If mKind were anything other than ImageElement, then we'd have set mayDisplayBrokenIcon = false up above, and we'd have already returned up there as a result.

Yeah good point, I changed my mind about whose behavior adapt to, thus this code remained :-)

I'll remove.

> As above, I'm not sure it makes sense to remove MOZ_ASSERT(imageLoader) assertions like this one; they're useful to document expectations/invariants that aren't otherwise obvious.
> 
> e.g. here, the do_QueryInterface() is technically allowed to return null -- and we're implicitly assuming that it does not do so, when we dereference it.  And the MOZ_ASSERT is documenting/enforcing that implicit assumption, & making it more explicit. (In an ideal world, it'd also have a 1-liner explanation of *why* we can make that assumption, but it's still useful even without that IMO.)
> 
> So: maybe revert this assertion-removal?

Yeah, fair enough.

	Daniel Holbert [:dholbert]
	Comment 83 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260706

> Good point. Any suggestions for a new name? I've renamed to NonGeneratedContentProperty, but not in love with it.

I don't have any better suggestions; "NonGeneratedContentProperty" seems not-too-terrible, as long as it's got clear documentation for what it means.

	Daniel Holbert [:dholbert]
	Comment 85 • 6 years ago
mozreview-review

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260760

::: layout/generic/nsImageFrame.h:367
(Diff revision 5)
> +  // An image request created for content: url(..).
> +  RefPtr<imgRequestProxy> mContentURLRequest;

(Possibly followup material):  I wonder if this should be stored in the frame's property table, rather than a normal member-var, to avoid bloating "normal" nsImageFrame instances?

(In probably >99% of nsImageFrame instances, this pointer will be unused...  And on pages with lots of images, and extra unused pointer per nsImageFrame is kinda worth worrying about.  So this seems like a good candidate for putting in the property table & having marginally slower access time as a tradeoff for all other images being more compact.)

	Daniel Holbert [:dholbert]
	Comment 86 • 6 years ago
mozreview-review-reply

Comment on attachment 8976241 [details]
Bug 215083: Implement content: url(..) for elements.

https://reviewboard.mozilla.org/r/244430/#review260760

> (Possibly followup material):  I wonder if this should be stored in the frame's property table, rather than a normal member-var, to avoid bloating "normal" nsImageFrame instances?
> 
> (In probably >99% of nsImageFrame instances, this pointer will be unused...  And on pages with lots of images, and extra unused pointer per nsImageFrame is kinda worth worrying about.  So this seems like a good candidate for putting in the property table & having marginally slower access time as a tradeoff for all other images being more compact.)

(If you end up addressing this here, it'd probably be worth doing one more round of review on the changes, to sanity-check the refcounting & property-table usage.)

	Timothy Nikkel (:tnikkel)
	Comment 87 • 6 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #79)
> (In reply to Timothy Nikkel (:tnikkel) from comment #77)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #75)
> > > Moved to Clone / Cancel instead of SyncClone. Though I think SyncClone is
> > > closer to what happens for normal images: AddNativeObserver does call all
> > > the notifications via ReplayImageStatus. I think I'd prefer to keep
> > > SyncClone, but Timothy has the last word there I guess.
> > 
> > Oh yeah, I guess you are right. That was added relatively recently in bug
> > 1103157. So you can view the problem two ways: either you replay all the
> > status whenever a new observer is added or image observers are responsible
> > for checking the status of the image when they add themselves as an observer
> > and acting appropriately.
> > 
> > I prefer the second because it lets us remove SyncClone at some point in the
> > future when we've refactored the code. The issue about the first reflow
> > having the correct intrinsic size and ratio should be fixed by just calling
> > one of the functions that updates that in Init.
> 
> Timothy, mind if I land this patch with SyncClone, then get rid of that and
> ReplayImageStatus in a different bug? Without SyncClone it causes some test
> to fail in a very funny way.

I certainly did not (and do not) intend to make removing ReplayImageStatus a blocker for this bug. Can you just call NotifyNewCurrentRequest or OnSizeAvailable directly after calling Clone? That should fix the problem no?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 88 • 6 years ago

This seems to work locally, how does it look?

	Timothy Nikkel (:tnikkel)
	Comment 89 • 6 years ago

Comment on attachment 8988922 [details] [diff] [review]
Remove sync clone.

Looks good. Thanks.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 90 • 6 years ago

(In reply to Daniel Holbert [:dholbert] from comment #85)
> (Possibly followup material):  I wonder if this should be stored in the
> frame's property table, rather than a normal member-var, to avoid bloating
> "normal" nsImageFrame instances?
> 
> (In probably >99% of nsImageFrame instances, this pointer will be unused... 
> And on pages with lots of images, and extra unused pointer per nsImageFrame
> is kinda worth worrying about.  So this seems like a good candidate for
> putting in the property table & having marginally slower access time as a
> tradeoff for all other images being more compact.)

I agree.

What do you think of doing something like:

struct RareData {
  RefPtr<nsImageMap> mImageMap;
  RefPtr<imgRequestProxy> mContentURLRequest;
};

Or something of the sort, and having a:

  UniquePtr<RareData> mRareData;

member on the image frame that uses to be null?

I can also add two frame properties (one for the image map and one from the content: url thing). I can also pack bits better (i.e., mKind can be a bit, and it can be packed with the other four bool members of nsImageFrame).

WDYT? Worth filing?
Though I generally try to prevent frame properties that only apply to a specific kind of frame.

Also, we cou

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 91 • 6 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #90)
> Though I generally try to prevent frame properties that only apply to a
> specific kind of frame.
> 
> Also, we cou

Err, I should really stop leaving content under my caret in bugzilla :P. Anyway, ignore this broken sentence.

	Daniel Holbert [:dholbert]
	Comment 92 • 6 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #90)
> What do you think of doing something like:
> 
> struct RareData {
>   RefPtr<nsImageMap> mImageMap;
>   RefPtr<imgRequestProxy> mContentURLRequest;
> };
> 
> Or something of the sort, and having a:
> 
>   UniquePtr<RareData> mRareData;
> 
> member on the image frame that uses to be null?

> I can also add two frame properties (one for the image map and one from the
> content: url thing).

I think frame properties are marginally better, since frame properties are already supposed to effectively be a generic "raredata" pointer, and it's better not do create a different technique to do the same thing unless we really need to. 

> I can also pack bits better (i.e., mKind can be a bit,
> and it can be packed with the other four bool members of nsImageFrame).

This might be worth it (not sure) - I would hope that the uint8_t would already pack nicely with the bools, but I don't know for sure. Anyway, I'm less worried about the incremental uint8_t than I am about the 64-bit pointer (though it's possible they are equally bad if the packing is bad).

> WDYT? Worth filing?

Likely, yeah. Thanks!

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 93 • 6 years ago

Just realized that I missed this nsImageFrame construction path and that needs fixing.

	Cameron McCormack (:heycam)
	Comment 94 • 6 years ago

Comment on attachment 8988953 [details] [diff] [review]
CreateContinuingFrame fix.

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

Seems fine.

	Pulsebot
	Comment 95 • 6 years ago

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca43c308ce1
Implement content: url(..) for elements. r=tnikkel,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/72416dd3422a
Remove sync image request clone. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4d140d2826
Fix CreateContinuingFrame to account for non-nsImageLoadingContent image frames. r=heycam

	Pulsebot
	Comment 96 • 6 years ago

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3983769db6d
Account for subclasses in the next-in-flow assertion. r=me

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 97 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11744 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 98 • 6 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

	Andreea Pavel [:apavel]
	Comment 99 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2ca43c308ce1
https://hg.mozilla.org/mozilla-central/rev/72416dd3422a
https://hg.mozilla.org/mozilla-central/rev/cd4d140d2826

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 100 • 6 years ago

Upstream PR merged

	Pulsebot
	Comment 101 • 6 years ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/aea3f3457f15
Account for subclasses in the next-in-flow assertion. r=me a=fix-uplift

	Andreea Pavel [:apavel]
	Comment 102 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/aea3f3457f15

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 103 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c3983769db6d

	Chris Peterson [:cpeterson]
	Comment 104 • 6 years ago

(In reply to Maire Reavy [:mreavy] Plz needinfo? from comment #46)
> Cameron -- As I mentioned in the Layout standup meeting last week, this is a
> P1 webcompat bug that I really want to see solved.

Emilio, is this fix something we would uplift to Beta 62? It's probably not a high priority uplift because the tier1 Google search page for Android is still blocked by other unfixed compat issues in Beta 62.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 105 • 6 years ago

Yeah, probably not worth uplifting. Unless Maire thinks otherwise?

	j.j.
	Comment 106 • 6 years ago

Please set "Target" field to avoid confusion

	Peter Simonyi
	Comment 107 • 6 years ago

This might have caused a regression; see bug 1473651.

	Chris Peterson [:cpeterson]
	Comment 108 • 6 years ago

I am assuming for now that we won't uplift this fix to Beta 62 because it introduced possible regressions (like bug 1473651) and this fix alone is not enough to satisfy Google's requirements for tier 1 search experience on Fennec.

	Boris Zbarsky [:bzbarsky]
	Comment 109 • 6 years ago

Is there a new bug tracking implementing "content: attr(foo)" for elements?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 110 • 6 years ago

Filed bug 1481150, though as I said there I'm not sure what that's supposed to compute to...