Currently the responsive modes of <img> take most actions synchronously, whereas some should queue a task. We also don't track responsive state at all before BindToTree for this reason

Adds a call to explicitly force image image selection to happen, optionally blowing away cached selection

Pegging :bz for this, since he's probably most familiar with <img> spec things, but let me know if you think someone else should be on the hook. This attempts to make us match the new "update the image data" algorithm for all "relevant mutations" in the spec, at least as they pertain to responsive mode -- srcset and <picture>. It doesn't give us full last/pending/next load symantics, which I'm going to work on in bug 1076583. https://www.w3.org/Bugs/Public/show_bug.cgi?id=24711 https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data https://html.spec.whatwg.org/multipage/embedded-content.html#relevant-mutations This also leaves us doing the older synchronous behavior for basic img.src, only using the new algorithm when we have srcset or <picture> in most cases, since the new "update the image data" steps 3 through 5.3 still replace some of the synchronous behavior. Using both behaviors adds a minor edge case in that setting img.src followed by img.srcset via script on an empty <img> may trigger two loads where the spec would have us only do one, but otherwise mostly works for making picture/srcset behavior sane without significant change on basic img content. I opened Bug 1076583 for the remaining non-picture work. This also rips out a good deal of logic landed with picture originally, since the old algorithm it was written against only updated the necessary state, whereas the new algorithm essentially re-runs all of the selection steps whenever any mutation event occurs. This is much simpler, and also much saner from the web author's point of view, in that mutating a <picture> will cause it to re-select its ideal image source, full stop. We also can now move on to the next <source> if the current one doesn't parse correctly, so ideally we would keep ResponsiveImageSelectors around for all sources we're aware of to avoid re-parsing previous ones to see if they are now valid when mutations occur. I'm going to open a follow-up to do that, but let me know if you think it should land with this. (These patches are rebased on the ones in bug 1076587, which are probably needed to apply cleanly)

Comment on attachment 8498610 [details] [diff] [review] Part 1 - Add ResponsiveImageSelector::SelectImage - In ResponsiveImageSelector.h + // Get the url and density for the selected best candidate. That + // these implicitly cause an image to be selected if necessary. Fix the comment, i.e. remove 'That'. r=jst

