Closed Bug 177822 Opened 22 years ago Closed 21 years ago

Move Netscape's P3P to Mozilla

Categories

(Core :: XML, defect)

Product:
Core
Component:
XML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hjtoi-bugzilla, Assigned: harishd)

Details

Attachments

(2 files, 3 obsolete files)

patch to build p3p by default in mozilla
1.02 KB, patch
Details | Diff | Splinter Review
patch v1.3
291.82 KB, patch
hjtoi-bugzilla
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
Permissions ok, let's get the patch for the Mozilla tree in here...
Attached patch patch v1.0 (obsolete) — — Splinter Review 
Netscape P3P --> Mozilla
Comment on attachment 104824 [details] [diff] [review]
patch v1.0 

this code uses the wrong free funciton in at least one case, it uses the wrong
license and years in many cases and has a bunch of other bugs. review sent by
email.
Attachment #104824 - Flags: needs-work+
please get db48x to approve your changes to the pageinfo dialog. some of them
look dead wrong, and others will probably make the dialog look bad.
Is there a site with a p3p policy where I can play with this? Are there
testcases I can play with?

My initial impression is ok, there's nothing that really makes the dialog ugly,
or breaks the rest of the dialog.

My concerns are that there doesn't appear to be a way of knowing if a site or
one of the links on the site has a policy without selecting each one and
clicking the policy button, and that the privacy tab in itself duplicates a lot
of the other information in the dialog. 

I think it may be better to find a way to present the p3p information in-line
with the information in the other tabs, for example by adding a column to the
trees that contains an icon, to indicate the p3p status (no policy/acceptable
policy/unacceptable policy). If that's not flexible enough, perhaps the best way
to arrange the information is to have the privacy tab simply display a summary
of the policy for the current document, with a button to open up another window
that contains a list of all linked-to items and their policies, as well as any
details that may be available for the current document but not listed in the
summary. This would be very much similar to the way the Security tab works, and
would allow a user to quickly view the tab and know that the page met their
preferences, and that nothing linked to from this page has a different policy.
(I'm assuming I read that part of the spec correctly, and that we implement it.
it's been a while).

Timeless: I went thro' your review and there are a few real concerns, such as
wrong free function, incorrect licence, etc., however, for the rest you're just
being an english teacher :-). I do agree that there are grammatical errors but I
doubt if I will have the time to correct 'em all. So, why don't you make those
changes after I land the code?
>Is there a site with a p3p policy where I can play with this? Are there
>testcases I can play with?

http://www.w3.org/P3P/compliant_sites
btw, timeless: Please post your comments in the bug. 
Response to timeless's review:




 > Index: extensions/p3p/README
 > ===================================================================
 > +and privacy policy viewing based on machine readable policy. To 
build p3p do
 > +the following:
 > +
 > +1) ac_add_options --enable-extensions=default,p3p
^ this is supposed to be added to a mozconfig, it's not a shell command
 > +2) cd mozilla/extensions/p3p

^before you do this, you need to run configure (client.mk will do this 
if 1 is in the mozconfig it finds)


Removed ac_add_options and replaced with ./configure --enable-extensions=default,p3p

 > Index: extensions/p3p/public/nsIP3PService.idl
 > ===================================================================
 > +  /**
 > +    * Call this method if you wanna set the policy url on the document.
 > +    */

that should be:

What do you mean?


/**
  * comment
  */

 > +  void setDocumentURL(in nsIDOMDocument aDocument, in nsIURI 
aPolicyURI);
 > +};
 > Index: extensions/p3p/public/nsIPolicyListener.idl
 > ===================================================================
 > RCS file: extensions/p3p/public/nsIPolicyListener.idl
 > diff -N extensions/p3p/public/nsIPolicyListener.idl
 > --- /dev/null	1 Jan 1970 00:00:00 -0000
 > +++ extensions/p3p/public/nsIPolicyListener.idl	1 Nov 2002 02:35:40 -0000
 > @@ -0,0 +1,60 @@
 > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; 
c-basic-offset: 2 -*- */
 > +/* ***** BEGIN LICENSE BLOCK *****
 > + * Version: NPL 1.1/GPL 2.0/LGPL 2.1
 > + *
 > + * The contents of this file are subject to the Netscape Public License
 > + * Version 1.1 (the "License"); you may not use this file except in
 > + * compliance with the License. You may obtain a copy of the License at
 > + * http://www.mozilla.org/NPL/
 > + *
 > + * Software distributed under the License is distributed on an "AS 
IS" basis,
 > + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the 
License
 > + * for the specific language governing rights and limitations under the
 > + * License.
 > + *
 > + * The Original Code is mozilla.org code.
 > + *
 > + * The Initial Developer of the Original Code is
 > + * Netscape Communications Corporation.
 > + * Portions created by the Initial Developer are Copyright (C) 1998
 > + * the Initial Developer. All Rights Reserved.
*cough*, you did not write this thing in 1998.
please grab the boilerplate MPL/LGPL/GPL license and fill in the correct 
year.

new files are supposed to be MPL/LGPL/GPL, not NPL/anything.


Good catch. Will make that change.



 > +};
 > Index: extensions/p3p/public/nsIPolicyReference.idl
 > ===================================================================
 > +[scriptable,uuid(8C51E90D-CBE7-4a2c-B9F3-77BF3AF12694)]
 > +interface nsIPolicyReference : nsISupports {
 > +  /*
 > +   * Main URI or a URI with the same host as the main URI.
 > +   */
