Closed Bug 1072535 Opened 5 years ago Closed 5 years ago

[Gallery] Downloaded images will not appear in Gallery until Gallery is refreshed.

Categories

(Firefox OS Graveyard :: Gaia::Gallery, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S7 (24Oct)
blocking-b2g 2.0+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jthomas, Assigned: dhylands)

References

()

Details

(Whiteboard: [2.1-exploratory-2])

Attachments

(9 files, 4 obsolete files)

155.72 KB, text/plain
Details
46 bytes, text/x-github-pull-request
dhylands
: review+
Details | Review
8.85 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
5.69 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
5.96 KB, patch
Details | Diff | Splinter Review
15.50 KB, patch
Details | Diff | Splinter Review
5.95 KB, patch
Details | Diff | Splinter Review
15.48 KB, patch
Details | Diff | Splinter Review
3.49 MB, video/mp4
Details
Description:
If gallery has been previously opened and the user decides to download an image, the image will not appear in the gallery unless the gallery has been completely closed.


Repro Steps:
1) Update a Flame to 20140924000243
2) Open Gallery
3) Download any image from the Browser.
4) Navigate back to Gallery.


Actual:
Newly downloaded images do not appear in Gallery.


Expected:
It is expected that the new images downloaded will appear in the Gallery.

Environmental Variables:
Device: Flame 2.1 KitKat Base (319mb)
Build ID: 20140924000243
Gaia: 93a99bea0b40d81bd063f7d8b1964dc1ba35ba7b
Gecko: d7e31ecd48af
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Repro frequency: 100%

See attached: Logcat
Video: https://www.youtube.com/watch?v=v0gl2qDaQ7w&edit=vd
This issue also occurs on the Flame 2.2 KitKat Base (319mb), Flame 2.1 KitKat Base (512mb) and the Flame 2.0 KitKat Base (319mb).

Description: Newly downloaded images do not appear in Gallery when Gallery is already running in background.

Flame 2.2 KitKat Base (319mb)

Environmental Variables:
Device: Flame Master
Build ID: 20140924013003
Gaia: ff6dbb006e4e60edba697639aa2a2a199b07069b
Gecko: 1e2993c99323
Version: 35.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KitKat Base (512mb)

Environmental Variables:
Device: Flame 2.1
BuildID: 20140924000243
Gaia: 93a99bea0b40d81bd063f7d8b1964dc1ba35ba7b
Gecko: d7e31ecd48af
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.0 KitKat Base (319mb)

Environmental Variables:
Device: Flame 2.0
Build ID: 20140924003005
Gaia: 263e3b201dca967ec5346e35901aa981ca47dce7
Gecko: 35d791e16d31
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Whiteboard: [2.1-exploratory-2]
adding qawanted to recheck branches on a shallow flash to see if the issue still occurs
Flags: needinfo?(dharris)
Keywords: qawanted
Flags: needinfo?(dharris)
Tested with Shallow Flash on 319mb

This bug repro's on Flame KK builds: Flame 2.2 KK, Flame 2.1 KK, Flame 2.0 KK, Flame 2.0 Base KK

Actual Results: With Gallery already open, images that are downloaded won't show up in the Gallery until the gallery is refreshed.

Repro Rate: 4/4

Environmental Variables:
Device: Flame Master KK
BuildID: 20141001060621
Gaia: a23d2c490b39c4699c9375e25c4acdf396a2fa85
Gecko: 835ef55e175e
Version: 35.0a1 (Master) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20141001060122
Gaia: b327c640fea887770d011a127e349838b3b44724
Gecko: 7359d0d0222d
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0
BuildID: 20141001060124
Gaia: 8079cba2133e6f5443dba24dad077f7f91e6c978
Gecko: 66c1ea78b6c1
Version: 32.0 (2.0) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0
BuildID: 20140904160718
Gaia: 506da297098326c671523707caae6eaba7e718da
Gecko: 
Gonk: 
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
[Blocking Requested - why for this release]: This is not a regression but it feels like users would expect the gallery to update 'live' and NOT have to stop the app and restart it.
blocking-b2g: --- → 2.0?
Hema - this issue was written first AND has more work complete (branch checks) - don't you think the other bug should be closed as a dupe (just asking)?
Assignee: nobody → dhylands
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(dharris)
Dave, please keep the bug updated on progress.
Flags: needinfo?(dhylands)
I'm very slowly making progress.
Flags: needinfo?(dhylands)
Duplicate of this bug: 1074632
I submitted a try run to see if the updated test passes:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5d8bee6f59e8

