Closed Bug 1482752 Opened Last year Closed Last year

Have Fetch bodies use File blobs for local files instead of regular blobs (like XHRs).

Categories

(Core :: DOM: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

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.
Have Fetch bodies use File blobs for local files instead of regular blobs.
Assignee: nobody → twisniewski
Priority: -- → P2
Whiteboard: [necko-triaged]
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+
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)
Yes, Thanks!
Flags: needinfo?(amarchesini)
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
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)
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)
I suggest to use path instead of nativePath. This is what the FileSystem API uses.
Flags: needinfo?(amarchesini)
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
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
https://hg.mozilla.org/mozilla-central/rev/f8d3aa56f14d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1527712
Depends on: 1534712
No longer depends on: 1534712
Regressions: 1534712
You need to log in before you can comment on or make changes to this bug.