Last Comment Bug 313768 - Implement 'mediatype' file filters in <upload>
: Implement 'mediatype' file filters in <upload>
Status: RESOLVED FIXED
: fixed1.8.0.8, fixed1.8.1.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
:
Mentors:
http://www.w3.org/TR/2006/REC-xforms-...
Depends on:
Blocks: 322255 326372 326373
  Show dependency treegraph
 
Reported: 2005-10-25 10:10 PDT by jhp (no longer active)
Modified: 2016-07-15 14:46 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase - taken from testuite 8.1.6.a (1.35 KB, application/xhtml+xml)
2006-07-24 11:44 PDT, Steve Speicher
no flags Details
Handle mediatype attribute (2.67 KB, patch)
2006-08-10 17:39 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
patch (2.67 KB, patch)
2006-08-15 11:52 PDT, Merle Sterling
bugs: review+
Details | Diff | Splinter Review
testcase - corrected (1.35 KB, application/xhtml+xml)
2006-08-21 10:25 PDT, aaronr
no flags Details

Description jhp (no longer active) 2005-10-25 10:10:53 PDT
Need to implement support for the 'mediatype' attribute on the <upload> element.
Comment 1 aaronr 2006-01-26 14:29:04 PST
Javier suggests that http://lxr.mozilla.org/seamonkey/source/netwerk/mime/public/nsIMIMEService.idl will be a valuable interface to understand in order to implement this in case he isn't the one that eventually does the work.
Comment 2 Steve Speicher 2006-03-20 11:48:48 PST
blocks 1.0 2nd ed test case: 8.1.6.a
this blocks bug 322255 though I don't have authority to update the bug
Comment 3 Steve Speicher 2006-07-24 11:44:36 PDT
Created attachment 230467 [details]
testcase - taken from testuite 8.1.6.a
Comment 4 Merle Sterling 2006-08-10 17:39:36 PDT
Created attachment 233187 [details] [diff] [review]
Handle mediatype attribute

If the mediatype attribute is present we parse the space delimited list of mime types, map the mime types to file extensions using nsIMIMEService, and build a file extension filter string for the file open dialog.

