[webkit-reviews] review requested: [Bug 26199] Implement a reflective XSS filter : [Attachment 31269] updated patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has asked  for review:
Bug 26199: Implement a reflective XSS filter
https://bugs.webkit.org/show_bug.cgi?id=26199

Attachment 31269: updated patch
https://bugs.webkit.org/attachment.cgi?id=31269&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
(In reply to comment #15)
> Why activeXSSAuditor?  Will there be more than one per scriptController in
the
> future?

Fixed.

> > +	 // The XSSAuditor associated with this ScriptController.
> > +	 OwnPtr<XSSAuditor> m_XSSAuditor;
> 
> Does the auditor have to be a pointer here? 

I make this a pointer because I didn't want to include XSSAuditor.h here
because ScriptController.h is included by the entire world (including outside
of WebCore).

> We don't use @param syntax.  In fact, neither of these comments are really
> necessary as the code is self explanatory.

Fixed.

> The message should be a static.

Fixed x3.

> > +// static
> 
> This comment is not necessary.

Fixed.

> Missing space after (allowNullCharacters).  The parens are not needed as
well.

Fixed.

> The use of # is inconsistent with the rest of the comment.

Fixed.

> We don't use @param syntax.  In fact, this comment is not necessary.

Fixed.

> Overall this looks great, but I want to defer r+ing this until we have some
> performance analysis done on it, even it only in the disabled state.

You mean you'd like to measure the performance impact when its enabled, right?

> Sunspider and some form of plt testing would be sufficient I think.

Sunspider I can do.  How do I do plt testing?

Thanks for reviewing the patch.  :)