Need to be able to view the machine readable policy in a friendly format where tags are replaced with some bioilerplate format, a la IE6. Also need to provide a way to easily view the human readable policy. This will most likely be implemented as a new tab in Page Info dialog.

Try to get as much as possible done in 0.9.9, although likely work will spill into 1.0.

FYI: It looks like there will be changes that affect localization even after 3/29.

Ccing John since he is testing P3P.

This is a small Perl script that reads a sitelist.txt in the same directory as the .pl file, tries to fetch the well known P3P policy located at <url>/w3c/p3p.xml and repotrs the results. HTTP OK result with an XML mime type is most likely a real policy. OK result with wrong mime type is probably a badly behaving webserver (sends HTML error page but claims success). HTTP error means most likely there was no policy file. The program will read the siteslist where every URL (without http:// part) is on a line of its own, and reports the result of the fetch attempt in stdout. Finally it will print a summary.

Quick review comments: 1) Please rename transform.xsl to p3p.xsl. 2) Make sure that all the licenses are the latest and greatest triple licenses. At least the xsl file needs a change, probably some others as well. 3) Is the page info overlay also too generic, should it be P3P overlay or page info P3P overlay or something? Same with property names etc. I started working on making the xslt stylesheet localizable...

a few nits on the stylesheet, nbsp it not ' ', but <!ENTITY nbsp CDATA "&#160;" -- no-break space = non-breaking space, U+00A0 ISOnum --> I found two occurences of To see the full human readable policy, click <a href="{@discuri}">here</a>.<br/> Don't show that to mpt, make it something like See the full <href="{@discuri}">human readable policy</a>.<br/> xsl:if doesn't change the context, so just do "@discuri", that's faster. Can there be a p3p:DATA without a p3p:ENTITY parent? (speed again) You may wanna put @ref into a var, and just test="$ref = '#business....'" You have alot of //p3p:foo expressions, I wonder if the schema of p3p is really that fuzzy. Every // gonna hit you in the jimmy, perf-wise. You might wanna add modes for PURPOSE and ACCESS and all the others, instead of adding a parent the patterns. Didn't read all the texts, I guess you get the idea.

I was told I was fuzzy on at least one, so here an example: +<xsl:template match="p3p:ACCESS"> + <xsl:apply-templates select="./node()"/> +</xsl:template> + +<xsl:template match="p3p:ACCESS/p3p:nonident"> + <li>This web site does not collect any personally identifiable information about you.</li> +</xsl:template> could be +<xsl:template match="p3p:ACCESS"> + <xsl:apply-templates mode="ACCESS" select="node()"/> <!-- or just "*" ? --> +</xsl:template> + +<xsl:template mode="ACCESS" match="p3p:nonident"> + <li>This web site does not collect any personally identifiable information about you.</li> +</xsl:template> this speeds you up in two ways, the list of matchable templates gets smaller, plus the pattern gets simpler.

Thanx Axel. Will apply your suggestions.

I have now made the XSLT stylesheet localizable (barely, there are some issues but I think we can live with that for now). However, I am now stuck as this does not work anymore. I get an assertion "Cannot set a second transform mediator" and the page loads blank.

1. Download dtd, xsl and xml into one folder. 2. Put dtd in bin/res/dtd 3. Open p3p.xml

<xsl:if test = ".[@required='opt-in']"> can be simplified to <xsl:if test = "@required='opt-in'"> in quite a few places <xsl:apply-templates select="./node()"/> can be <xsl:apply-templates select="node()" /> or simply <xsl:apply-templates/> I think this is the problem: The p3p-namespace in the stylesheet is different from the one in the stylesheet (and the stylesheet seems to think that the root-element is called POLICIES), which means that no elements gets constructed so when we notify contentsink we havn't got a root-content which means that we end up in the else { // Transform failed part of nsXMLContentSink::Observe, which seems bogusly assert

err, i mean "different from the one in the document" :-)

I updated the namespaces, added the POLICIES root element and put the js into a CDATA section.Then it works. Anyway, glancing over the spec, I expect more changes to at least the testcase. I had to add a local copy of p3p.dtd, 'cause there is that patch to load dtds from file: in my tree. That means, we need to do the right thing once we want to ship p3p.xsl, which may make it end up in a chrome URL. Not sure if we want a bug to try to load dtd from res/dtd if they're not found for chrome.

Posting this patch so that peterv can debug.

The latest DTD and XSL now work, and the DTD makes it localizable. My biggest problem was the wrong namespace (thanks for noticing!), and I also added CDATA section and made the root match on "/". I did not do any of the other suggestions. Also, once the stylesheet is deemed "final", we should duplicate all the namespace specific parts so this works on all the namespaces that are in use. If the stylesheet is too big and causes unacceptable performance we can split it to namespace specific stylesheets and trigger the correct one programmatically.

Axel Wrote: >I found two occurences of >To see the full human readable policy, click <a >href="{@discuri}">here</a>.<br/> This is the requested format. Don't show that to mpt, make it something like >Can there be a p3p:DATA without a p3p:ENTITY parent? (speed again) Yes there can be.

Note: Until a crash in layout is fixed ( peterv knows about this ) clicking on "View Summary" button would populate the transformed document in a small chromeless window ( this window can be resized however will not have a scroll bar ). This is a huge blocker for p3p and has to fixed either by the layout folks or someone who knows layout/scrolling.

