https://github.com/ResponsiveImagesCG/picture-element/pull/179

Hixie, please make the HTML parser and XML parser set the "parser-created flag" (data-x="img-parser-created") when creating <img> elements, both parser case and normal case.

(This isn't a result of <picture>, but more a result of awaiting a stable state before starting the fetch caused a perf regression when it was implemented.)

s/parser case/fragment case/

What do you mean by "both parser case and normal case"? Fragment case?

Yes. (I corrected my typo in comment 2)

I don't think requiring this behavior is acceptable, and I'm not sure why we're doing it.  UAs should be free to delay the load if they wish.

There is a hard compat requirement that all image loads synchronously check for an already-cached version of the image and if it exist switch to it (at least observably; a UA could still do this lazily if the page requests layout information for the image, which is known to be a web compat constraint, or tries to draw it into a canvas, which would be needed to match the "sync check" model, or whatnot).  But past that, requiring UAs to do sync load starts when those are not actually observable, modulo serviceworkers, is not great.

Actually, I just talked to Anne and afaict once fetch goes async how async it is is totally up to the UA.  So I'm really not sure what this requirement about sync loading can possibly mean in practice other than a sync hit on the image cache (which is required anyway).

This will probably be affected by the proposal for specifying dependencies between resources, and for specifying loading-on-demand, too.

Boris, I think we're talking past each other a bit. See https://bugzilla.mozilla.org/show_bug.cgi?id=893113#c10

(In reply to Boris Zbarsky from comment #5)
> UAs should be free to delay the load if they wish.

Yes, and they are.
 
> There is a hard compat requirement that all image loads synchronously check
> for an already-cached version of the image and if it exist switch to it (at
> least observably; a UA could still do this lazily if the page requests
> layout information for the image, which is known to be a web compat
> constraint, or tries to draw it into a canvas, which would be needed to
> match the "sync check" model, or whatnot).

Yes, that's specced.

> But past that, requiring UAs to
> do sync load starts when those are not actually observable, modulo
> serviceworkers, is not great.

JS-exposed state like .complete is observable.

(In reply to Boris Zbarsky from comment #6)
> Actually, I just talked to Anne and afaict once fetch goes async how async
> it is is totally up to the UA.

Yep.

> So I'm really not sure what this requirement
> about sync loading can possibly mean in practice

It's mostly about when you're allowed to start the load at the earliest, i.e. when the spec invokes "fetch", and what to expose to JS, and avoiding double downloads.

> It's mostly about when you're allowed to start the load at the earliest, i.e.
> when the spec invokes "fetch", and what to expose to JS, and avoiding double
> downloads.

I understand that.  I'm just not sure how the changes here are accomplishing these goals.

So I actually looked at what the proposed spec says now (assuming that copy-pasting the stuff out of https://raw.githubusercontent.com/ResponsiveImagesCG/picture-element/gh-pages/source into a .html file is a sane way of looking at the spec). 

As far as I can tell, the spec as written right now has the following effects:

1)  Images created via the parser do "update the source set" immediately on being inserted into their parent by the parser.  In particular, for innerHTML sets this is before the image is a descendant of the thing innerHTML is being set on, and it's not clear what the interaction of that with people wanting to mess with web component base URIs is.  Furthermore, for the innerHTML case this is before the parent is the <picture> element in the case when the innerHTML is being set on a <picture>.

2)  The spec now requires processing the image candidates and whatnot synchronously for the parser-created case.  In particular, this has to be done before the end of the innerHTML setter, or has to be done during the tree mutation a parser causes.

I don't think #2 is an acceptable requirement from Gecko's point of view.  We want to allow extensions to hook into image loads at the source set processing level, but we obviously don't want to enter extension code when we're not in a stable state (we do that right now, in some cases, but we're moving away from ever doing it).

So for a start, do we have information about whether the Blink performance regression is actually intrinsic to awaiting a stable state or whether it's an artefact of Blink implementation decisions?

(In reply to Boris Zbarsky from comment #9)
> I understand that.  I'm just not sure how the changes here are accomplishing
> these goals.
> 
> So I actually looked at what the proposed spec says now (assuming that
> copy-pasting the stuff out of
> https://raw.githubusercontent.com/ResponsiveImagesCG/picture-element/gh-
> pages/source into a .html file is a sane way of looking at the spec). 

You could look at http://www.whatwg.org/specs/web-apps/current-work/multipage/edits.html#the-img-element instead.

> As far as I can tell, the spec as written right now has the following
> effects:
> 
> 1)  Images created via the parser do "update the source set" immediately on
> being inserted into their parent by the parser.

Yes, but only if the document is the "active document", since "update the image data" aborts early otherwise.

> In particular, for
> innerHTML sets this is before the image is a descendant of the thing
> innerHTML is being set on, and it's not clear what the interaction of that
> with people wanting to mess with web component base URIs is.  Furthermore,
> for the innerHTML case this is before the parent is the <picture> element in
> the case when the innerHTML is being set on a <picture>.

In the innerHTML case, when the string is parsed the nodes are parsed into a separate document, which is not the "active document", so nothing happens at that point. When innerHTML moves the parsed nodes into the element, "update the image data" runs again and then the "update the source set" will be invoked.

> 2)  The spec now requires processing the image candidates and whatnot
> synchronously for the parser-created case.  In particular, this has to be
> done before the end of the innerHTML setter, or has to be done during the
> tree mutation a parser causes.
> 
> I don't think #2 is an acceptable requirement from Gecko's point of view. 
> We want to allow extensions to hook into image loads at the source set
> processing level, but we obviously don't want to enter extension code when
> we're not in a stable state (we do that right now, in some cases, but we're
> moving away from ever doing it).
> 
> So for a start, do we have information about whether the Blink performance
> regression is actually intrinsic to awaiting a stable state or whether it's
> an artefact of Blink implementation decisions?

Thanks for the feedback. I'll look into this.

[[
<yoav> zcorpan: The regressed test is at https://code.google.com/p/chromium/issues/detail?id=372971#c22
<yoav> Basically setting innerHTML with an "<img src=CACHED_IMAGE>" from script a gazillion times
]]

This was a sync case before the spec change, so it seems it wasn't necessary to change the spec for this test.

(In reply to Boris Zbarsky from comment #9)
> I don't think #2 is an acceptable requirement from Gecko's point of view. 
> We want to allow extensions to hook into image loads at the source set
> processing level, but we obviously don't want to enter extension code when
> we're not in a stable state (we do that right now, in some cases, but we're
> moving away from ever doing it).

Do you speculatively fetch images, or plan to? How would you expose that to extensions? (Blink speculatively fetches images.)

> You could look at http://www.whatwg.org/specs/web-apps/current-work/multipage/edits.html#the-img-element instead.

Ah, I hadn't realized that this had been propagated back to the main spec, thanks.

> Yes, but only if the document is the "active document", since "update the image
> data" aborts early otherwise.

Thank you, that makes sense.  I'd missed that.  Alright, so that makes sense.

> Do you speculatively fetch images

Yes, from a parser pre-scan.  This is exposed to extensions as "a load of URI xyz, of type image, no associated node", so they know it's a preload and can cancel it and wait for the main load if they want to mess with image processing, say.  We might end up changing what we do internally for this situation, of course (e.g. expose more information about the preload to extensions, like the actual srcset or whatnot).  But in any case, we do preloads off a task, so in stable state.

https://code.google.com/p/chromium/issues/detail?id=372971

[[
cbiesinger
Now that I'm pretty sure I've found the real cause of the performance bug, I'm happy with undoing the change to load on doc insertion for parser-created images. That will allow simplifying our code too.

yoav
+1 to reverting the insertedInto load, especially since Mozilla have objections to adding that to the spec.
]]

https://github.com/ResponsiveImagesCG/picture-element/commit/8886797f6712722c93aff4b4f195637e8db90574