Closed
Bug 1482752
Opened 6 years ago
Closed 6 years ago
Have Fetch bodies use File blobs for local files instead of regular blobs (like XHRs).
Categories
(Core :: DOM: Networking, enhancement, P2)
Core
DOM: Networking
Tracking
()
RESOLVED
FIXED
mozilla64
People
(Reporter: twisniewski, Assigned: twisniewski)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
In order to use the fetch engine to power XMLHttpRequests in bug 1449613, the fetch engine will need to yield File objects for blob() responses, as XHRs do (per bug 1337016). Either that, or we will need to decide to drop that expectation and the related test.
In this bug I'll split off the patch in bug 1449613 which handles this case.
Assignee | ||
Comment 1•6 years ago
|
||
Have Fetch bodies use File blobs for local files instead of regular blobs.
Assignee | ||
Comment 2•6 years ago
|
||
A try run seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=923cf429cf95e0fbccc728b0d04dc99453c88bf8
Updated•6 years ago
|
Assignee: nobody → twisniewski
Priority: -- → P2
Whiteboard: [necko-triaged]
Comment 3•6 years ago
|
||
Comment on attachment 8999488 [details]
Bug 1482752 - Have Fetch bodies use File blobs for local files instead of regular blobs. r?baku
Andrea Marchesini [:baku] has approved the revision.
Attachment #8999488 -
Flags: review+
Assignee | ||
Comment 4•6 years ago
|
||
baku, I've addressed your review comments and a fresh try-run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=167d3dae100224a7d2bed354e6ab15e388116921
Will the comments in that patch do?
OnBlobResult() is called when a blob body is ready to be consumed (when its
network transfer completes in BeginConsumeBodyRunnable or its local File has
been wrapped by FileCreationHandler). The blob is sent to the target thread and
ContinueConsumeBody is called. */
ContinueConsumeBody() is to be called on the target whenever the final
result of the fetch is known. The fetch promise is resolved or rejected
based on whether the fetch succeeded, and the body can be converted into
the expected type of JS object. */
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 6•6 years ago
|
||
Alright then, I've updated the phabricator patch. Requesting check-in.
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68805212630a
Have Fetch bodies use File blobs for local files instead of regular blobs. r=baku
Keywords: checkin-needed
Comment 8•6 years ago
|
||
Backed out changeset for causing build bustages on dom/fetch. So far from what I've seen they only fail on win 2012 builds.
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=68805212630a8a8c44dde795230ecb0b311490d8&filter-resultStatus=busted
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198249550&repo=autoland&lineNumber=20292
Backout link: https://hg.mozilla.org/integration/autoland/rev/d2ee1cb69484b3dde41a1751d0c8656855ce756c
17:34:17 INFO - z:/build/build/src/obj-firefox/dist/include/nsString.h(128,3): note: candidate constructor not viable: requires 2 arguments, but 1 was provided
17:34:17 INFO - NS_ConvertUTF8toUTF16(const char* aCString, uint32_t aLength)
17:34:17 INFO - ^
17:34:17 INFO - 2 errors generated.
17:34:17 INFO - z:/build/build/src/config/rules.mk:1110: recipe for target 'Unified_cpp_dom_fetch0.i_o' failed
17:34:17 INFO - mozmake.EXE[5]: *** [Unified_cpp_dom_fetch0.i_o] Error 1
17:34:17 INFO - mozmake.EXE[5]: Leaving directory 'z:/build/build/src/obj-firefox/dom/fetch'
17:34:17 INFO - z:/build/build/src/config/recurse.mk:74: recipe for target 'dom/fetch/target' failed
17:34:17 INFO - mozmake.EXE[4]: *** [dom/fetch/target] Error 2
17:34:17 INFO - mozmake.EXE[4]: *** Waiting for unfinished jobs....
Flags: needinfo?(twisniewski)
Assignee | ||
Comment 9•6 years ago
|
||
Hmm, looks like on win32 there's no need to call NS_ConvertUTF8toUTF16, as PathStrings are already char_16ts. I've verified that using an ifdef to avoid it works fine:
>#ifdef XP_WIN
> rv = file->InitWithPath(mBodyLocalPath);
>#else
> rv = file->InitWithPath(NS_ConvertUTF8toUTF16(mBodyLocalPath));
>#endif
> NS_ENSURE_SUCCESS(rv, rv);
(Try-run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd0fa45dfeaf56224d2860ce3db71169f38dfb7a )
baku, does that seems like a reasonable fix, or should I perhaps try something else?
Flags: needinfo?(twisniewski) → needinfo?(amarchesini)
Comment 10•6 years ago
|
||
I suggest to use path instead of nativePath. This is what the FileSystem API uses.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 11•6 years ago
|
||
Ah, of course. That works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f5b8a2486001a594d394151c73ae587376deca8
It does mean changing PathStrings to regular ns*Strings everywhere in the patch, but given that doesn't mean any functional changes, I don't think it warrants additional review.
Re-requesting checkin.
Keywords: checkin-needed
Comment 12•6 years ago
|
||
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8d3aa56f14d
Have Fetch bodies use File blobs for local files instead of regular blobs. r=baku
Keywords: checkin-needed
Comment 13•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•