In my previous patch I forgot to update a chrome URL. Please ignore patch v1.3 and use this instead.

Optimized Windows build from today with P3P: http://green.nscp.aoltw.net/heikki/mozilla-p3p.tgz Known problems: * Summary comes up in small window that you have to resize, and there are no scrollbars. You can scroll with arrow keys once you have clicked in the content area. * The full summary is generated properly only on sites with the exact policy version we have in the XSLT sheet. It works for www.att.com, for example. We will make a full stylesheet soon (just duplicating entries).

The binary was also updated. Other known things that don't work: * Keyboard accelerators: can't select privacy tabs with access keys, can't select links with access keys, buttons should be inactive if nothing (link) is selected * In XSLT stylesheet: target policy by name (support multiple POLICY elements). * Help button and help text (don't exist). One nit about the generated XHTML: we should probably use id attributes instead of name. No biggie if they both work.

Comment on attachment 77595 [details] [diff] [review] patch v1.3.2 !! [ force the result document to be xml to please transformiix ] Please write Doxygen style comments for all IDL interfaces and their members. It makes understanding this code a lot easier. Do we need to generate three (I think) different xpt files for P3P, wouldn't one be enough? >Index: extensions/p3p/jar.mn >=================================================================== >Index: extensions/p3p/makefile.win >=================================================================== >Index: extensions/p3p/public/Makefile.in >=================================================================== >- nsIP3PService.idl \ >+ nsIP3PService.idl \ >+ nsIPolicyReference.idl\ >+ nsIPolicyListener.idl \ >+ nsIPolicyTarget.idl \ I believe these HAVE to be tab characters to work on Linux/some other Unices, spaces won't work. Fix these in all Makefile.in files. Or if I am wrong, just ignore this... >Index: extensions/p3p/public/makefile.win >=================================================================== >- .\nsIP3PService.idl \ >+ .\nsIP3PService.idl \ >+ .\nsIPolicyReference.idl \ >+ .\nsIPolicyListener.idl \ >+ .\nsIPolicyTarget.idl \ This will work for makefile.win, but please either make all be spaces or all be tabs. >Index: extensions/p3p/public/nsIP3PService.idl >=================================================================== > interface nsIP3PService : nsISupports { >- long getConsent(in string aURI, >- in nsIHttpChannel aHttpChannel); >+ > }; Why would we need an interface without methods? >Index: extensions/p3p/public/nsIPolicyListener.idl >=================================================================== >+interface nsIPolicyListener; You don't need that. >Index: extensions/p3p/public/nsIPolicyReference.idl >=================================================================== >Index: extensions/p3p/public/nsIPolicyTarget.idl >=================================================================== >Index: extensions/p3p/public/nsP3PCIID.h >=================================================================== >Index: extensions/p3p/resources/content/contents.rdf >=================================================================== I don't have a clue about .rdf files so you need someone else to review this. >Index: extensions/p3p/resources/content/nsPolicyViewer.js >=================================================================== This file needs the proper license block. I am also wondering of some of this logic should be moved into native code, because embedders are going to have to implement this stuff if we don't provide it in native code. Will need to discuss this. JS is not my strongest area, especially when used with XUL, so I'd suggest seeking additional reviews for all JS & XUL etc. files as well. >+ * No magic constructor behaviour, as is de rigeur for XPCOM. >+ * If you must perform some initialization, and it could possibly fail (even >+ * due to an out-of-memory condition), you should use an Init method, which >+ * can convey failure appropriately (thrown exception in JS, >+ * NS_FAILED(nsresult) return in C++). >+ * >+ * In JS, you can actually cheat, because a thrown exception will cause the >+ * CreateInstance call to fail in turn, but not all languages are so lucky. >+ * (Though ANSI C++ provides exceptions, they are verboten in Mozilla code >+ * for portability reasons -- and even when you're building completely >+ * platform-specific code, you can't throw across an XPCOM method boundary.) What is the deal with those comments, I don't understand. Could they be removed? >+ var links = gTopDoc.getElementsByTagName("LINK"); >+ for (index = 0; index < links.length; index++) { It would be better to get links.length into a variable outside the loop because this could result into a function call to native code in every iteration. >+ if (links[index].getAttribute("rel") == "P3Pv1") { Is HTML DOM case insensitive for both element and attribute names? >+ QueryInterface: function (iid) { >+ if (!iid.equals(Components.interfaces.nsISupportsWeakReference)) { >+ throw Components.results.NS_ERROR_NO_INTERFACE; >+ } >+ return this; >+ } This seems strange, but then again I don't understand our JS stuff that well... >+ var nodelist = gXMLHttpRequest.responseXML.getElementsByTagName("POLICY"); >+ if (nodelist.length > 0) { >+ for (index = 0; index < nodelist.length; index++) { Same thing here, get the length of the list into a variable instead of using it 'raw' and potentially causing function calls. >+ if (gXMLHttpRequest.status == 200 || gXMLHttpRequest == 300) { Did you mean to compare if the |status| was 300? This happens in all of the |view...| functions. >+ var nodelist = gXMLHttpRequest.responseXML.getElementsByTagName("POLICY"); >+ if (nodelist.length > 0) { >+ for (index = 0; index < nodelist.length; index++) { And once again that loop stuff... >+ var errorMessage = getBundle().GetStringFromName("LoadFailed") + " " + gSelectedURI + "."; I don't believe this is localizable, shouldn't you have the whole string as single property. In the properties file it would be something like: LoadFailed = Cannot load privacy policy for %S. and then you'd need to replace that %S somehow in JS with the URL. I know how to do that in C++. Will continue with p3p.xsl...

> and then you'd need to replace that %S somehow in JS with the URL. I know > how to do that in C++. formatStringFromName in nsIStringBundle is your friend

Comment on attachment 77595 [details] [diff] [review] patch v1.3.2 !! [ force the result document to be xml to please transformiix ] >Index: extensions/p3p/resources/content/p3p.xsl >=================================================================== >+ <!ENTITY nbsp " "> Like Axel said, isn't this entity wrong? Or did you really mean |space|? >+ <xsl:value-of select="."/>&p3p.business.name.sep; This separator stuff does not localize very well. This is further complicated by the fact that we order stuff using XSLT. However, since The US is the most important market for P3P right now I think we can live with this slight glitch. >+ <xsl:attribute name="style"> >+ <xsl:text>text-decoration:underline;</xsl:text> >+ <xsl:text>color:blue;</xsl:text> How come we need style rules for links, shouldn't they get their coloring automatically? Also, once peterv's dynamic style lands we don't need individual style attributes: we can just create one style element and put stuff in there. And we need to duplicate the rules for other namespaces... Also, I don't really know XSLT so you will need someone else to look at this as well. >+ >+ >+ >+ >+ Some unneeded newlines at the end. >Index: extensions/p3p/resources/content/p3pDummy.xml >=================================================================== >\ No newline at end of file Add that newline so diff looks nicer :) But see below... >Index: extensions/p3p/resources/content/p3pSummary.xul >=================================================================== >+<window height="700" >+ width="600" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ xmlns:html="http://www.w3.org/1999/xhtml" >+ onload="renderMachineReadable();"> Should we persist the size and coordinates of the window? Now it always comes up the same size (too small usually) in the upper left corner. >+ <html:iframe id="content-frame" src="chrome://p3p/content/p3pDummy.xml" flex="1"/> Is there any way we could avoid this hack? If we don't have XML doc in the iframe, once we place the transformed document into the frame there are several things that don't work, like links etc. What gives? Also add a comment stating the purpose for the dummy XML file there. Since this is out of my expertise get someone else to look at this too. >Index: extensions/p3p/resources/content/pageInfoOverlay.js >=================================================================== >+ var linkTypes = >+ [ >+ // Tag Attr List node ID >+ ["a", "href", "linkKids"], >+ ["applet", "code", "appletKids"], >+ ["area", "href", "imageMapKids"], >+ ["form", "action", "formKids"], >+ ["frame", "src", "frameKids"], >+ ["iframe", "src", "frameKids"], >+ ["img", "src", "imageKids"], >+ ["image", "src", "imageKids"], >+ ["link", "href", "externalDocKids"], >+ ["object", "codebase", "objectKids"], >+ ["script", "src", "scriptKids"] What about simple XLinks that are defined by xlink:type="simple" attribute? ;) >+ for (i = 0; i < linkTypes.length; ++i) >+ for (j = 0; j < list.length; ++j) >+ if (aWin && aWin.frames.length > 0) >+ for (i = 0; i < aWin.frames.length; ++i) >+ for (i = 0; i < elts.length; ++i) The loop stuff... >Index: extensions/p3p/resources/content/pageInfoOverlay.xul >=================================================================== >Index: extensions/p3p/resources/content/test.xul >=================================================================== This file should not go in! >Index: extensions/p3p/resources/locale/en-US/contents.rdf >=================================================================== >+ chrome:localeVersion="0.9.9"/> Just curious: when will this version change to 1.0.0 or something? >Index: extensions/p3p/resources/locale/en-US/p3p.dtd >=================================================================== >Index: extensions/p3p/resources/locale/en-US/p3p.properties >=================================================================== >+LoadFailed = Cannot load privacy policy for See above for suggestion regarding this. >Index: extensions/p3p/resources/locale/en-US/pageInfoOverlay.dtd >=================================================================== >Index: extensions/p3p/resources/locale/en-US/transform.dtd >=================================================================== >Index: extensions/p3p/src/Makefile.in >=================================================================== >+ string \ >+ necko \ >+ pref \ >+ cookie \ >+ xmlextras \ >+ dom \ >+ $(NULL) >+ nsP3PService.cpp \ >+ nsPolicyReference.cpp \ >+ nsCompactPolicy.cpp \ >+ nsP3PModule.cpp \ Use tabs. >Index: extensions/p3p/src/makefile.win >=================================================================== >\ No newline at end of file Add newline. >Index: extensions/p3p/src/nsCompactPolicy.cpp >=================================================================== >Index: extensions/p3p/src/nsCompactPolicy.h >=================================================================== >Index: extensions/p3p/src/nsP3PModule.cpp >=================================================================== >Index: extensions/p3p/src/nsP3PService.cpp >=================================================================== >+ : mCompactPolicy(0) nsnull instead of 0 if pointer? >\ No newline at end of file Add newline. >Index: extensions/p3p/src/nsP3PService.h >=================================================================== >+ nsCompactPolicy* mCompactPolicy; Could this be strong ref (nsCOMPtr) or would that create circularity? If weak on purpose, add comment. >Index: extensions/p3p/src/nsP3PUtil.h >=================================================================== >+#define NOTIFY_IF_FAILURE(_result, _listener) \ Seems like this macro is not used anywhere. It would be better if you made a class nsP3PUtil and have all these functions be static members. Declare in .h and define in .cpp. Also, do we have a convention of calling this Utils instead of Util? >+GetAttributeValue(nsIDOMNode* aNode, >+ char* aAttrName, >+ nsAString& aAttrValue) >+{ Check in parameters are not null before use? Do this in all of the functions in this file... >+ nsresult rv = aNode->GetAttributes(getter_AddRefs(attributeNodes)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ attributeNodes->GetLength(&attrCount); Are we guaranteed to get nodelist if succesfull return? I would have guessed it would be legal to return null nodelist if no attributes. >+ for (PRUint32 i = 0; i < attrCount; i++) { Move definition of |i| outside the loop. >+ rv = attributeNodes->Item(i, getter_AddRefs(attributeNode)); >+ NS_ENSURE_SUCCESS(rv, rv); Would it be better to see if we got node instead of rv in this case? >+ domAttr = do_QueryInterface(attributeNode, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ attributeNode->GetLocalName(name); >+ if (!name.IsEmpty() && name.EqualsIgnoreCase(aAttrName)) { >+ rv = domAttr->GetValue(aAttrValue); Move QI for domAttr into the |if|, that way you can also declare it there. +void SkipWhitespace(nsAString::const_iterator& aStart, + nsAString::const_iterator& aEnd) +{ + nsAString::const_iterator end = aEnd; Since you don't change |end|, you can get rid of the variable and add |const| to |aEnd| parameter type. >+// This routine checks to see if the uri, >+// specified with INCLUDE, EXCLUDE, >+// EMBEDDED-INCLUDE, EMBEDDED-EXCLUDE, includes >+// the current path. Note: The '*' character is >+// treated as a wildcard. >+// Ex. http://www.*.com/* == http://www.netscape.com/* >+static PRBool >+IsPathIncluded(nsAString& aURI, >+ nsACString& aPath) Hmm... I don't understand which one can contain wildcards. Give an example in the comments. Also shouldn't both parameters be |const|? >+ while (rbegin != rend) { >+ PRUnichar rcurr = *rbegin; >+ if (rcurr == '*') { >+ SkipWhitespace(++rbegin, rend); >+ if (rbegin != rend) { >+ rcurr = *rbegin; >+ while (pbegin != pend && (*(++pbegin) != '.' && *pbegin != '/')); // skip until '.' or '/' >+ } >+ else { >+ break; >+ } >+ } >+ >+ if (PRUnichar(*pbegin) == rcurr) { >+ ++rbegin; >+ if (pbegin != pend ) { >+ ++pbegin; >+ } >+ } >+ else { >+ rv = PR_FALSE; >+ break; >+ } >+ } I am concerned that we could get UMR here... For example, take the inner |while| loop and say pbegin = pend -1: first test passes ok, then we advance pbegin so it is pend, and then compare. Is that ok, does pend point to the last character or beyond? >+DeterminePolicyScope(nsAutoVoidArray& aNodeList, >+ const char* aPath, >+ PRBool* aPathInScope) >+{ |const| missing from aNodeList? Should we set aPathInScope to false in the beginning? >+ for (PRInt32 i = 0; i < count && !(*aPathInScope); i++) { Move |i| outside. >+ nsIDOMNode* node = NS_STATIC_CAST(nsIDOMNode*, aNodeList.ElementAt(i)); >+ NS_ENSURE_TRUE(node, NS_OK); I'd prefer an assertion plus "if (!node) break". >+ node->GetFirstChild(getter_AddRefs(child)); >+ NS_ENSURE_TRUE(child, NS_OK); Same here. >+ nsresult result = aNode->GetChildNodes(getter_AddRefs(children)); >+ NS_ENSURE_SUCCESS(result, result); >+ children->GetLength(&count); Again is this safe? >+ for (PRUint32 i = 0; i < count; i++) { |i| out. >+ result = children->Item(i, getter_AddRefs(node)); >+ NS_ENSURE_SUCCESS(result, result); Would prefer assertion and "if (!node) break". >Index: extensions/p3p/src/nsPolicyReference.cpp >=================================================================== >+nsPolicyReference::HandleEvent(nsIDOMEvent *aEvent) >+ PRUint32 status = 200; >+ mXMLHttpRequest->GetStatus(&status); There is no need to set the value of status before call, but you should check the return value because it might return NS_ERROR_NOT_AVAILABLE in which case status would be uninitialized. >+ result = (mCurrentURI) ? mCurrentURI->Equals(mMainURI, &same) : NS_ERROR_FAILURE; >+ NS_ENSURE_SUCCESS(result, result); Does this really warrant "throwing an error"? >+ mXMLHttpRequest->GetStatus(&status); See comments above. >+// Call this method to load policy reference file for a given URI >+// >+// @param aFlag - Describes the state aURI. Possible values are: I am not sure if Doxygen can read C++ comments, maybe replace these with C-style comments? >+ // We already have a policy for the main URI. >+ // Note that if we're handling an embedded URI then >+ // we should first make sure that the embedded URI is >+ // not covered by the main URI's policy. Also, the >+ // embedded URI would become the selected URI if the >+ // embedded URI is not covered by the main URI's policy Do we support this? Is this XXX TODO? >+ // known location so try the policy refereced by the Typo: referenced. >+ // html LINK tag. Do we support XHTML link tag? >+nsPolicyReference::Load(const char* aURI) Check that we did not get null or empty string? >+ mXMLHttpRequest = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID); >+ NS_ENSURE_TRUE(mXMLHttpRequest, NS_ERROR_OUT_OF_MEMORY); do_CreateInstance() can take additional rv argument. Do that, and return that rv if error. >+ result = mXMLHttpRequest->Send(nsnull); >+ >+ return result; return mXMLHttpRequest->Send(nsnull); >+nsPolicyReference::ProcessPolicyReferenceFile(nsIDOMDocument* aDocument, >+ char** aPolicyLocation) >+{ >+ >+ NS_ENSURE_ARG_POINTER(aDocument); Test aPolicyLocation as well. >+ nsresult result = aDocument->GetDocumentElement(getter_AddRefs(domElement)); >+ NS_ENSURE_SUCCESS(result, result); >+ >+ nsCOMPtr<nsIDOMNode> root(do_QueryInterface(domElement)); >+ NS_ENSURE_SUCCESS(result, result); You can remove the first NS_ENSURE_SUCCESS, and fix the second one to test for |root|. >+ result = aDocument->GetElementsByTagName(NS_LITERAL_STRING("POLICY-REFERENCES"), getter_AddRefs(policyReferencesElement)); >+ NS_ENSURE_SUCCESS(result, result); >+ policyReferencesElement->GetLength(&count); Again I suspect this might not be safe. >+ if (policyLocation.Find("#", PR_TRUE, 0, 1) != -1) { Or see that !IsEmpty() and First() == '#'. >+ policyLocation = >+ PromiseFlatString(nsDependentString(NS_LITERAL_STRING("/w3c/p3p.xml").get(), 12) + policyLocation); I don't like how this looks, maybe introduce a temporary to do this without the dependent and flat part? >+ *aPolicyLocation = ToNewCString(absURI); Out of memory check missing. >+nsPolicyReference::ProcessPolicyRefElement(nsIDOMDocument* aDocument, >+ for (PRUint32 i = 0; i < count; i++) { |i| out. >+ nsCOMPtr<nsIDOMNode> node; >+ result = aNodeList->Item(i, getter_AddRefs(node)); >+ NS_ENSURE_SUCCESS(result, result); Would prefer assert and "if (!node) break". >+ result = ProcessPolicyRefChildren(node); >+ NS_ENSURE_SUCCESS(result, result); Would prefer assert and break. >+ result = aNodeList->Item(0, getter_AddRefs(node)); // There ought to be only one EXPIRY element >+ NS_ENSURE_SUCCESS(result, result); Safer to check node? >+ char cdate[100]; >+ date.ToCString(cdate, date.Length() + 1); Buffer overrun, eek! >+ // XXX - implement reldate Do we need this? >Index: extensions/p3p/src/nsPolicyReference.h >=================================================================== >Index: layout/xul/base/src/outliner/src/nsOutlinerContentView.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/outliner/src/Attic/nsOutlinerContentView.cpp,v Seems like this file was removed. >Index: layout/xul/base/src/tree/src/nsTreeContentView.cpp >=================================================================== I don't know this stuff. Is this part of some other bug or what? Done!

nsOutlinerContentView.cpp changes should not be part of this patch. The nsTreeContentView.cpp changes are part of this bug to fix a very visible defect in the ex-outliner/now-tree content model code. Specifically, when you initialize a closed container with treeitems they show up parallel to the container rather than not showing up at all (since the container is closed). The treeitems should only show up when the twisty for the container is clicked open. CC'ing hewitt to review this part of the patch that was from my front-end contribution here. Hewitt could also review the XUL/JS code while he's here. :o)

>+ <!ENTITY nbsp " "> This is nothing!. I meant to remove that. >How come we need style rules for links, shouldn't they get their coloring >automatically? Good question. Apparently, the transformed document somehow loses the automatic link coloring. peterv? >Index: extensions/p3p/public/nsIP3PService.idl >=================================================================== > interface nsIP3PService : nsISupports { >- long getConsent(in string aURI, >- in nsIHttpChannel aHttpChannel); >+ > }; Initially, cookie manager was dependent on it. Now, it doesn't. I think we can remove the idl. >+ * No magic constructor behaviour, as is de rigeur for XPCOM. >+ * If you must perform some initialization, and it could possibly fail (even >+ * due to an out-of-memory condition), you should use an Init method, which >+ * can convey failure appropriately (thrown exception in JS, >+ * NS_FAILED(nsresult) return in C++). >+ * >+ * In JS, you can actually cheat, because a thrown exception will cause the >+ * CreateInstance call to fail in turn, but not all languages are so lucky. >+ * (Though ANSI C++ provides exceptions, they are verboten in Mozilla code >+ * for portability reasons -- and even when you're building completely >+ * platform-specific code, you can't throw across an XPCOM method boundary.) >What is the deal with those comments, I don't understand. Could they be >removed? Yikes!. What the heck is that? Will removed it. >+ if (links[index].getAttribute("rel") == "P3Pv1") { >Is HTML DOM case insensitive for both element and attribute names? Donno. Have to check. >+ QueryInterface: function (iid) { >+ if (!iid.equals(Components.interfaces.nsISupportsWeakReference)) { >+ throw Components.results.NS_ERROR_NO_INTERFACE; >+ } >+ return this; >+ } >This seems strange, but then again I don't understand our JS stuff >that well... This is required to JS wrapper happy when handling weak refs. >+ if (gXMLHttpRequest.status == 200 || gXMLHttpRequest == 300) { >Did you mean to compare if the |status| was 300? This happens in >all of the |view...| functions. Yeah...good catch. >+ var errorMessage = getBundle().GetStringFromName("LoadFailed") + " " + >gSelectedURI + "."; >I don't believe this is localizable, shouldn't you have the whole string >as single property. I don't know how to that in JS either. Will ask samir. >+#define NOTIFY_IF_FAILURE(_result, _listener) >Seems like this macro is not used anywhere. I might be using this. >Also, do we have a convention of calling this Utils instead >of Util? Not sure. But will change it to Utils anyway. >Are we guaranteed to get nodelist if succesfull return? I would have >guessed it would be legal to return null nodelist if no attributes. Yes ( I think I checked that ).. >+ NS_ENSURE_SUCCESS(rv, rv); >Would it be better to see if we got node instead of rv in this case? It's important to propagate the error message back so that we can notify on an error. >+// Ex. http://www.*.com/* == http://www.netscape.com/* >+static PRBool >+IsPathIncluded(nsAString& aURI, >+ nsACString& aPath) >Hmm... I don't understand which one can contain wildcards. >Give an example in the comments. One can use wild characters in the policy reference file. i.e. you can something like <INCLUDE>http://*.netscape.com/*</INCLUDE> >I am concerned that we could get UMR here... For example, take the >inner |while| loop and say pbegin = pend -1: first test passes ok, >then we advance pbegin so it is pend, and then compare. Is that ok, >does pend point to the last character or beyond? This definitely needs work :) >+ nsIDOMNode* node = NS_STATIC_CAST(nsIDOMNode*, aNodeList.ElementAt(i)); >+ NS_ENSURE_TRUE(node, NS_OK); >I'd prefer an assertion plus "if (!node) break". I'm supposed to use NS_ENSURE_SUCCESS(...,...) instead. Becase, I need to propagate error messages. >+ result = (mCurrentURI) ? mCurrentURI->Equals(mMainURI, &same) : NS_ERROR_FAILURE; >+ NS_ENSURE_SUCCESS(result, result); >Does this really warrant "throwing an error"? This is one of the reasons why I have the macro NOTIFY_IF_FAILURE. I's thinking of using that instead. >+ // We already have a policy for the main URI. >+ // Note that if we're handling an embedded URI then >+ // we should first make sure that the embedded URI is >+ // not covered by the main URI's policy. Also, the >+ // embedded URI would become the selected URI if the >+ // embedded URI is not covered by the main URI's policy >Do we support this? Is this XXX TODO? uh? Why do you think it's TODO? It's all there. Is the comment giving you a wrong picture? >+ // html LINK tag. >Do we support XHTML link tag? Yes >+ *aPolicyLocation = ToNewCString(absURI); >Out of memory check missing. duh. >+ char cdate[100]; >+ date.ToCString(cdate, date.Length() + 1); >Buffer overrun, eek! oh...#$&% this code is still there? Will have to get rid of this.

>Index: extensions/p3p/resources/content/p3pSummary.xul >=================================================================== >+<window height="700" >+ width="600" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ xmlns:html="http://www.w3.org/1999/xhtml" >+ onload="renderMachineReadable();"> >Should we persist the size and coordinates of the window? Now it always >comes up the same size (too small usually) in the upper left corner. >+ <html:iframe id="content-frame" src="chrome://p3p/content/p3pDummy.xml" flex="1"/> >Is there any way we could avoid this hack? If we don't have XML doc >in the iframe, once we place the transformed document into the frame there >are several things that don't work, like links etc. What gives? All this hocus pocus can go away once the crash in transformiix is fixed. PettttteeeerV....need your help here :)

twalker run smoketests with P3P enabled, there were no problems.

* - * Contributor(s): + * Contributor(s): Harish Dhurvasula <harishd@netscape.com> * This is the wrong format for listing contributors, you're supposed to add new lines after the Contributor(s): line, not add to that line. - In nsP3PService.cpp, missing newline at end of file. - In nsP3PUtils::SkipWhitespace(): + PRBool skipped = PR_FALSE; + nsAString::const_iterator start = aStart; + + PRBool done = PR_FALSE; + while (start != aEnd && !done) { + switch (*start) { + case ' ' : case '\n' : + case '\r' : case '\t' : + ++start; break; + default : + done = PR_TRUE; break; + } + } + + aStart = start; Loose |done| alltogether and move the aStart = start code into the default case in the switch statement and return from there. Faster, and less code. - In nsP3PUtils::IsPathIncluded(): + PRBool done = (pbegin != pend) ? PR_FALSE : PR_TRUE; Why not: + PRBool done = pbegin == pend; Whow, this is confusing: + done = (pbegin != pend) ? (*pbegin == '.' || *pbegin == '/') : PR_TRUE; How about splitting this into an if/else in stead of this hard-to-read tri-statement? - nsP3PUtils::DeterminePolicyScope(const nsAutoVoidArray& aNodeList, const char* aPath, PRBool* aOut) Don't put nsAutoVoidArray in API's, use the baseclass nsVoidArray. Oh, and shouldn't that method also reset *aOut at the beginning? If it doesn't it might never get into the for loop. - nsP3PUtils::GetElementsByTagName(nsIDOMNode* aNode, const nsAString& aTagName, nsAutoVoidArray* aReturn) Again, no nsAutoVoidArray's in API's, and why is this a pointer, when the one in the method above it is a reference? - I see 200 and 300 used as magic status values here, couldn't those be in an interface, or at least #define'd somewhere for the C++ code? - In nsPolicyReference::ProcessPolicyReferenceFile(): + policyLocation = + PromiseFlatString(nsDependentString(NS_LITERAL_STRING("/w3c/p3p.xml").get(), 12) + policyLocation); What's up with the .get() and double wrapping of the literal string, why not just: + policyLocation = + PromiseFlatString(NS_LITERAL_STRING("/w3c/p3p.xml") + policyLocation); - nsPolicyReference::ProcessPolicyRefChildren(): + mError = !pathIncluded ? POLICY_LOAD_FAILURE : POLICY_LOAD_SUCCESS; Hmm, how about you loose the '!" and swap the values? This occurs in two places. Other than that, sr=jst

1) Now you changed all the tabs to spaces in the Makefile.in files! 2) You need to remove the IDL file that has the interface with no members. 3) There are still newlines at the end of the XSLT file. 4) I will now make the XSLT & DTD sheet accept all namespaces we support. 5) You need to add a comment explaining why the iframe loads the dummy XML file. 6) Still no persistence on the summary window? 7) Filed bug 135557 on myself to make P3P recognize simple XLinks. This can wait. 8) Missed one .length case: + for (j = 0; j < list.length; ++j) 9) You need to remove test.xul. 10) What is transform.dtd? I think you should remove that too. 11) Add newline to nsP3PService.cpp 12) You did not say if mCompactPolicy is weak on purpose (in which case you should add comment) or if it could be made into string ref by nsCOMPtr. 13) You need to remove nsP3PUtil.h (notice singular). 14) Do this: +void SkipWhitespace(nsAString::const_iterator& aStart, + const nsAString::const_iterator& aEnd) ====== 15) What I meant in the arguments to IsPathIncluded(), give sample arguments to the function and say what will happen. 16) Also IsPathIncluded() parameters should be |const|? 17) The condition is always true so instead of: + PRBool done = (pbegin != pend) ? PR_FALSE : PR_TRUE; say: + PRBool done = PR_FALSE; 18) DeterminePolicyScope() out paramer still not initialized in the beginning of the function. 19) We are still not using NOTIFY_IF_FAILURE, take it out. 20) I would make nsP3PUtils' default constructor and destructor private, and add a comment that you are supposed to use the static methods only. 21) Should GetAttributeValue() truncate aAttrValue on entering the function? 22) Change: + if (result != NS_ERROR_NOT_AVAILABLE && (status == 200 || status == 300)) { into (?) + if (NS_SUCCEEDED(result) && (status == 200 || status == 300)) { 23) You only added null check, also add check for empty string: +nsPolicyReference::Load(const char* aURI) 24) You did not change the flat & promise stuff. 25) I find that nsP3PUtils::ToCString() is practically useless function. 26) You need to remove the file that has been removed in CVS already.

