Add blob loader #11536

izgzhen · 2016-06-01T05:53:08Z

Add a blob loader to implement Blob URL. The related interfaces to script thread are also declared.

Progressing in parallel with PR #11534. Related to #11131.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix part of #10539

These changes do not require tests because not integrated yet.

This change is

highfive · 2016-06-01T05:53:12Z

Heads up! This PR modifies the following files:

@KiChjang: components/net_traits/blob_url_store.rs, components/net_traits/blob_url_store.rs, components/net/lib.rs, components/net/blob_loader.rs, components/net_traits/lib.rs, components/net_traits/lib.rs

highfive · 2016-06-01T05:53:13Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

izgzhen · 2016-06-01T05:58:40Z

cc @Manishearth. BTW, I have not plug the loader into the core_resource_thread or file_manager or something else yet. It would be more convenient to let CoreResourceManager::load to attach the proposed loader and dispatch request to it, since I almost mimicked the way about_loader::factory and friends works. But I still remember the clogging possibility you mentioned and might put it under file_manager (which might require us to factor the more general functionality from resource_thread.rs out.)

highfive · 2016-06-03T00:18:17Z

New code was committed to pull request.

izgzhen · 2016-06-03T10:28:58Z

r? @Manishearth

Manishearth · 2016-06-04T09:04:26Z

components/net/blob_loader.rs

+        }
+        Some((uuid, _fragment)) => {
+            let (tx, rx) = ipc::channel().unwrap();
+            let _ = blob_url_store.send(BlobURLStoreMsg::Request(uuid, tx)).unwrap();


Should this be messaging the blob store or directly accessing it?

Direct access means that you may have to put synchronization primitives on your blob store, so perhaps this is better.

As I observe, it is more natural to let threads in net task to handle the requests (instead of directly consume it inside script), so I need to somehow message script thread to fetch the content of blob. Not sure if it makes a lot of sense

(we discussed this and fixing it will be done in a future PR)

Manishearth · 2016-06-04T09:07:07Z

components/net/blob_loader.rs

+// TODO: Check on GET
+// https://w3c.github.io/FileAPI/#requestResponseModel
+
+pub fn factory(load_data: LoadData,


I don't think this is how the factories work. This function is more like load_for_consumer in http_loader, the factory is a function that returns functions that call load_for_consumer.

Additionally, there should be a thread involved here.

Manishearth · 2016-06-04T09:10:58Z

components/net/blob_loader.rs

+               start_chan: LoadConsumer,
+               classifier: Arc<MIMEClassifier>,
+               _cancel_listener: CancellationListener,
+               blob_url_store: IpcSender<BlobURLStoreMsg>) {


This is a different blob url store than the one from #11534, yes?

If not, I'm not sure if resource_task should ever depend on data from the script task. Additionally, the data in the script task would be destroyed when the task closes, which is a problem if you want to navigate to a blob url.

If it is a new blob url store, do we need a message and event loop? Can we make do with an Arc<Mutex<HashMap<Key, Mutex<Entry>>>>> or something that is shared in the loader? (Similar to how HttpState is shared in http_loader instances)

I am not sure how long the blob uuid should be valid, I think it is natural to let it die when the task closes, since it is the same task that create it by the DOM API.

Besides, I have to manage the store in script thread due to the Origin problem

highfive · 2016-06-06T12:03:51Z

New code was committed to pull request.

izgzhen · 2016-06-06T12:06:18Z

components/net/blob_loader.rs

+pub fn load(load_data: LoadData, consumer: LoadConsumer,
+            blob_url_store: Arc<RwLock<BlobURLStore>>,
+            classifier: Arc<MIMEClassifier>) { // XXX: Move it into net process later
+


@Manishearth Regarding factory, it is totally gone here. And load is just a function that will be called with the extended FileManager's attributes.

izgzhen · 2016-06-06T12:08:21Z

components/net/filemanager_thread.rs

@@ -41,6 +48,8 @@ impl FileManager {
        FileManager {
            receiver: recv,
            idmap: HashMap::new(),
+            classifier: Arc::new(MIMEClassifier::new()),
+            blob_url_store: Arc::new(RwLock::new(BlobURLStore::new())),


I moved the stuff here (although far from the final model). If this might work, I will change the concrete store previously implemented in script thread to IpcSenders.


        Add blob loader

highfive · 2016-06-06T12:32:49Z

New code was committed to pull request.

Manishearth · 2016-06-08T11:13:06Z

components/net_traits/blob_url_store.rs

+    /// Size of content in bytes
+    pub size: u64,
+    /// Content of blob
+    pub bytes: Vec<u8>,


This will eventually become an enum between bytes, on-disk file name, and slice, right?

Yes, the Optioned filename looks awkward itself :P

Manishearth · 2016-06-08T11:13:36Z

@bors-servo r+

This looks good to me! Next up we need to add ways for script to insert blobs into the store, and make script smart as to when it stores blobs locally and when it forwards them to net (though I recommend talking with the gecko folks first)

bors-servo · 2016-06-08T11:13:38Z

📌 Commit 679cf0a has been approved by Manishearth

bors-servo · 2016-06-08T11:13:40Z

⌛ Testing commit 679cf0a with merge 3977128...


        Auto merge of #11536 - izgzhen:add-blob-loader, r=Manishearth

Add blob loader Add a blob loader to implement [Blob URL](https://w3c.github.io/FileAPI/#url). The related interfaces to script thread are also declared. Progressing in parallel with PR #11534. Related to #11131.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix part of #10539  - [x] These changes do not require tests because not integrated yet.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11536)

bors-servo · 2016-06-08T12:08:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned asajeffrey Jun 1, 2016

highfive added the S-awaiting-review label Jun 1, 2016

izgzhen force-pushed the izgzhen:add-blob-loader branch from 39b2c9d to 94181c1 Jun 3, 2016

highfive assigned Manishearth and unassigned asajeffrey Jun 3, 2016

Manishearth reviewed Jun 4, 2016
View changes

Manishearth added S-needs-code-changes S-awaiting-answer and removed S-awaiting-review labels Jun 4, 2016

izgzhen force-pushed the izgzhen:add-blob-loader branch from 94181c1 to 1b95198 Jun 6, 2016

highfive removed the S-needs-code-changes label Jun 6, 2016

highfive added the S-awaiting-review label Jun 6, 2016

izgzhen reviewed Jun 6, 2016
View changes

Add blob loader

Loading status checks…

679cf0a

izgzhen force-pushed the izgzhen:add-blob-loader branch from 1b95198 to 679cf0a Jun 6, 2016

Manishearth reviewed Jun 8, 2016
View changes

highfive removed S-awaiting-review S-awaiting-answer labels Jun 8, 2016

highfive added the S-awaiting-merge label Jun 8, 2016

bors-servo merged commit 679cf0a into servo:master Jun 8, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 8, 2016

servo / servo

Add blob loader #11536

Add blob loader #11536

izgzhen commented Jun 1, 2016 •

edited

highfive commented Jun 1, 2016

highfive commented Jun 1, 2016

izgzhen commented Jun 1, 2016

highfive commented Jun 3, 2016

izgzhen commented Jun 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

highfive commented Jun 6, 2016

This comment has been minimized.

This comment has been minimized.

highfive commented Jun 6, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 8, 2016

bors-servo commented Jun 8, 2016

bors-servo commented Jun 8, 2016

bors-servo commented Jun 8, 2016

servo / servo

Join GitHub today

Add blob loader #11536

Add blob loader #11536

Conversation

izgzhen commented Jun 1, 2016 • edited

highfive commented Jun 1, 2016

highfive commented Jun 1, 2016

izgzhen commented Jun 1, 2016

highfive commented Jun 3, 2016

izgzhen commented Jun 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth Jun 4, 2016 • edited

This comment has been minimized.

izgzhen Jun 4, 2016 • edited

highfive commented Jun 6, 2016

This comment has been minimized.

izgzhen Jun 6, 2016 • edited

This comment has been minimized.

highfive commented Jun 6, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 8, 2016

bors-servo commented Jun 8, 2016

bors-servo commented Jun 8, 2016

bors-servo commented Jun 8, 2016

izgzhen commented Jun 1, 2016 •

edited

Manishearth Jun 4, 2016 •

edited

izgzhen Jun 4, 2016 •

edited

izgzhen Jun 6, 2016 •

edited