Sorry for the lag here; I need to read through the spec bits a bit. :(

Split to part 3 to not complicate reviewing, but this could be folded into part 2 -- Working on mixed content blocking for <picture> i found a few cases where our behavior was confused because we were only checking if (mResponsiveSelector) without considering a image task might be queued. This just makes the interim behavior paths more explicit behind InResponsiveMode() HaveSrcsetOrInPicture is split out because it is used in the mixed content blocking patches, which explicitly have "if the image has a srcset attribute or a parent element of <picture>" condition (Bug 1055750)

Sorry for the lag here.... I'll try to get to this tomorrow; need to sort through the spec bits.

Comment on attachment 8498617 [details] [diff] [review] Part 2 - Line up with new spec for <img srcset> and <picture> mutations >+++ b/content/html/content/src/HTMLImageElement.cpp > HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName, >+ if (nsContentUtils::IsImageSrcSetDisabled()) { >+ return NS_OK; That can't be right. With this change, if I do: var img = new Image(); img.setAttribute("foo", "bar"); alert(img.getAttribute("foo")); I won't get "bar", will I? Please fix, and add a test that would have caught this. > HTMLImageElement::UnbindFromTree(bool aDeep, bool aNullParent) Posting these events when tearing down a document seems a bit annoying. Could we skip doing the QueueImageLoadTask in that case? Followup might be ok here as long as it gets uplifted to whatever branches this lands on. >+HTMLImageElement::QueueImageLoadTask() >+ if (!LoadingEnabled() || !doc || !doc->IsActive()) { I thought the documentation on nsIDocument::IsActive is pretty clear: this does NOT match the "active document" concept in the WHATWG spec. Though neither does what it suggests using; the closest thing is nsPIDOMWindow::HasActiveDocument, but with our document instead of mDoc. We should add a better way of doing this test and use it here. Also, no need to null-check "doc". It can't ever be null. Please document that this is not used in the cases when we just have an src, so the fact that those cases need to pick a selected source synchronously is ok. > HTMLImageElement::PictureSourceSrcsetChanged(nsIContent *aSourceNode, >+ MOZ_ASSERT(isSelf || IsPreviousSibling(aSourceNode, AsContent()), Why AsContent() instead of "this"? Same for all the other places where AsContent() is used here. >+ nsIContent *currentSrc = \ Why the backslash? Shouldn't be needed. This also happens several times. >+HTMLImageElement::UpdateResponsiveSource() >+ candidateSource = thisContent; That's a confusing way to write: candidateSource = this; >+ MOZ_ASSERT(candidateSource, >+ "Did not hit ourselves before reaching end of siblings?"); I don't think this assert (an some other asserts here) is necessarily valid when XBL is involved.... (e.g. consider when "this" is an anon subtree root in the binding, so our parent is the bound element). >+++ b/content/html/content/src/HTMLPictureElement.cpp >+ if (nextSibling->Tag() == nsGkAtoms::img) { >+ HTMLImageElement *img = static_cast<HTMLImageElement*> (nextSibling.get()); how about using NS_IMPL_FROMCONTENT_HTML_WITH_TAG in the header and then here: if (HTMLImageElement* img = HTMLImageElement::FromContent(nextSibling)) { .... r+, but please do fix the above issues.

Comment on attachment 8498618 [details] [diff] [review] img src/srcset mutation tests r=me

Comment on attachment 8498619 [details] [diff] [review] <picture> mutation tests A very skimmy r+.

Comment on attachment 8501357 [details] [diff] [review] Part 3 - Be more explicit about when we're falling back to synchronous behavior r=me

Addressed review comment, carrying over r=jst

So it turns out not nulling out mResponsiveSelector in UnbindFromTree creates a cycle, and HTMLImageElement is CC'd (inherited) but not explicitly CC'd

Comment on attachment 8504991 [details] [diff] [review] Part 4 - Cycle collect HTMLImageElement/ResponsiveImageSelector Review of attachment 8504991 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/ResponsiveImageSelector.cpp @@ +15,5 @@ > #include "nsIMediaList.h" > #include "nsRuleNode.h" > #include "nsRuleData.h" > > +#include "nsCycleCollectionParticipant.h" nit: you don't really need this, as you are including it via the header of this file, IMO. @@ +23,5 @@ > > namespace mozilla { > namespace dom { > > +// Cycle collection nit: don't really need this comment ::: content/html/content/src/HTMLImageElement.cpp @@ +27,5 @@ > #include "nsIDOMWindow.h" > #include "nsFocusManager.h" > #include "mozilla/dom/HTMLFormElement.h" > #include "nsAttrValueOrString.h" > +#include "nsCycleCollectionParticipant.h" nit: you don't really need this, as you are including it via the header of this file, IMO. @@ +116,5 @@ > > NS_IMPL_ADDREF_INHERITED(HTMLImageElement, Element) > NS_IMPL_RELEASE_INHERITED(HTMLImageElement, Element) > > +// Cycle collection nit: you don't really need the comment here. ::: content/html/content/src/HTMLImageElement.h @@ +37,5 @@ > const Optional<uint32_t>& aWidth, > const Optional<uint32_t>& aHeight, > ErrorResult& aError); > > + // CC nit: you don't need this comment

From addressing the review comments for part 2 -- this bit should probably be re-reviewed by bz. Also, should we rev the IID for this class when adding a non-virtual?

Updated comment in QueueImageLoadTask to document that it is not used yet in the img.src case (from part 2 review comments)

Addressed comments as noted below, carrying over r+ but let me know if you'd like further changes (In reply to Boris Zbarsky [:bz] from comment #9) > Comment on attachment 8498617 [details] [diff] [review] > Part 2 - Line up with new spec for <img srcset> and <picture> mutations > > >+++ b/content/html/content/src/HTMLImageElement.cpp > > HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName, > > >+ if (nsContentUtils::IsImageSrcSetDisabled()) { > >+ return NS_OK; > > That can't be right. With this change, if I do: > > var img = new Image(); > img.setAttribute("foo", "bar"); > alert(img.getAttribute("foo")); > > I won't get "bar", will I? Please fix, and add a test that would have > caught this. .. if you have | dom.disable_image_src_set | set, which is unused (except maybe in seamonkey?) and being removed in Bug 773429! I moved the bailout to the right spot, but I don't think we should be adding tests for dom.disable_image_src_set if it's waiting for someone to remove it > > HTMLImageElement::UnbindFromTree(bool aDeep, bool aNullParent) > > Posting these events when tearing down a document seems a bit annoying. > Could we skip doing the QueueImageLoadTask in that case? Followup might be > ok here as long as it gets uplifted to whatever branches this lands on. I filed Bug 1082828 for this, and put it on my list for picture followups > >+HTMLImageElement::QueueImageLoadTask() > >+ if (!LoadingEnabled() || !doc || !doc->IsActive()) { > > I thought the documentation on nsIDocument::IsActive is pretty clear: > > this does NOT match the "active document" concept in the WHATWG spec. > > Though neither does what it suggests using; the closest thing is > nsPIDOMWindow::HasActiveDocument, but with our document instead of mDoc. > > We should add a better way of doing this test and use it here. I added part 1.99 for this and asked for separate review since its adding new nsIDocument fun. Changed this patch to use IsCurrentActiveDocument from that patch > Also, no need to null-check "doc". It can't ever be null. Dropped > Please document that this is not used in the cases when we just have an src, > so the fact that those cases need to pick a selected source synchronously is > ok. Part 3 adds more comments about the img.src case, updated it with a better description for QueueImageLoadTask > > HTMLImageElement::PictureSourceSrcsetChanged(nsIContent *aSourceNode, > >+ MOZ_ASSERT(isSelf || IsPreviousSibling(aSourceNode, AsContent()), > > Why AsContent() instead of "this"? > > Same for all the other places where AsContent() is used here. I had assumed, for some reason, we couldn't simply downcast to nsIContent. I'm not sure why. I removed some of the superfluousness here. > >+ nsIContent *currentSrc = \ > > Why the backslash? Shouldn't be needed. This also happens several times. Delimiting uncommon linebreak points, just a style thing for readability (but removed since its not in line with mozilla style either way) > >+HTMLImageElement::UpdateResponsiveSource() > >+ candidateSource = thisContent; > > That's a confusing way to write: > > candidateSource = this; Also fixed > >+ MOZ_ASSERT(candidateSource, > >+ "Did not hit ourselves before reaching end of siblings?"); > > I don't think this assert (an some other asserts here) is necessarily valid > when XBL is involved.... (e.g. consider when "this" is an anon subtree root > in the binding, so our parent is the bound element). Removed assert and just null out mResponsiveSelector here (like we would have if we hit <img> with still no candidate). If we run into other asserts with odd XBL usage on <picture>, it would probably be worth the bug to make sure whatever something is doing with XBL is what we actually expect... > >+++ b/content/html/content/src/HTMLPictureElement.cpp > >+ if (nextSibling->Tag() == nsGkAtoms::img) { > >+ HTMLImageElement *img = static_cast<HTMLImageElement*> > (nextSibling.get()); > > how about using NS_IMPL_FROMCONTENT_HTML_WITH_TAG in the header and then > here: > > if (HTMLImageElement* img = HTMLImageElement::FromContent(nextSibling)) { > .... That is much cleaner, done

Comment on attachment 8498617 [details] [diff] [review] Part 2 - Line up with new spec for <img srcset> and <picture> mutations [ obsolete w/ comment 18 ]

Comment on attachment 8501357 [details] [diff] [review] Part 3 - Be more explicit about when we're falling back to synchronous behavior [ obsolete w/ comment 17 ]

Addressed review comments

Comment on attachment 8505013 [details] [diff] [review] Part 1.99 - Add nsIDocument::IsCurrentActiveDocument >+ * differentiate the *current* activate document from any active s/Activate/Active/ >+ (inner->IsCurrentInnerWindow() || No, this isn't right. We could have inner->IsCurrentInnerWindow(), but inner->GetDoc() != this, in which case this is not the active document. >+ outer->GetCurrentInnerWindow()->GetDoc() == this)); On the other hand, this bit is, I think, not needed. If our inner window is not current we can't be the active doc. The case when a window is non-current but has an active document is when the document has gotten a new inner window, so the old inner still has an active doc but isn't current anymore. So I think this test should just be: inner && inner->IsCurrentInnerWindow() && inner->GetDoc() == this r=me with that

> Also, should we rev the IID for this class when adding a non-virtual? No. The vtable is no changing, so no need. > .. if you have | dom.disable_image_src_set | set Ah, fair. OK. The rest of comment 18 sounds great.

Addressed review comments

Minor fix found by try: the QueueImageLoadTask() in UnbindFromTree() was missing a IsPictureEnabled check, causing unexpected pass on some platform tests

https://hg.mozilla.org/integration/mozilla-inbound/rev/987133583ef3 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c739aa459fb https://hg.mozilla.org/integration/mozilla-inbound/rev/f43030eb22a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/73a28e6fbe84 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0376b11638 https://hg.mozilla.org/integration/mozilla-inbound/rev/f81dc8f11838 https://hg.mozilla.org/integration/mozilla-inbound/rev/56cee6c6cd5c Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8af04bb0dc51

https://hg.mozilla.org/mozilla-central/rev/987133583ef3 https://hg.mozilla.org/mozilla-central/rev/4c739aa459fb https://hg.mozilla.org/mozilla-central/rev/f43030eb22a4 https://hg.mozilla.org/mozilla-central/rev/73a28e6fbe84 https://hg.mozilla.org/mozilla-central/rev/7e0376b11638 https://hg.mozilla.org/mozilla-central/rev/f81dc8f11838 https://hg.mozilla.org/mozilla-central/rev/56cee6c6cd5c