Closed Bug 275453 Opened 20 years ago Closed 19 years ago

Implement <xforms:upload>

Categories

(Core Graveyard :: XForms, enhancement)

Product:
Core Graveyard
Component:
XForms
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: darin.moz, Assigned: jhpedemonte)

References

(
URL
)

Details

(Keywords: fixed1.8)

Attachments

(3 files, 9 obsolete files)

v1 patch (checked in)
28.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Testcase
2.31 KB, application/xhtml+xml
Details
second patch v1.8
44.10 KB, patch
allan
: review+
aaronr
: review+
Details | Diff | Splinter Review 
Implement <xforms:upload>
OK, here's a v1 patch that implements partial support for <xforms:upload>. 
There's currently no support for the mediatype attribute or the <filename> and
<mediatype> child elements.

Care is taken to not allow script to modify the path of the file to be
uploaded.  A content property is used to store the nsILocalFile to be uploaded.


I think it might be useful to land this patch as is, and then go back and
complete the rest of the features of <xforms:upload>.
Attachment #169246 - Flags: review?(smaug)
Comment on attachment 169246 [details] [diff] [review]
v1 patch (checked in)

This looks ok for the
initial patch. But add comments about missing features. 
And don't mark this bug fixed ;)
Attachment #169246 - Flags: review?(smaug) → review+
v1 patch checked in on trunk.  leaving this bug open for further work.
Attached patch second patch (obsolete) — — Splinter Review 
Work-in-progress, with some minor bugs still left to work out.

This patch XBLizes <upload>, making it extend "xformswidget-input".  Sets
<filename> and <mediatype> depending on the file chosen in the file picker.  I
made the "mediatype" attribute on <upload> use <html:input>'s "accept"
attribute; unfortunately, that isn't implemented yet (bug 83749), but if we
want to fix it, the work should take place in that bug, so both HTML and XForms
can make use of it.

This patch doesn't have support for "base64Binary" or "hexBinary".

