Closed Bug 786922 Opened 7 years ago Closed 7 years ago

Device Storage - use a properties file instead of the mime service

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Whiteboard: [WebAPI:P1])

Attachments

(2 files, 2 obsolete files)

We should use a properties file instead of the mime service.  One to model after is:

chrome://global/content/filepicker.properties
Assignee: nobody → doug.turner
Attached patch properties fileSplinter Review
david, can you verify these are the mime types you'd like to support on gaia?
Attachment #658218 - Flags: review?(dflanagan)
Attached patch patch v.1 (obsolete) — Splinter Review
bent, this does a few things:

1) Instead of using the MIME service to determine if a blob is something that can be stored or not.

2) I changed AddNamed() to check to see if both the file path argument as well as the mime type on the blob are expected.  For example, we should prevent people from storing text/plain files as images in the device storage object for 'pictures'.

3) I more correctly implemented Add().  We now try to determine the file extension based on the MIME type of the blob.  Sadly, our internal db of mime types is really weak and we'll fail on a number of mime types.  See bug 788273.  For now, gaia callers should/are using AddNamed().

4) I added two new tests.  One to test the new Add() implementation.  Another to test to ensure we can't pass the wrong mimetype or file extension.

5) I removed some debugging code.
Attachment #658224 - Flags: review?(bent.mozilla)
OK - commenting on the correct bug this time.

I was expecting that the sample files (which are .ogg) should be recognized as well as the .mp3 files.

After applying this patch, I was able to see my mp3 files.
okay -- i'll add .ogg to the music section.
Duplicate of this bug: 788101
Comment on attachment 658224 [details] [diff] [review]
patch v.1

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

::: dom/devicestorage/DeviceStorage.h
@@ +49,5 @@
>                               uint8_t aArgc, 
>                               bool aEditable, 
>                               nsIDOMDeviceStorageCursor** aRetval);
>  
> +  bool IsMimeTypeCorrectForStorageType(nsAString& aType, nsIDOMBlob* aBlob);

Nit: Can this be static?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +151,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  // String bundles are cached by the bundle service.
> +  nsCOMPtr<nsIStringBundleService> stringService = mozilla::services::GetStringBundleService();

Nit: I can't remember if you're trying to hold to 80 chars in your files but this looks long.

@@ +157,1 @@
>      return false;

Nit: A NS_WARNING about what went wrong would be nice here and the other times you bail below.