Note that the test case has a typo: image/jpg must be image/jpeg
Comment 5 alexander :surkov 2006-08-10 19:06:39 PDT
(In reply to comment #4)
> Created an attachment (id=233187) [edit]
> Handle mediatype attribute
> 
> If the mediatype attribute is present we parse the space delimited list of mime
> types, map the mime types to file extensions using nsIMIMEService, and build a
> file extension filter string for the file open dialog.
> 
> Note that the test case has a typo: image/jpg must be image/jpeg
> 

I'd propose to wait for the bug 347327 since your patch ingores mediatype element but mentioned bug adds its support. Later you can see mozType:mediatype attribute on upload element (taking into account I should do a little modification in my patch to support upload :)). What do you think?

Comment 6 alexander :surkov 2006-08-10 20:19:57 PDT
I guess @mediatype gives a hint how to render upload control, i.e. we should have several xf:upload bindings for different types of @mediatype. Do we want to have this? If true then I guess we should post new bug since current bug is not about it. Right?
Comment 7 aaronr 2006-08-11 09:38:31 PDT
(In reply to comment #6)
> I guess @mediatype gives a hint how to render upload control, i.e. we should
> have several xf:upload bindings for different types of @mediatype. Do we want
> to have this? If true then I guess we should post new bug since current bug is
> not about it. Right?
> 

I can't think of any examples where the control would be different based on the mime type.  Do you have an idea of which ones would be different?
Comment 8 Merle Sterling 2006-08-11 09:49:37 PDT
(In reply to comment #5)
> I'd propose to wait for the bug 347327 since your patch ingores mediatype
> element but mentioned bug adds its support. Later you can see mozType:mediatype
> attribute on upload element (taking into account I should do a little
> modification in my patch to support upload :)). What do you think?

I think it could go either way.  I could wait for bug 347327 to be complete and then change the two lines of code in my patch that simply gets the mediatype attribute to check for the mediatype element instead OR we can check in this bug and the change could be made as part of bug 347327.  Given that this bug fixes many of the failing test cases in the test suite, I would prefer the latter.
Comment 9 aaronr 2006-08-11 10:04:09 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > I'd propose to wait for the bug 347327 since your patch ingores mediatype
> > element but mentioned bug adds its support. Later you can see mozType:mediatype
> > attribute on upload element (taking into account I should do a little
> > modification in my patch to support upload :)). What do you think?
> 
> I think it could go either way.  I could wait for bug 347327 to be complete and
> then change the two lines of code in my patch that simply gets the mediatype
> attribute to check for the mediatype element instead OR we can check in this
> bug and the change could be made as part of bug 347327.  Given that this bug
> fixes many of the failing test cases in the test suite, I would prefer the
> latter.
> 

I'd vote that this bug wait for 347327, which is already in the review process.
Comment 10 alexander :surkov 2006-08-11 10:25:19 PDT
(In reply to comment #7)

> I can't think of any examples where the control would be different based on the
> mime type.  Do you have an idea of which ones would be different?
> 

Nothing special, I'm just looking at specs (http://www.w3.org/TR/xforms/index-all.html#ui-upload). Here there is upload's picture for mediatype="image/*". I don't know do we need something like this? At least the showed upload control looks friendly I guess.
Comment 11 Steve Speicher 2006-08-14 08:16:05 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > I'd propose to wait for the bug 347327 since your patch ingores mediatype
> > > element but mentioned bug adds its support. Later you can see mozType:mediatype
> > > attribute on upload element (taking into account I should do a little
> > > modification in my patch to support upload :)). What do you think?
> > 
> > I think it could go either way.  I could wait for bug 347327 to be complete and
> > then change the two lines of code in my patch that simply gets the mediatype
> > attribute to check for the mediatype element instead OR we can check in this
> > bug and the change could be made as part of bug 347327.  Given that this bug
> > fixes many of the failing test cases in the test suite, I would prefer the
> > latter.
> > 
> 
> I'd vote that this bug wait for 347327, which is already in the review process.
> 

Perhaps I'm missing something, bug 347327 deals with mediatype element on output element.  This bug deals with mediatype attribute on the upload element.  Where is the conflict?
Comment 12 Steve Speicher 2006-08-14 08:18:05 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=233187) [edit]
> > Handle mediatype attribute
> > 
> > If the mediatype attribute is present we parse the space delimited list of mime
> > types, map the mime types to file extensions using nsIMIMEService, and build a
> > file extension filter string for the file open dialog.
> > 
> > Note that the test case has a typo: image/jpg must be image/jpeg
> > 
> 
> I'd propose to wait for the bug 347327 since your patch ingores mediatype
> element but mentioned bug adds its support. Later you can see mozType:mediatype
> attribute on upload element (taking into account I should do a little
> modification in my patch to support upload :)). What do you think?
> 

Well, upload's mediatype element has been supported for sometime.  This bug doesn't deal with that node.

See http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsUploadElement.cpp#342
Comment 13 alexander :surkov 2006-08-14 08:42:36 PDT
(In reply to comment #11)

> Perhaps I'm missing something, bug 347327 deals with mediatype element on
> output element.

Not only, bug 347327 deals with @mediatype attribute too ;)

>  This bug deals with mediatype attribute on the upload element.
>  Where is the conflict?
> 

