Bug 112179 – Pass the XSSAuditor's report URL to the XSSAuditorDelegate on the main thread.

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike West]

Pass the XSSAuditor's report URL to the XSSAuditorDelegate on the main thread.

[Mike West]

Created attachment 192790 [details]
Patch

[Mike West]

Forking this from https://bugs.webkit.org/show_bug.cgi?id= for clarity.

[Adam Barth]

Comment on attachment 192790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192790&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:538
>      m_xssAuditor.init(document());
> +    m_xssAuditorDelegate.setReportURL(m_xssAuditor.reportURL());

This will cause us to set the report URL every time HTMLDocumentParser::pumpTokenizer is called.  It seems like we only need to do that once...  Maybe we should pass the m_xssAuditorDelegate to m_xssAuditor in init and have it store the report URL only in the delegate?

> Source/WebCore/html/parser/XSSAuditor.h:63
> +    KURL reportURL() const { return m_reportURL; }

KURL -> const KURL&

[Mike West]

(In reply to comment #3)
> (From update of attachment 192790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192790&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:538
> >      m_xssAuditor.init(document());
> > +    m_xssAuditorDelegate.setReportURL(m_xssAuditor.reportURL());
> 
> This will cause us to set the report URL every time HTMLDocumentParser::pumpTokenizer is called.  It seems like we only need to do that once...  Maybe we should pass the m_xssAuditorDelegate to m_xssAuditor in init and have it store the report URL only in the delegate?

I didn't like this idea until I actually read your comment. :) Throwing away the property on both XSSAuditor and XSSInfo would be great.

> 
> > Source/WebCore/html/parser/XSSAuditor.h:63
> > +    KURL reportURL() const { return m_reportURL; }
> 
> KURL -> const KURL&

*ahem* We can throw this away too. :)

[Mike West]

Created attachment 192801 [details]
Patch

[Adam Barth]

Comment on attachment 192801 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192801&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:302
>          m_reportURL = xssProtectionReportURL; // FIXME: Combine the two report URLs in some reasonable way.

You can delete m_reportURL now.  It's not needed.

[Mike West]

(In reply to comment #6)
> (From update of attachment 192801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192801&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:302
> >          m_reportURL = xssProtectionReportURL; // FIXME: Combine the two report URLs in some reasonable way.
> 
> You can delete m_reportURL now.  It's not needed.

I was planning on doing that in a separate patch, along with the XSSInfo::m_reportURL, as I figured it would make this patch easier to review. I'm happy to do it all in one if you'd prefer.

[Mike West]

Created attachment 192814 [details]
Patch

[Mike West]

(In reply to comment #8)
> Created an attachment (id=192814) [details]
> Patch

This wraps up everything except the one-report-only change; I'd still like to do that in the other bug, as I'd also like to reorder some of the statements in that block, and the diff will be ugly if I do both here. :)

[Adam Barth]

Comment on attachment 192814 [details]
Patch

This looks great.  Thanks for iterating on it.

[WebKit Review Bot]

Comment on attachment 192814 [details]
Patch

Rejecting attachment 192814 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 192814, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13882 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
54>At revision 13882.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17189262

[Adam Barth]

Comment on attachment 192814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192814&action=review

> Source/WebCore/ChangeLog:16
> +        No new tests (OOPS!).

The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.

[Mike West]

Created attachment 192881 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 192881 [details]
Patch for landing

Clearing flags on attachment: 192881

Committed r145695: <http://trac.webkit.org/changeset/145695>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.