JEEEEZZZZ heikki !!!! :)

I also have to make MAC makefile changes :(

27) Mac makefile changes 28) Comments in IDL for interfaces & methods and data members.

I made the stylesheet work on all 5 namespaces we recognize, but I think that makes it a bit slow, one site I tested took something like 5 seconds to bring up the summary on Win2k, 512MR RAM, 1.5GHz machine. (Besides, I did not solve all issues with this huge stylesheet and ended up with some duplication in the generated content that I did not want).

Also contains the five .xsl files and MAC changes ( hopefully, what I did was correct. Peterv, can you double check the mac makefile changes? Thanx ).

Latest opt win build: http://green.nscp.aoltw.net/heikki/mozilla-p3p-20020404.tgz This one has support for all the 5 P3P formats listed on W3C.

Comment on attachment 77801 [details] [diff] [review] patch v1.4 [ includes heikki's/jst's suggestions ] r=heikki

Comment on attachment 77801 [details] [diff] [review] patch v1.4 [ includes heikki's/jst's suggestions ] Fix the funky indentation (tabs?) in the new version of nsP3PUtils::SkipWhitespace(), and also in nsP3PUtils::IsPathIncluded(). sr=jst

Requesting permission to check in to the Netscape tree. harishd will attach the latest patch, after which all the serious bugs are the ones marked as blocking this. One crasher on one site (if you open P3P summary), other bugs affect usability of summary window. Smoketests passed on earlier builds, and we haven't really changed any code that could affect smoketests since then. The plan is to check in, enabled by default on Windows and Linux, and after we have tested on Mac, turn on the Mac as well. If someone builds P3P in the Mozilla tree this should just cleanly overwrite those object files.

