1410184 - [META] Investigate replacing the JS CSS lexer

Status: ASSIGNED

(DevTools :: Inspector, task, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In bug 1265787, we landed a JS version of Gecko's CSS lexer, so that the inspector could use it when running in a tab.

Now that the platform is moving towards Stylo, it would be good to adapt this to be based on Stylo's lexer instead.

At the same time, it would also be an interesting test case for compiling Rust to WebAssembly, which should improve speed over the pure JS version we have today.

Comment 1

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 2

[Chris Peterson [:cpeterson]]

Tagging this bug "[stylo-]" to remove it from Style bug triage queries because this is not a Stylo bug.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Slowly making some progress on this task.  The Rust WASM tooling advances quickly, so I had been waiting a bit to let it mature first.

I hit a conflict between wasm-bindgen and cssparser while exploring this, but I have sent a PR to workaround it:

https://github.com/rustwasm/wasm-bindgen/pull/165

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

After thinking about bug 1410716 a bit more, I think it is pretty likely that changing to use WASM for the lexer would regress that bug to some extent, because WASM cannot access normal JS objects, and thus we have to duplicate the CSS content into WASM's memory before invoking it.

And I think a proper solution to that bug is to construct lexer only once for each stylesheet, so that we don't repeatedly duplicate the content. That would probably be a prerequisite to this bug then.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

In some early analysis for Bug 1836873 , I came across our JS-written CSS lexer again, which I guess should be modified to account for the new syntax added for CSS Nesting.

I was wondering if that would be a good time to try to revive this specific bug instead of replicating work that was already done on the platform.
Another solution that we could consider is to allow (privileged) JS to directly call the Stylo Lexer, so we won't have to go through the WASM conversion (at the time this bug was created, we wanted to get rid of privileged code in DevTools, but this isn't the case anymore).

Emilio, what do you think about this? Does this sounds like something that would be hard to do/not worth the benefit?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

What kind of API would you need from the platform? cssparser is rather standalone so compiling to wasm shouldn't be hard in theory... But if a WebIDL/XPIDL api is simpler that's fine too. Do you just want a tokenized range of source text? Something else

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

What kind of API would you need from the platform? cssparser is rather standalone so compiling to wasm shouldn't be hard in theory... But if a WebIDL/XPIDL api is simpler that's fine too. Do you just want a tokenized range of source text? Something else

Yeah a tokenizer would be great so we can replace https://searchfox.org/mozilla-central/rev/27e4816536c891d85d63695025f2549fd7976392/devtools/shared/css/parsing-utils.js#42-111

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1410184 - [devtools] Add InspectorUtils.getCSSParser method. r=emilio.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

I have a somewhat working c++/rust implementation to iterate over the tokens so I could see the impact on getRuleText, which currently uses the lexer (won't be the case after Bug 1882964 , but this is an easy way to assert the impact):

• before https://share.firefox.dev/496zknG , ~530ms
• after https://share.firefox.dev/3vp3Zir ~140ms , so almost 4 times faster

Comment 10

[Phabricator Automation]

Comment on attachment 9387991 [details]
Bug 1410184 - [devtools] Add InspectorUtils.getCSSParser method. r=emilio.

Revision D202909 was moved to bug 1887638. Setting attachment 9387991 [details] to obsolete.