1148535 - Fix remaining failures in parse-a-srcset-attribute.html

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

There are only a couple failures left after bug 1139560 and bug 1139554.

Comment 1

[John Schoenick [:johns]]

A few of the remaining failures are due to using IsHTMLWhitespace() where the spec calls for 'is a space character' which are different definitions in the spec.

Comment 2

[Edgar Chen [:edgar]]

I would like to try on this bug.

Comment 3

[Josh Matthews [:jdm]]

I suspect all the relevant parsing code lives in dom/base/ResponsiveImageSelector.cpp.

Comment 4

[Edgar Chen [:edgar]]

The remaining failures seems come from the floating-point parser. We use PromiseFlatString.ToDouble() to parse floating-point, but the result seem not fully align with the spec, https://html.spec.whatwg.org/multipage/infrastructure.html#valid-floating-point-number.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Check if the density descriptor in srcset consists of a valid floating-point number, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Update srcset web-platform expectations, v1

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8730092 [details] [diff] [review]
Check if the density descriptor in srcset consists of a valid floating-point number, v1

I add a parser to check the validation of floating-point number per spec, https://html.spec.whatwg.org/multipage/infrastructure.html#valid-floating-point-number.

And http://w3c-test.org/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html are all passed with this patch.

Hi :jdm, could you give me some feedback on this patch?

Thank you.

Comment 8

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0e3d41b88e9&group_state=expanded

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8730092 [details] [diff] [review]
Check if the density descriptor in srcset consists of a valid floating-point number, v1

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

Nice work! I saw the WPT test exepectation changes in the try push, so I would go ahead and land this with those two patches squashed together.

::: dom/base/ResponsiveImageSelector.cpp
@@ +46,5 @@
> +static bool
> +ParseFloat(const nsAString& aString, double& aDouble)
> +{
> +  // Check if it is a valid floating-point number first since the result of
> +  // PromiseFlatString.ToDouble() doesn't fully align with the spec,

Let's say nsString.ToDouble() instead.

@@ +47,5 @@
> +ParseFloat(const nsAString& aString, double& aDouble)
> +{
> +  // Check if it is a valid floating-point number first since the result of
> +  // PromiseFlatString.ToDouble() doesn't fully align with the spec,
> +  // https://html.spec.whatwg.org/#valid-floating-point-number

Can we be more specific here? I'm assuming that ToDouble is more lenient than the spec, but it would be nice if this comment confirmed it.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #9)
> Comment on attachment 8730092 [details] [diff] [review]
> Check if the density descriptor in srcset consists of a valid floating-point
> number, v1
> 
> Review of attachment 8730092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work! I saw the WPT test exepectation changes in the try push, so I
> would go ahead and land this with those two patches squashed together.

Okay, I will squashed them together before landing.

> 
> ::: dom/base/ResponsiveImageSelector.cpp
> @@ +46,5 @@
> > +static bool
> > +ParseFloat(const nsAString& aString, double& aDouble)
> > +{
> > +  // Check if it is a valid floating-point number first since the result of
> > +  // PromiseFlatString.ToDouble() doesn't fully align with the spec,
> 
> Let's say nsString.ToDouble() instead.

Will update the comment.

> 
> @@ +47,5 @@
> > +ParseFloat(const nsAString& aString, double& aDouble)
> > +{
> > +  // Check if it is a valid floating-point number first since the result of
> > +  // PromiseFlatString.ToDouble() doesn't fully align with the spec,
> > +  // https://html.spec.whatwg.org/#valid-floating-point-number
> 
> Can we be more specific here? I'm assuming that ToDouble is more lenient
> than the spec, but it would be nice if this comment confirmed it.

Exactly, the ToDouble is more lenient than the spec, I will update the comment.

Thank you.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Check if the density descriptor in srcset consists of a valid floating-point number, v2, r=jdm

Address review comment #9.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7067e2812c26

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7067e2812c26