[CSS Modules] synchronously process blob URLs [chromium/src : main]
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
This seems ok to me (if a little worrying) so I'll mark it +1. But it'd be good to wait for Kouhei to also +1 it.
Related details
- Kouhei Ueno
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
This seems ok to me (if a little worrying) so I'll mark it +1. But it'd be good to wait for Kouhei to also +1 it.
Thanks - yeah this is not great long-term, but it's basically broken right now so I appreciate the un-blocking.
FYI I initially had a different approach that used a FileReader and manually inserted the entry into the module map (instead of a sync fetch), but went with this because it's much simpler - see iteration 2: https://chromium-review.googlesource.com/c/chromium/src/+/7093679/2
I'll definitely wait for @kou...@chromium.org's input before landing, and I'm open to any better ways to accomplish this!
Related details
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55779.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 3 comments![]( "Open in Gerrit")
Kurt Catti-SchmidtThis seems ok to me (if a little worrying) so I'll mark it +1. But it'd be good to wait for Kouhei to also +1 it.
Thanks - yeah this is not great long-term, but it's basically broken right now so I appreciate the un-blocking.
FYI I initially had a different approach that used a FileReader and manually inserted the entry into the module map (instead of a sync fetch), but went with this because it's much simpler - see iteration 2: https://chromium-review.googlesource.com/c/chromium/src/+/7093679/2
I'll definitely wait for @kou...@chromium.org's input before landing, and I'm open to any better ways to accomplish this!
Thanks for cc-ing. I'd say using data/blob url for this is very high overhead, so I wouldn't recommend this design in the long run. I'm ok with landing as long as this is well documented + made exceptional only for this use-case.
if (module_request.Options().GetSync()) {Would it be possible to centralize all the change here? I don't follow why plumbing through ScriptFetchOptions are necessary.
can we peek at ModuleScriptFetchRequest and see if that's a CSS module that we are loading from blob/data url to trigger this?
// Declarative CSS Modules perform a one-time synchronous fetch forWould you add more context why we need to perform a synchronous fetch?
Related details
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Commit-Queue | +1 |
2 comments
if (module_request.Options().GetSync()) {Would it be possible to centralize all the change here? I don't follow why plumbing through ScriptFetchOptions are necessary.
can we peek at ModuleScriptFetchRequest and see if that's a CSS module that we are loading from blob/data url to trigger this?
I added the `SetSync` and `GetSync` because it's possible to create an imperative CSS module today and use a blob URL or dataURI, and I only want to change the behavior for the new declarative version.
e.g. https://codepen.io/Kurt-Catti-Schmidt/pen/vELVLoE for Blob and https://codepen.io/Kurt-Catti-Schmidt/pen/KwVGVJX for dataURI.
An alternative is to add a declarative/imperative flag, and change behaviors here based on that, but I think sync terminology makes the intent clearer, and I don't intend to add more differences between the imperative and declarative versions. Or I can change the behavior of the existing imperative version, but that doesn't feel like a safe change to make. What do you think?
// Declarative CSS Modules perform a one-time synchronous fetch forWould you add more context why we need to perform a synchronous fetch?
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
if (module_request.Options().GetSync()) {Kurt Catti-SchmidtWould it be possible to centralize all the change here? I don't follow why plumbing through ScriptFetchOptions are necessary.
can we peek at ModuleScriptFetchRequest and see if that's a CSS module that we are loading from blob/data url to trigger this?
I added the `SetSync` and `GetSync` because it's possible to create an imperative CSS module today and use a blob URL or dataURI, and I only want to change the behavior for the new declarative version.
e.g. https://codepen.io/Kurt-Catti-Schmidt/pen/vELVLoE for Blob and https://codepen.io/Kurt-Catti-Schmidt/pen/KwVGVJX for dataURI.
An alternative is to add a declarative/imperative flag, and change behaviors here based on that, but I think sync terminology makes the intent clearer, and I don't intend to add more differences between the imperative and declarative versions. Or I can change the behavior of the existing imperative version, but that doesn't feel like a safe change to make. What do you think?
Also, what do you think of this earlier version that kept a map of Blob URL's and then used a FileReader instead? https://chromium-review.googlesource.com/c/chromium/src/+/7093679/2