I am using |EvaluateNodeBinding()| in order to get the bindings for the
<filename> and <mediatype> child elements.  If it looks like this, |<filename
ref="@name">|, and the instance data node has an attribute called "name", then
my code works just as expected.  However, if the bound instance data node
doesn't have an attribute by that name, then |EvaluateNodeBinding()| returns an
empty list.  If I understand correctly, then a non-existant attribute on an
existing node should be created when setting, but if we bind directly to a node
(not its attr) and it doesn't exist, then we shouldn't create the instance node
(unless we are lazy authoring).  Correct?  So how do I go about creating the
attribute if it doesn't exist?
Assignee: darin → jhpedemonte
Status: NEW → ASSIGNED
Attachment #169246 - Attachment description: v1 patch → v1 patch (checked in)
(In reply to comment #4)
>...if the bound instance data node doesn't have an attribute by that name, then
>|EvaluateNodeBinding()| returns an  empty list.  If I understand correctly, then
>a non-existant attribute on an existing node should be created when setting, but
>if we bind directly to a node (not its attr) and it doesn't exist, then we
>shouldn't create the instance node (unless we are lazy authoring).  Correct?  So
>how do I go about creating the attribute if it doesn't exist?

In general (except as you point out for 'lazy authoring'), in XForms, nodes must
exist before they can be bound to.  If the bound node does not exist, then the
form control is treated as not relevant.  The case of attributes in the child
element semi-widgets of upload raises an interesting question, which has perhaps
already been answered in the XForms WG and you may know more than I do, but my
naive assumption would be that the filename or mediatype would fail to bind, and
either the entire upload would be not relevant, or the filename / mediatype
would silently fail.  As an author, I'd prefer the former, it's difficult to
debug the latter, but absent any WG opinion, it's dealer's choice.



The magic 8 ball says filename and mediatype are not form controls and so in the
case of failed @ref of single-node-binding attributes, the filename or mediatype
element is ignored and the information is not put into the instance, but other
operation of the upload is unaffected (in particular, the relevant property is
not affected).  In the case of failed @model or @bind, an error is signalled, as
described in 3.2.3 single-node binding attributes.
Attached patch second patch v1.1 (obsolete) — — Splinter Review 
Still work-in-progress.  This patch fixes several bugs, as well as adding
base64 & hex encoding.	Hex encoding doesn't yet validate, but Doron promised
to write that code!  Have several questions, though:

- From the spec: "... on activation upload places a URI in the content of the
node."	Currently, this code runs on "onblur", which isn't correct.  My
interpretation of "on activation" is: (1) if the user clicks the "Browse"
button, then the file is handled when the filepicker returns, and (2) if the
user types in the file path, then the file is handled when focus leaves the
text field ("onblur"; should it also work on keypress=Enter?).	I thought of
using "onchange" on the <html:input>, but that gets called too often to be
useful.  So I was leaning towards implementing my own textfield and button
(rather than depending on <html:input type="text">), and assigning separate
functions to each.  Anyone have a better idea?

- From spec 8.3.1 <filename>: "For security reasons, upload must not take
action due to any existing value of the node."	I assume this means that the
<upload> code should treat the <filename> node as write-only.  Correct?
Attachment #195191 - Attachment is obsolete: true
(In reply to comment #7)
> Created an attachment (id=195870) [edit]
> second patch v1.1

Hmm, you expose a "file" attribute, which stores the file content on the node.
What is preventing someone using script to set this to some arbitry file?

Why is it called UpdateElement btw.? Do you have bigger plans for it? If not,
please call it Upload...

> Still work-in-progress.  This patch fixes several bugs, as well as adding
> base64 & hex encoding. Hex encoding doesn't yet validate, but Doron promised
> to write that code!

Is there a bug on that? If not, please create it.

Have several questions, though:
> 
> - From the spec: "... on activation upload places a URI in the content of the
> node."	Currently, this code runs on "onblur", which isn't correct. My
> interpretation of "on activation" is: (1) if the user clicks the "Browse"
> button, then the file is handled when the filepicker returns, and (2) if the
> user types in the file path, then the file is handled when focus leaves the
> text field ("onblur"; should it also work on keypress=Enter?).	I thought of
> using "onchange" on the <html:input>, but that gets called too often to be
> useful.  So I was leaning towards implementing my own textfield and button
> (rather than depending on <html:input type="text">), and assigning separate
> functions to each.  Anyone have a better idea?

Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll
write a message to the WG. But with the current wording, onblur is wrong, yes.

> - From spec 8.3.1 <filename>: "For security reasons, upload must not take
> action due to any existing value of the node."	I assume this means that the
> <upload> code should treat the <filename> node as write-only.  Correct?

Something like that, yes.
The spec also states: "Implementations which cannot support upload for the given
mediatype must make this apparent to the user."

We need to handle that. Will you include that Javier, or create a seperate bug?
(In reply to comment #8)
> Hmm, you expose a "file" attribute, which stores the file content on the node.
> What is preventing someone using script to set this to some arbitry file?

According to Doron, doing QI in JS is a priviledged operation, and would result
in a security error in non-chrome JS.  So it shouldn't be a problem.  I'll try
to create a testcase to check this out, though.

> Why is it called UpdateElement btw.? 

Ha!  Every so often I caught myself writing "update" instead of "upload".  Guess
I let this one slip through.  Thanks...

> Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll
> write a message to the WG. But with the current wording, onblur is wrong, yes.

Well, I got "onchange" to work.  I guess I was doing something wrong the first
time that I evaluated it.  But now, it's working much better.

(In reply to comment #9)
> The spec also states: "Implementations which cannot support upload for the given
> mediatype must make this apparent to the user."
> We need to handle that. Will you include that Javier, or create a seperate bug?

Well, at the very least, we'll allow the user to upload a file that matches that
mediatype, so there's nothing for us to warn the user about.  So, for example,
if the mediatype is set to "audio/*", one implementation may choose to allow the
user to open a recording dialog, whereas we'll just let the user upload a file
matching that mimetype.  Are you interpreting that line differently?
(In reply to comment #10)
> (In reply to comment #8)
> > Hmm, you expose a "file" attribute, which stores the file content on the node.
> > What is preventing someone using script to set this to some arbitry file?
> 
> According to Doron, doing QI in JS is a priviledged operation, and would result
> in a security error in non-chrome JS.  So it shouldn't be a problem.

Forgot the non-automatic QI.

> I'll try to create a testcase to check this out, though.

Yes please. We'll better be sure :-)

> > Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll
> > write a message to the WG. But with the current wording, onblur is wrong, yes.
> 
> Well, I got "onchange" to work.  I guess I was doing something wrong the first
> time that I evaluated it.  But now, it's working much better.

I still thinks the "on activation" is a bit vague. But, when you have a patch
ready I'll evaluated the UI "experience".
 
> (In reply to comment #9)
> > The spec also states: "Implementations which cannot support upload for the given
> > mediatype must make this apparent to the user."
> > We need to handle that. Will you include that Javier, or create a seperate bug?
> 
> Well, at the very least, we'll allow the user to upload a file that matches that
> mediatype, so there's nothing for us to warn the user about.  So, for example,
> if the mediatype is set to "audio/*", one implementation may choose to allow the
> user to open a recording dialog, whereas we'll just let the user upload a file
> matching that mimetype.  Are you interpreting that line differently?

I was referring to the setting of the mediatype element, but I did not notice
the "and a mediatype is available" the first time. Just forget my comment.
(In reply to comment #7)
> - From spec 8.3.1 <filename>: "For security reasons, upload must not take
> action due to any existing value of the node."	I assume this means that the
> <upload> code should treat the <filename> node as write-only.  Correct?
Yes.  Presumably it also excludes reading the mediatype binding as well.
The language is written broadly to encourage the XForms processor developer to
think broadly about the security aspects of this control, which is what's
happening in this bug, so it's working ;-)

As for medaitype, there's a mediatype attribute on upload.  For security
reasons, there's no corresponding way to suggest filenames, just as you suspect. 


> I still thinks the "on activation" is a bit vague. But, when you have a patch
> ready I'll evaluated the UI "experience".

The only help I can offer is
http://www.w3.org/TR/xforms/slice4.html#sequence-for-input which describes input
event sequencing though you may wish to check for published and unpublished errata.
Attached patch second patch v1.2 (obsolete) — — Splinter Review 
Ready for review.  This patch has the following changes:
- Using the "onchange" attribute on <html:input> in order to kick of the file
path processing.
- Moved the <upload> element initialization code form the "refresh" method to
the "constructor".
- So Allan was right: a Javascript on the webpage can QI and set the "file"
attribute.  So I changed that to a "fileUpdated" method, that lets the element
know when the user has changed the file path.  I also exposed a getter, so I
can retrieve the value from the <html:input> element.
- 'Multipart-post' was broken in several places, but it now works.  One of the
things I did was make |ParseTypeFromNode()| return the actual namespace URI,
rather than just returning the prefix used.
- Fixed a crash in the submission code: we weren't checking the return value of
|SerializeDataXML()|.

I looked over the spec one final time, and I think I've covered everything. 
But as mentioned earlier, for the "mediatype" attribute on <upload>, I am
making use of <html:input>'s "accept" attr, which hasn't been implemented, yet.
Attachment #195870 - Attachment is obsolete: true
Attachment #196249 - Flags: review?(allan)
Attachment #196249 - Flags: review?(aaronr)
(In reply to comment #14)
> Created an attachment (id=196249) [edit]
> second patch v1.2

Please correct the nits:
http://lab.cph.novell.com/~abeaufour/jst-review/?patch=196249

I'll start looking at the rest now. Sorry for the delay.
Comment on attachment 196249 [details] [diff] [review]
second patch v1.2

> Index: nsIXFormsInputUIElement.idl
> ===================================================================
> +interface nsIDOMElement;
> +
> +/**
> + * Interface implemented by the XBL part of the input element.
> + */
> +[scriptable, uuid(82c82c46-f111-416c-a3e6-ce9a0864d00f)]
> +interface nsIXFormsInputUIElement : nsISupports
> +{
> +  /**
> +   * Returns the underlying <html:input> element.
> +   */
> +  readonly attribute nsIDOMElement inputField;
> +};

> Index: nsIXFormsUploadElement.idl
> ===================================================================
> +[scriptable, uuid(6dc00fd0-674c-41b3-8b0b-ba6c02aa769a)]
> +interface nsIXFormsUploadElement : nsISupports
> +{
> +  /**
> +   * Let the upload element know that the user has selected or entered
> +   * a file path.
> +   */
> +  void fileUpdated();
> +};

An "evil" upload control could return whatever as the inputField (ie, an
input control with the value always set to /etc/passwd...)

This leads to something I should have mentioned some time ago (sorry, I forgot
about it). It should not be possible to "skin" an upload control. As far as I
can see this will never be safe. The file name should never come from a
control/code that is user-changeable.

This means that the upload should either be done in XTF, or (if it's possible)
only be bindable to chrome content.

I think you should split the submission fixes, etc. to another bug and only
have "pure upload" stuff in this one. It is crucial that we get the upload
part right.
Attachment #196249 - Flags: review?(allan) → review-
Comment on attachment 196249 [details] [diff] [review]
second patch v1.2

will review when allan's comments are addressed.
Attachment #196249 - Flags: review?(aaronr)
(In reply to comment #16)
> I think you should split the submission fixes, etc. to another bug and only
> have "pure upload" stuff in this one.

-> bug 309546
(In reply to comment #16)
> This means that the upload should either be done in XTF...

I thought we wanted the controls to be XBL.  This prevents the out-of-order
issue that was fixed on other controls by going to XBL.  I was talking to Doron
about how best to provide security, but also have the control defined in XBL,
but didn't come up with anything definitive.  What's your take?

> ... or (if it's possible)
> only be bindable to chrome content.

Can you explain this a little better?

(In reply to comment #19)
> (In reply to comment #16)
> > This means that the upload should either be done in XTF...
> 
> I thought we wanted the controls to be XBL.  This prevents the out-of-order
> issue that was fixed on other controls by going to XBL.  I was talking to Doron
> about how best to provide security, but also have the control defined in XBL,
> but didn't come up with anything definitive.  What's your take?

My best shot at it right now: Let "file picking" happen in XTF and _only_ the UI
in XBL. That is, expose a .pickFile() command on the XTF upload element. So the
XBL binding can create "beautiful" buttons/icons, and then on activation they
just call .pickFile() which (in XTF) opens the file picker dialog, get's the
file name, stores the file, etc. etc.

Of course, we limit ourselves to picking files, but I'm fine with that for now.

> > ... or (if it's possible) only be bindable to chrome content.
> 
> Can you explain this a little better?

Assuming that chrome content is trusted, then if we can restrict bindings to the
upload control to chrome:// then we could implement everything in XBL.

Another note: You need to run the "value changed" sequence when you set
mediatype and filename, do you do that?
Blocks: 309546
(In reply to comment #20)
> My best shot at it right now: Let "file picking" happen in XTF and _only_ the UI
> in XBL.

What's good about the approach I took in the last patch is that I got to
piggyback on the <html:input type="file"> code.  In your approach, I'd have to
duplicate a lot of that code.  But we may have no choice...
Also, using Allan's approach, we would have to remove the ability for the user
to manually input the path of the file, since a script could change the input
field value.  So here are our choices:
1) Go along the lines of the existing upload code (returning the upload element
through |nsIXTFVisual::GetVisualContent()|, etc).  From what I understand,
scripts wouldn't have access to the <html:input>.  However, it wouldn't be
skinnable.
2) Go with Allan's suggestion of putting the UI in XBL, but the actual code in
XTF.  This has the advantage of being skinnable, but we would have to duplicate
some of the <html:input> code, and we would have to make the text field
readonly, for security reasons.  Or we could remove the text field altogether
and just have a button that brings up the filepicker.  This is what the other
XForms engines do.  Personally, I prefer keeping the text field; gives feedback
on which file was chosen.

What do you guys think?
Attached patch second patch v1.3 (obsolete) — — Splinter Review 
So this patch goes with Allan's suggestion.  Clicking the "Browse..." button
calls a method on the XTF side.  That method takes care of opening the file
picker and setting the file path/contents into the instance data.

As discussed above, I added a readonly text field that holds the path of the
selected file.	Since the user can no longer select the text and clear it out,
should we add a "Clear"/"Reset" button, which would remove the currently
selected file?

In this patch, the "Browse..." text of the button is hard coded in
'xforms.xml'.  How would I go about getting the localized string (available in
HtmlForm.properites)?

This patch does not handle mediatype filters on the filepicker.
Attachment #196249 - Attachment is obsolete: true
Attachment #197217 - Attachment description: patch v1.3 → second patch v1.3
I've been thinking about this some more.  The <html:input type="file"> element
already comes with some pretty good security.  A script cannot set its value,
and if it tries to convert a regular <html:input> to a 'type="file"', its value
gets reset.

So looking back at 'second patch v1.2' (attachment 196249 [details] [diff] [review]), in the function
|GetHTMLInputValue()|, once I've know I've got an nsIDOMHTMLInputElement, I need
to check to see if it is a 'type="file"'.

Now the question is, could a malicious script somehow fake that, and say that it
it 'type="file"', when it is really not?
(In reply to comment #24)
> I've been thinking about this some more.  The <html:input type="file"> element
> already comes with some pretty good security.  A script cannot set its value,
> and if it tries to convert a regular <html:input> to a 'type="file"', its value
> gets reset.
> 
> So looking back at 'second patch v1.2' (attachment 196249 [details] [diff] [review] [edit]), in the function
> |GetHTMLInputValue()|, once I've know I've got an nsIDOMHTMLInputElement, I need
> to check to see if it is a 'type="file"'.
> 
> Now the question is, could a malicious script somehow fake that, and say that it
> it 'type="file"', when it is really not?


If someone skins upload, how can you be sure it'll have an input field for you
to talk to?
> Now the question is, could a malicious script somehow fake that, and say that it
> it 'type="file"', when it is really not?

Probably.  Perhaps a QI to an interface would work, but that could be faked too.
 We do have a way of making sure we QI to an interface that isn't being spoofed,
NativeWrapper or such.  Asking on IRC would work.


(In reply to comment #25)
> If someone skins upload, how can you be sure it'll have an input field for you
> to talk to?

This should't matter.  When a user selects a file (by typing, or through
picker), the value of the element gets set, whether or not there is an input field.
Attached patch second patch v1.4 (obsolete) — — Splinter Review 
So from talking to Mozilla devs on IRC, it looks like there may still be
security issues with using <html:input>.  This patch, then, continues on the
last patch, where I created my own <upload> widget, rather than depending on
"xformswidget-input".  Changes:
- Added a "Clear" button and supporting code.
- Set localized button text from XTF, on |WidgetAttached()|.
- Cleaned up and merged some code.
- Properly send model update events.
Attachment #197217 - Attachment is obsolete: true
Attachment #197760 - Flags: review?(allan)
Attached file Spec. upload example (obsolete) — 
Upload example from the XForms specification
Comment on attachment 197760 [details] [diff] [review]
second patch v1.4

When I try the simple example from the spec. and pick a random file from my
disk, I get this:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Permission denied to get property Object.file' when calling
method: [nsIFilePicker::file]"	nsresult: "0x8057001e
(NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame ::
http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" 
data: no]
************************************************************
JavaScript error: , line 0: uncaught exception: [Exception... "'Permission
denied to get property Object.file' when calling method: [nsIFilePicker::file]"
 nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"	location: "JS frame ::
http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" 
data: no]
Attachment #197760 - Flags: review?(allan) → review-
Hmm, that's very strange.  It works fine for me on the latest trunk build.  Can
anyone else try out this patch?
Supporting @mediatype now that you have your own custom control and aren't
depending on xhtml input/@type='file' would be nice.  The request for mediatype
control over the file types presented pre-dates the creation of the XForms WG,
and the has been a hot button for years.  A simple mechanism would just use
default file extensions looked up from the MIME type list.
Yes, I was going to look into supporting 'mediatype' soon, but I wanted to get
this patch up and reviewed.  I'll either do that here in another patch, or open
a new bug.
(In reply to comment #31)
> Hmm, that's very strange.  It works fine for me on the latest trunk build.  Can
> anyone else try out this patch?

Works fine for me on a Windows trunk pull that I did last night.  Was yours a
branch build, Allan?  I assume Linux?
With the latest patch the test case doesn't do anything (Fedora 3).
I can open and select a file, but the data model is not updated.
I don't see the errors Allan has.
...or am I missing something here...
The problem with the attached testcase is that the <attachment> node has no data
type.  From the spec:

"Data Binding Restrictions: This form control can only be bound to datatypes
xsd:anyURI, xsd:base64Binary or xsd:hexBinary, or types derived by restriction
from these."

So the question is what should happen?  FormsPlayer updates the <filename> and
<mediatype> nodes, but does nothing with <attachment>.  The Novell plugin
disables the <upload> element altogether.

Aaron pointed out this sentence in 4.2.2:
"The binding expression is evaluated to ensure that it points to a node that
exists. If this is not the case then the form control should behave in the same
manner as if it had bound to a model item with the relevant model item property
resolved to false."

He says that the Data Binding Restriction should be stated more clearly, like
this last sentence.  Allan, can you bring this up with the WG, to ask what
should happen if <upload> is not bound to one of those datatypes?
Added a working testcase to the URL field at top of bug.
URL: https://bugzilla.mozilla.org/attachme...
Attached file Testcase — 
Ok, better testcase
Attachment #197838 - Attachment is obsolete: true
(In reply to comment #34)
> (In reply to comment #31)
> > Hmm, that's very strange.  It works fine for me on the latest trunk build.  Can
> > anyone else try out this patch?
> 
> Works fine for me on a Windows trunk pull that I did last night.  Was yours a
> branch build, Allan?  I assume Linux?

It was a Linux trunk build... a new build this morning worked. Weird.
(In reply to comment #36)
> The problem with the attached testcase is that the <attachment> node has no data
> type.  From the spec:
> 
> "Data Binding Restrictions: This form control can only be bound to datatypes
> xsd:anyURI, xsd:base64Binary or xsd:hexBinary, or types derived by restriction
> from these."
> 
> So the question is what should happen?  FormsPlayer updates the <filename> and
> <mediatype> nodes, but does nothing with <attachment>.  The Novell plugin
> disables the <upload> element altogether.
> 
> Aaron pointed out this sentence in 4.2.2:
> "The binding expression is evaluated to ensure that it points to a node that
> exists. If this is not the case then the form control should behave in the same
> manner as if it had bound to a model item with the relevant model item property
> resolved to false."
> 
> He says that the Data Binding Restriction should be stated more clearly, like
> this last sentence.  Allan, can you bring this up with the WG, to ask what
> should happen if <upload> is not bound to one of those datatypes?

Hmmm, well it's not just upload, f.x. range also has type restrictions. I think
the Novell plugin does the right thing. It should be altogether disabled.
Comment on attachment 197760 [details] [diff] [review]
second patch v1.4

* Could you include a note somewhere about why "@incremental" is ignored?

* Include an XXX about mediatype filtering and a new bug?

> Index: nsIXFormsUploadElement.idl
> ===================================================================

Please include a comment for the interface

> +
> +
> +[scriptable, uuid(6dc00fd0-674c-41b3-8b0b-ba6c02aa769a)]
> +interface nsIXFormsUploadElement : nsISupports
> +{
> +  /**
> +   * Tells <upload> element to display file picker dialog so user can
> +   * select a file.
> +   */
> +  void pickFile();

Please use \<upload\>, that will make Doxygen oh so happy :) Also in the rest
of the patch.

> +
> +  /**
> +   * Tells <upload> element to clear file.
> +   */
> +  void clearFile();

That's not the most descriptive comment. What does "clear" mean? "Clear" it
from the disk? :)

> Index: nsXFormsSubmissionElement.cpp
> ===================================================================

> -        nsILocalFile *file =
> -            NS_STATIC_CAST(nsILocalFile *,
> +        nsIFile *file =
> +            NS_STATIC_CAST(nsIFile *,
>                             content->GetProperty(nsXFormsAtoms::uploadFileProperty));

Why this change? Because we do not need the extra functionality in
nsILocalFile?


> Index: nsXFormsUploadElement.cpp
> ===================================================================
> +  /**
> +   * Sets the value of the instance data node bound to <filename> or
> +   * <mediatype> child elements.
> +   **/
> +  nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue,
> +                             PRBool* aChanged);

Comment not entirely correct, it can set an arbitrary node value, right? Which
makes me think that it should 1) use
nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to
nsXFormsUtils as a generic function.

> +
> +  /**
> +   * Read the contents of the file and encode in Base64 or Hex.  |aResult| must
> +   * be freed by nsMemory::Free().
> +   **/
> +  nsresult EncodeFileContents(nsIFile* aFile, nsEncodingType aType,
> +                              PRUnichar** aResult);
> +
> +  void BinaryToHex(const char* aBuffer, PRUint32 aCount,
> +                   PRUnichar** aHexString);
> +

> -NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsUploadElement,
> -                             nsXFormsControlStub,
> -                             nsIDOMFocusListener,
> -                             nsIDOMEventListener)
> +  nsString mFilePickerTitle;
> +  nsString mClearButtonText;

You do not use this do you?

> +
> +  nsCOMPtr<nsIDOMNode>  mFilenameNode;
> +  nsCOMPtr<nsIDOMNode>  mMediatypeNode;
> +};

> -// nsIXTFXMLVisual
> +NS_IMPL_ISUPPORTS_INHERITED1(nsXFormsUploadElement,
> +                             nsXFormsDelegateStub,
> +                             nsIXFormsUploadElement)

>  NS_IMETHODIMP
> -nsXFormsUploadElement::OnCreated(nsIXTFXMLVisualWrapper *aWrapper)
> +nsXFormsUploadElement::WidgetAttached()
>  {
> -  nsresult rv = nsXFormsControlStub::OnCreated(aWrapper);
> +  nsresult rv = nsXFormsDelegateStub::WidgetAttached();
>    NS_ENSURE_SUCCESS(rv, rv);

> -  // Our anonymous content structure will look like this:
> -  //
> -  // <label>                         (mLabel)
> -  //   <span/>                       (insertion point)
> -  //   <input/>                      (mInput)
> -  // </label>
> -
> -  nsCOMPtr<nsIDOMDocument> domDoc;
> -  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> -
> -  domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
> -                          NS_LITERAL_STRING("label"),
> -                          getter_AddRefs(mLabel));
> -  NS_ENSURE_STATE(mLabel);
> -
> -  nsCOMPtr<nsIDOMElement> element;
> -  nsCOMPtr<nsIDOMNode> childReturn;
> -
> -  domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
> -                          NS_LITERAL_STRING("span"),
> -                          getter_AddRefs(element));
> -  NS_ENSURE_STATE(element);
> -
> -  mLabel->AppendChild(element, getter_AddRefs(childReturn));
> -
> -  domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
> -                          NS_LITERAL_STRING("input"),
> -                          getter_AddRefs(element));
> -  mInput = do_QueryInterface(element);
> -  NS_ENSURE_STATE(mInput);
> -
> -  mInput->SetType(NS_LITERAL_STRING("file"));
> +  // get string bundle
> +  nsCOMPtr<nsIStringBundleService> bundleService =
> +            do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIStringBundle> bundle;
> +  rv = bundleService->CreateBundle(NS_HTMLFORM_BUNDLE_URL,
> +                                   getter_AddRefs(bundle));
> +  NS_ENSURE_SUCCESS(rv, rv);

> -  mLabel->AppendChild(mInput, getter_AddRefs(childReturn));
> +  // get localized "Browse..." text and set into button
> +  nsXPIDLString value;
> +  rv = bundle->GetStringFromName(NS_LITERAL_STRING("Browse").get(),
> +                                 getter_Copies(value));
> +  if (NS_FAILED(rv)) {
> +    // fall back to English text
> +    value.AssignASCII("Browse...");
> +  }
> +  nsCOMPtr<nsIXFormsUploadUIElement> uiUpload = do_QueryInterface(mElement);
> +  if (uiUpload) {
> +    uiUpload->SetBrowseButtonText(value);
> +  }

Get mFilePickerTitle before this QI, and then exit if the QI fails. No reason
to fetch strings if you cannot set them.

> -  // We can't use xtf handleDefault here because editor stops blur events from
> -  // bubbling, and we can't use a system event group handler because blur
> -  // events don't go to the system event group (bug 263240).  So, just install
> -  // a bubbling listener in the normal event group.  This works out because
> -  // content can't get at the anonymous content to install an event handler,
> -  // and the event doesn't bubble up.
> +  // get localized file picker title text
> +  rv = bundle->GetStringFromName(NS_LITERAL_STRING("FileUpload").get(),
> +                                 getter_Copies(value));
> +  if (NS_SUCCEEDED(rv)) {
> +    mFilePickerTitle.Assign(value);
> +  } else {
> +    // fall back to English text
> +    mFilePickerTitle.AssignASCII("File Upload");
> +  }

> -  nsCOMPtr<nsIDOMEventTarget> targ = do_QueryInterface(mInput);
> -  NS_ASSERTION(targ, "input must be an event target!");
> +  // get localized text for clear button
> +  rv = bundleService->CreateBundle(NS_XFORMS_BUNDLE_URL,
> +                                   getter_AddRefs(bundle));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = bundle->GetStringFromName(NS_LITERAL_STRING("clearButtonText").get(),
> +                                 getter_Copies(value));
> +  if (NS_FAILED(rv)) {
> +    // fall back to English text
> +    value.AssignASCII("Clear");
> +  }
> +  if (uiUpload) {
> +    uiUpload->SetClearButtonText(value);
> +  }

> -  targ->AddEventListener(NS_LITERAL_STRING("blur"), this, PR_FALSE);
>    return NS_OK;
>  }

> -NS_IMETHODIMP
> -nsXFormsUploadElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
> +static void
> +ReleaseObject(void    *aObject,
> +              nsIAtom *aPropertyName,
> +              void    *aPropertyValue,
> +              void    *aData)

I know you just moved this, but for consistency in the file please move the
"*" next to the type. (I'd rather have them next to the name for all
functions, but as long as it's consitant in the individual files...)

>  {
> -  if (aName == nsXFormsAtoms::accesskey) {
> -    // accesskey
> -    mInput->SetAttribute(NS_LITERAL_STRING("accesskey"), aValue);
> -  }
> -
> -  return NS_OK;
> +  NS_STATIC_CAST(nsISupports *, aPropertyValue)->Release();
>  }

>  NS_IMETHODIMP
> -nsXFormsUploadElement::AttributeRemoved(nsIAtom *aName)
> +nsXFormsUploadElement::PickFile()
>  {
> -  if (aName == nsXFormsAtoms::accesskey) {
> -    // accesskey
> -    mInput->RemoveAttribute(NS_LITERAL_STRING("accesskey"));
> -  }
> +  nsresult rv;

> -  return NS_OK;
> -}
> +  // get nsIDOMWindowInternal
> +  nsCOMPtr<nsIDOMDocument> doc;
> +  mElement->GetOwnerDocument(getter_AddRefs(doc));

null-check mElement

> +  PRBool changedM = PR_FALSE;

changedM? More descriptive name.

> +  if (!aFile) {
> +    // clear instance data
> +    content->DeleteProperty(nsXFormsAtoms::uploadFileProperty);
> +    rv = mModel->SetNodeValue(mBoundNode, EmptyString(), &changedM);
> +  } else {
> +    // set file into instance data

> -  // We need to special case an empty value.  If the input field is blank,
> -  // then I assume that the intention is that all bound nodes will become
> -  // (or stay) empty and that there shouldn't be any file kept in the property.
> -  if (!value.IsEmpty()) {
> -    NS_NewLocalFile(value, PR_FALSE, &file);
> -    NS_ENSURE_STATE(file);
> -    NS_GetURLSpecFromFile(file, spec);
> -    content->SetProperty(nsXFormsAtoms::uploadFileProperty, file, 
> +    nsIFile* fileCopy = nsnull;
> +    rv = aFile->Clone(&fileCopy);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    content->SetProperty(nsXFormsAtoms::uploadFileProperty, fileCopy,
>                           ReleaseObject);

Check that this succeeds? And how about moving this down below the type
checking. If the type's unknown you shouldn't store the property I guess.

> -  } else {
> -    content->DeleteProperty(nsXFormsAtoms::uploadFileProperty);
> -  }

> +    nsAutoString type, prefix, nsuri;
> +    rv = nsXFormsUtils::ParseTypeFromNode(mBoundNode, type, prefix);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // get the namespace url from the prefix
> +    nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = domNode3->LookupNamespaceURI(prefix, nsuri);
> +
> +    if (NS_SUCCEEDED(rv) && nsuri.EqualsLiteral(NS_NAMESPACE_XML_SCHEMA))
> +    {
> +      if (type.EqualsLiteral("anyURI")) {
> +        // set fully qualified path as value in instance data node
> +        nsCAutoString spec;
> +        NS_GetURLSpecFromFile(aFile, spec);
> +        rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec),
> +                                  &changedM);
> +      }
> +      else if (type.EqualsLiteral("base64Binary") ||

move else up

> +               type.EqualsLiteral("hexBinary"))

Use named string, or somewhat ugly |(isBinary =
type.EqualsLiteral("hexBinary"))|

> +      {
> +        // encode file contents in base64/hex and set into instance data node
> +        nsEncodingType encoding;
> +        if (type.EqualsLiteral("base64Binary"))
> +          encoding = ENCODE_BASE64;
> +        else
> +          encoding = ENCODE_HEX;
> +
> +        PRUnichar* fileData;
> +        rv = EncodeFileContents(aFile, encoding, &fileData);
> +        if (NS_SUCCEEDED(rv)) {
> +          rv = mModel->SetNodeValue(mBoundNode, nsDependentString(fileData),
> +                                    &changedM);
> +          nsMemory::Free(fileData);
> +        }
> +      }
> +      else {

move else up

> +        NS_ERROR("Unknown type");

Please ReportError() for both unknown type and no type.

> +      }
> +
> +      // XXX need to handle derived types

Please create a bug for that

> +    }
> +  }
> +  NS_ENSURE_SUCCESS(rv, rv);

> -  // update the instance data node.  this value is never used by the submission
> -  // code.  instead, it exists so that the instance data will appear to be in-
> -  // sync with what is actually submitted.
> -
> -  PRBool changed;
> -  nsresult rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), 
> -                                     &changed);
> +  PRBool changedH;

more descriptive naming

> +  rv = HandleChildElements(aFile, &changedH);
>    NS_ENSURE_SUCCESS(rv, rv);
> -  if (changed) {
> +
> +  if (changedM || changedH) {
>      nsCOMPtr<nsIDOMNode> model = do_QueryInterface(mModel);
> +
>      if (model) {
>        rv = nsXFormsUtils::DispatchEvent(model, eEvent_Recalculate);
>        NS_ENSURE_SUCCESS(rv, rv);
>        rv = nsXFormsUtils::DispatchEvent(model, eEvent_Revalidate);
>        NS_ENSURE_SUCCESS(rv, rv);
>        rv = nsXFormsUtils::DispatchEvent(model, eEvent_Refresh);
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>    }
> +
>    return NS_OK;
>  }

> -// nsIXFormsControl
> +nsresult
> +nsXFormsUploadElement::HandleChildElements(nsILocalFile* aFile,
> +                                           PRBool* aChanged)
> +{
> +  *aChanged = PR_FALSE;
> +
> +  // return immediately if we have no children
> +  PRBool hasNodes;
> +  mElement->HasChildNodes(&hasNodes);

> +  if (!hasNodes)
> +    return NS_OK;
> +
> +  nsresult rv = NS_OK;

> -NS_IMETHODIMP
> -nsXFormsUploadElement::Refresh()
> +  // look for the <filename> & <mediatype> elements in the <upload> children
> +  if (!mFilenameNode || !mMediatypeNode) {

If the instance data is changed, these could point to the wrong nodes.

> +    nsCOMPtr<nsIDOMNode> child, temp;
> +    mElement->GetFirstChild(getter_AddRefs(child));
> +    while (child && (!mFilenameNode || !mMediatypeNode)) {
> +      if (!mFilenameNode &&
> +          nsXFormsUtils::IsXFormsElement(child,
> +                                         NS_LITERAL_STRING("filename")))
> +      {
> +        mFilenameNode = child;
> +      }
> +
> +      if (!mMediatypeNode &&
> +          nsXFormsUtils::IsXFormsElement(child,
> +                                         NS_LITERAL_STRING("mediatype")))
> +      {
> +        mMediatypeNode = child;
> +      }
> +
> +      temp.swap(child);
> +      temp->GetNextSibling(getter_AddRefs(child));
> +    }
> +  }
> +
> +  // handle "filename"
> +  PRBool changedF = PR_FALSE;
> +  if (mFilenameNode) {
> +    if (aFile) {
> +      nsAutoString filename;
> +      rv = aFile->GetLeafName(filename);
> +      if (!filename.IsEmpty()) {
> +        rv = SetChildNodeValue(mFilenameNode, filename, &changedF);
> +      }
> +    } else {
> +      rv = SetChildNodeValue(mFilenameNode, EmptyString(), &changedF);
> +    }
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // handle "mediatype"
> +  PRBool changedM = PR_FALSE;
> +  if (mMediatypeNode) {
> +    if (aFile) {
> +      nsCOMPtr<nsIMIMEService> mimeService =
> +                do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
> +      if (NS_SUCCEEDED(rv)) {
> +        nsCAutoString contentType;
> +        rv = mimeService->GetTypeFromFile(aFile, contentType);
> +        if (NS_FAILED(rv)) {
> +          contentType.AssignLiteral("application/octet-stream");

Where is this specified?

> +        }
> +        rv = SetChildNodeValue(mMediatypeNode,
> +                               NS_ConvertUTF8toUTF16(contentType), &changedM);
> +      }
> +    } else {
> +      rv = SetChildNodeValue(mMediatypeNode, EmptyString(), &changedM);
> +    }
> +  }
> +
> +  *aChanged = changedF || changedM;
> +  return rv;
> +}
> +
> +nsresult
> +nsXFormsUploadElement::EncodeFileContents(nsIFile* aFile, nsEncodingType aType,
> +                                          PRUnichar** aResult)
>  {
> -  if (!mInput || !mBoundNode)
> -    return NS_OK;
> +  nsresult rv;

> -  nsCOMPtr<nsIContent> content = do_QueryInterface(mBoundNode);
> -  NS_ENSURE_STATE(content);
> +  // get file contents
> +  nsCOMPtr<nsIInputStream> fileStream;
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), aFile,
> +                                  PR_RDONLY, -1,

-1? Please comment.

> +                                  nsIFileInputStream::CLOSE_ON_EOF);
> +  NS_ENSURE_SUCCESS(rv, rv);

> -  nsILocalFile *file =
> -      NS_STATIC_CAST(nsILocalFile *,
> -                     content->GetProperty(nsXFormsAtoms::uploadFileProperty));
> -
> -  // get the text value for the control
> -  nsAutoString value;
> -  if (file)
> -    file->GetPath(value);
> +  PRUint32 size;
> +  rv = fileStream->Available(&size);
> +  NS_ENSURE_SUCCESS(rv, rv);

Possibly warn user when trying to upload BIG file... hmm, if html:upload does
it.

> -  mInput->SetValue(value);
> -  mInput->SetReadOnly(GetReadOnlyState());
> +  char* fileData = new char[size + 1];

Could we use a smart pointer for this? I would feel safer.

> +  if (!fileData) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Please ReportError() about it.

> -  return NS_OK;
> +  PRUint32 bytesRead;
> +  rv = fileStream->Read(fileData, size, &bytesRead);
> +  NS_ASSERTION(NS_SUCCEEDED(rv) && bytesRead == size,
> +               "fileStream->Read failed");
> +
> +  if (NS_SUCCEEDED(rv)) {
> +
> +    if (aType == ENCODE_BASE64) {
> +      // encode file contents
> +      *aResult = nsnull;
> +      char* buffer = PL_Base64Encode(fileData, bytesRead, nsnull);
> +      if (buffer) {
> +        *aResult = ToNewUnicode(nsDependentCString(buffer));
> +      }

is buffer automatically deleted?

> +      if (!*aResult) {
> +        rv = NS_ERROR_OUT_OF_MEMORY;
> +      }
> +    }

move else up

> +    else if (aType == ENCODE_HEX) {
> +      // create buffer for hex encoded data
> +      PRUint32 length = bytesRead * 2 + 1;
> +      PRUnichar* fileDataHex =
> +        NS_STATIC_CAST(PRUnichar*, nsMemory::Alloc(length * sizeof(PRUnichar)));
> +      if (!fileDataHex) {
> +        rv = NS_ERROR_OUT_OF_MEMORY;
> +      } else {
> +        // encode file contents
> +        BinaryToHex(fileData, bytesRead, &fileDataHex);
> +        fileDataHex[bytesRead * 2] = 0;
> +        *aResult = fileDataHex;
> +      }
> +    }
> +
> +    else {

move else up

> +      NS_ERROR("Unknown encoding type for <upload> element");
> +      rv = NS_ERROR_INVALID_ARG;
> +    }
> +  }
> +
> +  delete [] fileData;
> +
> +  return rv;
>  }

I did not get further that this, gotta go :(
(In reply to comment #41)
> * Could you include a note somewhere about why "@incremental" is ignored?

Hmm, you are right.  I thought this was working (maybe with old patch).  For
<upload>, when "true", this will "generate additional xforms-value-changed
events".  Not sure what this means exactly.  But with this patch, I see
'xforms-value-changed' events sent whether "@incremental" is true or false.  So
I guess when false, I shouldn't send those event?

> * Include an XXX about mediatype filtering and a new bug?

Since this is a generic but for <upload>, I'll just handle it in another patch
here, rather than creating a new bug.

> > -        nsILocalFile *file =
> > -            NS_STATIC_CAST(nsILocalFile *,
> > +        nsIFile *file =
> > +            NS_STATIC_CAST(nsIFile *,
> >                            
content->GetProperty(nsXFormsAtoms::uploadFileProperty));
> 
> Why this change? Because we do not need the extra functionality in
> nsILocalFile?

Correct.  Plus, when setting property in 'nsXFormsUploadElement.cpp', I clone
the file, which returns an nsIFile.

> > +  nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue,
> > +                             PRBool* aChanged);
> 
> Comment not entirely correct, it can set an arbitrary node value, right? Which
> makes me think that it should 1) use
> nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to
> nsXFormsUtils as a generic function.

I don't know about _using_ |GetSingleNodeBindingValue|, but I could definitely
make a |Set...| function that looks very much like it, in nsXFormsUtils.

> I know you just moved this, but for consistency in the file please move the
> "*" next to the type. (I'd rather have them next to the name for all
> functions, but as long as it's consitant in the individual files...)

Well, I decided to be consistent with the rest of XForms, and put "*" next to
the name.

> > +        rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec),
> > +                                  &changedM);
> > +      }
> > +      else if (type.EqualsLiteral("base64Binary") ||
> 
> move else up

I usually do this to break up the blocks and make it easier to read at a glance.
   But I can move it up if you really want.

> > +      nsCOMPtr<nsIMIMEService> mimeService =
> > +                do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
> > +      if (NS_SUCCEEDED(rv)) {
> > +        nsCAutoString contentType;
> > +        rv = mimeService->GetTypeFromFile(aFile, contentType);
> > +        if (NS_FAILED(rv)) {
> > +          contentType.AssignLiteral("application/octet-stream");
> 
> Where is this specified?

What are you referring to?  I took the mimeService code from nsHTMLInputElement.cpp.

> Possibly warn user when trying to upload BIG file... hmm, if html:upload does
> it.

No, <html:input type="file"> doesn't warn the user, but it doesn't encode the
file contents.  I don't think we should warn the user.  It should be up to the
form developer to choose the appropriate way to transmit the file.  The form
developer should know that if there is the possibility of sending large files,
they should be transmitted as "multipart-post".

> > +  if (!fileData) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> 
> Please ReportError() about it.

For an out of memory error?  Why?
(In reply to comment #42)
> (In reply to comment #41)
> > * Could you include a note somewhere about why "@incremental" is ignored?
> 
> Hmm, you are right.  I thought this was working (maybe with old patch).  For
> <upload>, when "true", this will "generate additional xforms-value-changed
> events".  Not sure what this means exactly.  But with this patch, I see
> 'xforms-value-changed' events sent whether "@incremental" is true or false.  So
> I guess when false, I shouldn't send those event?

For this type of control, I believe there should be no difference. Just note
that. But if it where a text field the user typed into, it should behave as
input (sending events for each keypress).
 
> > > +  nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue,
> > > +                             PRBool* aChanged);
> > 
> > Comment not entirely correct, it can set an arbitrary node value, right? Which
> > makes me think that it should 1) use
> > nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to
> > nsXFormsUtils as a generic function.
> 
> I don't know about _using_ |GetSingleNodeBindingValue|, but I could definitely
> make a |Set...| function that looks very much like it, in nsXFormsUtils.

Why can you not use GetSingleNodeBindingValue() do you not do the same?
 
> > > +        rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec),
> > > +                                  &changedM);
> > > +      }
> > > +      else if (type.EqualsLiteral("base64Binary") ||
> > 
> > move else up
> 
> I usually do this to break up the blocks and make it easier to read at a glance.
>    But I can move it up if you really want.

I _really_ want it :) It's what we do in the rest of the code (or most of it
anyway).
 
> > > +      nsCOMPtr<nsIMIMEService> mimeService =
> > > +                do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
> > > +      if (NS_SUCCEEDED(rv)) {
> > > +        nsCAutoString contentType;
> > > +        rv = mimeService->GetTypeFromFile(aFile, contentType);
> > > +        if (NS_FAILED(rv)) {
> > > +          contentType.AssignLiteral("application/octet-stream");
> > 
> > Where is this specified?
> 
> What are you referring to?

That contentType defaults to "application/octet-stream". But if that's how
html:input handles it, let's just follow that.

> > Possibly warn user when trying to upload BIG file... hmm, if html:upload does
> > it.
> 
> No, <html:input type="file"> doesn't warn the user, but it doesn't encode the
> file contents.  I don't think we should warn the user.  It should be up to the
> form developer to choose the appropriate way to transmit the file.  The form
> developer should know that if there is the possibility of sending large files,
> they should be transmitted as "multipart-post".
> 
> > > +  if (!fileData) {
> > > +    return NS_ERROR_OUT_OF_MEMORY;
> > > +  }
> > 
> > Please ReportError() about it.
> 
> For an out of memory error?  Why?

I might be overfocused on OOM errors, but this is just _the_ easy place to
generate that for XForms (except for _big_ instances), because it lets the user
choose an arbitrary large file. The form author has no clue to whether the user
chooses a 400M core dump file, and tries to upload it.

It's not a must for me to report this to the user, but I think it would be
useful. At least we should emit some debug messages. What do the rest of you think?
(In reply to comment #41)
> (From update of attachment 197760 [details] [diff] [review] [edit])
> I did not get further that this, gotta go :(

I just checked the rest. No further comments from me.

(In reply to comment #43)
>The form author has no clue to whether the user
> chooses a 400M core dump file, and tries to upload it.
> 
> It's not a must for me to report this to the user, but I think it would be
> useful. At least we should emit some debug messages. What do the rest of you
think?

I have two suggestions:
1. The look and feel of the xf:upload control is not specified by XForms, nor 
does it *have* to look like the <input type="file"> cousin, so there's no reason
you can't include the file size in the display; you could even property-ize a
minimum size for this display to happen.

2. The upload control can populate the instance with filename and mediatype. 
There's no reason it couldn't also populate a size for use by form authors.
You could suggest this to the WG as a reasonable enhancement for a future
version of XForms. (If you don't use method='multipart-post' then it's possible
that you would run out of memory during the population of the base64 or
hexbinary encoding, but that will rapidly teach authors not to do that.) 


> (In reply to comment #43)
> >The form author has no clue to whether the user
> > chooses a 400M core dump file, and tries to upload it.
> > 
> > It's not a must for me to report this to the user, but I think it would be
> > useful. At least we should emit some debug messages. What do the rest of you
> think?
> 
> I have two suggestions:
> 1. The look and feel of the xf:upload control is not specified by XForms, nor 
> does it *have* to look like the <input type="file"> cousin, so there's no reason
> you can't include the file size in the display; you could even property-ize a
> minimum size for this display to happen.
> 
> 2. The upload control can populate the instance with filename and mediatype. 
> There's no reason it couldn't also populate a size for use by form authors.
> You could suggest this to the WG as a reasonable enhancement for a future
> version of XForms. (If you don't use method='multipart-post' then it's possible
> that you would run out of memory during the population of the base64 or
> hexbinary encoding, but that will rapidly teach authors not to do that.) 
> 
> 

Could also make the error style-able.  Of course, if you do this, you'd want to
report the error.  If reporting the error, please include the size of the
allocation that failed.
Attached patch second patch v1.5 (obsolete) — — Splinter Review 
Hopefully I covered all of Allan's comments with this patch.  I also made it so
that if the element is not bound to the corrent datatype, then the control is
marked as not-relevant, and the buttons are disabled.
Attachment #197760 - Attachment is obsolete: true
Attachment #199993 - Flags: review?(allan)
> Created an attachment (id=199993) [edit]
Something I forgot: since |refresh()| in xforms.xml can get called quite a bit,
I should be caching the refs to the buttons.
Attachment #199993 - Flags: review?(aaronr)
(In reply to comment #47)
> Created an attachment (id=199993) [edit]
> second patch v1.5
> 
> Hopefully I covered all of Allan's comments with this patch.  I also made it so
> that if the element is not bound to the corrent datatype, then the control is
> marked as not-relevant, and the buttons are disabled.

&%¤&%¤, now I get that permission denied stuff again (comment 30) when testing
attachment 197983 [details] or attachment 197763 [details]. Why is this happening to me...


Comment on attachment 199993 [details] [diff] [review]
second patch v1.5

> Index: nsXFormsUploadElement.cpp
> ===================================================================
> +nsXFormsUploadElement::Refresh()
...
> +        if (NS_SUCCEEDED(rv) && nsuri.EqualsLiteral(NS_NAMESPACE_XML_SCHEMA)) {
> +          NS_NAMED_LITERAL_STRING(anyURIStr, "anyURI");
> +          NS_NAMED_LITERAL_STRING(base64BinStr, "base64Binary");
> +          NS_NAMED_LITERAL_STRING(hexBinStr, "hexBinary");

Why? You only use them once.

> +    // If it is not bound to 'anyURI', 'base64Binary', or 'hexBinary', then
> +    //  mark as not relevant.
> +    if (mType == TYPE_DEFAULT) {
> +      /// @bug Set via attributes right now. Bug 271720. (XXX)
> +      mElement->SetAttribute(NS_LITERAL_STRING("disabled"), 
> +                             NS_LITERAL_STRING("1"));
> +      mElement->RemoveAttribute(NS_LITERAL_STRING("enabled"));
> +      nsXFormsUtils::ReportError(NS_LITERAL_STRING("uploadBoundTypeError"),
> +                                 mElement);
> +    } else {
> +      /// @bug Set via attributes right now. Bug 271720. (XXX)
> +      mElement->SetAttribute(NS_LITERAL_STRING("enabled"), 
> +                             NS_LITERAL_STRING("1"));
> +      mElement->RemoveAttribute(NS_LITERAL_STRING("disabled"));
> +    }
>    }

Some jerk changed this :), so that would be:
nsCOMPtr<nsIXTFElementWrapper> xtfWrap(do_QueryInterface(mElement));
NS_ENSURE_STATE(xtfWrap);
xtfWrap->SetIntrinsicState(mType == TYPE_DEFAULT ? NS_EVENT_STATE_DISABLED
						 : NS_EVENT_STATE_ENABLED);

1) I feel there's something fishy about this setting the event state, even
though there's a bound node... but that may just be me, and definately another
bug.

2) Hmmm, that's how we (I) do it in ResetBoundNode(), but that's actually
wrong. Argh. Bug 313118 created. Do the above and "// XXX bug 313118 should fix
this"

> +nsresult
> +nsXFormsUploadElement::SetFile(nsILocalFile *aFile)
>  {
> -  if (!mInput || !mBoundNode || !mModel ||
> -      !nsXFormsUtils::EventHandlingAllowed(aEvent, mElement))
> +  if (!mBoundNode || !mModel)
>      return NS_OK;

> -  nsAutoString value;
> -  mInput->GetValue(value);
> -
> -  // store the file as a property on the selected content node.  the submission
> -  // code will read this value.
> +  nsresult rv;

>    nsCOMPtr<nsIContent> content = do_QueryInterface(mBoundNode);
>    NS_ENSURE_STATE(content);

> -  nsILocalFile *file = nsnull;
> -  nsCAutoString spec;
> -
> -  // We need to special case an empty value.  If the input field is blank,
> -  // then I assume that the intention is that all bound nodes will become
> -  // (or stay) empty and that there shouldn't be any file kept in the property.
> -  if (!value.IsEmpty()) {
> -    NS_NewLocalFile(value, PR_FALSE, &file);
> -    NS_ENSURE_STATE(file);
> -    NS_GetURLSpecFromFile(file, spec);
> -    content->SetProperty(nsXFormsAtoms::uploadFileProperty, file, 
> -                         ReleaseObject);
> -  } else {
> +  PRBool dataChanged = PR_FALSE;
> +  if (!aFile) {
> +    // clear instance data
>      content->DeleteProperty(nsXFormsAtoms::uploadFileProperty);
> -  }
> +    rv = mModel->SetNodeValue(mBoundNode, EmptyString(), &dataChanged);
> +  } else {
> +    // set file into instance data
> +
> +    if (mType == TYPE_ANYURI) {
> +      // set fully qualified path as value in instance data node
> +      nsCAutoString spec;
> +      NS_GetURLSpecFromFile(aFile, spec);
> +      rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec),
> +                                &dataChanged);
> +    } else if (mType == TYPE_BASE64 || mType == TYPE_HEX) {
> +      // encode file contents in base64/hex and set into instance data node
> +      PRUnichar *fileData;
> +      rv = EncodeFileContents(aFile, mType, &fileData);
> +      if (NS_SUCCEEDED(rv)) {
> +        rv = mModel->SetNodeValue(mBoundNode, nsDependentString(fileData),
> +                                  &dataChanged);
> +        nsMemory::Free(fileData);
> +      }
> +    }

> +    // XXX need to handle derived types
> +
> +    // Set nsIFile object as property on instance data node, so submission
> +    // code can use it.
> +    if (NS_SUCCEEDED(rv)) {

If (aFile) and the type is not one of the three known ones, rv is undefined
here.

> +      nsIFile *fileCopy = nsnull;
> +      rv = aFile->Clone(&fileCopy);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = content->SetProperty(nsXFormsAtoms::uploadFileProperty, fileCopy,
> +                                ReleaseObject);
> +    }
> +  }
> +  NS_ENSURE_SUCCESS(rv, rv);

> Index: nsXFormsUtils.cpp
> ===================================================================
> @@ -791,16 +790,50 @@ nsXFormsUtils::GetSingleNodeBindingValue

> -  nsXFormsUtils::GetNodeValue(singleNode, aValue);
> +  singleNode.swap(*aNode);
> +  if (aModel)
> +    model.swap(*aModel);

could you add "// transfers ref"
to the swap lines?

> Index: resources/content/xforms.xml
> ===================================================================
> +      <method name="refresh">
> +        <body>
> +          // disable buttons if control is not relevant
> +          var browseButton =
> +            document.getAnonymousElementByAttribute(this, "anonid",
> +                                                    "upload_browse_button");
> +          var clearButton =
> +            document.getAnonymousElementByAttribute(this, "anonid",
> +                                                    "upload_clear_button");

cache them, as you suggest in comment 48.

> +          if (this.delegate.isEnabled) {
> +            browseButton.disabled = false;
> +            clearButton.disabled = false;
> +          } else {
> +            browseButton.disabled = true;
> +            clearButton.disabled = true;
> +          }

browseButton.disabled = clearButton.disabled = !this.delegate.isEnabled;

... hmmm, which is actually wrong in the case where the type's wrong, because
this.delegate.isEnabled returns whether mBoundNode is enabled. You want to know
if the upload control is enabled. You might have to use same mechanism as we
use to disable the submit button during submission.

> Index: resources/locale/en-US/xforms.properties
> ===================================================================
> +# XForms upload element
> +clearButtonText = Clear

I wonder, since upload is XBL, can it not use entity references and the dtd? Or
does it have to be pure XUL for that?


To summarize, I need the nits above and :
* to figure out why I get the security error (and run some testcases)
* a resolution of the undefined rv
* "proper disabling"
* an answer to whether we could use entity refs instead

With those solved, you have r=me
Attachment #199993 - Flags: review?(allan) → review-
Comment on attachment 199993 [details] [diff] [review]
second patch v1.5


>Index: nsXFormsUploadElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v
>retrieving revision 1.10
>diff -u -6 -p -r1.10 nsXFormsUploadElement.cpp
>--- nsXFormsUploadElement.cpp	22 Jul 2005 18:47:11 -0000	1.10
>+++ nsXFormsUploadElement.cpp	18 Oct 2005 22:04:17 -0000

> NS_IMETHODIMP
>-nsXFormsUploadElement::AttributeRemoved(nsIAtom *aName)
>+nsXFormsUploadElement::Refresh()
> {
>-  if (aName == nsXFormsAtoms::accesskey) {
>-    // accesskey
>-    mInput->RemoveAttribute(NS_LITERAL_STRING("accesskey"));
>+  // This is called when the 'bind', 'ref' or 'model' attribute has changed.
>+  // So we need to make sure that the element is still bound to a node of
>+  // type 'anyURI', 'base64Binary', or 'hexBinary'.
>+
>+  mType = TYPE_DEFAULT;
>+
>+  if (mBoundNode) {
>+    // get type bound to node
>+    nsAutoString type, prefix, nsuri;
>+    nsresult rv = nsXFormsUtils::ParseTypeFromNode(mBoundNode, type, prefix);
>

This is what Allan usually catches me on :=)  Don't put all this work in a big
'if' statement if you can help it.  Use 
if (!mBoundNode) {
  return return nsXFormsDelegateStub::Refresh();
}


> NS_IMETHODIMP
>-nsXFormsUploadElement::GetInsertionPoint(nsIDOMElement **aPoint)
>+nsXFormsUploadElement::PickFile()

>-// nsIDOMEventListener
>+  // set filter "All Files"
>+  // XXX implement "@mediatype" file filters
>+  filePicker->AppendFilters(nsIFilePicker::filterAll);

Make sure that you open a bug on this and attach a testcase while you are
thinking about it.

Pausing for now.  More review to come...
Attached patch second patch v1.6 (obsolete) — — Splinter Review 
This patch does "proper disabling" (I think).  The only trick is that I want to
disable the buttons if mBound == null or if the control is not bound to a node
of the right type.  I'm also using an entity ref for the "Clear" text.	Not
sure if you also want me to do the "Browse" text the same way.
Attachment #199993 - Attachment is obsolete: true
Attachment #200316 - Flags: review?(allan)
Attachment #200316 - Flags: review?(aaronr)
Attachment #199993 - Flags: review?(aaronr)
> Created an attachment (id=200316) [edit]
Ah, crap!  Didn't check the console output closely.  This patch spits out
several warnings that " NS_ENSURE_TRUE(widget) failed", since the first few
times that |Refresh()| is called, there is no mElement.  I guess I could just
remove the " NS_ENSURE_TRUE(widget)" and add "if (widget)" in front of ever ref
to widget.  But in general, I dislike how I coded that method.  I'll have to
think on it some more tomorrow.

(In reply to comment #30)
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'Permission denied to get property Object.file' when calling
> method: [nsIFilePicker::file]"	nsresult: "0x8057001e
> (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame ::
> http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" 
> data: no]
> ************************************************************

I have yet to see this error.  From what you describe, it seems like you press
the "Browse" button, which brings up the file dialog, allowing you to select a
file.  And when you click OK in the file dialog, you get this error message,
correct?  It looks to me like something internal to the file picker is producing
this error.  What platform are you on?  Does this happen consistently enough
that you could debug it?
(In reply to comment #53)
> (In reply to comment #30)
> > ************************************************************
> > * Call to xpconnect wrapped JSObject produced this error:  *
> > [Exception... "'Permission denied to get property Object.file' when calling
> > method: [nsIFilePicker::file]"	nsresult: "0x8057001e
> > (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame ::
> > http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" 
> > data: no]
> > ************************************************************
> 
> I have yet to see this error.  From what you describe, it seems like you press
> the "Browse" button, which brings up the file dialog, allowing you to select a
> file.  And when you click OK in the file dialog, you get this error message,
> correct?

Yes

> It looks to me like something internal to the file picker is producing
> this error.  What platform are you on?  Does this happen consistently enough
> that you could debug it?

Hmmm, this is the same behaviour as last time. This only happens on my home
computer (same Linux as at work...). And it happens all the time at home. This
could be a setup/profile problem?
(In reply to comment #52)
> Created an attachment (id=200316) [edit]
> second patch v1.6
> 
> This patch does "proper disabling" (I think).  The only trick is that I want to
> disable the buttons if mBound == null or if the control is not bound to a node
> of the right type.  I'm also using an entity ref for the "Clear" text.	Not
> sure if you also want me to do the "Browse" text the same way.

Wouldn't that be the better approach? Then all strings are stored in the DTD.
Attached patch second patch v1.7 (obsolete) — — Splinter Review 
One last go, hopefully.  Includes "if (widget)" checks in |Refresh()|, and puts
the "Browse..." button text in DTD file (which allows me to get rid of some
code).
Attachment #200316 - Attachment is obsolete: true
Attachment #200348 - Flags: review?(allan)
Attachment #200316 - Flags: review?(allan)
Attachment #200316 - Flags: review?(aaronr)
I'll try to get to the latest patch today. Just a note: I think upload was the
only user of nsXFormsControlStubBase::GetReadOnlyState(), and you do not use it
now do you? If true, please kill it on check in.
Allan gave me the idea of using CSS to do the disabling of the buttons: if the
element is bound to the right type, it uses the "xformswidget-upload" binding
which works as expected;  otherwise, it binds to "xforswidget-upload-disabled",
which does nothing but show the UI, disabled.

This patch should also fix Allan's second testcase in this bug.  I just needed
to check the bound type in |SetFile()|, also.  If bug 313313 gets fixed, I can
get rid of the |Refresh()| method altogether, and just set the 'relevant' state
in XBL code.
Attachment #200348 - Attachment is obsolete: true
Attachment #200378 - Flags: review?(allan)
Attachment #200378 - Flags: review?(aaronr)
Comment on attachment 200378 [details] [diff] [review]
second patch v1.8


>Index: nsXFormsUploadElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v
>retrieving revision 1.10
>diff -u -6 -p -r1.10 nsXFormsUploadElement.cpp
>--- nsXFormsUploadElement.cpp	22 Jul 2005 18:47:11 -0000	1.10
>+++ nsXFormsUploadElement.cpp	21 Oct 2005 20:40:47 -0000
>+  // If it is not bound to 'anyURI', 'base64Binary', or 'hexBinary', then
>+  //  mark as not relevant.
>+  // XXX Bug 313313 - the 'relevant' state should be handled in XBL constructor.
>+  nsCOMPtr<nsIXTFElementWrapper> xtfWrap(do_QueryInterface(mElement));
>+  NS_ENSURE_STATE(xtfWrap);
>+  if (GetBoundType() == TYPE_DEFAULT) {
>+    xtfWrap->SetIntrinsicState(NS_EVENT_STATE_DISABLED);
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("uploadBoundTypeError"),
>+                               mElement);
>+  } else {
>+    xtfWrap->SetIntrinsicState(NS_EVENT_STATE_ENABLED);
>   }
> 
>   return NS_OK;
> }

You shouldn't be setting intrinsic state to disabled, right?  But rather adding
the disabled state and removing the enabled state?  See comment #50.

>-// nsIXFormsControl
>+nsresult
>+nsXFormsUploadElement::HandleChildElements(nsILocalFile *aFile,
>+                                           PRBool *aChanged)
>+{
>+  *aChanged = PR_FALSE;
>+

Shouldn't you check to see if aChanged exists first?

>+
>+static void
>+ReportEncodingMemoryError(nsIDOMElement* aElement, nsIFile *aFile)
> {
>-  if (!mInput || !mBoundNode)
>-    return NS_OK;
>+  nsAutoString filename;
>+  if (NS_SUCCEEDED(aFile->GetLeafName(filename))) {
>+    const PRUnichar *strings[] = { filename.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("encodingMemoryError"),
>+                               strings, 1, aElement, aElement);
>+  }
>+}

Can you report the allocation size that failed, please?

Otherwise seems ok to me.
Attachment #200378 - Flags: review?(aaronr) → review+
(In reply to comment #59)
> You shouldn't be setting intrinsic state to disabled, right?  But rather adding
> the disabled state and removing the enabled state?  See comment #50.

As Allan mentions, that will be fixed by Bug 313118.  Of course, that whole
function in Upload should disappear if Bug 313313 is fixed.
Attachment #200348 - Flags: review?(allan)
Comment on attachment 200378 [details] [diff] [review]
second patch v1.8

right on, r=me
Attachment #200378 - Flags: review?(allan) → review+
checked in to trunk.
Whiteboard: xf-to-branch
checked in to branch. ->FIXED
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: