Last Comment Bug 870022 - (picture) Implement `picture` element
(picture)
: Implement `picture` element
Status: RESOLVED FIXED
[v3.3 try: https://tbpl.mozilla.org/?...
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 46 votes (vote)
: mozilla33
Assigned To: John Schoenick [:johns]
:
: Andrew Overholt [:overholt]
Mentors:
http://picture.responsiveimages.org/
Depends on: srcset 1030606 1134131
Blocks: 1017878 picture-prefon 1023519 1037643
  Show dependency treegraph
 
Reported: 2013-05-08 11:04 PDT by Mat Marquis
Modified: 2016-04-27 12:55 PDT (History)
43 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[WIP v1] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage (10.79 KB, patch)
2014-05-05 13:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar (15.48 KB, patch)
2014-05-05 13:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing (6.12 KB, patch)
2014-05-05 13:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 3.2 - Add sizes support to ResponsiveImageSelector (11.53 KB, patch)
2014-05-05 13:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 4.1 - Add sizes to HTMLImageElement & atoms (4.15 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 4.2 - Teach parser about sizes attr (needs generate) (3.64 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 4.3 - Teach parser about sizes attr (generated bits) (60.37 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 5.1 - Add HTMLPictureElement & atom (9.74 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 5.2 - Add sizes/srcset to HTMLSourceElement (3.50 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 5.3 - Teach parser about <picture> (non-generated bits) (2.63 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 5.4 - Teach parser about <picture> (generated bits) (29.39 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement (8.01 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[WIP v1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes (26.05 KB, patch)
2014-05-05 13:23 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage (11.76 KB, patch)
2014-05-21 16:19 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar (15.66 KB, patch)
2014-05-21 16:19 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing (6.10 KB, patch)
2014-05-21 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 3.2 - Add sizes support to ResponsiveImageSelector (11.47 KB, patch)
2014-05-21 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 4.1 - Add sizes to HTMLImageElement & atoms (3.95 KB, patch)
2014-05-21 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 4.2 - Teach parser about sizes attr (non-generated bits) (3.63 KB, patch)
2014-05-21 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 4.2 - Teach parser about sizes attr (generated bits) (60.46 KB, patch)
2014-05-21 16:20 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 5.1 - Add HTMLPictureElement & atom (9.76 KB, patch)
2014-05-21 16:21 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 5.2 - Add sizes/srcset to HTMLSourceElement (3.46 KB, patch)
2014-05-21 16:21 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 5.3 - Teach parser about <picture> (non-generated bits) (5.58 KB, patch)
2014-05-21 16:21 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 5.4 - Teach parser about <picture> (generated bits) (26.43 KB, patch)
2014-05-21 16:21 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 5.5 - Add <picture> to nsHTMLEditUtils (1.33 KB, patch)
2014-05-21 16:21 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement (8.01 KB, patch)
2014-05-21 16:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes (23.99 KB, patch)
2014-05-21 16:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v2] Part 8 - Pref picture off behind dom.image.picture.enabled (16.83 KB, patch)
2014-05-21 16:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage (11.76 KB, patch)
2014-05-23 15:37 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar (15.66 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing (6.16 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 3.2 - Add sizes support to ResponsiveImageSelector (11.40 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 4.1 - Add sizes to HTMLImageElement & atoms (3.95 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 4.2 - Teach parser about sizes attr (non-generated bits) (3.63 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 4.3 - Teach parser about sizes attr (generated bits) (60.46 KB, patch)
2014-05-23 15:38 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 5.1 - Add HTMLPictureElement & atom (9.76 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 5.2 - Add sizes/srcset to HTMLSourceElement (3.46 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 5.3 - Teach parser about <picture> (non-generated bits) (5.58 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 5.4 - Teach parser about <picture> (generated bits) (26.43 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 5.5 - Add <picture> to nsHTMLEditUtils (1.33 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement (8.01 KB, patch)
2014-05-23 15:39 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes (24.10 KB, patch)
2014-05-23 15:40 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3] Part 8 - Pref picture off behind dom.image.picture.enabled (16.83 KB, patch)
2014-05-23 15:40 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame (1.92 KB, patch)
2014-05-23 16:22 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage (11.76 KB, patch)
2014-05-29 14:10 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar (15.81 KB, patch)
2014-05-29 14:13 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing (6.16 KB, patch)
2014-05-29 14:13 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector (12.41 KB, patch)
2014-05-29 14:14 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms (3.95 KB, patch)
2014-05-29 14:14 PDT, John Schoenick [:johns]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
[v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits) (3.63 KB, patch)
2014-05-29 14:14 PDT, John Schoenick [:johns]
hsivonen: review+
Details | Diff | Splinter Review
[v3.2] Part 4.3 - Teach parser about sizes attr (generated bits) (60.36 KB, patch)
2014-05-29 14:14 PDT, John Schoenick [:johns]
hsivonen: review+
Details | Diff | Splinter Review
[v3.2] Part 5.1 - Add HTMLPictureElement & atom (9.76 KB, patch)
2014-05-29 14:14 PDT, John Schoenick [:johns]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement (3.46 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits) (5.58 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
hsivonen: review+
Details | Diff | Splinter Review
[v3.2] Part 5.4 - Teach parser about <picture> (generated bits) (26.43 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
hsivonen: review+
Details | Diff | Splinter Review
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils (1.33 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
ehsan: review-
Details | Diff | Splinter Review
[v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement (8.01 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
cpearce: review+
Details | Diff | Splinter Review
[v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes (24.10 KB, patch)
2014-05-29 14:15 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled (16.83 KB, patch)
2014-05-29 14:16 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[v3.3] Part 5.5 - Add <picture> to nsHTMLEditUtils (5.43 KB, patch)
2014-06-04 14:13 PDT, John Schoenick [:johns]
ehsan: review+
Details | Diff | Splinter Review
[v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar (15.42 KB, patch)
2014-06-04 14:32 PDT, John Schoenick [:johns]
bzbarsky: review+
Details | Diff | Splinter Review
[v3.3] Part 3.2 - Add sizes support to ResponsiveImageSelector. r=jst (12.60 KB, patch)
2014-06-04 15:21 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
[v3.3] Part 4.1 - Add sizes to HTMLImageElement & atoms. r=jst, sr=jst (4.78 KB, patch)
2014-06-04 15:45 PDT, John Schoenick [:johns]
john: review+
john: superreview+
Details | Diff | Splinter Review
[v3.3] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst (10.61 KB, patch)
2014-06-04 15:46 PDT, John Schoenick [:johns]
john: review+
john: superreview+
Details | Diff | Splinter Review
[v3.3] Part 5.2 - Add sizes/srcset to HTMLSourceElement. r=jst, sr=jst (3.54 KB, patch)
2014-06-04 15:46 PDT, John Schoenick [:johns]
john: review+
john: superreview+
Details | Diff | Splinter Review
Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst (25.04 KB, patch)
2014-06-04 15:51 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
Add test for dom.image.picture.enabled (4.94 KB, patch)
2014-06-04 15:54 PDT, John Schoenick [:johns]
bzbarsky: review+
Details | Diff | Splinter Review
[v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. r=bz (15.62 KB, patch)
2014-06-24 16:11 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
[v3.3.1] Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen (60.47 KB, patch)
2014-06-24 16:12 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
[v3.3.1] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst (10.65 KB, patch)
2014-06-24 16:12 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
[v3.3.1] Part 5.4 - Teach parser about <picture> (generated bits). r=hsivonen (26.47 KB, patch)
2014-06-24 16:14 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
[v3.3.1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst (25.07 KB, patch)
2014-06-24 16:15 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
Add test for dom.image.picture.enabled. r=bz (4.94 KB, patch)
2014-06-24 16:16 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review

Description Mat Marquis 2013-05-08 11:04:05 PDT
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/
Comment 1 Marcos Caceres [:marcosc] 2013-05-13 02:04:46 PDT
A small data point: 
http://blog.yoav.ws/2013/05/How-Big-Is-Art-Direction
Comment 2 Mat Marquis 2013-05-22 09:45:56 PDT
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
Comment 3 Dave Newton 2013-05-29 07:38:05 PDT
+1 would love to see this implemented.
Comment 4 Johnny Martin 2013-05-29 07:41:18 PDT
Let's make it happen!
Comment 5 John Schoenick [:johns] 2014-02-28 11:16:43 PST
I'm working on the implementation for this now - I hope to have something stood up in a few weeks.
Comment 6 John Schoenick [:johns] 2014-05-05 13:22:42 PDT
Created attachment 8417609 [details] [diff] [review]
[WIP v1] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Comment 7 John Schoenick [:johns] 2014-05-05 13:22:47 PDT
Created attachment 8417610 [details] [diff] [review]
[WIP v1] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Comment 8 John Schoenick [:johns] 2014-05-05 13:22:52 PDT
Created attachment 8417612 [details] [diff] [review]
[WIP v1] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Comment 9 John Schoenick [:johns] 2014-05-05 13:22:57 PDT
Created attachment 8417613 [details] [diff] [review]
[WIP v1] Part 3.2 - Add sizes support to ResponsiveImageSelector
Comment 10 John Schoenick [:johns] 2014-05-05 13:23:03 PDT
Created attachment 8417614 [details] [diff] [review]
[WIP v1] Part 4.1 - Add sizes to HTMLImageElement & atoms
Comment 11 John Schoenick [:johns] 2014-05-05 13:23:08 PDT
Created attachment 8417615 [details] [diff] [review]
[WIP v1] Part 4.2 - Teach parser about sizes attr (needs generate)
Comment 12 John Schoenick [:johns] 2014-05-05 13:23:14 PDT
Created attachment 8417616 [details] [diff] [review]
[WIP v1] Part 4.3 - Teach parser about sizes attr (generated bits)
Comment 13 John Schoenick [:johns] 2014-05-05 13:23:19 PDT
Created attachment 8417617 [details] [diff] [review]
[WIP v1] Part 5.1 - Add HTMLPictureElement & atom
Comment 14 John Schoenick [:johns] 2014-05-05 13:23:25 PDT
Created attachment 8417618 [details] [diff] [review]
[WIP v1] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Comment 15 John Schoenick [:johns] 2014-05-05 13:23:31 PDT
Created attachment 8417619 [details] [diff] [review]
[WIP v1] Part 5.3 - Teach parser about <picture> (non-generated bits)
Comment 16 John Schoenick [:johns] 2014-05-05 13:23:36 PDT
Created attachment 8417620 [details] [diff] [review]
[WIP v1] Part 5.4 - Teach parser about <picture> (generated bits)
Comment 17 John Schoenick [:johns] 2014-05-05 13:23:42 PDT
Created attachment 8417621 [details] [diff] [review]
[WIP v1] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Comment 18 John Schoenick [:johns] 2014-05-05 13:23:47 PDT
Created attachment 8417622 [details] [diff] [review]
[WIP v1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Comment 19 John Schoenick [:johns] 2014-05-05 13:29:51 PDT
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.
Comment 20 John Schoenick [:johns] 2014-05-05 14:40:59 PDT
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
Comment 21 John Schoenick [:johns] 2014-05-21 16:19:49 PDT
Created attachment 8426638 [details] [diff] [review]
[v2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Comment 22 John Schoenick [:johns] 2014-05-21 16:19:59 PDT
Created attachment 8426639 [details] [diff] [review]
[v2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Comment 23 John Schoenick [:johns] 2014-05-21 16:20:09 PDT
Created attachment 8426641 [details] [diff] [review]
[v2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Comment 24 John Schoenick [:johns] 2014-05-21 16:20:19 PDT
Created attachment 8426642 [details] [diff] [review]
[v2] Part 3.2 - Add sizes support to ResponsiveImageSelector
Comment 25 John Schoenick [:johns] 2014-05-21 16:20:28 PDT
Created attachment 8426643 [details] [diff] [review]
[v2] Part 4.1 - Add sizes to HTMLImageElement & atoms
Comment 26 John Schoenick [:johns] 2014-05-21 16:20:39 PDT
Created attachment 8426644 [details] [diff] [review]
[v2] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Comment 27 John Schoenick [:johns] 2014-05-21 16:20:50 PDT
Created attachment 8426645 [details] [diff] [review]
[v2] Part 4.2 - Teach parser about sizes attr (generated bits)
Comment 28 John Schoenick [:johns] 2014-05-21 16:21:12 PDT
Created attachment 8426647 [details] [diff] [review]
[v2] Part 5.1 - Add HTMLPictureElement & atom
Comment 29 John Schoenick [:johns] 2014-05-21 16:21:22 PDT
Created attachment 8426649 [details] [diff] [review]
[v2] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Comment 30 John Schoenick [:johns] 2014-05-21 16:21:32 PDT
Created attachment 8426650 [details] [diff] [review]
[v2] Part 5.3 - Teach parser about <picture> (non-generated bits)
Comment 31 John Schoenick [:johns] 2014-05-21 16:21:41 PDT
Created attachment 8426651 [details] [diff] [review]
[v2] Part 5.4 - Teach parser about <picture> (generated bits)
Comment 32 John Schoenick [:johns] 2014-05-21 16:21:53 PDT
Created attachment 8426652 [details] [diff] [review]
[v2] Part 5.5 - Add <picture> to nsHTMLEditUtils
Comment 33 John Schoenick [:johns] 2014-05-21 16:22:03 PDT
Created attachment 8426653 [details] [diff] [review]
[v2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Comment 34 John Schoenick [:johns] 2014-05-21 16:22:13 PDT
Created attachment 8426654 [details] [diff] [review]
[v2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Comment 35 John Schoenick [:johns] 2014-05-21 16:22:20 PDT
Created attachment 8426655 [details] [diff] [review]
[v2] Part 8 - Pref picture off behind dom.image.picture.enabled
Comment 36 John Schoenick [:johns] 2014-05-21 16:27:08 PDT
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
Comment 37 John Schoenick [:johns] 2014-05-23 15:37:45 PDT
Created attachment 8428102 [details] [diff] [review]
[v3] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Comment 38 John Schoenick [:johns] 2014-05-23 15:38:00 PDT
Created attachment 8428103 [details] [diff] [review]
[v3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Comment 39 John Schoenick [:johns] 2014-05-23 15:38:10 PDT
Created attachment 8428104 [details] [diff] [review]
[v3] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Comment 40 John Schoenick [:johns] 2014-05-23 15:38:19 PDT
Created attachment 8428105 [details] [diff] [review]
[v3] Part 3.2 - Add sizes support to ResponsiveImageSelector
Comment 41 John Schoenick [:johns] 2014-05-23 15:38:30 PDT
Created attachment 8428106 [details] [diff] [review]
[v3] Part 4.1 - Add sizes to HTMLImageElement & atoms
Comment 42 John Schoenick [:johns] 2014-05-23 15:38:41 PDT
Created attachment 8428107 [details] [diff] [review]
[v3] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Comment 43 John Schoenick [:johns] 2014-05-23 15:38:53 PDT
Created attachment 8428108 [details] [diff] [review]
[v3] Part 4.3 - Teach parser about sizes attr (generated bits)
Comment 44 John Schoenick [:johns] 2014-05-23 15:39:04 PDT
Created attachment 8428109 [details] [diff] [review]
[v3] Part 5.1 - Add HTMLPictureElement & atom
Comment 45 John Schoenick [:johns] 2014-05-23 15:39:14 PDT
Created attachment 8428110 [details] [diff] [review]
[v3] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Comment 46 John Schoenick [:johns] 2014-05-23 15:39:21 PDT
Created attachment 8428111 [details] [diff] [review]
[v3] Part 5.3 - Teach parser about <picture> (non-generated bits)
Comment 47 John Schoenick [:johns] 2014-05-23 15:39:33 PDT
Created attachment 8428112 [details] [diff] [review]
[v3] Part 5.4 - Teach parser about <picture> (generated bits)
Comment 48 John Schoenick [:johns] 2014-05-23 15:39:42 PDT
Created attachment 8428113 [details] [diff] [review]
[v3] Part 5.5 - Add <picture> to nsHTMLEditUtils
Comment 49 John Schoenick [:johns] 2014-05-23 15:39:51 PDT
Created attachment 8428114 [details] [diff] [review]
[v3] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Comment 50 John Schoenick [:johns] 2014-05-23 15:40:01 PDT
Created attachment 8428115 [details] [diff] [review]
[v3] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Comment 51 John Schoenick [:johns] 2014-05-23 15:40:11 PDT
Created attachment 8428116 [details] [diff] [review]
[v3] Part 8 - Pref picture off behind dom.image.picture.enabled
Comment 52 John Schoenick [:johns] 2014-05-23 15:41:28 PDT
Try push for v3: https://tbpl.mozilla.org/?tree=Try&rev=17b503f372f9
Comment 53 John Schoenick [:johns] 2014-05-23 16:22:30 PDT
Created attachment 8428131 [details] [diff] [review]
[v3.1] Part 5.2 - Ask content to compute its intrinsic size in nsImageFrame

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.
Comment 54 John Schoenick [:johns] 2014-05-23 16:23:39 PDT
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*.
Comment 55 John Schoenick [:johns] 2014-05-23 16:34:08 PDT
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
Comment 56 John Schoenick [:johns] 2014-05-29 14:10:34 PDT
Created attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage
Comment 57 John Schoenick [:johns] 2014-05-29 14:13:02 PDT
Created attachment 8431114 [details] [diff] [review]
[v3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar
Comment 58 John Schoenick [:johns] 2014-05-29 14:13:59 PDT
Created attachment 8431116 [details] [diff] [review]
[v3.2] Part 3.1 - Add computed width to ResponsiveImageCandidate and Descriptor parsing
Comment 59 John Schoenick [:johns] 2014-05-29 14:14:23 PDT
Created attachment 8431117 [details] [diff] [review]
[v3.2] Part 3.2 - Add sizes support to ResponsiveImageSelector
Comment 60 John Schoenick [:johns] 2014-05-29 14:14:30 PDT
Created attachment 8431118 [details] [diff] [review]
[v3.2] Part 4.1 - Add sizes to HTMLImageElement & atoms
Comment 61 John Schoenick [:johns] 2014-05-29 14:14:39 PDT
Created attachment 8431119 [details] [diff] [review]
[v3.2] Part 4.2 - Teach parser about sizes attr (non-generated bits)
Comment 62 John Schoenick [:johns] 2014-05-29 14:14:46 PDT
Created attachment 8431120 [details] [diff] [review]
[v3.2] Part 4.3 - Teach parser about sizes attr (generated bits)
Comment 63 John Schoenick [:johns] 2014-05-29 14:14:55 PDT
Created attachment 8431121 [details] [diff] [review]
[v3.2] Part 5.1 - Add HTMLPictureElement & atom
Comment 64 John Schoenick [:johns] 2014-05-29 14:15:04 PDT
Created attachment 8431122 [details] [diff] [review]
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement
Comment 65 John Schoenick [:johns] 2014-05-29 14:15:16 PDT
Created attachment 8431123 [details] [diff] [review]
[v3.2] Part 5.3 - Teach parser about <picture> (non-generated bits)
Comment 66 John Schoenick [:johns] 2014-05-29 14:15:25 PDT
Created attachment 8431124 [details] [diff] [review]
[v3.2] Part 5.4 - Teach parser about <picture> (generated bits)
Comment 67 John Schoenick [:johns] 2014-05-29 14:15:33 PDT
Created attachment 8431126 [details] [diff] [review]
[v3.2] Part 5.5 - Add <picture> to nsHTMLEditUtils
Comment 68 John Schoenick [:johns] 2014-05-29 14:15:49 PDT
Created attachment 8431127 [details] [diff] [review]
[v3.2] Part 6 - Export MatchesCurrentMedia form HTMLSourceElement, replace usage in HTMLMediaElement
Comment 69 John Schoenick [:johns] 2014-05-29 14:15:56 PDT
Created attachment 8431128 [details] [diff] [review]
[v3.2] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes
Comment 70 John Schoenick [:johns] 2014-05-29 14:16:08 PDT
Created attachment 8431129 [details] [diff] [review]
[v3.2] Part 8 - Pref picture off behind dom.image.picture.enabled
Comment 71 John Schoenick [:johns] 2014-05-29 14:19:00 PDT
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
Comment 72 John Schoenick [:johns] 2014-05-29 14:30:35 PDT
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.
Comment 73 John Schoenick [:johns] 2014-05-29 15:21:05 PDT
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)
Comment 74 John Schoenick [:johns] 2014-05-29 15:40:35 PDT
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
Comment 75 John Schoenick [:johns] 2014-05-29 15:43:12 PDT
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.
Comment 76 John Schoenick [:johns] 2014-05-29 15:44:42 PDT
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
Comment 77 John Schoenick [:johns] 2014-05-29 15:45:24 PDT
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
Comment 78 John Schoenick [:johns] 2014-05-29 15:46:19 PDT
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
Comment 79 John Schoenick [:johns] 2014-05-29 15:47:11 PDT
Comment on attachment 8431121 [details] [diff] [review]
[v3.2] Part 5.1 - Add HTMLPictureElement & atom

Adds stub HTMLPictureElement
Comment 80 John Schoenick [:johns] 2014-05-29 15:48:09 PDT
Comment on attachment 8431122 [details] [diff] [review]
[v3.2] Part 5.2 - Add sizes/srcset to HTMLSourceElement

Adds srcset and sizes to <source>
Comment 81 John Schoenick [:johns] 2014-05-29 15:49:42 PDT
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
Comment 82 John Schoenick [:johns] 2014-05-29 15:50:14 PDT
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
Comment 83 John Schoenick [:johns] 2014-05-29 15:52:16 PDT
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
Comment 84 John Schoenick [:johns] 2014-05-29 15:53:12 PDT
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
Comment 85 John Schoenick [:johns] 2014-05-29 15:59:56 PDT
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.)
Comment 86 John Schoenick [:johns] 2014-05-29 16:04:57 PDT
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
Comment 87 :Ehsan Akhgari 2014-05-29 16:26:40 PDT
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.
Comment 88 Johnny Stenback (:jst, jst@mozilla.com) 2014-05-29 17:30:08 PDT
Comment on attachment 8431113 [details] [diff] [review]
[v3.2] Part 1 - Split nsAttrValue::StringToInteger out to nsContentUtils, update usage

Looks good! r=jst
Comment 89 Johnny Stenback (:jst, jst@mozilla.com) 2014-05-29 17:55:55 PDT
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
Comment 90 Boris Zbarsky [:bz] (still a bit busy) 2014-05-29 20:08:12 PDT
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 91 Henri Sivonen (:hsivonen) 2014-05-30 05:18:23 PDT
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.)
Comment 92 John Schoenick [:johns] 2014-05-30 13:44:00 PDT
(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 93 Johnny Stenback (:jst, jst@mozilla.com) 2014-05-30 14:48:51 PDT
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
Comment 94 Boris Zbarsky [:bz] (still a bit busy) 2014-05-30 16:22:11 PDT
> it could be inlined

Unless we think we'll have other callers of GatherSourceSizeList, I think we should inline it.
Comment 95 Johnny Stenback (:jst, jst@mozilla.com) 2014-05-30 16:44:22 PDT
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
Comment 96 John Schoenick [:johns] 2014-06-04 14:13:29 PDT
Created attachment 8434452 [details] [diff] [review]
[v3.3] Part 5.5 - Add <picture> to nsHTMLEditUtils

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
Comment 97 John Schoenick [:johns] 2014-06-04 14:32:23 PDT
Created attachment 8434464 [details] [diff] [review]
[v3.3] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar

(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)
Comment 98 :Ehsan Akhgari 2014-06-04 15:00:55 PDT
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!
Comment 99 John Schoenick [:johns] 2014-06-04 15:21:27 PDT
Created attachment 8434499 [details] [diff] [review]
[v3.3] Part 3.2 - Add sizes support to ResponsiveImageSelector. r=jst

Addressed review comment
Comment 100 John Schoenick [:johns] 2014-06-04 15:45:08 PDT
Created attachment 8434521 [details] [diff] [review]
[v3.3] Part 4.1 - Add sizes to HTMLImageElement & atoms. r=jst, sr=jst

Folded pref (part 8) in
Comment 101 John Schoenick [:johns] 2014-06-04 15:46:19 PDT
Created attachment 8434523 [details] [diff] [review]
[v3.3] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

Folded pref (part 8) in
Comment 102 John Schoenick [:johns] 2014-06-04 15:46:53 PDT
Created attachment 8434525 [details] [diff] [review]
[v3.3] Part 5.2 - Add sizes/srcset to HTMLSourceElement. r=jst, sr=jst

Folded pref (Part 8) in
Comment 103 John Schoenick [:johns] 2014-06-04 15:51:53 PDT
Created attachment 8434527 [details] [diff] [review]
Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst

Folded in pref (Part 8)
Comment 104 John Schoenick [:johns] 2014-06-04 15:54:28 PDT
Created attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

Test to ensure dom.image.picture.enabled properly disables everything
Comment 105 Boris Zbarsky [:bz] (still a bit busy) 2014-06-19 00:15:00 PDT
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.
Comment 106 Boris Zbarsky [:bz] (still a bit busy) 2014-06-19 00:18:00 PDT
Comment on attachment 8434530 [details] [diff] [review]
Add test for dom.image.picture.enabled

r=me
Comment 107 John Schoenick [:johns] 2014-06-24 16:11:05 PDT
Created attachment 8445536 [details] [diff] [review]
[v3.3.2] Part 2 - Add ParseSourceSizeList to CSS parser for <picture sizes> grammar. r=bz

(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.
Comment 108 John Schoenick [:johns] 2014-06-24 16:12:07 PDT
Created attachment 8445537 [details] [diff] [review]
[v3.3.1] Part 4.3 - Teach parser about sizes attr (generated bits). r=hsivonen

Re-generated since things landed underneith. Carrying over r=hsivonen
Comment 109 John Schoenick [:johns] 2014-06-24 16:12:50 PDT
Created attachment 8445538 [details] [diff] [review]
[v3.3.1] Part 5.1 - Add HTMLPictureElement & atom. r=jst, sr=jst

rebased for nsINodeInfo rename, carrying over r,sr=jst
Comment 110 John Schoenick [:johns] 2014-06-24 16:14:17 PDT
Created attachment 8445539 [details] [diff] [review]
[v3.3.1] Part 5.4 - Teach parser about <picture> (generated bits). r=hsivonen

Regenerated due to other landings, carrying over r=hsivonen
Comment 111 John Schoenick [:johns] 2014-06-24 16:15:09 PDT
Created attachment 8445540 [details] [diff] [review]
[v3.3.1] Part 7 - HTMLImageElement - Make <picture> aware, react to <source> tag and sizes attr changes. r=jst

rebased for nsINodeInfo rename, no functional changes, carrying over r=jst
Comment 112 John Schoenick [:johns] 2014-06-24 16:16:38 PDT
Created attachment 8445542 [details] [diff] [review]
Add test for dom.image.picture.enabled. r=bz

s/blue.png/red.png/ because some other test in this directory is checking load counts of blue.png (which isn't strictly proper)
Comment 113 Boris Zbarsky [:bz] (still a bit busy) 2014-06-24 19:34:10 PDT
> I updated the comment, but let me know if there's a better way to do this.

Seems fine, thanks.
Comment 116 nick.lemouton 2014-09-07 13:22:30 PDT
Has this landed? I'm running FF 34 and I still can't see this working
Comment 117 Andrew McCreight [:mccr8] 2014-09-07 13:25:41 PDT
(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.
Comment 118 Henri Sivonen (:hsivonen) 2016-04-27 03:21:11 PDT
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.
Comment 119 John Schoenick [:johns] 2016-04-27 12:55:28 PDT
(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)

Note You need to log in before you can comment on or make changes to this bug.