I'll do something a bit more extensive once I see the linux ones passing.
Attachment #8501369 - Flags: review?(aus)
Attachment #8501387 - Flags: review?(mhabicher)
I needed to change the event data sent from created to modified to be consistent with the other media file producers. And I tested in gallery this time.
Attachment #8501369 - Attachment is obsolete: true
Attachment #8501369 - Flags: review?(aus)
Attachment #8501407 - Flags: review?(aus)
Comment on attachment 8501407 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v2

Review of attachment 8501407 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/downloads/DownloadsAPI.jsm
@@ +143,5 @@
> +
> +    // Add test to verify that the download-watcher-notify event was actually sent.
> +    // Put it in test_downloads_basic.html
> +
> +    if (download.state == "succeeded") {

A download succeeding is not guaranteed to be the last notification you receive for that download. It is also possible that a successful download is added to the list without transitioning from other states.

As I understand it, you need to fire a "download-watcher-notify" observer notification because you need to interface with the Device Storage code, which is C++ only, and an observer notification is simpler than calling a method from JavaScript on the C++ interfaces. However, this operation is required in B2G only, because other platforms don't use Device Storage, so it doesn't belong to the Downloads system for all platforms.

I'd use a JS component tied to Device Storage, listening to the same onDownloadAdded / onDownloadChanged notifications from the JavaScript API for Downloads, similar to what you see here, but keeping track of which downloads have already been completed (on the assumption that notifying multiple times has a performance issue).

As you've noticed, the DOM Downloads API sends notifications to child processes using nsIMessageBroadcaster, and as I understand it you're working entirely in the parent process, so apparently you shouldn't be a client of "Downloads:Added" and "Downloads:Changed" messages, despite the concept would be the same for those notifications.

Hope this makes sense! :-)
Attachment #8501407 - Flags: review?(aus)
(In reply to :Paolo Amadini from comment #15)
> Comment on attachment 8501407 [details] [diff] [review]
> Pt 1 - Have Download API notify device storage when a download completes. v2
> 
> Review of attachment 8501407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/downloads/DownloadsAPI.jsm
> @@ +143,5 @@
> > +
> > +    // Add test to verify that the download-watcher-notify event was actually sent.
> > +    // Put it in test_downloads_basic.html
> > +
> > +    if (download.state == "succeeded") {
> 
> A download succeeding is not guaranteed to be the last notification you
> receive for that download. It is also possible that a successful download is
> added to the list without transitioning from other states.

Would that mean that a new file was created? 

> As I understand it, you need to fire a "download-watcher-notify" observer
> notification because you need to interface with the Device Storage code,
> which is C++ only, and an observer notification is simpler than calling a
> method from JavaScript on the C++ interfaces. However, this operation is
> required in B2G only, because other platforms don't use Device Storage, so
> it doesn't belong to the Downloads system for all platforms.

I believe that DeviceStorage is available on all platforms.

> I'd use a JS component tied to Device Storage, listening to the same
> onDownloadAdded / onDownloadChanged notifications from the JavaScript API
> for Downloads, similar to what you see here, but keeping track of which
> downloads have already been completed (on the assumption that notifying
> multiple times has a performance issue).

Currently DeviceStorage has no JS component, so this is the part where I'm lost. How do you get the navigator.mozDownloadManager? The code I'd write wouldn't have a window, so my understanding is that the navigator doesn't exist.

> As you've noticed, the DOM Downloads API sends notifications to child
> processes using nsIMessageBroadcaster, and as I understand it you're working
> entirely in the parent process, so apparently you shouldn't be a client of
> "Downloads:Added" and "Downloads:Changed" messages, despite the concept
> would be the same for those notifications.
> 
> Hope this makes sense! :-)

At a conceptual level, it makes sense, but I'm not sure where to start.
More complete try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f2d48b638f89

Does anybody know where the download tests are run? I looked through M1 thru M5 and can't find any evidence (I was searching for test_down)
Now I'm confused.

Paolo, is DownloadAPI B2G only?
comment 15 seems to imply that is isn't
But the tests are only run for B2G:
http://dxr.mozilla.org/mozilla-central/source/dom/downloads/moz.build#7-8
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8501387 [details] [diff] [review]
Pt 2 - Have devicestorage notify all storage types based on extension. v2

Review of attachment 8501387 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, with the nits/questions below addressed.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +459,5 @@
> +{
> +  // This function determines if aType shares a root directory with the
> +  // other media types (so only applies to music, videos, pictures and sdcard).
> +
> +  if (aType.EqualsLiteral(DEVICESTORAGE_APPS) ||

Would it be better/more future-proof to explicitly test for DEVICESTORAGE_PICTURES, _VIDEOS, _MUSIC, and _SDCARD here?

@@ +466,5 @@
> +  }
> +#ifdef MOZ_WIDGET_GONK
> +  return true;
> +#else
> +  // For dekstop, if the directories have been overridden, then they share

nit: desktop

@@ +495,5 @@
>  
>  NS_IMETHODIMP
>  FileUpdateDispatcher::Observe(nsISupports *aSubject,
>                                const char *aTopic,
>                                const char16_t *aData)

nit: not in your patch, but the *'s above should go with the type.

@@ +544,5 @@
> +#endif
> +    dsf = new DeviceStorageFile(NS_LITERAL_STRING(DEVICESTORAGE_SDCARD), volName, path);
> +
> +  } else if (!strcmp(aTopic, "file-watcher-notify")) {
> +    dsf = static_cast<DeviceStorageFile*>(aSubject);

do_QueryInterface()?

@@ +596,2 @@
>    } else {
> +    obs->NotifyObservers(dsf, "file-watcher-update", aData);

Can we slurp out references to "file-watcher-update" to a const char* kFileWatcherUpdateTopic?
Attachment #8501387 - Flags: review?(mhabicher) → review+
(In reply to Dave Hylands [:dhylands] from comment #18)
> Now I'm confused.
> 
> Paolo, is DownloadAPI B2G only?
> comment 15 seems to imply that is isn't
> But the tests are only run for B2G:
> http://dxr.mozilla.org/mozilla-central/source/dom/downloads/moz.build#7-8

It's a little confusing for sure, we have a few download apis :) 

The jsdownloads component is included in firefox desktop, android and b2g. It lives here -- http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/

The Downloads *Web API* is currently *only* on b2g builds that are *NOT* for desktop. It lives here -- http://mxr.mozilla.org/mozilla-central/source/dom/downloads/

Hope that helps. Some of the Download Web API tests are disabled on b2g emulator because of intermittent issues but should be re-enabled in the near future.
Flags: needinfo?(paolo.mozmail)
I figured out how to do what needs to be done purely in C++.

The test fails, so I can't land it as is. I need to figure out how to enable SpecialPowers.

This is the try run and failure: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3796b753fa13

Manual testing works though, so I'd also be willing to fix the test up in a folloup bug.
Attachment #8501407 - Attachment is obsolete: true
Attachment #8502260 - Flags: review?(paolo.mozmail)
(In reply to Ghislain Aus Lacroix [:aus] from comment #20)
> The Downloads *Web API* is currently *only* on b2g builds that are *NOT* for
> desktop. It lives here --
> http://mxr.mozilla.org/mozilla-central/source/dom/downloads/
> 
> Hope that helps. Some of the Download Web API tests are disabled on b2g
> emulator because of intermittent issues but should be re-enabled in the near
> future.

Ah, that helps, thanks!
Comment on attachment 8502260 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v3

The approach of using a notification for Device Storage in the platform-specific section of the code looks good to me.

This doesn't need to be C++, however, we should work on the JavaScript side when possible. The place you have to modify is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#615

You'd need a similar try-catch, and you can use Services.prefs.getBoolPref and Services.obs to access the services you need.

(In reply to Dave Hylands [:dhylands] from comment #21)
> The test fails, so I can't land it as is. I need to figure out how to enable
> SpecialPowers.

These are xpcshell tests, so you should use Services.obs and Services.prefs directly. You'll need to register a cleanup function, similarly to what is done here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#709

Note that Services.prefs.clearUserPref doesn't always yield the expected result (depending on how the profile is set up), you may need to call setBoolPref with the old value instead.

For simplicity, I suggest you add a check to the existing platform integration test, rather than adding a new one. The test you need to modify is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/common_test_Download.js#1728
Attachment #8502260 - Flags: review?(paolo.mozmail) → feedback+
I redid the test as you suggested. Here's my successful try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f86d4c9b5d3

Currently, I don't see the advantage of changing the NotifyObservers code from C++ to JS, so I just left it as C++. Won't writing it in JS wind up using more memory?
Attachment #8502260 - Attachment is obsolete: true
Attachment #8502926 - Flags: review?(paolo.mozmail)
Comment on attachment 8502926 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v4

Review of attachment 8502926 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dave Hylands [:dhylands] from comment #24)
> Currently, I don't see the advantage of changing the NotifyObservers code
> from C++ to JS, so I just left it as C++. Won't writing it in JS wind up
> using more memory?

Our final goal for the "jsdownloads" module is to remove all the C++ code, so that the module is implemented using only one language. We still have C++ code, partly because in some cases there is no JavaScript alternative, and partly because we didn't have a strong need to migrate to js-ctypes yet.

This is why I'd prefer for this to be done in JavaScript, though it's not a big deal if you prefer to keep this C++, this bit would be the easiest to migrate later.

The advantage of JavaScript here would be conciseness. The C++ code needs more lines of code, in fact this implementation has some issues I didn't point out earlier, that need to be fixed:

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
@@ +126,5 @@
>      ::CFRelease(observedObject);
>  #endif
> +    if (mozilla::Preferences::GetBool("device.storage.enabled", true)) {
> +      // Tell DeviceStorage that a new file may have been added.
> +      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();

This needs to be null-checked.

@@ +128,5 @@
> +    if (mozilla::Preferences::GetBool("device.storage.enabled", true)) {
> +      // Tell DeviceStorage that a new file may have been added.
> +      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +      nsCOMPtr<nsISupportsString> pathString
> +        = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);

This too, or check rv.

@@ +129,5 @@
> +      // Tell DeviceStorage that a new file may have been added.
> +      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +      nsCOMPtr<nsISupportsString> pathString
> +        = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);
> +      pathString->SetData(path);

This is fallible as well, need to check rv.

@@ +131,5 @@
> +      nsCOMPtr<nsISupportsString> pathString
> +        = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);
> +      pathString->SetData(path);
> +      obs->NotifyObservers(pathString, "download-watcher-notify",
> +                           MOZ_UTF16("modified"));

nit: (void)obs->NotifyObservers to make it explicit that we don't care about whether this is successful.

With regard to JavaScript memory usage, I don't think there's any difference, but it's a secondary concern anyways given the frequency with which this code is called.
Attachment #8502926 - Flags: review?(paolo.mozmail) → review+
I filed followup bug 1082714 to rewrite the devices storage notification in JS.
And my merge to gaia
https://github.com/mozilla-b2g/gaia/commit/e7eec5a773865f8b6557d05927ac902782eaa41c

NOTE: If for some reason this bug needs to be backed out, only the gecko portion needs to be backed out. The gaia portion only affects my test app, and the test app doesn't need the gecko portion.
https://hg.mozilla.org/mozilla-central/rev/902dfed4c120
https://hg.mozilla.org/mozilla-central/rev/9d0bba1b36dd
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Duane, please verify this on tomorrow's build.
Keywords: verifyme
Issue is verified fixed in Flame 2.2

Actual Results: Newly saved images downloaded via Browser appear in Gallery app correctly. 

Device: Flame Master
Build ID: 20141016040204
Gaia: 841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9
Gecko: a280a03c9f3c
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
-------------------------------------------------------------------------------
-------------------------------------------------------------------------------
Waiting for patch uplift for Flame branches: 2.1, 2.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Comment on attachment 8501387 [details] [diff] [review]
Pt 2 - Have devicestorage notify all storage types based on extension. v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MTP
User impact if declined: Gallery/Music/Video app don't update to reflect deleted files.
Testing completed: TBPL, manual testing.
Risk to taking this patch (and alternatives if risky): I think that the patch is fairly low risk.
String or UUID changes made by this patch: None
Attachment #8501387 - Flags: approval-mozilla-b2g34?
Attachment #8501387 - Flags: approval-mozilla-b2g32?
Comment on attachment 8502926 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MTP
User impact if declined: Gallery/Music/Video app don't update to reflect deleted files.
Testing completed: TBPL, manual testing.
Risk to taking this patch (and alternatives if risky): I think that the patch is fairly low risk.
String or UUID changes made by this patch: None
Attachment #8502926 - Flags: approval-mozilla-b2g34?
Attachment #8502926 - Flags: approval-mozilla-b2g32?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Attachment #8501387 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8502926 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Needs rebasing for uplift.
Flags: needinfo?(dhylands)
[Triage Comment] Transferred approval-b2g34+ from the other patch.

This patch has been rebased on b2g34.
Flags: needinfo?(dhylands)
Attachment #8507213 - Flags: approval-mozilla-b2g34+
[Triage Comment] Transferred approval-b2g34+ from the other patch.

This patch has been rebased on b2g34.
Attachment #8507214 - Flags: approval-mozilla-b2g34+
Comment on attachment 8502926 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v4

Cleared b2g34+ since this patch doesn't apply cleanly.
Attachment #8502926 - Flags: approval-mozilla-b2g34+
Comment on attachment 8501387 [details] [diff] [review]
Pt 2 - Have devicestorage notify all storage types based on extension. v2

Cleared b2g34+ since this patch doesn't apply cleanly.
Attachment #8501387 - Flags: approval-mozilla-b2g34+
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MTP
User impact if declined: Gallery/Music/Video app don't update to reflect deleted files.
Testing completed: TBPL, manual testing.
Risk to taking this patch (and alternatives if risky): I think that the patch is fairly low risk.
String or UUID changes made by this patch: None
Attachment #8507248 - Flags: approval-mozilla-b2g32?
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MTP
User impact if declined: Gallery/Music/Video app don't update to reflect deleted files.
Testing completed: TBPL, manual testing.
Risk to taking this patch (and alternatives if risky): I think that the patch is fairly low risk.
String or UUID changes made by this patch: None
Attachment #8507249 - Flags: approval-mozilla-b2g32?
Comment on attachment 8501387 [details] [diff] [review]
Pt 2 - Have devicestorage notify all storage types based on extension. v2

Removed b2g32? from this patch since I attached a b2g32 specific patch.
Attachment #8501387 - Flags: approval-mozilla-b2g32?
Comment on attachment 8502926 [details] [diff] [review]
Pt 1 - Have Download API notify device storage when a download completes. v4

Removed b2g32? from this patch since I attached a b2g32 specific patch.
Attachment #8502926 - Flags: approval-mozilla-b2g32?
Attachment #8507248 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Attachment #8507249 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
This issue is verified fixed on Flame 2.0 and 2.1.

Result: The downloaded image is displayed in Gallery app properly without closing and re-opening the app.

Device: Flame 2.0 (319mb, KK, Full Flash)
BuildID: 20141105160207
Gaia: 5ee26701a4d8db266bfb203b2179f686ce14d8b6
Gecko: dbf49343e889
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1 (319mb, KK, Full Flash)
BuildID: 20141106001204
Gaia: 9658b93b412bdcc0f953d668e8c8e68318c99fb8
Gecko: 76880403db44
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Contact: croesch
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Verify passed, this issue can't be repro on Woodduck 2.0.
Attached: Verify_Woodduck_Figuredisplay.mp4
Reproducing rate: 0/5
Gaia-Rev        ee5cf148b4c546beea9bfb799d2a3ee62074957d
Gecko-Rev       73601b71861cbc2f180c4d2653cec3e9fbb39db5
Build-ID        20141114050313
Version         32.0
Comment on attachment 8501387 [details] [diff] [review]
Pt 2 - Have devicestorage notify all storage types based on extension. v2

>+    static const nsLiteralString kMediaTypes[] = {
nsLiteralString isn't a POD type, so creating static instances might not be ideal. (But I haven't looked at what compilers actually do here, so you might be OK.)
(In reply to neil@parkwaycc.co.uk from comment #49)
> Comment on attachment 8501387 [details] [diff] [review]
> Pt 2 - Have devicestorage notify all storage types based on extension. v2
> 
> >+    static const nsLiteralString kMediaTypes[] = {
> nsLiteralString isn't a POD type, so creating static instances might not be
> ideal. (But I haven't looked at what compilers actually do here, so you
> might be OK.)

Good point. I've filed bug 1157062 to rectify this.
Blocks: 1157062
You need to log in before you can comment on or make changes to this bug.