Oh, and layout/xul/base/src/tree/src/nsTreeContentView.cpp needs to go into the Mozilla tree.

Comment on attachment 77961 [details] [diff] [review] patch v1.6 r=heikki

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0, pending sr=

This is now Bugscape http://bugscape/show_bug.cgi?id=13192

Comment on attachment 77961 [details] [diff] [review] patch v1.6 sr=jst

Landed in the Netscape tree, enabled on Windows and Linux.

removing Mach V keywords. I added them to the bugscape bug since that will be what we track.

I don't think this is topembed since it is in the NS tree now, removing keyword.

Comment on attachment 78400 [details] [diff] [review] Outliner patch [ proposed by sgehani modified by Jan Varga ] Outliner/tree issue is now bug 137178.

This bug is covered in bugscape ( bug 13192 ). Marking INVALID.

why is this invalid for Mozilla then?

Verified invalid. Lahmann, this feature is not in mozilla.

I know, but this sounds, as if a P3P Policy Viewer in't allowed to be in Mozilla

Kai: For now p3p will not be a part of mozilla. When a decision is made to include p3p I will open up a new bug.

> I know, but this sounds, as if a P3P Policy Viewer in't allowed to be in Mozilla If that were the case, the resolution would be WONTFIX. This feature was originally planned for Mozilla but is now only in the commercial builds (of course, it may return to Mozilla some time in the future). Therefore this bug now serves no purpose so it is INVALID.