/** <- you need two *s
  * otherwise the comments won't be caught by the documentation system.
  */

Hmmm...did not know that. Will make that change.

 > +  const unsigned long IS_LINKED_URI       = (1<<2);
 > +  /*
 > +   * Signals that a policy load was successful
 > +   */
^ the comment is in the past tense. the type is not

Will change.

 > +  /*
 > +   * Signals that a policy contains XML error.
an XML error
however, the constant doesn't imply xml, please make the constant and 
comment agree.

One possible error is an xml error. Will change the comment accordingly.

 > +   * @param aMailURI -> Current document's URI
^ mail <- typo

Will change.

 > +  /**
 > +   * Call this method to locate p3p full policy from the policy 
reference file
^ this isn't a valid sentence.

It should probably read "Call this method to locate a p3p policy from the policy 
reference file"

 > +  /**
 > +   * This method releases all the objects used for policy reference.
 > +   *
 > +   */
v finalize? why? this isn't java, i can't think of any mozilla idl's 
which use this name.
 > +  void finalize();
 > +};

Why not? "finalize" does sound good to me though I prefer "release" or something
like that.
Please file a new bug if you're really serious about function names. I don't
want these kind
of issues to block the landing.

 > Index: extensions/p3p/resources/content/nsPolicyViewer.js
 > ===================================================================
 > +const nsIIOService        = Components.interfaces.nsIIOService
 > +const nsIPromptService    = Components.interfaces.nsIPromptService
^ please add ;'s to these lines.\

Will do.

 > +const FAILURE       = 0;
 > +const SUCCESS       = 1;
err
 > +
 > +const LOAD_POLICY   = 2;
 > +const LOAD_SUMMARY  = 3;
 > +const LOAD_OPTIONS  = 4;
err
 > +
 > +const POLICY_ERROR  = 5;
 > +const SUMMARY_ERROR = 6;
 > +const OPTIONS_ERROR = 7;
err

why don't you use the idl constants?

Did not spend time on that. Could probably use idl const. Like I said before
I don't want these kind of issues to block the landing.



 > + * Used for viewing sites' privacy policy, policy summary ( generated ),
two sites, one policy? you probably want one site's policy.

Will make that change ( in other places too ).

 > +   * Initiate privacy policy loading for a URI selected
 > +   * in the Page-Info-Privacy tab window.

have you gotten approval from db48x? he owns that window, and you're 
about to trample it.

The reason for opening bugs and posting patches is to get necessary reviews and
_approvals_.

 > +    if (aSelectedURI.match(/^\s*https?:/i)) {
could someone load http:20/something ?

I'm not sure. Please apply the patch and try it out!

// Yoohoo! we have located the full p3p policy location from
'yoohoo'? remove it.

Yeah sure. Are you concerned it's not professional? :-)

 > +      // the policy reference file. Now let's try to load full p3p
load _the_ full p3p ...

Sigh!.....will do.

 > +      // If a fragment identifier is specified then we have to locate
 > +      // the POLICY with a name specified by the fragment.
 > +      // Ex. <POLICY name="one"> .... </POLICY> <POLICY name="two"> 
...</POLICY>
 > +      // if the fragment identifier is "#two" then we're supposed to 
locate the
 > +      // the POLICY with name equals "two" POLICY named "two". 
[the current string has at least the following 
problems: "the the", the POLICY name equals "two", ...]


What?


 > +            }
 > +            // Yay!, we have found the policy location no need to 
search the other link tags.
... location, so there is no need ...

OKAY...will do .

 > +            return this.handleURI(this.mLinkedURI, this.mFlag = 
nsIPolicyReference.IS_LINKED_URI);
 > +          }
 > +          catch (ex) {
 > +            // fall through to report error.
 > +          }
 > +        }
 > +      }
remove the following line:
 > +      this.reportError(this.mAction);
 > +    }
remove the following line:
 > +    else {
this line can be shared:
 > +      this.reportError(this.mAction);
remove the following line:
 > +    }
 > +  },
 

