[1.1] Implement XPath function digest()

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: msterlin, Assigned: msterlin)

Tracking

({fixed1.8.1.17})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 4 obsolete attachments)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008031713 Minefield/3.0b4pre

We need to implement the XPath function digest() per spec section 7.8.3.


Reproducible: Always
Posted file testcase: 7.8.3.a
Posted file testcase: 7.8.3.b
Posted file testcase: 7.8.3.c
Posted file testcase: 7.8.3.d
Posted file testcase: 7.8.3.e (obsolete) —
Posted patch patch (obsolete) — Splinter Review
Attachment #313255 - Flags: review?(aaronr)
Assignee: nobody → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 313255 [details] [diff] [review]
patch

I don't have any problem with the patch, but it is missing some things.

1) According to the spec the keywords can be either lower or uppercase.  But you are using EqualsLiteral which I believe is case sensitive.

2) You always generate xforms-compute-exceptions.  But the spec says that digest() should follow section 7.6 to generate exceptions.  7.6 says, "If an error occurs in an XPath function, an xforms-compute-exception occurs if the function appears in the expression of a model item property. If the error occurs in a function that appears in any other XForms attribute that contains an XPath expression, such as nodeset, ref or at, then an xforms-binding exception occurs."  So you need to figure out if the resolver node is a xf:bind or not and generate the exception based on that information.
Attachment #313255 - Flags: review?(aaronr) → review-
Status: NEW → ASSIGNED
(In reply to comment #7)
> (From update of attachment 313255 [details] [diff] [review])
> I don't have any problem with the patch, but it is missing some things.
> 1) According to the spec the keywords can be either lower or uppercase.  But
> you are using EqualsLiteral which I believe is case sensitive.
I don't see where it is mentioned anywhere that the keywords can be either lower or uppercase. DIGEST would be invalid, so I read the spec literally and concluded that the keywords must be uppercase and the encodng method of hex or base64 must be lowercase.

This issue brings up another point. The CERT_Hexify function returns a hex string that is all uppercase. The spec examples and the W3C testsuite test against an all lowercase hex string, so I make the result lowercase. The value of the hex string doesn't change if it is upper or lowercase but it will be 'wrong' fifty percent of the time if the spec doesn't explicitly state that the hex string is upper or lowercase.


> 2) You always generate xforms-compute-exceptions.  But the spec says that
> digest() should follow section 7.6 to generate exceptions.  7.6 says, "If an
> error occurs in an XPath function, an xforms-compute-exception occurs if the
> function appears in the expression of a model item property. If the error
> occurs in a function that appears in any other XForms attribute that contains
> an XPath expression, such as nodeset, ref or at, then an xforms-binding
> exception occurs."  So you need to figure out if the resolver node is a xf:bind
> or not and generate the exception based on that information.

It's actually section 7.5 which is worded slightly differently and covers a lot of different types of functions: "If an error occurs in an XPath function, an xforms-compute-exception occurs if the function appears in a computed expression. If the error occurs in a function that appears in any other XForms attribute that contains an XPath expression, such as nodeset, ref or at, then an xforms-binding-exception occurs."

Is it even possible for a function like digest() to appear in an attribute that contains an XPath expression? I didn't think so and thought compute-exception is the one and only exception that could be generated. It's what all the testcases expect too.


(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 313255 [details] [diff] [review] [details])
> > I don't have any problem with the patch, but it is missing some things.
> > 1) According to the spec the keywords can be either lower or uppercase.  But
> > you are using EqualsLiteral which I believe is case sensitive.
> I don't see where it is mentioned anywhere that the keywords can be either
> lower or uppercase. DIGEST would be invalid, so I read the spec literally and
> concluded that the keywords must be uppercase and the encodng method of hex or
> base64 must be lowercase.
> 

My bad.  I was looking at a beta draft of the 1.1 spec that I had saved locally.  It looks like they removed the lower case keywords.  That is also why I quoted section 7.6 instead of 7.5 (it was sect. 7.6 in the version I was looking at).  So forget my comment about supporting lowercase keywords.

> Is it even possible for a function like digest() to appear in an attribute that
> contains an XPath expression? I didn't think so and thought compute-exception
> is the one and only exception that could be generated. It's what all the
> testcases expect too.
> 

Sure, xpath can appear on controls and xf:binds.  xf:setvalue and xf:output, for example, are used all the time because authors can put xpath expressions in their @value.
Testcase e should throw xforms-binding-exception not xforms-compute-exception.
Attachment #313253 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
This patch throws xforms-compute-exception if the digest function appears in a computed expression and xforms-binding-exception otherwise.
Attachment #313255 - Attachment is obsolete: true
Attachment #315665 - Flags: review?(aaronr)
Attachment #315665 - Flags: review?(Olli.Pettay)
Any tests for the optionality of the 3rd parameter?
We cannot support optional parameters with idl because the last parameter is
the return value. If you look at other functions, like Instance, that also have
optional params, we don't support it.
Comment on attachment 315665 [details] [diff] [review]
patch

