Closed Bug 1148535 Opened 9 years ago Closed 8 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jdm, Assigned: edgar, Mentored)

References

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 2 obsolete files)

There are only a couple failures left after bug 1139560 and bug 1139554.
Assignee: nobody → josh
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.
Assignee: josh → nobody
I would like to try on this bug.
Assignee: nobody → echen
I suspect all the relevant parsing code lives in dom/base/ResponsiveImageSelector.cpp.
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.
Whiteboard: [tw-dom] → [tw-dom] btpp-active
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.
Attachment #8730092 - Flags: feedback?(josh)
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.
Attachment #8730092 - Flags: feedback?(josh) → review+
(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.
Address review comment #9.
Attachment #8730092 - Attachment is obsolete: true
Attachment #8730094 - Attachment is obsolete: true
Attachment #8731112 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7067e2812c26
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.