[1.1] Add support for resource element on submission to allow more flexibility

RESOLVED FIXED

Status

Core Graveyard
XForms
--
enhancement
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: Steve Speicher, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070305 Minefield/3.0a3pre
Build Identifier: 

Need a mechanism to submit contents to a variable number of locations, which can all be configured to be different submission targets.  It would be ideal to specify the submission targets via instance data.  This is possible in XForms1.1 with the new <resource> element, that is a child of <submission>.

This mechanism will be used for the Integrating the Healthcare Enterprise (http://www.ihe.net) and it's Request Form for Data capture (RFD) profile.

Reproducible: Always

Comment 1

11 years ago
I agree that this is worth doing sooner rather than later.  Good for RSS and Atom feed type interaction, too.

Please attach testcase.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

10 years ago
Created attachment 261390 [details]
test case

Should be how it works in 1.1

Comment 3

10 years ago
Created attachment 264416 [details]
testcase 2

patch that fixes this bug should also pass testcase 2, I think.

Updated

10 years ago
Assignee: xforms → msterlin
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 years ago
Created attachment 264433 [details] [diff] [review]
patch
Attachment #264433 - Flags: review?(aaronr)

Comment 5

10 years ago
Comment on attachment 264433 [details] [diff] [review]
patch

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

>-  nsCOMPtr<nsIInputStream> stream;
>-  nsCAutoString uri, contentType;
>-  nsAutoString action;
>-  mElement->GetAttribute(NS_LITERAL_STRING("action"), action);
>-  CopyUTF16toUTF8(action, uri);
>+  nsCAutoString uri;
>+  GetSubmissionURI(uri);
> 
>+  nsCOMPtr<nsIInputStream> stream;
>+  nsCAutoString contentType;

nit: I'd leave things like they were where the declarations were grouped together since there is no way we can return before the call to SerializeData.  You just have to delete the declaration of action since we don't use it anymore.

> nsresult
>+nsXFormsSubmissionElement::GetSubmissionURI(nsACString &uri)
>+{
>+  // Precedence:
>+  // 1. If the submission element has a resource element as its first child,
>+  // the URI can be specifed by either the 'value' attribute or the string
>+  // content of the resource element. The resource element has precedence over
>+  // both the 'resource' and 'action' attributes.
>+  //
>+  // 2. If there is no resource element as the first child, the URI may be
>+  // specified by either the 'resource' or 'action' attributes with 'resource'
>+  // having precedence over 'action'.
>+  //
>+  // If no URI is specified via any of the above mechanisms we write a warning
>+  // message to the error console.
>+

Nice comments!!!

>+          rv = nsXFormsUtils::EvaluateNodeBinding(resourceElement, 0,
>+                                                  NS_LITERAL_STRING("value"),
>+                                                  NS_LITERAL_STRING("/"),
>+                                                  nsIDOMXPathResult::STRING_TYPE,
>+                                                  getter_AddRefs(model),
>+                                                  getter_AddRefs(xpRes),
>+                                                  &usesModelBind);

your 4th parameter is incorrect.  There shouldn't be a default ref on a xf:resource element.  Should probably be an empty string (EmptyString()).


>+  // If no URI is specified, write a warning to the console.
>+  if (value.IsEmpty() && resource.IsEmpty() && action.IsEmpty())

Why not just test if uri is empty?  By now it should have whatever the value is, right?


>Index: resources/locale/en-US/xforms.properties
>===================================================================

>+warnSubmitURI             = XForms Warning (9): Submission element must specify the URI via a resource element, resource attribute or action attribute

nit: Maybe change the warning to include that resource must be the first child element.  Like, "Submission element must specify the URI via a resource element (must be the first child element of submission), a resource attribute or an action attribute

with these fixed, r=me
Attachment #264433 - Flags: review?(aaronr) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 264508 [details] [diff] [review]
patch2
Attachment #264433 - Attachment is obsolete: true
Attachment #264508 - Flags: review?(Olli.Pettay)

Comment 7

10 years ago
I would prefer to use nsIContent and nsIAtom, string usage is not very protected from typos.

Comment 8

10 years ago
nsIContent is sort-of-internal-interface for gecko, and IMO, we should
keep its usage at minimum.

Comment 9

10 years ago
It's a shame the WG didn't additionally approve a way of serializing XML leaf nodes as parameters into the resource with application/x-www-form-urlencoded serialization.  I foresee a lot of bugs with people doing <resource value="concat('a=', a, '&amp;', b, '&amp;')" /> where a and b contain characters that needs % coding (such as =).  There's no way for resource to know what interpretation is being given to its value, so there's no way for it to quote meta characters for you.  This note is here so that when people file bugs on resource/@value and concat not quoting meta characters they can see it's not Mozilla's fault.

Comment 10

10 years ago
Comment on attachment 264508 [details] [diff] [review]
patch2


> nsresult
>+nsXFormsSubmissionElement::GetSubmissionURI(nsACString &uri)

Parameter should be nsACString& aURI

>+  nsAutoString resource, value, action;

Could there be just one nsAutoString
nsAutoString uri
which could be then just before error reporting code copied to aURI.
So wherever uri is used, use aURI, and wherever resource, value or action
is used, use uri. And have only one CopyUTF16toUTF8(uri, aURI) call.

With that change r=me
Attachment #264508 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 11

10 years ago
Created attachment 264926 [details] [diff] [review]
patch3

OK, just one nsAutoString.
Attachment #264508 - Attachment is obsolete: true

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Comment 12

10 years ago
Created attachment 265068 [details] [diff] [review]
patch with changes for 1.8 branch

To work on the 1.8 branch 'patch3' requires that we truncate the nsAutoString before assigning the xpath result from EvaluateNodeBinding because xpres->GetStringValue appends to the buffer rather than replacing it; also changing to always make the call to CopyUTF16toUTF8 to ensure that the returned uri from GetSubmissionURI is empty if no valid uri was found even if the caller passed in a non-empty string.


Apply 'patch with changes for 1.8 branch' to the trunk.
Apply 'patch3' and 'patch with changes for 1.8 branch' to the branch.

Updated

10 years ago
Attachment #265068 - Flags: review+

Updated

10 years ago
Attachment #265068 - Flags: review+

Comment 13

10 years ago
checked in 'patch with changes for 1.8 branch' to trunk

Comment 14

10 years ago
checked in 'patch3' and 'patch with changes for 1.8 branch' to the 1.8 and 1.8.0 branches
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch

Comment 15

10 years ago
I backed out 'patch with changes for 1.8 branch' on trunk to help mochitest debugging for ispiked.  I will put it back in when they are done.  I'm reopening bug so that we remember to do this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 16

10 years ago
'patch with changes for 1.8 branch' is now back on trunk.  Resolving bug as fixed again.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.