Chunked ReadFile from file manager #12577

izgzhen · 2016-07-24T10:57:38Z

Introduce a ReadFileProgress sender in the ReadFile msg to file manager, and implement the related I/O operations
Change tests/unit/net/test.jpeg from a 4.8K file to a 39K file to better test the chunked reading (Since one chunk is maximally 8129 Bytes).

r? @Manishearth

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

This change is

highfive · 2016-07-24T10:57:43Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/filemanager_thread.rs, components/net/blob_loader.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/script/dom/blob.rs

Ms2ger · 2016-07-24T12:13:20Z

I know Lenna is a common testing image, but it seems problematic both copyright-wise and in terms of its source and all that entails.

izgzhen · 2016-07-24T12:49:04Z

really O_o? I will use another one

izgzhen · 2016-07-24T12:55:44Z

Done, only resolution of picture is changed.

Manishearth · 2016-07-25T07:05:58Z

components/net/blob_loader.rs

        })
    }
 }

 fn load_blob(load_data: LoadData, start_chan: LoadConsumer,
             classifier: Arc<MimeClassifier>,
-             filemanager_chan: IpcSender<FileManagerThreadMsg>) {
+             filemanager_chan: IpcSender<FileManagerThreadMsg>,
+             // XXX(izgzhen): we should utilize cancel_listener


file a bug for this and note the bug number

Manishearth · 2016-07-25T07:09:18Z

components/net/filemanager_thread.rs

@@ -443,36 +447,31 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
                                       .map_err(|e| BlobURLStoreError::External(e.to_string())));

                if seeked_start == (range.start as u64) {


Why do we call the file a "handler" here? Perhaps just call it "file"?

Manishearth · 2016-07-25T07:09:44Z

components/net/filemanager_thread.rs

@@ -563,3 +562,46 @@ fn select_files_pref_enabled() -> bool {
    PREFS.get("dom.testing.htmlinputelement.select_files.enabled")
         .as_boolean().unwrap_or(false)
 }
+
+const CHUNK_SIZE: usize = 8192;


What's the chunk size for other browsers?

As I can find it is 512 bytes to several KB:

https://dxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/nsZipDataStream.cpp#164

https://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestCloneInputStream.cpp#149

https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/test/Converters.cpp#41

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#608

But I can't find the exact number used for the same purpose here. By the way, 8192 is stolen from net/file_loader.rs. I once tried to reuse the code in it but found its code too specialized and hard to refactor.

Manishearth · 2016-07-25T07:09:55Z

components/net/filemanager_thread.rs

+        }
+    }
+
+    // Send the rest chunks


s/rest/remaining

Manishearth · 2016-07-25T07:11:50Z

tests/unit/net/filemanager_thread.rs

+                            panic!("Invalid FileManager reply");
+                        }
+                        ReadFileProgress::Partial(mut bytes_in) => {
+                            bytes.append(&mut bytes_in);


We should probably count the number of chunks and ensure that number doesn't change.

Why it should not change?

Because the chunk threshhold and the file being tested aren't changing 😄

(The number here can be updated if they do change)

Okay I see. In chunked_read, read is used, which I suppose means that, the length of real chunk we read might be different depending on the system status.

Oh, because it could be less than CHUNK_SIZE. Right.

Manishearth · 2016-07-25T07:12:07Z

Minor nits, overall lgtm.

Manishearth · 2016-07-25T07:12:18Z

What's the source of the new image?

izgzhen · 2016-07-25T07:17:32Z

@Manishearth Source is Google .... and should be a website like this http://tomatoz.ru/jmir/3340-ne-budite-muzhiki.html (but I can't find the exact one sorry). Well if this is also problematic maybe I should upload a cat picture taken by myself O_o (if the cat's lawyer is okay with it)

Manishearth · 2016-07-25T07:35:00Z

Yeah, any photo you took is okay. Otherwise find something public domain on commons.wikimedia.org 😄


        Chunked ReadFile from file manager

izgzhen · 2016-07-25T17:41:19Z

I changed the picture to a photo taken by me (plus all other nits).

Manishearth · 2016-07-26T07:42:42Z

@bors-servo r+

bors-servo · 2016-07-26T07:42:43Z

📌 Commit 9c8ebd3 has been approved by Manishearth

bors-servo · 2016-07-26T07:42:46Z

⌛ Testing commit 9c8ebd3 with merge b41bf4f...


        Auto merge of #12577 - izgzhen:fm-chunked, r=Manishearth

@Manishearth

Chunked ReadFile from file manager + Introduce a `ReadFileProgress` sender in the `ReadFile` msg to file manager, and implement the related I/O operations + Change `tests/unit/net/test.jpeg` from a 4.8K file to a 39K file to better test the chunked reading (Since one chunk is maximally 8129 Bytes). r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12577)