@@ +157,5 @@
>      return false;
>    }
>  
> +  nsCOMPtr<nsIStringBundle> filterBundle;
> +  if (NS_FAILED(stringService->CreateBundle("chrome://global/content/devicestorage.properties",

Nit: This should probably be a #define at the top so it's easily tweakable.

@@ +172,3 @@
>    }
>  
> +  nsString extensionMatch;

nsAutoString to avoid multiple mallocs.

@@ +176,5 @@
> +  extensionMatch.Append(Substring(path, dotIdx));
> +  extensionMatch.AppendASCII(";");
> +
> +  nsString extensionListStr;
> +  if (NS_FAILED(filterBundle->GetStringFromName(nsString(aType).get(), getter_Copies(extensionListStr)))) {

Nit: Instead of |nsString(aType).get()| just use |aType.BeginReading()|

@@ +182,3 @@
>    }
>  
> +  if (!FindInReadable(extensionMatch, extensionListStr)) {

Nit: looks like you could just do |return FindInReadable(...)|

@@ +1544,5 @@
> +bool
> +nsDOMDeviceStorage::IsMimeTypeCorrectForStorageType(nsAString& aType, nsIDOMBlob* aBlob)
> +{
> +  if (!aBlob) {
> +    return false;

For utility functions like this wouldn't it be better to just assert aBlob instead?

@@ +1547,5 @@
> +  if (!aBlob) {
> +    return false;
> +  }
> +
> +  nsString type(aType);

Nit: unused.

@@ +1550,5 @@
> +
> +  nsString type(aType);
> +
> +  nsString mimeType;
> +  aBlob->GetType(mimeType);

Looks like certain blob types can fail here. Need to check.

@@ +1576,5 @@
> +  }
> +
> +  nsCOMPtr<nsIMIMEService> mimeSvc = do_GetService(NS_MIMESERVICE_CONTRACTID);
> +  if (!mimeSvc) {
> +    return NS_OK;

This should throw, otherwise a page may think it stored something that it didn't.

@@ +1580,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsString mimeType;
> +  aBlob->GetType(mimeType);

This can fail.

@@ +1582,5 @@
> +
> +  nsString mimeType;
> +  aBlob->GetType(mimeType);
> +
> +  nsAutoCString extension;

Nit: nsCString, you don't need the stack storage here.

@@ +1583,5 @@
> +  nsString mimeType;
> +  aBlob->GetType(mimeType);
> +
> +  nsAutoCString extension;
> +  mimeSvc->GetPrimaryExtension(NS_LossyConvertUTF16toASCII(mimeType), EmptyCString(), extension);

This makes some OS calls and such, so please check for failure here.

@@ +1592,3 @@
>    // possible race here w/ unique filename
>    char buffer[128];
>    NS_MakeRandomString(buffer, 128);

Nit: ArrayLength(buffer) rather than hardcoding 128 there to be safe.

@@ +1592,5 @@
>    // possible race here w/ unique filename
>    char buffer[128];
>    NS_MakeRandomString(buffer, 128);
>  
> +  nsCString path;

nsAutoCString here.

@@ +1600,4 @@
>  
> +  nsString p;
> +  CopyASCIItoUTF16(path, p);
> +  return AddNamed(aBlob, p, _retval);

Nit: You could just avoid the temporary and do |return AddNamed(aBlob, NS_ConvertASCIItoUTF16(path), _retval);|
> > +  aBlob->GetType(mimeType);
> This can fail.

This is okay.  It will fall through to AddNamed which will post an onerror event.  I added a comment.

> This makes some OS calls and such, so please check for failure here.

Same with this.  (which has a comment)
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #658224 - Attachment is obsolete: true
Attachment #658224 - Flags: review?(bent.mozilla)
Attachment #658530 - Flags: review?(bent.mozilla)
Comment on attachment 658530 [details] [diff] [review]
patch v.2

damn tabs.
Attachment #658530 - Flags: review?(bent.mozilla) → review-
Comment on attachment 658218 [details] [diff] [review]
properties file

music needs .ogg as dhylands pointed out.

The music app now also supports AAC files. Wikipedia lists these as AAC extensions: .m4a, .m4b, .m4p, .m4v, .m4r, .3gp, .mp4, .aac (see http://en.wikipedia.org/wiki/Advanced_Audio_Coding).  Some of those extensions overlap with video extensions.  I don't know if that is a problem or not.

In general, music and video formats are completely outside my area of expertise, though.  Dominic Kuo is in charge of the Music app.  Maybe someone who does media work could advise you on the set of extensions for media that gecko can actually play.

However, it would be great to get this landed (with tweaks made as necessary in followup bugs) so r=me if you add .ogg and at least the AAC extension itunes uses.
Attachment #658218 - Flags: review?(dflanagan) → review+
Attached patch patch v.2Splinter Review
Attachment #658530 - Attachment is obsolete: true
Attachment #658532 - Flags: review?(bent.mozilla)
Comment on attachment 658532 [details] [diff] [review]
patch v.2

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

I still think that not reporting failures eagerly will mean less intelligible errors for web devs, but since the only thing they could do is file a bug anyway I guess I won't insist.
Attachment #658532 - Flags: review?(bent.mozilla) → review+
Being able to reliably load music is a v1 blocker.
blocking-basecamp: --- → +
Backed out for orange

https://hg.mozilla.org/mozilla-central/rev/fd4d9c386f97
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ae4a4bfde41e
https://hg.mozilla.org/mozilla-central/rev/ea472ffba523
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 789008
Backed out for causing bug 789008 on the push it landed & frequently thereafter:
https://hg.mozilla.org/mozilla-central/rev/e5bffe88e184
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ed, 
Isn't this test failure just a dup of 774368?
Duplicate of this bug: 788339
Whiteboard: [WebAPI:P1]
https://hg.mozilla.org/mozilla-central/rev/96b314d423ab
https://hg.mozilla.org/mozilla-central/rev/748af2073517
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Hi Doug,
I've test it and it works fine on 9/10 build
9/10 build info:
gaia : f8343c79f20c4940027fc0d71df161499d6b0e96
mozilla-central : b191823ee5889e068b581aa29316780c11edc919

But it fail on my 9/11, 9/12 test
9/11 build info:
gaia : f8343c79f20c4940027fc0d71df161499d6b0e96
mozilla-central : b191823ee5889e068b581aa29316780c11edc919

9/12 build info:
gaia : fc4057f2bd3158c563ea06e9a5f0f5ea8e20dd8c
mozilla-central : 0ce1f32125257961232c65c7d6251314f9e0f2be

can you check it? thanks
QA Contact: jshih
I am waiting on bug 788612.  They should land together.
Depends on: 788612
You need to log in before you can comment on or make changes to this bug.