Closed
Bug 991977
Opened 10 years ago
Closed 10 years ago
Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S1 (9may)
People
(Reporter: rainwoo7, Assigned: hansu9866)
Details
Attachments
(1 file)
864 bytes,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.154 Safari/537.36 Steps to reproduce: 1) Connect to test url: http://nactsft.net -> MMS -> audio-> mp3 2) Try to download Actual results: Can not download any content on Internal storage Actually,there are no action when user try to link click Expected results: These content have to downloaded on Internal storage
Cause of problem which contents can't downloaded in internal storage is checking mount lock of volume. when Mountlock is true, can't download contents. That can be found _getDefaultDownloadDirectory() api below code. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm So, we opened issue why mount lock of internal storage is set by true? PLz Look at below bug No.989894 and you can see detail information. -> https://bugzilla.mozilla.org/show_bug.cgi?id=989894 During we discussed that issue, and then we want know why download toolkit in browser component check mount lock of Volume(storage). Mount lock of volume is only used to prevent that volume is being shared with PC. As we see, mount lock is not checked in toolkit for download contents.
Summary: Why mount lock of Volume(storage) is checked during download contents? → Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?
Comment 2•10 years ago
|
||
Sorry - I don't understand what you're asking me for (needinfo). Also - please needinfo with my mozilla email.
Flags: needinfo?(dhylands)
To Dave Hylands As I think, this issue need your comment about mount lock of volume. So I requested you. To Wain Chang Could you arrange browser member for this issue? I heard about you are manager of mozilla.
Flags: needinfo?(wchang)
Comment 4•10 years ago
|
||
I made lots of comments about mount locks in bug 989894 What is your specific question? I still don't know what you're trying to ask me.
Comment 5•10 years ago
|
||
Woo, if you look at the "blame" of the code you linked, you'll see that it was implemented by bug 936342 with this changeset http://hg.mozilla.org/mozilla-central/rev/9e31c9c04ccf blame: http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289
Flags: needinfo?(edilee)
I checked below code as you said, and I want to know why mount lock is used in _getDefaultDownloadDirectory() http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289 As I understand, below code is't necessary in _getDefaultDownloadDirectory(). -> L289 : !volume.isMountLocked && Because Mount lock of volume is only used to prevent that volume is being shared with PC.
To fabrice, We had problem which can't download contents in browser. That is caused by checking mount lock of volume in _getDefaultDownloadDirectory() http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289 As I understand, below code is't necessary in _getDefaultDownloadDirectory(). -> L289 : !volume.isMountLocked && Could you tell me about reason checking mount lock?
Yes, we could do that, mount lock of volume is only used to prevent that volume is being shared with PC. So, if we try to write on the sdcard, we don't need to check whether volume is mount locked. As a result, below code is needed to remove. As I understand, below code is't necessary in _getDefaultDownloadDirectory(). -> L289 : !volume.isMountLocked &&
Comment 10•10 years ago
|
||
Woo, please submit a patch and ask dhylands to review. thanks!
Flags: needinfo?(fabrice)
Reporter | ||
Comment 11•10 years ago
|
||
To, dhylands Could I patch like below? http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289 As a result, below code will be removed in _getDefaultDownloadDirectory(). -> L289 : !volume.isMountLocked &&
Flags: needinfo?(dhylands)
Comment 12•10 years ago
|
||
That isn't sufficient. For example, the volume could be unmounted, or it could be being formatted. If you're going to use the volume object directly, then you should check that the state is equal to mounted, like this: http://dxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#137 mounted is the only state where you can read/write the volume and it implies !sharing and media present. It would be preferable not to use the volume stuff at all and to use device storage to perform the I/O. When running under b2g-desktop for example, none of the volume API is available (but the device storage api is available). Using device storage, you should check storage.available() returns "available".
Flags: needinfo?(dhylands)
Reporter | ||
Comment 13•10 years ago
|
||
As you said, I agree to need to checking !sharing and media preset. and I can see that below in getDefaultDownloadDirectory() at DownloadIntegration.jsm 287 if (volume && 288 volume.isMediaPresent && 289 !volume.isMountLocked && 290 !volume.isSharing) { That is implemented as you mentioned, but !volume.isMountLocked is not necessary. As a result, there is no problem about removing below code which check mount lock. -> ## !volume.isMountLocked &&
Comment 14•10 years ago
|
||
Please see comment 12 You shouldn't check isMediaPresent and you shouldn't check isSharing. You SHOULD check that the status is MOUNTED. This means that the volume is accessible to FxOS (and it implies isMediaPresent and it also implies !isSharing) The test from comment 13 would incorrectly pass if the volume state is 1, 2, 3, 5, or 6. See http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIVolume.idl#13 for the various states.
Flags: needinfo?(dhylands)
Reporter | ||
Comment 15•10 years ago
|
||
Okay, as you mentioned, I understand, We should check whether state of volume is MOUNTED. if (preferredStorageName) { let volume = volumeService.getVolumeByName(preferredStorageName); if (volume && volume.state !== Ci.nsIVolume.STATE_MOUNTED) { directoryPath = OS.Path.join(volume.mountPoint, "downloads"); yield OS.File.makeDir(directoryPath, { ignoreExisting: true }); } } Is it right?
Comment 16•10 years ago
|
||
Almost. You seem to be testing !== mounted, which it should be === mounted
Flags: needinfo?(dhylands)
Reporter | ||
Comment 17•10 years ago
|
||
Thanks Dave. That's correct, we test do that and that problem is solved.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Comment 18•10 years ago
|
||
hansu What did you reopen this? please give details
Flags: needinfo?(hansu9866)
Assignee | ||
Comment 19•10 years ago
|
||
The patch proposed by Woo fixed the problem, and I understand that the comment 16 was a r+. So we should uplift this patch before changing to resolved.
Assignee | ||
Comment 20•10 years ago
|
||
Clear the needinfo
Comment 21•10 years ago
|
||
(In reply to hansu9866 from comment #19) > The patch proposed by Woo fixed the problem, and I understand that the > comment 16 was a r+. > So we should uplift this patch before changing to resolved. In which case, I think you'll need to upload a proper patch and request for review :)
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Wayne Chang [:wchang] from comment #21) > (In reply to hansu9866 from comment #19) > > The patch proposed by Woo fixed the problem, and I understand that the > > comment 16 was a r+. > > So we should uplift this patch before changing to resolved. > > In which case, I think you'll need to upload a proper patch and request for > review :) OK, thanks your guide.
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8411457 [details] [diff] [review] Modify DownloadIntegration.jsm file for volume check condition To. Dave I upload the patch. Could you check it for review? It is based on your comment. Thanks.
Attachment #8411457 -
Flags: review?(dhylands)
Updated•10 years ago
|
Attachment #8411457 -
Flags: review?(dhylands) → review+
Keywords: checkin-needed
Comment 25•10 years ago
|
||
Thanks for the patch! Can you please add commit information to this patch so I can land it? Thanks! https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > Thanks for the patch! Can you please add commit information to this patch so > I can land it? Thanks! > https://developer.mozilla.org/en-US/docs/ > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check- > in_for_me.3F Thanks for your guide. Is it possible? Author name: Hansu Kim <hansu9866@gmail.com> Commit message: "Bug 991977 - Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?; r=dhylands".
Flags: needinfo?(hansu9866)
Comment 27•10 years ago
|
||
(In reply to hansu9866 from comment #26) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > > Thanks for the patch! Can you please add commit information to this patch so > > I can land it? Thanks! > > https://developer.mozilla.org/en-US/docs/ > > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check- > > in_for_me.3F > > Thanks for your guide. > Is it possible? > > Author name: > Hansu Kim <hansu9866@gmail.com> > Commit message: > "Bug 991977 - Why mount lock of Volume(storage) is checked during download > contents in Browser Toolkit?; r=dhylands". You checkin comment should describe the fix (which may not bear any resemblence to the original reported problem). A better comment would something along the lines of: Bug 991977 - Ensure volume is mounted before starting download. r=dhylands
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #27) > (In reply to hansu9866 from comment #26) > (In reply to Ryan VanderMeulen > [:RyanVM UTC-4] from comment #25) > > Thanks for the patch! Can you please > add commit information to this patch so > > I can land it? Thanks! > > > https://developer.mozilla.org/en-US/docs/ > > > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check- > > > in_for_me.3F > > Thanks for your guide. > Is it possible? > > Author name: > > Hansu Kim <hansu9866@gmail.com> > Commit message: > "Bug 991977 - Why > mount lock of Volume(storage) is checked during download > contents in > Browser Toolkit?; r=dhylands". You checkin comment should describe the fix > (which may not bear any resemblence to the original reported problem). A > better comment would something along the lines of: Bug 991977 - Ensure > volume is mounted before starting download. r=dhylands Thanks, I'm agree, it is the better.
Assignee | ||
Comment 29•10 years ago
|
||
Dear RyanVM UTC-4, Is it possible? Author name: Hansu Kim <hansu9866@gmail.com> Commit message: Bug 991977 - Ensure volume is mounted before starting download. r=dhylands
Flags: needinfo?(ryanvm)
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c5d205aa662c Thanks for the patch!
Keywords: checkin-needed
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5d205aa662c
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
You need to log in
before you can comment on or make changes to this bug.
Description
•