When we deal with some mediatype issues then we should take into account attribute and element I guess since mediatype can be specified by @mediatype attribute or by mediatype element both.
Comment 14 aaronr 2006-08-14 17:06:29 PDT
The more I look at this, the more I realize that there is no overlap as far as 1.0 goes.  In XForms 1.0, xf:mediatype has no affect on xf:upload (though xf:upload does affect xf:mediatype's bound value).  In XForms 1.1, xf:mediatype affects the control that contains it.  While in XForms 1.1 or 1.2 there might possibly be overlap (an upload without @mediatype might be able to inherit this from a contained xf:mediatype in the future), there isn't yet.  So let's keep mediatype as it pertains to upload and mediatype as it pertains to output seperate.  We can merge them together if a future XForms spec changes that seperation.
Comment 15 aaronr 2006-08-14 18:33:06 PDT
Comment on attachment 233187 [details] [diff] [review]
Handle mediatype attribute

>Index: nsXFormsUploadElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 nsXFormsUploadElement.cpp
>--- nsXFormsUploadElement.cpp	10 Aug 2006 21:41:25 -0000	1.18
>+++ nsXFormsUploadElement.cpp	11 Aug 2006 00:34:15 -0000
>@@ -202,19 +202,65 @@ nsXFormsUploadElement::PickFile()
>   nsCOMPtr<nsIFilePicker> filePicker =
>             do_CreateInstance("@mozilla.org/filepicker;1");
>   if (!filePicker)
>     return NS_ERROR_FAILURE;
> 
>   rv = filePicker->Init(internal, filepickerTitle, nsIFilePicker::modeOpen);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // set filter "All Files"
>-  // XXX implement "@mediatype" file filters
>-  filePicker->AppendFilters(nsIFilePicker::filterAll);
>+  // Set the file picker filters based on the mediatype attribute.
>+  nsAutoString mediaType;
>+  mElement->GetAttribute(NS_LITERAL_STRING("mediatype"), mediaType);
>+
>+  if (mediaType.IsEmpty()) {
>+    filePicker->AppendFilters(nsIFilePicker::filterAll);
>+  }
>+  else {

nit: else goes on line above to keep style consistent

>+    // The mediatype attribute contains a space delimited list of mime types.
>+    nsresult rv;
>+    nsCOMPtr<nsIMIMEService> mimeService =
>+      do_GetService("@mozilla.org/mime;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsAString::const_iterator start, end, iter;
>+    mediaType.BeginReading(start);
>+    mediaType.BeginReading(iter);
>+    mediaType.EndReading(end);
>+
>+    nsAutoString fileFilter;
>+    nsAutoString mimeType;
>+    while (iter != end) {
>+      if (FindCharInReadable(' ', iter, end)) {
>+         mimeType = Substring(start, iter);
>+         // Skip the space.
>+         ++iter;
>+         // Save the starting position for the next mime type (if any).
>+         start = iter;
>+      }
>+      else {
>+        // Only 1 media type or we've reached the end of the list.
>+        mimeType = Substring(start, end);
>+      }
>+
>+      // Map the mime type to a file extension and add the extension to the file
>+      // type filter for the file dialog.
>+      nsCAutoString fileExtension;
>+      rv = mimeService->GetPrimaryExtension(NS_ConvertUTF16toUTF8(mimeType),
>+                                            EmptyCString(), fileExtension);
>+      if (NS_SUCCEEDED(rv) && !fileExtension.IsEmpty()) {
>+        fileFilter.AppendLiteral("*.");
>+        fileFilter.Append(NS_ConvertUTF8toUTF16(fileExtension));
>+        fileFilter.AppendLiteral(";");
>+      }
>+    }
>+
>+    // Append the file extension filter.
>+    filePicker->AppendFilter(fileFilter, fileFilter);
>+  }

I'm thinking maybe the file filter should be 'all files' if we don't find any mime types.  The file picker shows all the files anyhow and it just looks weird if the "files of type" field is blank.

With those, r=me
Comment 16 Merle Sterling 2006-08-15 11:52:49 PDT
Created attachment 233818 [details] [diff] [review]
patch

Always add 'All Files' as a filter.  Do not append the semicolon to the last filter in the list.
Comment 17 aaronr 2006-08-21 10:25:19 PDT
Created attachment 234798 [details]
testcase - corrected

corrected testcase per merle's suggestion
Comment 18 aaronr 2006-08-21 11:52:20 PDT
checked into trunk for msterlin
Comment 19 aaronr 2006-10-17 14:25:26 PDT
checked into 1.8.0 branch on 2006/09/21
Comment 20 aaronr 2007-01-11 15:52:04 PST
checked into 1.8 branch on 2006/11/21

Note You need to log in before you can comment on or make changes to this bug.