Have you test your suggestion? Sure there are ways to optimize but we CANNOT remove
the lines, like you suggested, without changing the logic a bit. If you think it is
possible to share then please open up another bug with your suggested changes.


 > +  QueryInterface: function (iid)
 > +  {
 > +    if (!iid.equals(Components.interfaces.nsISupportsWeakReference) &&
 > +        !iid.equals(Components.interfaces.nsIDOMEventListener)) {
this makes me unhappy. please add a check for  Components.interfaces.nsISupports

Why so? Anyway, will add the check you requested.


 > +      throw Components.results.NS_ERROR_NO_INTERFACE;
 > +    }
 > +    return this;
 > +  },
 > +
 > +  /*
 > +   * The purpose of this method is to display a human readable
 > +   * version of the sites' policy. That's, we load, on a new window,
one site's or two sites'? probably one site's, especially since the is 
singular
That's? yuck. Hrm, i have no idea what you're trying to say.

Ok will rephrase that comment a bit.

 > +   */
 > +  viewMachineReadablePolicy : function ()
this is a sucky name. how about makeGookReadable ?

May be sucky but I'm not willing to make any change to function names at this point.


 > +            // When a node gets removed it will be reflected on the 
nodelist.
 > +            // That's, the nodelist length will decrement. Note that 
the index
decrement => decrease? things don't usually decrement on their own.

What?

 > +  /*
 > +   * The purpose of this method is to display a human readable
 > +   * version of the sites' opt-in / opt-out policy. That's, we
i'm going to stop complaining about "the sites'" (always wrong, use "the 
site's") and That's (don't use it), just fix all instances

Point noted.


 > +  getDocument : function ()
 > +  {
 > +    var document = null;
 > +    var status   = P3P_HTTP_OK;
 > +    if (this.mFlag & nsIPolicyReference.IS_MAIN_URI) {
 > +      if (!this.mDocument) {
 > +        status = this.mXMLHttpRequest.status;
 > +        if (status == P3P_HTTP_OK || status == P3P_HTTP_REDIRECT) {
 > +          this.mDocument = this.mXMLHttpRequest.responseXML;
 > +        }
 > +        else {
 > +          this.reportError(this.mAction);
 > +          return null;
 > +        }
this is a backwards else after return.

common...give me a break. It's just a style issue. If you don't like it 
then open up a bug and will discuss there. 


 > +    // An onload event ( triggered when a result window is opened ) would
start the transformation.
strange sentence.

Oh..really. Please come up with a good one.



 > +    gPromptService.alert(window, getBrandName(), errorMessage);
alert's are evil. could you perhaps add code to report the silly error 
to the jsconsole?

A naive user is not going to open up a javascript console to find out the
problem loading the policy.
Be realistic.


getStrBundleService().createBundle("chrome://global/locale/brand.properties");
 > +    gBrandName = brandBundle.GetStringFromName("brandShortName")
please add a ;

Will do.
 > +/*
 > + * This functions compares two URLs for equality
 > + * That's, if the scheme, host, and port match then
 > + * the URLs are considered to be equal.
 > + * Ex. http://www.example.com/ == http://www.example.com/doc
 > + *     http://www.example.com:80/ == http://www.example.com
 > + */
IOService can't do this for you?

There were reasons for not using IOService. Can't remember right now.

 > +/* This method displays additional information, on an alert box, 
upon request.
you don't display additional information _on an alert box_, you might 
display it /in/ an alert box, but you shouldn't do that either.

Ok...got it


 > +</window>
 > Index: extensions/p3p/resources/content/pageInfoOverlay.js
 > ===================================================================

 > +
 > +// XXX remove me before checking in
^^ hello !!
 > +function dumpList(list, tag, attr)
 > +{
 > +  var i;
 > +  dump("\n\t *** list [" +tag+ "][" +attr+ "]\n\t *** \n");
 > +  var length = list.length;
 > +  for (i = 0; i < length; ++i)
 > +  {
 > +    dump("\t *** " + list[i] + "\n");
 > +  }
 > +}
 >

Will remove it.



 > Index: extensions/p3p/resources/locale/en-US/p3p.dtd
 > ===================================================================
 > RCS file: extensions/p3p/resources/locale/en-US/p3p.dtd
 > +<!ENTITY p3p.clickhereforinfo "Click here for more information.">
 > +<!ENTITY p3p.policy.discuri 'To see the full privacy policy of this 
web site, click  <a target="_blank" href="{@discuri}">here</a>.<br/>'>
does this work if the no new windows prefs are set?

Not sure.

 >
 >  EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS)
 > Index: extensions/p3p/src/nsCompactPolicy.cpp
 > ===================================================================
 > RCS file: /cvsroot/mozilla/extensions/p3p/src/nsCompactPolicy.cpp,v
 > retrieving revision 1.2
 > diff -u -r1.2 nsCompactPolicy.cpp
 > --- extensions/p3p/src/nsCompactPolicy.cpp	8 Feb 2002 02:59:29 -0000	1.2
 > +++ extensions/p3p/src/nsCompactPolicy.cpp	1 Nov 2002 02:35:40 -0000
 > @@ -19,7 +19,8 @@
 >   * Portions created by the Initial Developer are Copyright (C) 1998
 >   * the Initial Developer. All Rights Reserved.
 >   *
 > - * Contributor(s):
 > + * Contributor(s):
ok, the above change is both bad and wrong, please don't add trailing 
whitespace. please fix globally, this was the first time i bothered to 
select the lines to find out what was going on.


Will do.


 >  enum Tokens {
 > Index: extensions/p3p/src/nsP3PModule.cpp
 > ===================================================================
 > RCS file: /cvsroot/mozilla/extensions/p3p/src/nsP3PModule.cpp,v
 >  static nsModuleComponentInfo gP3PComponents[] =
 >  {
 > @@ -53,6 +56,16 @@
 >     NS_P3PSERVICE_CID,
 >     NS_COOKIECONSENT_CONTRACTID,
 >     nsP3PServiceConstructor,
 > +  },
 > +  {"P3P Service",
 > +   NS_P3PSERVICE_CID,
 > +   "@mozilla.org/p3pservice;1",

egads. no. please don't pollute the top level mozilla.org namespace with 
this junk.

If you think it's junk then please feel free to write quality code.
Anyway, will make the change.

Note: Please use proper words in a public quorum.


 > Index: extensions/p3p/src/nsP3PService.cpp
 > ===================================================================
 > RCS file: /cvsroot/mozilla/extensions/p3p/src/nsP3PService.cpp,v

 > Index: extensions/p3p/src/nsP3PUtils.cpp
 > ===================================================================
 > RCS file: extensions/p3p/src/nsP3PUtils.cpp
 > +          if (*rhs_end != *curr_posn) {
 > +            return PR_FALSE;
 > +          }
else after return.

YEs, I can remove the else.

 > +    }
 > +    else if (*curr_posn == '*') {
 > +      // Matching pattern between asterisks. That is, in "h*ll*" we
 > +      // check to see if "ll" exists in the rhs string.
 > +      nsAString::const_iterator tmp_end = rhs_end;
 > +      CopyUnicodeTo(lhs_begin, curr_posn, pattern);
 > +      if (FindInReadable(pattern, rhs_begin, rhs_end)) {
 > +        rhs_begin = rhs_end;
 > +        rhs_end   = tmp_end;
 > +        lhs_begin = curr_posn;
 > +      }
 > +      else {
 > +        return PR_FALSE;
 > +      }
return after else.

uh?

 > +
 > Index: extensions/p3p/src/nsPolicyReference.cpp
 > ===================================================================
 > RCS file: extensions/p3p/src/nsPolicyReference.cpp

 > +  else if (mFlags & IS_EMBEDDED_URI) {
 > +    // If you're here then we are handing a host
'you' 'we', err. normally you should only use one of those two, ideally 
you shouldn't use either.

Point noted.

 > +
 > +  // The root element MUST be META
 > +  mError = name.Equals(NS_LITERAL_STRING("META")) ? 
POLICY_LOAD_SUCCESS : POLICY_SYNTAX_ERROR;
use an early return so that you don't indent needlessly.

Once again it's a style issue. I'm not going to do this at this time.
Please feel free to make those changes post landing.

 
 > +      nsCRT::free(cdate);
err no no no. nsMemory::Free, read the docs.

Will make the change.

in that case you have:
/**
  * comment
  */
but the preferred format (which also appears in the patch) is:
/**
 * comment
 */
i hope that's clear

this next one is hard, so please bare with me:
// if the fragment identifier is "#two" then we're supposed to locate the
// the POLICY with name equals "two" POLICY named "two". 

reformatting (just changing where newlines are) we have:
// if the fragment identifier is "#two" then we're supposed to 
// locate the the POLICY 
// with name equals "two" POLICY named "two". 
the last two lines in this comment block are troubled.

unfortunately, while the spellcheckers in wordperfect and microsoft word catch
'the the' they probably won't catch 'the // the'.

as for the last line, um too many 'name(d)'s and 'two's.  I think you can just
write the following (but i had and am continuing to have a hard time figuring
out the intent of the comment, so i could be wrong):

if the fragment identifier is "#two" then we're supposed to find the POLICY
named "two". 

(yes i changed 'locate' to 'find', if locate is correct then you probably need
to indicate a search scope)

> > please add a check for  Components.interfaces.nsISupports
> Why so?
long story. in short: QI is part of nsISupports so your QI says that you don't
implement the interface that defines QI.
this confuses callers and results in lots of very annoying exceptions being
tossed whenever someone tries to use venkman while the broken code is running.

// When a node gets removed it will be reflected on the nodelist.
// That's, the nodelist length will decrement. Note that the index

ok, short explanation 'decrement' is a noun. 'decrease' can be a verb. 'will'
wants a verb.

> common...give me a break. It's just a style issue. If you don't like it 
> then open up a bug and will discuss there. 
it's an easy change. and i think brendan would agree with me. 