nit: You should clarify what a 'computed expression' is.  Someone reading the code shouldn't have to go to the spec to see exactly what that term means.  With that r=me.

I'm not a big fan of having to bring in nss, but I guess we can't avoid it and still implement this.
Attachment #315665 - Flags: review?(aaronr) → review+
Comment on attachment 315665 [details] [diff] [review]
patch

> 		layout \
> 		transformiix \
> 		schemavalidation \
> 		intl \
> 		pref \
> 		htmlparser \
> 		exthandler \
> 		chardet \
>+                pipnss \
You're using spaces, not tabs.

> # Standard Mozilla make rules
> EXTRA_DSO_LDOPTS = \
> 		$(MOZ_COMPONENT_NSPR_LIBS) \
> 		$(LIBXUL_DIST)/lib/$(LIB_PREFIX)unicharutil_s.$(LIB_SUFFIX) \
> 		$(XPCOM_GLUE_LDOPTS) \
>+                $(DIST)/lib/nss3.$(LIB_SUFFIX) \
Same here
Attachment #315665 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #14)
> (From update of attachment 315665 [details] [diff] [review])
> nit: You should clarify what a 'computed expression' is.  Someone reading the
> code shouldn't have to go to the spec to see exactly what that term means. 
> With that r=me.
> I'm not a big fan of having to bring in nss, but I guess we can't avoid it and
> still implement this.

The spec doesn't make it clear in my mind. I had to get clarification from John Boyer. I suppose I could say 'an expression in a MIP or a bind'.

No we cannot avoid nss. The code behind the interfaces I used are in nss. At least we can hide behind a relatively simple interface instead of using the HASH_ functions directly.
(In reply to comment #15)
> (From update of attachment 315665 [details] [diff] [review])
> > 		layout \
> > 		transformiix \
> > 		schemavalidation \
> > 		intl \
> > 		pref \
> > 		htmlparser \
> > 		exthandler \
> > 		chardet \
> >+                pipnss \
> You're using spaces, not tabs.
> > # Standard Mozilla make rules
> > EXTRA_DSO_LDOPTS = \
> > 		$(MOZ_COMPONENT_NSPR_LIBS) \
> > 		$(LIBXUL_DIST)/lib/$(LIB_PREFIX)unicharutil_s.$(LIB_SUFFIX) \
> > 		$(XPCOM_GLUE_LDOPTS) \
> >+                $(DIST)/lib/nss3.$(LIB_SUFFIX) \
> Same here

Arghh. I can't seem to get VisualSlick to use tabs instead of spaces unless I copy the line above and then change the text, but I'll fix it.
Posted patch patchSplinter Review
Patch for check-in. Adding comment to define 'computed expression' and changing spaces to tabs in the makefile.
Attachment #315665 - Attachment is obsolete: true
I needed to change 
$(DIST)/lib/nss3.$(LIB_SUFFIX) \
to
$(LIBXUL_DIST)/lib/$(LIB_PREFIX)nss3$(DLL_SUFFIX) \

Aaron, could you check that this is ok also on Windows?
On linux this works both with libxul and non-libxul builds

(the change is also for bug 428445)
Attachment #322506 - Flags: review?(aaronr)
Or Alex, if you could test?
The makefile changes in the shared lib patch causes a linker error: ../../dist/lib/nss3.dll : fatal error LNK1107: invalid or corrupt file: cannot read at 0x2D0

The linker expects an import library, so the line must be:
$(LIBXUL_DIST)/lib/$(LIB_PREFIX)nss3.$(LIB_SUFFIX)

I'd bet that both LIB_SUFFIX and DLL_SUFFIX evaluate to .so on Linux so maybe the above would work for both Linux and Windows.
nope, LIB_SUFFIX seems to evaluate to .a and DLL_SUFFIX to .so
Olli,
Have you found a solution for the Linux build? Is there any variable we can key off of to ifdef the makefile to use the line I have when on Windows and the other way for Linux?
Attachment #322506 - Flags: review?(aaronr) → review+
Hey Olli, will this work for you on Linux?  Seems to compile on Windows.  Merle, can you test on windows to make sure this works for you?
Attachment #322506 - Attachment is obsolete: true
Attachment #330291 - Flags: review?(Olli.Pettay)
I tested it on a fresh checkout and it builds and runs correctly.
Comment on attachment 330291 [details] [diff] [review]
how about this?

compiles here
Attachment #330291 - Flags: review?(Olli.Pettay) → review+
Attachment #330291 - Flags: review?(msterlin)
r+. I'm not authorized to add the r+ directly.
Comment on attachment 330291 [details] [diff] [review]
how about this?

adding Merle's r+.  I'll get this checked in today.
Attachment #330291 - Flags: review?(msterlin) → review+
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch-11
Attachment #330854 - Flags: review?(aaronr)
Attachment #330854 - Flags: review?(aaronr) → review+
Attachment #330854 - Flags: review?(Olli.Pettay)
Attachment #330854 - Flags: review?(Olli.Pettay) → review+
checked in 1.8 branch patch for msterlin
Keywords: fixed1.8.1.17
Whiteboard: xf-to-branch-11
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.