Closed
Bug 426671
Opened 16 years ago
Closed 16 years ago
[1.1] Implement XPath function digest()
Categories
(Core Graveyard :: XForms, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: msterlin, Assigned: msterlin)
Details
(Keywords: fixed1.8.1.17)
Attachments
(8 files, 4 obsolete files)
2.78 KB,
text/xml
|
Details | |
2.26 KB,
text/xml
|
Details | |
1.10 KB,
text/xml
|
Details | |
1.16 KB,
text/xml
|
Details | |
1.14 KB,
text/xml
|
Details | |
7.13 KB,
patch
|
Details | Diff | Splinter Review | |
7.55 KB,
patch
|
smaug
:
review+
aaronr
:
review+
|
Details | Diff | Splinter Review |
16.05 KB,
patch
|
aaronr
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
Assignee | ||
Comment 3•16 years ago
|
||
Assignee | ||
Comment 4•16 years ago
|
||
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Comment 6•16 years ago
|
||
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-
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 10•16 years ago
|
||
Testcase e should throw xforms-binding-exception not xforms-compute-exception.
Attachment #313253 -
Attachment is obsolete: true
Assignee | ||
Comment 11•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #315665 -
Flags: review?(Olli.Pettay)
Comment 12•16 years ago
|
||
Any tests for the optionality of the 3rd parameter?
Assignee | ||
Comment 13•16 years ago
|
||
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 14•16 years ago
|
||
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 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
(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.
Assignee | ||
Comment 17•16 years ago
|
||
(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.
Assignee | ||
Comment 18•16 years ago
|
||
Patch for check-in. Adding comment to define 'computed expression' and changing spaces to tabs in the makefile.
Attachment #315665 -
Attachment is obsolete: true
Comment 19•16 years ago
|
||
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)
Comment 20•16 years ago
|
||
Or Alex, if you could test?
Assignee | ||
Comment 21•16 years ago
|
||
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.
Comment 22•16 years ago
|
||
nope, LIB_SUFFIX seems to evaluate to .a and DLL_SUFFIX to .so
Assignee | ||
Comment 23•16 years ago
|
||
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+
Comment 24•16 years ago
|
||
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)
Assignee | ||
Comment 25•16 years ago
|
||
I tested it on a fresh checkout and it builds and runs correctly.
Comment 26•16 years ago
|
||
Comment on attachment 330291 [details] [diff] [review] how about this? compiles here
Attachment #330291 -
Flags: review?(Olli.Pettay) → review+
Attachment #330291 -
Flags: review?(msterlin)
Assignee | ||
Comment 27•16 years ago
|
||
r+. I'm not authorized to add the r+ directly.
Comment 28•16 years ago
|
||
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+
Comment 29•16 years ago
|
||
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch-11
Assignee | ||
Comment 30•16 years ago
|
||
Attachment #330854 -
Flags: review?(aaronr)
Attachment #330854 -
Flags: review?(aaronr) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #330854 -
Flags: review?(Olli.Pettay)
Updated•16 years ago
|
Attachment #330854 -
Flags: review?(Olli.Pettay) → review+
Comment 31•16 years ago
|
||
checked in 1.8 branch patch for msterlin
Keywords: fixed1.8.1.17
Whiteboard: xf-to-branch-11
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•