1408305 - make DOMIntersectionObserver use Servo for parsing the root margin value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

Currently is uses nsCSSParser.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: style: Add FFI function for parsing IntersectionObserver rootMargin values.

Review commit: https://reviewboard.mozilla.org/r/189676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189676/

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1408305 - Use Servo to parse IntersectionObserver rootMargin values.

Review commit: https://reviewboard.mozilla.org/r/189678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189678/

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bf8e305e1b336451adda89a4d2e2ccddfe1392

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8918819 [details]
style: Add FFI function for parsing IntersectionObserver rootMargin values.

https://reviewboard.mozilla.org/r/189676/#review195158

::: servo/ports/geckolib/glue.rs:4287
(Diff revision 1)
> +    let url_data = unsafe { dummy_url_data() };
> +    let context = ParserContext::new(
> +        Origin::Author,
> +        url_data,
> +        Some(CssRuleType::Style),
> +        PARSING_MODE_DEFAULT,
> +        QuirksMode::NoQuirks,
> +    );

I guess we can probably even have a dummy parser context so that we don't need to create it every time.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8918820 [details]
Bug 1408305 - Use Servo to parse IntersectionObserver rootMargin values.

https://reviewboard.mozilla.org/r/189678/#review195162

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8918819 [details]
style: Add FFI function for parsing IntersectionObserver rootMargin values.

https://reviewboard.mozilla.org/r/189676/#review195278

::: servo/ports/geckolib/glue.rs:4287
(Diff revision 1)
> +    let url_data = unsafe { dummy_url_data() };
> +    let context = ParserContext::new(
> +        Origin::Author,
> +        url_data,
> +        Some(CssRuleType::Style),
> +        PARSING_MODE_DEFAULT,
> +        QuirksMode::NoQuirks,
> +    );

The ParserContext creation doesn't really do any meaningful work, just initializes those fields, so I don't think we really need a re-usable dummy parser context.  Though if I write another patch to do some single value parsing I might factor it out into a helper function.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Servo PR

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8918820 [details]
Bug 1408305 - Use Servo to parse IntersectionObserver rootMargin values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189678/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3166813b276c
Use Servo to parse IntersectionObserver rootMargin values. r=xidorn

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3166813b276c

Comment 11

[Ionuț Goldan [:igoldan]]

Looks like this bug brought a steady improvement:

== Change summary for alert #10033 (as of October 17 2017 07:20 UTC) ==

Improvements:

  7%  Strings PerfIsASCIIExample3 osx-10-10 opt      8,267.29 -> 7,654.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10033

Comment 12

[Cameron McCormack (:heycam)]

Maybe another bug caused that improvement.  My patch shouldn't be related to the performance of that benchmark.