Skip to content
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

Implement blob url support in the fetch stack. #13750

Merged
merged 7 commits into from Oct 14, 2016
Merged

Implement blob url support in the fetch stack. #13750

merged 7 commits into from Oct 14, 2016

Conversation

Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 13, 2016

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/filemanager_thread.rs, components/net/fetch/methods.rs, components/net/resource_thread.rs, components/net/blob_loader.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 13, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 13, 2016

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit a3760c2 with merge 30effbe...

bors-servo pushed a commit that referenced this pull request Oct 13, 2016
Implement blob url support in the fetch stack.
@Ms2ger Ms2ger force-pushed the fetch-blob branch 3 times, most recently from 8ce01a9 to 699287e Compare October 13, 2016 15:52
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 14, 2016

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 699287e with merge ff2ad32...

bors-servo pushed a commit that referenced this pull request Oct 14, 2016
Implement blob url support in the fetch stack.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13750)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 14, 2016
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Oct 14, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 14, 2016

r? @Manishearth

@highfive highfive assigned Manishearth and unassigned emilio Oct 14, 2016
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be passing tests here, if not add some.

let _ = sender.send(Err(FileManagerThreadError::BlobURLStoreError(e)));
}
})
if let Err(e) = store.try_read_file(&sender, id, check_url_validity,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file I/O is slow too, shouldn't we spawn a thread for that? ditto for promotememory

@@ -119,3 +120,71 @@ fn load_blob<UI: 'static + UIProvider>
send_error(load_data.url.clone(), format_err, start_chan);
}
}

// TODO: make async.
pub fn load_blob_sync<UI: 'static + UIProvider>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec link

The spawned threads remain for select_file and select_files, as those may
need to wait indefinitely for the user's response.
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 14, 2016

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 6eaef7c has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 14, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 6eaef7c with merge d692cf1...

bors-servo pushed a commit that referenced this pull request Oct 14, 2016
Implement blob url support in the fetch stack.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13750)
<!-- Reviewable:end -->
@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 14, 2016
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit 6eaef7c into master Oct 14, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 14, 2016
@Ms2ger Ms2ger deleted the fetch-blob branch October 19, 2016 08:07
@Ms2ger Ms2ger mentioned this pull request Nov 7, 2016
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants