124503 - Implement P3P Policy Viewer

Status: VERIFIED FIXED

(Core :: XML, enhancement, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 3

[Jimmy Lee]

Ccing John since he is testing P3P.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: sitelist.txt

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: p3psites.pl

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.

Comment 6

[harishd]

Attachment: patch v1.1 [ support for policy viewer ]

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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...

Comment 8

[Axel Hecht]

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.

Comment 9

[Axel Hecht]

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.

Comment 10

[harishd]

Thanx Axel. Will apply your suggestions.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: p3p.dtd - WIP

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: p3p.xsl - WIP

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.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testacse: p3p.xml

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

<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

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 16

[Axel Hecht]

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.

Comment 17

[harishd]

Attachment: patch v1.2 [ Warning this patch would crash ]

Posting this patch so that peterv can debug.

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: p3p.dtd

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: p3p.xsl

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 21

[harishd]

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.

Comment 22

[harishd]

Attachment: patch v1.3 [ Supports http://www.w3.org/2001/09/P3Pv1 namespace only ]

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.

Comment 23

[harishd]

Attachment: patch v1.3.1

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

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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).

Comment 25

[harishd]

Attachment: patch v1.3.2 !! [ force the result document to be xml to please transformiix ]

Comment 26

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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...

Comment 28

[Axel Hecht]

> 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 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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!

Comment 30

[Samir Gehani]

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)

Comment 31

[harishd]

>+  <!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.

Comment 32

[harishd]

>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 :)

Comment 33

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 34

[harishd]

Attachment: patch v1.4 [ includes most of heikki's suggestions - I might have missed a few]

Comment 35

[Johnny Stenback (:jst)]

  *
- * Contributor(s):
+ * Contributor(s): Harish Dhurvasula <[e-mail]>
  *

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

Comment 36

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 37

[harishd]

JEEEEZZZZ heikki !!!! :)

Comment 38

[harishd]

I also have to make MAC makefile changes :(

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

27) Mac makefile changes

28) Comments in IDL for interfaces & methods and data members.

Comment 40

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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).

Comment 41

[harishd]

Attachment: patch v1.4 [ includes heikki's/jst's suggestions ]

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 ).

Comment 42

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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 43

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

r=heikki

Comment 44

[Johnny Stenback (:jst)]

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

Comment 45

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 46

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Oh, and

layout/xul/base/src/tree/src/nsTreeContentView.cpp

needs to go into the Mozilla tree.

Comment 47

[harishd]

Attachment: patch v1.6

Comment 48

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

r=heikki

Comment 49

[Jaime Rodriguez, Jr.]

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

Comment 50

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 51

[Johnny Stenback (:jst)]

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

sr=jst

Comment 52

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 53

[scottputterman]

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

Comment 54

[harishd]

Attachment: Outliner patch [ proposed by sgehani modified by Jan Varga ]

Comment 55

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 56

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 78400 [details] [diff] [review]
Outliner patch [ proposed by sgehani modified by Jan Varga ]

Outliner/tree issue is now bug 137178.

Comment 57

[harishd]

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

Comment 58

[Kai Lahmann (is there, where MNG is)]

why is this invalid for Mozilla then?

Comment 59

[John Unruh]

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

Comment 60

[Kai Lahmann (is there, where MNG is)]

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

Comment 61

[harishd]

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.

Comment 62

[Alex Bishop]

> 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.

Comment 63

[Phillip Stewart :P]

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.

Comment 64

[Jeremy M. Dolan]

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?

Comment 65

[Grey Hodge (jX)]

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.

Comment 66

[Grey Hodge (jX)]

Wontfix as per last comment

Comment 67

[Alex Bishop]

> 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.

Comment 68

[Grey Hodge (jX)]

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.

Comment 69

[HJ]

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

Comment 70

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 71

[HJ]

"Please don't close bugs you don't own." 

Who me? I didn't close anything you know!

Comment 72

[Grey Hodge (jX)]

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 73

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

There is no patch. Yet.

Comment 74

[harishd]

>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
[e-mail] 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.

Comment 75

[Christopher Hoess (gone)]

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

Comment 76

[Christopher Hoess (gone)]

->nobody

Comment 77

[Grey Hodge (jX)]

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. ;)

Comment 78

[Sander]

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

Comment 79

[Grace Bush]

taking P3P bugs

Comment 80

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 81

[Grace Bush]

verified on 4/1 build