Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support replacing external scripts with local copies from disk #26711

Merged
merged 1 commit into from Jun 2, 2020
Merged

Support replacing external scripts with local copies from disk #26711

merged 1 commit into from Jun 2, 2020

Conversation

@skrzyp1
Copy link
Contributor

skrzyp1 commented May 29, 2020 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #26456(GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented May 29, 2020

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.
@highfive
Copy link

highfive commented May 29, 2020

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented May 29, 2020

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@skrzyp1 skrzyp1 marked this pull request as ready for review May 30, 2020
@skrzyp1
skrzyp1 reviewed May 30, 2020
View changes
components/script/dom/window.rs Outdated
//path.push(host);
//let _ = fs::remove_dir_all(&path);
match fs::create_dir_all(&path) {
Ok(_) => {
*self.unminified_js_dir.borrow_mut() =
Comment on lines 1987 to 1991

This comment has been minimized.

Sign in to view
Copy link
@skrzyp1

skrzyp1 May 30, 2020

Author Contributor

I think code that creates directory can be removed from here because directories are now also created when scripts are stored.
New directory structure makes fs::remove_dir_all(&path) clear also other windows stored requests so I think it might be good to remove it too.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jun 1, 2020

Member

I agree. Let's remove this code.
@jdm jdm changed the title Unminify store get Support replacing external scripts with local copies from disk Jun 1, 2020
@jdm jdm added the S-needs-rebase label Jun 1, 2020
@jdm jdm assigned jdm and unassigned SimonSapin Jun 1, 2020
@jdm
jdm requested changes Jun 1, 2020
View changes
Copy link
Member

jdm left a comment

This looks really good so far! Please squash all of the commits into one, then rebase that commit against master to remove any merge conflicts.
@CYBAI
CYBAI reviewed Jun 2, 2020
View changes
components/script/dom/window.rs Outdated Show resolved Hide resolved
components/script/dom/window.rs Outdated Show resolved Hide resolved
@skrzyp1 skrzyp1 force-pushed the skrzyp1:unminify-store-get branch from be42545 to 88b2bc2 Jun 2, 2020
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 2, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1591112382232.
@skrzyp1 skrzyp1 force-pushed the skrzyp1:unminify-store-get branch from 88b2bc2 to 14e81ae Jun 2, 2020
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 2, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1591116406560.
@skrzyp1 skrzyp1 force-pushed the skrzyp1:unminify-store-get branch from 14e81ae to eff44a5 Jun 2, 2020
@skrzyp1
reading unminified scripts from disk
Loading status checks…
ee69064
@skrzyp1 skrzyp1 force-pushed the skrzyp1:unminify-store-get branch from eff44a5 to ee69064 Jun 2, 2020
@jdm
Copy link
Member

jdm commented Jun 2, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2020

📌 Commit ee69064 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2020

⌛ Testing commit ee69064 with merge 07369c4...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 07369c4 to master...
@bors-servo bors-servo merged commit 07369c4 into servo:master Jun 2, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Support loading unminified JS files from disk instead of network
7 participants
@skrzyp1 @highfive @servo-wpt-sync @jdm @bors-servo @CYBAI @SimonSapin
You can’t perform that action at this time.