"An onload event ( triggered when a result window is opened ) would
start the transformation."
> > strange sentence.
> Oh..really. Please come up with a good one.
helpwanted. had i been able to come up with a good one i would have written it
in the comments :-(

on alerts: i don't have a good solution, all i can say is:
1. neither invasive nor passive mechanisms work
2. the invasive style drives people nuts with the following possible consequences: 
 A. they stop using the feature/product
 B. they learn to ignore the alert.

> There were reasons for not using IOService. Can't remember right now.
hopefully you'll be able to remember them and add a comment :)

> Note: Please use proper words in a public quorum.
sure, but that email was private.

> > return after else.
my variation on brendan's nit. here's how you avoid it:

if (!FindInReadable(pattern, rhs_begin, rhs_end))
  return PR_FALSE;

rhs_begin = rhs_end;
rhs_end   = tmp_end;
lhs_begin = curr_posn;
I don't care about grammer unless it really makes the intent of the code
unclear. I especially don't care about "sites'" versus "site's". Really.

As for the alerts, I don't care for them, but it's not that bad. Afterall, they
only come up when a user clicks on a button. Perhaps you should consider just
disabling the button rather than having a popup.

Third, mozilla crashes for me when I click on the Summary button in the privacy
tab. I'll give you the stack when I get home. It's crashing because the
CSSLoader's document is null or something... I can't remeber exactly, it's been
awhile since the morning.
Uh oh, that crasher might be the result of the big XSLT landing a few weeks
back. It might go away if you do this in nsXMLHttpRequest.cpp, or we might need
some other changes in addition to this:

   rv = document->StartDocumentLoad(kLoadAsData, mChannel,
                                    loadGroup, nsnull,
                                    getter_AddRefs(listener),
-                                   PR_FALSE);
+                                   PR_TRUE);

 #ifdef IMPLEMENT_SYNC_LOAD
   if (NS_FAILED(rv)) {
yes, very likely. That crasher is bug 176186 which has a patch but needs reviews
(*nudge* *nudge* :) )
nope, still crashes. takes longer though
#0  0x0000006d in ?? ()
#1  0x423a0e3c in nsPresContext::ResolveStyleContextFor(nsIContent*,
nsIStyleContext*, nsIStyleContext**) (this=0x8d63248, aContent=0x8d1f7c0,
aParentContext=0x8c344ac, aResult=0xbfffc53c) at
/home/db48x/mozilla/layout/base/src/nsPresContext.cpp:931
#2  0x421e21b4 in FrameManager::ReResolveStyleContext(nsIPresContext*,
nsIFrame*, nsIContent*, int, nsIAtom*, nsStyleChangeList&, nsChangeHint,
nsChangeHint&) (this=0x8d02a78, aPresContext=0x8d63248, aFrame=0x8c34934,
aParentContent=0x8c783e0, aAttrNameSpaceID=-1, aAttribute=0x0,
aChangeList=@0xbfffc814, aMinChange=nsChangeHint_None,
aResultChange=@0xbfffc62c) at
/home/db48x/mozilla/layout/html/base/src/nsFrameManager.cpp:1856
#3  0x421e252d in FrameManager::ReResolveStyleContext(nsIPresContext*,
nsIFrame*, nsIContent*, int, nsIAtom*, nsStyleChangeList&, nsChangeHint,
nsChangeHint&) (this=0x8d02a78, aPresContext=0x8d63248, aFrame=0x8c345c4,
aParentContent=0x0, aAttrNameSpaceID=-1, aAttribute=0x0,
aChangeList=@0xbfffc814, aMinChange=nsChangeHint_None,
aResultChange=@0xbfffc71c) at
/home/db48x/mozilla/layout/html/base/src/nsFrameManager.cpp:1936
#4  0x421e252d in FrameManager::ReResolveStyleContext(nsIPresContext*,
nsIFrame*, nsIContent*, int, nsIAtom*, nsStyleChangeList&, nsChangeHint,
nsChangeHint&) (this=0x8d02a78, aPresContext=0x8d63248, aFrame=0x8c34244,
aParentContent=0x0, aAttrNameSpaceID=-1, aAttribute=0x0,
aChangeList=@0xbfffc814, aMinChange=nsChangeHint_None,
aResultChange=@0xbfffc7cc) at
/home/db48x/mozilla/layout/html/base/src/nsFrameManager.cpp:1936

is that stack corruption?
I've seen that stack too. Not sure whos fault it is, if it's transformiix or
layout. I'll investigate when i get back home
Attached patch patch v1.1 (obsolete) — — Splinter Review 
Addresses timeless's concerns ( most of 'em ).
Attachment #104824 - Attachment is obsolete: true
Comment on attachment 106005 [details] [diff] [review]
patch v1.1

i meant to mention this, but i might have forgotten it:
+    chrome:localeVersion="1.1b">
please make sure that the localeVersion matches the rest of the app if/when you
land.

Index: extensions/p3p/resources/content/nsPolicyViewer.js