bors-servo · 2016-07-26T08:38:59Z

💔 Test failed - linux-rel

highfive · 2016-07-26T08:39:02Z

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/browsing_context_name.html:
  └ PASS [expected FAIL] Retaining window.name on history traversal

Manishearth · 2016-07-26T08:40:46Z

@ConnorGBrewster that passing test looks unrelated -- any idea what's going on here?

jdm · 2016-07-26T12:34:24Z

@bors-servo: retry

#12568

bors-servo · 2016-07-26T12:34:26Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev are reusable. Rebuilding only linux-rel...

bors-servo · 2016-07-26T13:31:47Z

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

highfive assigned Manishearth Jul 24, 2016

highfive added the S-awaiting-review label Jul 24, 2016

izgzhen force-pushed the izgzhen:fm-chunked branch from 0566047 to 98e039f Jul 24, 2016

Manishearth reviewed Jul 25, 2016
View changes

components/net/filemanager_thread.rs Outdated

}

}

// Send the rest chunks

This comment has been minimized.

Sign in to view

Copy link

Manishearth Jul 25, 2016
Member

s/rest/remaining

Manishearth reviewed Jul 25, 2016
View changes

izgzhen mentioned this pull request Jul 25, 2016

Utilise cancel listener for reading content from file manager #12589

Closed

izgzhen force-pushed the izgzhen:fm-chunked branch from 98e039f to 08ca28a Jul 25, 2016

Chunked ReadFile from file manager

Loading status checks…

9c8ebd3

izgzhen force-pushed the izgzhen:fm-chunked branch from 08ca28a to 9c8ebd3 Jul 25, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 26, 2016

bors-servo merged commit 9c8ebd3 into servo:master Jul 26, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

servo / servo

Chunked ReadFile from file manager #12577

Chunked ReadFile from file manager #12577

izgzhen commented Jul 24, 2016 •

edited by larsbergstrom

highfive commented Jul 24, 2016

Ms2ger commented Jul 24, 2016

izgzhen commented Jul 24, 2016

izgzhen commented Jul 24, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jul 25, 2016

Manishearth commented Jul 25, 2016

izgzhen commented Jul 25, 2016

Manishearth commented Jul 25, 2016

izgzhen commented Jul 25, 2016

Manishearth commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

highfive commented Jul 26, 2016

Manishearth commented Jul 26, 2016

jdm commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

		@@ -443,36 +447,31 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
		.map_err(\|e\| BlobURLStoreError::External(e.to_string())));

		if seeked_start == (range.start as u64) {

servo / servo

Join GitHub today

Chunked ReadFile from file manager #12577

Chunked ReadFile from file manager #12577

Conversation

izgzhen commented Jul 24, 2016 • edited by larsbergstrom

highfive commented Jul 24, 2016

Ms2ger commented Jul 24, 2016

izgzhen commented Jul 24, 2016

izgzhen commented Jul 24, 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.

This comment has been minimized.

This comment has been minimized.

izgzhen Jul 25, 2016 • edited

This comment has been minimized.

Manishearth commented Jul 25, 2016

Manishearth commented Jul 25, 2016

izgzhen commented Jul 25, 2016

Manishearth commented Jul 25, 2016

izgzhen commented Jul 25, 2016

Manishearth commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

highfive commented Jul 26, 2016

Manishearth commented Jul 26, 2016

jdm commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

izgzhen commented Jul 24, 2016 •

edited by larsbergstrom

izgzhen Jul 25, 2016 •

edited