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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[Jason Smith [:jsmith]]

This also happens on device as well.

Comment 2

[Jason Smith [:jsmith]]

This will block shipping the download manager given that this is a core piece of the web not working here.

Comment 3

[Aus Lacroix [:aus]]

Attachment: Patch - Add test for 'download.path'

Comment 4

[Aus Lacroix [:aus]]

It actually works as expected for me and the test will ensure that it stays that way.

Comment 5

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Add test for 'download.path'

Added reviewer to commit message. r+ carries over. r=qdot

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/013f33d242e2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/1696624002cd

https://tbpl.mozilla.org/php/getParsedLog.php?id=33107959&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33108850&tree=B2g-Inbound

Comment 8

[Aus Lacroix [:aus]]

Yep, totally still broken in the emulator. That sucks. Investigating...

Comment 9

[Aus Lacroix [:aus]]

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.

Comment 10

[Aus Lacroix [:aus]]

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.

Comment 11

[Jason Smith [:jsmith]]

Going to assume the flags were reset by accident.

Comment 12

[Aus Lacroix [:aus]]

Attachment: Patch - Try 1 - Implement SetContentDisposition/GetContentDisposition and Filename on ExtAppHandlerChild

This is an attempt at fixing the issue.

Comment 13

[Aus Lacroix [:aus]]

Attachment: Patch - Create fake content disposition header for the child

Comment 14

[Aus Lacroix [:aus]]

Attachment: Patch - Create fake content disposition header for the child

Comment 15

[Aus Lacroix [:aus]]

Attachment: Patch - Create fake content disposition header for the child

Comment 16

[Aus Lacroix [:aus]]

Attachment: Patch - Create fake content disposition header for the child

Last update to the patch, I swear. :)

Comment 17

[Johnny Stenback (: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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[Boris Zbarsky [:bzbarsky]]

To answer my own question, it's using that because it was the expedient thing back in bug 589292, apparently.  :(

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Aus Lacroix [:aus]]

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 22

[Aus Lacroix [:aus]]

Attachment: Patch - Update ExternalHelperAppParent and Child to also use ContentDispositionHint and ContentDispositionFilename

Comment 23

[Boris Zbarsky [:bzbarsky]]

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...

Comment 24

[Aus Lacroix [:aus]]

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!

Comment 25

[Aus Lacroix [:aus]]

Attachment: Patch - Final (Enable download attribute to work as expected with Content Processes)

r=bz, final patch removes anjuta .hgignore bits.

Comment 26

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1f32afbaa419

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f32afbaa419