+  notifyPolicyLocation : function (aLocation, aError) 
+  {
+    if (aError == nsIPolicyReference.POLICY_LOAD_SUCCESS) {
+     if (!this.mPolicyURI) {
+	this.mPolicyURI = gIOService.newURI(aLocation, null, null);
     ^ indentation failed...

+  if (!gIOService) {
+    gIOService = 
+     
Components.classes["@mozilla.org/network/io-service;1"].getService(nsIIOService
);
+  }
+
+  if (url.indexOf(':') > -1)
+    return url;
useless perf consideration. you have to do the indexOf so you can save a
gIOService check if you reorder.

js note: [0,
	 ] /*is legal, the advantage is for diffs where you insert new lines at
the end. although {a:0,} isn't legal */

i can't get myself to read the dtd file, and i don't parse xslt files. good
luck.
Note: Entry for p3p is already found in allmakefiles.sh
....	   
p3p ) MAKEFILES_extensions="$MAKEFILES_extensions
	    extensions/p3p/Makefile
	    extensions/p3p/public/Makefile
	    extensions/p3p/src/Makefile
.....
Attachment #106005 - Flags: superreview?(bzbarsky)
Attachment #106005 - Flags: review?(heikki)
Attachment #106005 - Flags: superreview?(bzbarsky) → superreview?(darin)
QA Contact: petersen → rakeshmishra
Comment on attachment 106005 [details] [diff] [review]
patch v1.1

>Index: extensions/p3p/Makefile.in
>===================================================================
>Index: extensions/p3p/README
>===================================================================

I think you could remove this file. If you don't want to remove the whole file,
at least remove the build instructions sinc this will build by default.

>Index: extensions/p3p/jar.mn
>===================================================================
>Index: extensions/p3p/macbuild/p3p.xml
>===================================================================
>Index: extensions/p3p/macbuild/p3pIDL.xml
>===================================================================

Better have some Mac person at hand when you check in, just in case...

>Index: extensions/p3p/public/MANIFEST
>===================================================================
>Index: extensions/p3p/public/MANIFEST_IDL
>===================================================================
>Index: extensions/p3p/public/Makefile.in
>===================================================================
>Index: extensions/p3p/public/nsIP3PService.idl
>===================================================================

Empty interface seems a bit strange.

>Index: extensions/p3p/public/nsIPolicyListener.idl
>===================================================================
>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
>===================================================================
>Index: extensions/p3p/resources/content/nsPolicyViewer.js
>===================================================================
>+      this.mFragment = (tmp && tmp.length == 2)? tmp[1] : null;

It would be more foolproof to construct a URI object and getting the fragment
using its method. This actually applies to all cases where you handle URI in
string format, actually, so please check all methods.

>+      var nodelist = document.getElementsByTagName("POLICY");

You might want to add comments every time you access elements by name without
namespace that this is a bit fragile but because we need to support so many
different namespaces we do this for perf reasons. Actually, a better way might
be to look for the first P3P namespace in the document, and use that ns from
there on.

>+function isEquivalent (aLHS, aRHS)

timeless: we need this function because the nsIURI object does not offer this.

>Index: extensions/p3p/resources/content/p3p200005.xsl
>===================================================================

>+<!DOCTYPE xsl:stylesheet SYSTEM "chrome://p3p/locale/p3p.dtd" [
>+  <!ENTITY nbsp "&#160;">
>+]>

The nbsp entity declaration should probably be moved to the DTD.

>Index: extensions/p3p/resources/content/p3p200010.xsl
>===================================================================
>Index: extensions/p3p/resources/content/p3p200012.xsl
>===================================================================
>Index: extensions/p3p/resources/content/p3p200109.xsl
>===================================================================
>Index: extensions/p3p/resources/content/p3p200201.xsl
>===================================================================

Btw, the reason we have so many XSL files instead of one large file is
performance; we tested the single large file first and it was noticeably slower
even on fast machines.

We only have one DTD file because of the extra work that would be needed by
translators.

In the future the XSLT files may start to differ because the specs are
different, and we might also need some small DTD tweaks for each version.

>Index: extensions/p3p/resources/content/p3pDummy.xml
>===================================================================

We don't seem to be using the dummy file anymore(?) so this file could be
removed, and the entry in the jar.mn as well.

>Index: extensions/p3p/resources/content/p3pSummary.js
>===================================================================
>+    service.setDocumentURL(result, policyuri);

nsIP3PService does not have any member functions, so what is this method?

Will continue...
+/* This method displays additional information, in an alert box, upon request.
+ * Note that since javascript is disabled, for security reasons, in the policy
viewer
+ * we have to capture the specific event, that bubbles up to the chrome, containing
+ * the requested message.
+ */
+function captureContentClick(aEvent)
+{
+  if ("parentNode" in aEvent.target &&
aEvent.target.parentNode.hasAttribute("message"))
+    alert(aEvent.target.parentNode.getAttribute("message"));
+}

I seem to recall alert was no-no in chrome, and should be replaced with dialog
or something. Please check with UI developers.

+function makeURLAbsolute(url, base)
+{
+  if (!gIOService) {
+    gIOService = 
+     
Components.classes["@mozilla.org/network/io-service;1"].getService(nsIIOService);
+  }
+
+  if (url.indexOf(':') > -1)
+    return url;

Move this if to the top of the function.

+  while ((ch = *aSet)) {

Too many parens.

+      if (rhs_begin == rhs_end &&
+          curr_posn == lhs_end) {

+          done = --rhs_end == rhs_begin || curr_posn == lhs_begin;

Would be nice to have some grouping parens.

+nsP3PUtils::GetElementsByTagName(nsIDOMNode* aNode, 
+                                 const nsAString& aTagName, 
+                                 nsVoidArray& aReturn)

I think this function should addref the node after each succesfull
AppendElement, and the caller should release them when done. Otherwise there is
no guarantee that the caller will have valid node references.

+#define P3P_HTTP_OK       200
+#define P3P_HTTP_REDIRECT 300
+#define P3P_HTTP_FOUND    302

Doesn't necko define these already?

+      value += "/w3c/p3p.xml";

+      mCurrentURI->SetPath(NS_LITERAL_CSTRING("/w3c/")); 

Make a define/const/static for that string since you use it in more than one place. 


>+#define P3P_HTTP_OK       200

This sounds wrong to me, shouldn't, rather, only be checked if the first digit
is 2, not if the entire string is 200?

darin?
> +        if (status == P3P_HTTP_OK || status == P3P_HTTP_REDIRECT) {

Wouldn't it make more sense to get the .channel off the XMLHttpRequest, QI to
nsIHttpChannel, and use the handy method on that to tell whether it succeeded?
Comment on attachment 106005 [details] [diff] [review]
patch v1.1

Let's fix all the issues I and others raised and look at a new patch.
Attachment #106005 - Flags: review?(heikki) → review-
>+  while ((ch = *aSet)) {
>
>Too many parens.

No, that's necessary to avoid gcc warnings.  I think it's better style, also,
than writing an unparenthesized assignment expression in a loop condition, for
the same reason that caused gcc authors to add a warning: it's too easy to typo
== as = and produce something that looks the same.  Personally, I use an
appended != 0 (or '\0', or NULL in C, or nsnull in Mozilla C++), but that's
overkill.

/be
Doh, I read this:

>+  while ((ch = *aSet)) {

as if it was using '=='. So I think I like Brendan's "overkill" idea, makes it
easier to spot :)
re comment #21: nsIHttpChannel::responseSucceeded is a better way to check for
2xx response codes :)
re bare alert(), yes please use the promptservice instead.
>Index: extensions/p3p/public/nsIP3PService.idl
>Empty interface seems a bit strange.

No it's not empty. It has the method setDocumentURL.

>Index: extensions/p3p/resources/content/p3pSummary.js
>+    service.setDocumentURL(result, policyuri);
>nsIP3PService does not have any member functions, so what is this method?

Refer to my previous comment.

>I seem to recall alert was no-no in chrome, and should be replaced with dialog
>or something. Please check with UI developers.

Is there a documentation avilable for DOs and DONTs? Could someone point me to
it? Thanks.

>+nsP3PUtils::GetElementsByTagName(nsIDOMNode* aNode, 
>+                                 const nsAString& aTagName, 
>+                                 nsVoidArray& aReturn)
>I think this function should addref the node after each succesfull
>AppendElement, and the caller should release them when done. Otherwise there is
>no guarantee that the caller will have valid node references.

I don't think that's necessary because the array is populated with the
childNodes of the passed in node ( aNode ). And aNode is guaranteed to exist in
the caller's scope.



>I seem to recall alert was no-no in chrome, and should be replaced with dialog
>or something. Please check with UI developers.

see nsIPromptService
Christian: I just noticed that I did use nsIPromptService in nsPolicyViewer.js,
however, failed to use the service in p3pSummary.js :-(
Attached patch patch v1.2 (obsolete) — — Splinter Review 
Addressing reviewers comments.

Note: For some reason p3p does not work in mozilla/netscape anymore.
Apparently, loading the stylesheet ( xsl ) is failing ( hitting an error in
nsChromeRegistry, NS_ERROR("unknown provider"), before failing ). When I remove
boris's change in nsChromeProtocolHandler I don't get the error, however, I get
a blank policy viewer window! 

Boris, any idea what's going on here?
Attachment #106005 - Attachment is obsolete: true
as bug 164822 notes
<xsl:if test = ".[@required='opt-in']">
is not standards conformant.

is 1.2 not with -N, it doesn't seem to have the stylesheets.



>is 1.2 not with -N, it doesn't seem to have the stylesheets.

Looks like it. I'll post another patch.
Attached patch patch v1.3 — — Splinter Review 

    
  

        Attachment #111721 -
        Attachment is obsolete: true

    
  

        Attachment #116223 -
        Flags: superreview?(darin)

        Attachment #116223 -
        Flags: review?(heikki)

    
    

    
  
Comment on attachment 116223 [details] [diff] [review]
patch v1.3

In GetElementsByTagName() you do not need to do NS_IF_ADDREF because it is
quaranteed we have the node, just do NS_ADDREF.

Let's fix the XSLT stylesheets later to be standards compliant.

Another reminder to check the localeVersion before checkin in...

r=heikki

        Attachment #116223 -
        Flags: review?(heikki) → review+

    
    

    
  
Comment on attachment 116223 [details] [diff] [review]
patch v1.3

Just two thoughts on getElementsByTagName:

1)  Please document clearly that your GetElementsByTagName is _NOT_ live,
unlike the DOM one (people get very confused by functions with the same name
and different functionality).

2)  The DOM getElementsByTagName is lazy; so in cases when you only want the
first node in the list that matches some criterion and you care about
performance, do not get the list length; just get nodes for an ever-increasing
count and bail if you get a null node -- that means end of list.

    
    

    
  
Comment on attachment 116223 [details] [diff] [review]
patch v1.3

>Index: p3p/macbuild/p3pIDL.xml

>-                    0001000101010F636F6F6B6965732E6865616465727300000000000000000000
>+                    0001000101010B7033702E686561646572736465727300000000000000000000

you should probably go through and clean up the .xml files... codewarrior has a
habbit of adding wierd things like this into the .xml files.  on the other
hand,
since there is no CFM tinderbox, why even bother?  just send you .xml file
diffs
to simon ;-)


>Index: p3p/resources/content/p3p200005.xsl
>Index: p3p/resources/content/p3p200010.xsl
>Index: p3p/resources/content/p3p200012.xsl
>Index: p3p/resources/content/p3p200109.xsl
>Index: p3p/resources/content/p3p200201.xsl

i don't pretend to know a damn thing about XSLT!


>Index: p3p/src/nsPolicyReference.cpp

>+NS_IMETHODIMP
>+nsPolicyReference::Finalize() {

nit: this brace is inconsistent with the rest of the file.. there
i found something to pick on! :-/


sr=darin

        Attachment #116223 -
        Flags: superreview?(darin) → superreview+

    
    

    
  
Comment on attachment 116223 [details] [diff] [review]
patch v1.3

p3p/Makefile.in and p3p/public/Makefile.in
>-# The Initial Developer of the Original Code is International
The idea of changing the initial developer is broken.
at least get someone from IBM to sign off on it.

'atleast' is not a word
'call back' is usually written callback
your use of "that's" bothers me, in some cases you want i.e. (that is to say)
and in some places "that is" would be ok, and in most you're probably best off
dropping it entirely.

i think i complained about 'yoohoo' last time, and in fact you said you'd fix
it :(

'increamented' is not spelled correctly

please don't include trailing whitespace:
>+            break;              

>+   * The purpose of this method is to display errors to the
>+   * the user in a localized fashion.

double the, drop one. i complained about this in at least two earlier comments.
how about this:
+   * The purpose of this method is to display localized errors
+   * to the user.

>+function initP3PTab()
>+{
>+  var linkTypes = 
>+  [
>+  //  Tag       Attr         List node ID
>+    ["a",      "href",      "linkKids"],
...
>+    ["iframe", "src",       "frameKids"],
>+    ["object", "codebase",  "objectKids"],
i don't understand this list, but what about <object data=...> ?

the same darin nit applies here:
>+nsP3PService::~nsP3PService() {

>+ *  Note: The '*' character, that is used in the policy reference file, 
>+ *  is treated as a wildcard.
by wildcard do you mean one or more characters or zero or more characters?
>+  // any of the INCLUDES, but not matched by an EXCLUDE. It's is legal, but pointles,
this is like the the, except that it's hidden by a contraction: It [i]s is <-
drop an is.

    
    

    
  
>The idea of changing the initial developer is broken.
>at least get someone from IBM to sign off on it.

Good point but other than the file name nothing ( I believe ) has been taken
from IBM's implementation. Do you think I should leave that line untouched?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsP3PService.cpp&root=/cvsroot&subdir=mozilla/extensions/p3p/src&command=DIFF_FRAMESET&rev1=1.11&rev2=1.12

    
    

    
  
The mac changes look OK.

    
    

    
  
Doh! please ignore my last comment ( #39 ). I was looking at a completely
different file.

    
    

    
  
Changes landed. Thanks everyone for the reviews and suggestions.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

    
    

    
  
Harish,

I am not seeing the Privacy Tab on Page Info window- using Mozilla builds 20030311
(tried both trunk and branch) Shouldn't that be viewable now?

    
    

    
  
Grace: The code is not enabled yet.

    
  

        Attachment #106005 -
        Flags: superreview?(darin)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: