Closed Bug 945323 Opened 8 years ago Closed 7 years ago

[Download API] Downloaded path isn't honoring download attribute

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jsmith, Assigned: aus)

References

Details

(Keywords: verifyme, Whiteboard: [systemsfe][p=8])

Attachments

(1 file, 8 obsolete files)

Apply the patch in bug 945099 & run that mochitest. You'll notice that the download path uses serve_file.sjs rather than test.bin. This means that the download API isn't honoring the download attribute specified in the anchor tag for determining the file name of the downloaded file.
Whiteboard: [systemsfe]
This also happens on device as well.
Summary: [Download API] Downloaded path isn't honoring download attribute in the emulator → [Download API] Downloaded path isn't honoring download attribute
This will block shipping the download manager given that this is a core piece of the web not working here.
blocking-b2g: --- → 1.4?
Assignee: nobody → aus
Status: NEW → ASSIGNED
It actually works as expected for me and the test will ensure that it stays that way.
Attachment #8360584 - Flags: review?(kyle)
Attachment #8360584 - Flags: review?(kyle) → review+
Keywords: checkin-needed
Added reviewer to commit message. r+ carries over. r=qdot
Attachment #8360698 - Flags: review+
Attachment #8360584 - Attachment is obsolete: true
Yep, totally still broken in the emulator. That sucks. Investigating...
Somehow, our HelperAppDialog.js that specifically causes all files to be saved to disk must be getting the wrong default filename, I'm trying to confirm this now. I am, however, having trouble finding where "promptForSaveToFile" actually gets called.
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
blocking-b2g: 1.4? → 1.4+
I've traced through the entire code path and while running on desktop, despite enabling remote tabs, it is impossible to reproduce the issue. There are no obvious areas where the code paths diverge when compiling Gonk so I'm a little bit stumped. I'm going to make a debug build of the emulator and see what's going on.
blocking-b2g: 1.4+ → 1.4?
Target Milestone: 1.3 C3/1.4 S3(31jan) → ---
Going to assume the flags were reset by accident.
blocking-b2g: 1.4? → 1.4+
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Attachment #8360698 - Attachment is obsolete: true
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S1 (14feb)
Attachment #8368266 - Attachment is obsolete: true
Attachment #8370771 - Flags: review?(jst)
Whiteboard: [systemsfe] → [systemsfe][p=8]
Attachment #8370771 - Attachment is obsolete: true
Attachment #8370771 - Flags: review?(jst)
Attachment #8370772 - Flags: review?(jst)
Attachment #8370772 - Attachment is obsolete: true
Attachment #8370772 - Flags: review?(jst)
Attachment #8370774 - Flags: review?(jst)
Last update to the patch, I swear. :)
Attachment #8370774 - Attachment is obsolete: true
Attachment #8370774 - Flags: review?(jst)
Attachment #8370815 - Flags: review?(jst)
Comment on attachment 8370815 [details] [diff] [review]
Patch - Create fake content disposition header for the child

r=jst, but bz should sign off on this one too.
Attachment #8370815 - Flags: review?(jst)
Attachment #8370815 - Flags: review?(bzbarsky)
Attachment #8370815 - Flags: review+
Comment on attachment 8370815 [details] [diff] [review]
Patch - Create fake content disposition header for the child

Why is this code using GetContentDispositionHeader at all?  That's not reliable in all sorts of cases; there's a reason it's deprecated!

Why can't we just send the contentDispositionFilename and contentDisposition to the parent instead of sending the header string?
Attachment #8370815 - Flags: review?(bzbarsky) → review-
To answer my own question, it's using that because it was the expedient thing back in bug 589292, apparently.  :(
I guess we should probably send all three, actually, since we still need the header too, in theory.  I wonder if we can just kill off the remaining uses of GetContentDispositionHeader.
Indeed. It's unfortunate that it's implemented the way it is right now. In theory, this issue should go away as well once we short-circuit things. Currently we need to do this because we have to go Parent->Child->Parent with the info and the *Child* is the one that didn't have the correct data.

I'm OK with updating the IPDL to receive *all* 3 instead but it will add another 2 to 3 days of work (round-trip with all the reviews and the push to try before actually landing on inbound).

I'll have an updated patch for this soon but I need to sort out a different 1.3 blocker bug first.
Comment on attachment 8375817 [details] [diff] [review]
Patch - Update ExternalHelperAppParent and Child to also use ContentDispositionHint and ContentDispositionFilename

Should the Anjuta bits really be landing as part of this bug?

r=me on the rest, though I wish some of that stuff had better (any?) API documentation...
Attachment #8375817 - Flags: review?(bzbarsky) → review+
Meh, I can land the Anjuta bits separately in a bug for that purpose. Some of that stuff does have API documentation but only for the actual public interfaces that our exposed. None of the IPC specific interfaces have any documentation.

Also, I get to try out my shiny new commit access. Hooray!
r=bz, final patch removes anjuta .hgignore bits.
Attachment #8375817 - Attachment is obsolete: true
Attachment #8375853 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1f32afbaa419
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.