Summary: | Pass the XSSAuditor's report URL to the XSSAuditorDelegate on the main thread. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||
Component: | New Bugs | Assignee: | Mike West <mkwst> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dbates, esprehn+autocc, ojan.autocc, tsepez, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mike West
2013-03-12 12:50:53 PDT
Created attachment 192790 [details]
Patch
Forking this from https://bugs.webkit.org/show_bug.cgi?id= for clarity. 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& (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. :) Created attachment 192801 [details]
Patch
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. (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. Created attachment 192814 [details]
Patch
(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. :) Comment on attachment 192814 [details]
Patch
This looks great. Thanks for iterating on it.
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 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. Created attachment 192881 [details]
Patch for landing
Comment on attachment 192881 [details] Patch for landing Clearing flags on attachment: 192881 Committed r145695: <http://trac.webkit.org/changeset/145695> All reviewed patches have been landed. Closing bug. |