Closed Bug 870022 (picture) Opened 11 years ago Closed 10 years ago

Implement `picture` element

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mat, Assigned: johns)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [v3.3 try: https://tbpl.mozilla.org/?tree=Try&rev=03b4e71563b5])

Attachments

(15 files, 58 obsolete files)

11.76 KB, patch
jst
: review+
Details | Diff | Splinter Review
6.16 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.63 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
5.58 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
8.01 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.43 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
12.60 KB, patch
johns
: review+
Details | Diff | Splinter Review
4.78 KB, patch
johns
: review+
Details | Diff | Splinter Review
3.54 KB, patch
johns
: review+
Details | Diff | Splinter Review
15.62 KB, patch
johns
: review+
Details | Diff | Splinter Review
60.47 KB, patch
johns
: review+
Details | Diff | Splinter Review
10.65 KB, patch
johns
: review+
Details | Diff | Splinter Review
26.47 KB, patch
johns
: review+
Details | Diff | Splinter Review
25.07 KB, patch
johns
: review+
Details | Diff | Splinter Review
4.94 KB, patch
johns
: review+
Details | Diff | Splinter Review
The `picture` element is a markup pattern that allows developers to declare multiple sources for an image using `media` attributes on `source` elements, similar to the `video` element. This allows authors to specify requirements for when/if image sources are requested/displayed.

Specification published by the HTML WG:
http://www.w3.org/TR/html-picture-element/

Note that the `picture` element is able to make use of the resolution feature of the `srcset` attribute, but does not strictly require it. The two patterns are complementary, and together fulfill the full list of Use Cases and Requirements for Standardizing Responsive Images as published by the W3C:
http://www.w3.org/TR/2013/WD-respimg-usecases-20130226/
We ran an open survey, and 42% of the 264 responses (at time of posting this) said that they are making use of “art direction” with their current “responsive images” solution.

This survey is largely comprised of attendees of last week’s Mobilism conference and developers following http://twitter.com/smashingmag (702,117 followers), so I assume it represents a fairly wide cross-section of the developer community. It’s also worth noting that developers heavily favor the `picture` element (or equivalent polyfill).

https://docs.google.com/forms/d/1LIbd9wiM7M_m-rUB85WAiGT-h8QUY-PRJwjiBRiuY4Y/viewanalytics
+1 would love to see this implemented.
Let's make it happen!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → jschoenick
I'm working on the implementation for this now - I hope to have something stood up in a few weeks.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Alias: picture
These patches provide a basically working <picture> element as well as sizes attr support when applied on top of those in bug 870021. I need to finish reviewing my list-o-issues and add a pref toggle and tests before these are ready for review.

Try builds at:
https://tbpl.mozilla.org/?tree=Try&rev=041e5fbff244

Note that this implementation does not fully match the spec yet, including error-handling and various edge case behaviors that were in flux spec-wise. The plan is to land this behind a pref, then file bugs for conformance issues blocking preffing on by default.
Missing header + fatal clang warning busted a few builds, woo!

Repushed try for just failed OS X / Win32 w/fix:
https://tbpl.mozilla.org/?tree=Try&rev=4a1342079d79
Attachment #8417613 - Attachment is obsolete: true
Attachment #8417614 - Attachment is obsolete: true
Attachment #8417615 - Attachment is obsolete: true
Attachment #8417617 - Attachment is obsolete: true
Attachment #8417618 - Attachment is obsolete: true
Attachment #8417619 - Attachment is obsolete: true
Attachment #8417620 - Attachment is obsolete: true
Updated patchset that I believe is review-ready, pending try sanity-run:

[prefs off] https://tbpl.mozilla.org/?tree=Try&rev=1648cec7316b
[prefs on]  https://tbpl.mozilla.org/?tree=Try&rev=3a96e069387a
Attachment #8426642 - Attachment is obsolete: true
Attachment #8426643 - Attachment is obsolete: true
Attachment #8426644 - Attachment is obsolete: true
Attachment #8417616 - Attachment is obsolete: true
Attachment #8426645 - Attachment is obsolete: true
Attachment #8426647 - Attachment is obsolete: true
Attachment #8426649 - Attachment is obsolete: true
Attachment #8426650 - Attachment is obsolete: true
Attachment #8426651 - Attachment is obsolete: true
Attachment #8426652 - Attachment is obsolete: true
Attachment #8426655 - Attachment is obsolete: true
Try push for v3: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9
Whiteboard: [v3 try: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9]
So with a >1.0 density image, our 'default' size ('intrinsic size' in the spec) is not the same as the "real" intrinsic size of the image layout sees. ImageFrame already assumes its content peer is ImageLoadingContent, so ask ImageLoadingContent for its NaturalWidth/Height and use it in place of the real intrinsic size for ComputeSize.

r?roc because he last reviewed changes to this code, but feel free to pass this off. Should be reviewed the the caveat that I have no idea what I'm doing in layout land.
Attachment #8428131 - Flags: review?(roc)
Comment on attachment 8428131 [details] [diff] [review]
[v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame

Annnd attached to the wrong bug Looking for 87002*1*.
Attachment #8428131 - Attachment is obsolete: true
Attachment #8428131 - Flags: review?(roc)
Updated try, patchset in bug 870021 this is based on inadvertently dropped an important change in previous push:
https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d
Whiteboard: [v3 try: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9] → [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d]
Attachment #8428105 - Attachment is obsolete: true
Attachment #8428106 - Attachment is obsolete: true
Attachment #8428108 - Attachment is obsolete: true
Attachment #8428109 - Attachment is obsolete: true
Attachment #8428110 - Attachment is obsolete: true
Attachment #8428112 - Attachment is obsolete: true
Attachment #8428113 - Attachment is obsolete: true
Attachment #8428116 - Attachment is obsolete: true
Try with failures from inadvertently dropped patch being re-included all fixed and some sizes media-query-matching weirdness fixed due to two separate accidentally inverted conditions almost but not quite canceling each other out:

https://tbpl.mozilla.org/?tree=Try&rev=b79964d32edc
Whiteboard: [v3.1 try: https://tbpl.mozilla.org/?tree=Try&rev=263bdc53b84d] → [v3.2 try: https://tbpl.mozilla.org/?tree=Try&rev=b79964d32edc]
Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

This splits out the spec-compliant integer parsing from nsAttrValue. Let me know if the enum flags are overkill, we could split this into multiple functions for the different use-cases.
Attachment #8431113 - Flags: review?(jst)
Blocks: 1017878
Comment on attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

@bz flagging you because touched similar CSS parser stuff recently, but let me know if there's someone more appropriate

This adds basic parser support for the picture 'sizes' grammar:
http://picture.responsiveimages.org/#parse-sizes-attr

This is already slightly out of date with the spec and doesn't handle error paths correctly, I filed bug 1017878 blocking pref'ing any of this on to bring it in sync with the spec + tests, so this should be mostly ensuring it is the right approach and not doing anything stupid (Which it may be, I have not touched the CSS parser before)
Attachment #8431114 - Flags: review?(bzbarsky)
Comment on attachment 8431116 [details] [diff] [review]
[v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing

Adds a width type to ResponsiveImageCandidate and update the SetParamaterFromDescriptor parser, used by ResponsiveImageSelector in next piece
Attachment #8431116 - Flags: review?(jst)
Comment on attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector

Add SetSizesFromDescriptor which uses the new CSS parser grammar to get a list of lengths and their associated queries, store them, use them to compute final densities when computed-from-width types are added.

This is already slightly out of date with the spec, I opened bug 1017878 blocking prefing this on updating it, specifically error handling.
Attachment #8431117 - Flags: review?(jst)
Comment on attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

Exposes HTMLImageElement::Sizes, pref'd off in part 8
Attachment #8431118 - Flags: superreview?(jst)
Attachment #8431118 - Flags: review?(jst)
Comment on attachment 8431119 [details] [diff] [review]
[v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits)

Add sizes attr to javasrc, generated bits in next patch
Attachment #8431119 - Flags: review?(hsivonen)
Comment on attachment 8431120 [details] [diff] [review]
[v3.2] Part 4.3 - Teach parser about sizes attr (generated bits)

Results of make translate_from_snapshot w/previous patch
Attachment #8431120 - Flags: review?(hsivonen)
Comment on attachment 8431121 [details] [diff] [review]
[v3.2] Part 5.1 - Add HTMLPictureElement & atom

Adds stub HTMLPictureElement
Attachment #8431121 - Flags: superreview?(jst)
Attachment #8431121 - Flags: review?(jst)
Comment on attachment 8431122 [details] [diff] [review]
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Adds srcset and sizes to <source>
Attachment #8431122 - Flags: superreview?(jst)
Attachment #8431122 - Flags: review?(jst)
Comment on attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)

Adds picture element to parser, non-generated parts
Attachment #8431123 - Flags: review?(hsivonen)
Comment on attachment 8431124 [details] [diff] [review]
[v3.2] Part 5.4 - Teach parser about <picture> (generated bits)

Regenerated bits from make translate_from_snapshot
Attachment #8431124 - Flags: review?(hsivonen)
Comment on attachment 8431128 [details] [diff] [review]
[v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes

Get events from <source> srcset/sizes changing and maybe switch what element mResponsiveSelector is pulling from. This doesn't handle all the microtask-ordering in the spec or dynamically responding to MQ changes, which will be follow-ups blocking pref-on
Attachment #8431128 - Flags: review?(jst)
Comment on attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled

The changes to block this behind a pref in one place, but can be folded into above patches before landing to avoid causing bisect pain
Attachment #8431129 - Flags: review?(jst)
Comment on attachment 8431127 [details] [diff] [review]
[v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement

@cpearce because he reviewed this code when it was added, but feel free to pass to more appropriate reviewer.

Since <source media> is now used by picture parents as well, this exposes HTMLSourceElement::MatchesCurrentMedia and uses it from HTMLMediaElement instead of handling the contents of the attr inline.

(<source> keeps its medialist around because it will need to dynamically respond to changes in a future patch, but <video> will not make use of dynamic source switching.)
Attachment #8431127 - Flags: review?(cpearce)
Comment on attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils

@ehsan the modules page leads me to believe you own this, but let me know if that's out of date
Attachment #8431126 - Flags: review?(ehsan)
Comment on attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils

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

::: editor/libeditor/html/nsHTMLEditUtils.cpp
@@ +734,5 @@
>         GROUP_SELECT_CONTENT | GROUP_OPTIONS, GROUP_LEAF),
>    ELEM(output, true, true, GROUP_SPECIAL, GROUP_INLINE_ELEMENT),
>    ELEM(p, true, false, GROUP_BLOCK | GROUP_P, GROUP_INLINE_ELEMENT),
>    ELEM(param, false, false, GROUP_OBJECT_CONTENT, GROUP_NONE),
> +  ELEM(picture, true, true, GROUP_SPECIAL, GROUP_INLINE_ELEMENT),

So first of all, picture cannot contain itself.  Also I think you want to create a new group for the contents of this element since GROUP_INLINE_ELEMENT doesn't match what the spec says for this.  Please see the comments around line 516 in this file and also nsHTMLEditUtils::CanContain to see how this setup works.
Attachment #8431126 - Flags: review?(ehsan) → review-
Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Looks good! r=jst
Attachment #8431113 - Flags: review?(jst) → review+
Attachment #8431116 - Flags: review?(jst) → review+
Comment on attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector

- In ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport(int32_t *aWidth):

+  for (i = 0; i < numSizes; i++) {
+    if (pctx && mSizeQueries[i]->Matches(pctx, nullptr)) {

You return early above if pctx is null, so the pctx condition is redundant here.

r=jst
Attachment #8431117 - Flags: review?(jst) → review+
Comment on attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

I'll try to read this in detail tomorrow, but why do we need the separate GatherSourceSizeList function?
Comment on attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)

(We really ought to get rid of nsElementTable, etc, at some point.)
Attachment #8431123 - Flags: review?(hsivonen) → review+
(In reply to Boris Zbarsky [:bz] from comment #90)
> Comment on attachment 8431114 [details] [diff] [review]
> [v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes>
> grammar
> 
> I'll try to read this in detail tomorrow, but why do we need the separate
> GatherSourceSizeList function?

I was just following the pattern in the file where ParseSourceSizeList is the external API wrapper, it could be inlined
Comment on attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms

># HG changeset patch
># User John Schoenick <jschoenick@mozilla.com>
>
>Bug 870022 - Part 4.1 - Add sizes to HTMLImageElement & atoms
>
>diff --git a/content/html/content/src/HTMLImageElement.cpp b/content/html/content/src/HTMLImageElement.cpp
>index 39b9246..15a46a7 100644
>--- a/content/html/content/src/HTMLImageElement.cpp
>+++ b/content/html/content/src/HTMLImageElement.cpp
>@@ -82,16 +82,17 @@ NS_IMPL_ELEMENT_CLONE(HTMLImageElement)
> 
> NS_IMPL_STRING_ATTR(HTMLImageElement, Name, name)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Align, align)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Alt, alt)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Border, border)
> NS_IMPL_INT_ATTR(HTMLImageElement, Hspace, hspace)
> NS_IMPL_BOOL_ATTR(HTMLImageElement, IsMap, ismap)
> NS_IMPL_URI_ATTR(HTMLImageElement, LongDesc, longdesc)
>+NS_IMPL_STRING_ATTR(HTMLImageElement, Sizes, sizes)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Lowsrc, lowsrc)
> NS_IMPL_URI_ATTR(HTMLImageElement, Src, src)
> NS_IMPL_STRING_ATTR(HTMLImageElement, Srcset, srcset)
> NS_IMPL_STRING_ATTR(HTMLImageElement, UseMap, usemap)
> NS_IMPL_INT_ATTR(HTMLImageElement, Vspace, vspace)
> 
> bool
> HTMLImageElement::IsSrcsetEnabled()
>diff --git a/content/html/content/src/HTMLImageElement.h b/content/html/content/src/HTMLImageElement.h
>index ee46b0d..9c28e02 100644
>--- a/content/html/content/src/HTMLImageElement.h
>+++ b/content/html/content/src/HTMLImageElement.h
>@@ -154,16 +154,20 @@ public:
>   void SetAlign(const nsAString& aAlign, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::align, aAlign, aError);
>   }
>   void SetLongDesc(const nsAString& aLongDesc, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::longdesc, aLongDesc, aError);
>   }
>+  void SetSizes(const nsAString& aSizes, ErrorResult& aError)
>+  {
>+    SetHTMLAttr(nsGkAtoms::sizes, aSizes, aError);
>+  }
>   void SetBorder(const nsAString& aBorder, ErrorResult& aError)
>   {
>     SetHTMLAttr(nsGkAtoms::border, aBorder, aError);
>   }
> 
>   int32_t X();
>   int32_t Y();
>   // Uses XPCOM GetLowsrc.
>diff --git a/dom/interfaces/html/nsIDOMHTMLImageElement.idl b/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>index 65d9084..1419ed7 100644
>--- a/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>+++ b/dom/interfaces/html/nsIDOMHTMLImageElement.idl
>@@ -11,22 +11,23 @@
>  *
>  * This interface is trying to follow the DOM Level 2 HTML specification:
>  * http://www.w3.org/TR/DOM-Level-2-HTML/
>  *
>  * with changes from the work-in-progress WHATWG HTML specification:
>  * http://www.whatwg.org/specs/web-apps/current-work/
>  */
> 
>-[uuid(939f4ea1-cb8d-49d0-a4e1-23bce758f4af)]
>+[uuid(e83e726a-0aef-4292-938b-253fec691e2f)]
> interface nsIDOMHTMLImageElement : nsISupports
> {
>            attribute DOMString        alt;
>            attribute DOMString        src;
>            attribute DOMString        srcset;
>+           attribute DOMString        sizes;
>            attribute DOMString        crossOrigin;
>            attribute DOMString        useMap;
>            attribute boolean          isMap;
>            attribute unsigned long    width;
>            attribute unsigned long    height;
>   readonly attribute unsigned long    naturalWidth;
>   readonly attribute unsigned long    naturalHeight;
>   readonly attribute boolean          complete;
>diff --git a/dom/webidl/HTMLImageElement.webidl b/dom/webidl/HTMLImageElement.webidl
>index 94c714b..9f26477 100644
>--- a/dom/webidl/HTMLImageElement.webidl
>+++ b/dom/webidl/HTMLImageElement.webidl
>@@ -55,16 +55,18 @@ partial interface HTMLImageElement {
> 
>   [TreatNullAs=EmptyString,SetterThrows] attribute DOMString border;
> };
> 
> // [Update me: not in whatwg spec yet]
> // http://picture.responsiveimages.org/#the-img-element
> [Pref="dom.image.srcset.enabled"]
> partial interface HTMLImageElement {
>+           [SetterThrows]
>+           attribute DOMString sizes;
>            readonly attribute DOMString? currentSrc;
> };
> 
> // Mozilla extensions.
> partial interface HTMLImageElement {
>            attribute DOMString lowsrc;
> 
>   // These attributes are offsets from the closest view (to mimic
Attachment #8431118 - Flags: superreview?(jst)
Attachment #8431118 - Flags: superreview+
Attachment #8431118 - Flags: review?(jst)
Attachment #8431118 - Flags: review+
Attachment #8431121 - Flags: superreview?(jst)
Attachment #8431121 - Flags: superreview+
Attachment #8431121 - Flags: review?(jst)
Attachment #8431121 - Flags: review+
Attachment #8431122 - Flags: superreview?(jst)
Attachment #8431122 - Flags: superreview+
Attachment #8431122 - Flags: review?(jst)
Attachment #8431122 - Flags: review+
> it could be inlined

Unless we think we'll have other callers of GatherSourceSizeList, I think we should inline it.
Attachment #8431128 - Flags: review?(jst) → review+
Comment on attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled

Looks good, but please re-order these changes so that we don't land pieces of this enabled before we land the pref for letting us disable it. r=jst
Attachment #8431129 - Flags: review?(jst) → review+
Attachment #8431127 - Flags: review?(cpearce) → review+
Okay, slightly more careful attempt at this. Mimiced some of the other GROUP_*_CONTENT classes and added source and img to GROUP_PICTURE_CONTENT. (Looks like <video> was never properly specified to contain <source> either :( )

Let me know if this looks proper
Attachment #8431126 - Attachment is obsolete: true
Attachment #8434452 - Flags: review?(ehsan)
(In reply to Boris Zbarsky [:bz] from comment #94)
> > it could be inlined
>
> Unless we think we'll have other callers of GatherSourceSizeList, I think we
> should inline it.

Inlined GatherSourceSizeList into ParseSourceSizeList (no other differences)
Attachment #8431114 - Attachment is obsolete: true
Attachment #8431114 - Flags: review?(bzbarsky)
Attachment #8434464 - Flags: review?(bzbarsky)
Comment on attachment 8434452 [details] [diff] [review]
[v3.3] Part 5.5 - Add <picture> to nsHTMLEditUtils

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

Looks great!
Attachment #8434452 - Flags: review?(ehsan) → review+
Addressed review comment
Attachment #8431117 - Attachment is obsolete: true
Attachment #8434499 - Flags: review+
Folded pref (part 8) in
Attachment #8431118 - Attachment is obsolete: true
Attachment #8434521 - Flags: superreview+
Attachment #8434521 - Flags: review+
Folded pref (part 8) in
Attachment #8431121 - Attachment is obsolete: true
Attachment #8434523 - Flags: superreview+
Attachment #8434523 - Flags: review+
Folded pref (Part 8) in
Attachment #8431122 - Attachment is obsolete: true
Attachment #8434525 - Flags: superreview+
Attachment #8434525 - Flags: review+
Folded in pref (Part 8)
Attachment #8431128 - Attachment is obsolete: true
Attachment #8431129 - Attachment is obsolete: true
Attachment #8434527 - Flags: review+
Test to ensure dom.image.picture.enabled properly disables everything
Attachment #8434530 - Flags: review?(bzbarsky)
Blocks: 1023519
Comment on attachment 8434464 [details] [diff] [review]
[v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

I'm sorry about the terrible lag.

>+CSSParserImpl::ParseSourceSizeList(const nsAString& aBuffer,
>+                           bool aHTMLMode);

Are there any callers that pass false?  If not, why do we need that argument?

In fact, as far as I can tell mHTMLMediaMode is write-only and we can just remove it.  Followup bug on that?

Also, document what the return value means and that aQueries and aValues are out params that will be overwritten (as opposed to appended to).

>+      hitError = true;
>+      break;

Yeah, this definitely doesn't seem to match the current spec.  The spec says to basically recover at the net toplevel comma, right?

I'm OK with a followup here if you want to, but this is really the guts of the patch, so I'm not sure how worthwhile it is to review this bit if it'll all be gutted anyway.  :(

>+      // Empty conditions (e.g. just a bare value) should be treated as always
>+      // matching.
>+      query->SetNegated();

Why does "always matching" have anything to do with SetNegated?

>+    // Since we are only trying to consume a single condition, which precludes
>+    // media types and not/only, this should be the same as reaching immediate
>+    // EOF (no condition to parse)

Again, pretty different from what the spec says now.

>+    if (!singleCondition &&
>+        eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {

Why the !singleCondition bit?

r=me as far as it goes, but we really need to fix the parsing to follow the spec.
Attachment #8434464 - Flags: review?(bzbarsky) → review+
Comment on attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

r=me
Attachment #8434530 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #105)
> Comment on attachment 8434464 [details] [diff] [review]
> [v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes>
> grammar
>
> Are there any callers that pass false?  If not, why do we need that argument?
>
> In fact, as far as I can tell mHTMLMediaMode is write-only and we can just
> remove it.  Followup bug on that?

The comment mentions that it's for "parsing media lists for HTML attributes, where we have to ignore CSS comments", but I didn't catch that it was in fact write-only. Filed bug 1029867

> Also, document what the return value means and that aQueries and aValues are
> out params that will be overwritten (as opposed to appended to).

Done

> >+      hitError = true;
> >+      break;
>
> Yeah, this definitely doesn't seem to match the current spec.  The spec says
> to basically recover at the net toplevel comma, right?
>
> I'm OK with a followup here if you want to, but this is really the guts of
> the patch, so I'm not sure how worthwhile it is to review this bit if it'll
> all be gutted anyway.  :(

The spec at the time bailed at parse error, and has gone through a few changes since. There should be no differences with well formatted strings, and people are very interested in testing this on nightly, hence the land-and-follow-up approach.

> >+      // Empty conditions (e.g. just a bare value) should be treated as always
> >+      // matching.
> >+      query->SetNegated();
>
> Why does "always matching" have anything to do with SetNegated?

A query with no expressions never matches, and thus a negated-empty-query matches anything. I updated the comment, but let me know if there's a better way to do this.

> >+    if (!singleCondition &&
> >+        eCSSToken_Symbol == mToken.mType && mToken.mSymbol == ',') {
>
> Why the !singleCondition bit?

If this is single condition mode, we want to check if the next item is "and" otherwise UngetToken() and break. We could add another | if (singleCondition) { UngetToken(); } | check inside this block, but the check it's doing is superfluous for singleCondition, so this seemed cleaner.
Attachment #8434464 - Attachment is obsolete: true
Attachment #8445536 - Flags: review+
Re-generated since things landed underneith. Carrying over r=hsivonen
Attachment #8431120 - Attachment is obsolete: true
Attachment #8445537 - Flags: review+
rebased for nsINodeInfo rename, carrying over r,sr=jst
Attachment #8434523 - Attachment is obsolete: true
Attachment #8445538 - Flags: review+
Regenerated due to other landings, carrying over r=hsivonen
Attachment #8431124 - Attachment is obsolete: true
Attachment #8445539 - Flags: review+
rebased for nsINodeInfo rename, no functional changes, carrying over r=jst
Attachment #8434527 - Attachment is obsolete: true
Attachment #8445540 - Flags: review+
s/blue.png/red.png/ because some other test in this directory is checking load counts of blue.png (which isn't strictly proper)
Attachment #8434530 - Attachment is obsolete: true
Attachment #8445542 - Flags: review+
Attachment #8445537 - Attachment description: Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen → [v3.3.1] Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen
Attachment #8445536 - Attachment description: [v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. → [v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. r=bz
> I updated the comment, but let me know if there's a better way to do this.

Seems fine, thanks.
Depends on: 1030606
Blocks: 1037643
Has this landed? I'm running FF 34 and I still can't see this working
(In reply to nick.lemouton from comment #116)
> Has this landed? I'm running FF 34 and I still can't see this working

It has landed, but it is preffed off by default.  See bug 1017875 for remaining work needed to enable it.
Depends on: 1134131
It looks like the addition of PICTURE to ElementName.java was not done by actually re-running ElementName.java's self-regeneration code and, as a result, the sort order is wrong. ಠ_ಠ I guess we're lucky if binary search has still managed to find the right item.

Anyway, if weird <picture> parsing behavior shows up in old builds, the table will get sorted correctly as a side effect of bug 1266495.
(In reply to Henri Sivonen (:hsivonen) from comment #118)
> It looks like the addition of PICTURE to ElementName.java was not done by
> actually re-running ElementName.java's self-regeneration code and, as a
> result, the sort order is wrong. ಠ_ಠ I guess we're lucky if binary search
> has still managed to find the right item.
> 
> Anyway, if weird <picture> parsing behavior shows up in old builds, the
> table will get sorted correctly as a side effect of bug 1266495.

Doesn't look conspicuous at all!
>      252135604,
> +    251841204,
>      252317348,

(this was certainly my fault, I don't remember exactly what I did, but I suspect rebase/merge fail)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.