Why is this INVALID? "Implement P3P Policy Viewer" is a valid RFE. This bug needs to be reopened OR perhaps mark this MOVED and open a new bug for the RFE for Mozilla. You may not want to work on implementing this, but others might.

Why are new p3p icons being checked into the *mozilla's* classic and modern skins, if mozilla.org has decided not to implement p3p? If it's Netscape only, can we ditch the bloat at least?

This isn't invalid, it's just not. At worst, it's a WONTFIX. It's a PERFECTLY valid request, and if NS doesn't want IT'S P3P code in Moz, fine, but being open source kinda bites you in the ass here, because someoen else can write some P3P code for this feature, not using any NS code.

Wontfix as per last comment

> This isn't invalid, it's just not. At worst, it's a WONTFIX. "WONTFIX - The problem described is a bug which will never be fixed." This contradicts the other stuff you said. Maybe LATER is closer. > It's a PERFECTLY valid request, and if NS doesn't want IT'S P3P code in Moz, > fine, but being open source kinda bites you in the ass here, because someoen > else can write some P3P code for this feature, not using any NS code. The Netscape code is in the tree. It's located at http://lxr.mozilla.org/seamonkey/source/extensions/p3p/. It's just not built by default right now.

Technically, I've been informed that WONTFIX can also mean the current component owner won't fix it, but somneone else is welcome to take it.

Just to be sure, this is going to be a netscape commercial feature only, right?

Please don't close bugs you don't own. Currently the P3P code is in Netscape's tree because MOZILLA.ORG DID NOT WANT IT IN MOZILLA TREE (the code that is in the mozilla.org tree is very outdated). However, our understanding and hope is that once we can show that the P3P code is good, mozilla.org will let us land the latest and greatest P3P code into the mozilla tree. Setting milestone to 1.2 alpha tentatively.

"Please don't close bugs you don't own." Who me? I didn't close anything you know!

I didn't close anything. It was RESOLVED INVALID, and I REOPENED it and changed it to RESOLVED WONTFIX since it's not really an invalid bug. I've never CLOSED a bug ever. PS: Now YOU reopened it, so that means YUOU'LL catch hell instead of me. ;) The coded attached isn't the issue, it's the idea that this is being resolved as INVALID saying that no P3P code will ever make it in the tree that's bother some. If this codee sucks, so be it. Then this should be WONTFIX or HELPWANTED or LATER, but not INVALID. Also, I'm glad you're on my side, from what I can read. P3P isn't evil, this implementation might be though. I'm not commenting on this patch itself.

Comment on attachment 77961 [details] [diff] [review] patch v1.6 There is no patch. Yet.

>it's the idea that this is being resolved as INVALID saying that no P3P code will >ever make it in the tree that's bother some. Okay, I buy that and may be marking the bug INVALID does give the impression that p3p is not valid in mozilla. However, marking it WONTFIX isn't any better. So, I'm going to mark this bug LATER and reopen the bug if the drivers@mozilla.org agree to accept p3p into mozilla. >If this codee sucks, so be it. Where did you get this idea from? If you think you can write superior code then PLEASE DO and make your contribution to mozilla.

Um, we've been trying to deprecate LATER. Reopening and reassigning to nobody.

->nobody

This keyboard is awful, the keys stick. I was NOT saying the code writer sucked. I am still getting used to this new KB. I was replying to the statement, "(the code that is in the mozilla.org tree is very outdated)". Meaning if THAT bit of source was outdated, bad, greek, whatever, then fine, I defer to the greater judgement of Drivers on that opinion. They're far more qualified in that area than I am. Please don't think I was insulting anyone. I have a fantastic amount of respect for everyone who has made Mozilla what it is today. But I will say that IE is made by epileptic chimpanzees. ;)

*** Bug 153990 has been marked as a duplicate of this bug. ***

taking P3P bugs

This was actually fixed when P3P landed in Mozilla a week or two ago. There are bugs right now, but we are working on it.

verified on 4/1 build