Bug 54381

Created attachment 82299 [details] work-in-progress (needs test)

Comment on attachment 82299 [details] work-in-progress (needs test) View in context: https://bugs.webkit.org/attachment.cgi?id=82299&action=review I think i'm too tired/sick to review this for real. But I look forward to the tested patch. > Source/WebCore/page/ContentSecurityPolicy.cpp:151 > + m_scriptSrc = adoptPtr(new ScriptSrcDirective(value)); Should we have a default case? Like logging to the console?

(In reply to comment #2) > (From update of attachment 82299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82299&action=review > > > Source/WebCore/page/ContentSecurityPolicy.cpp:151 > > + m_scriptSrc = adoptPtr(new ScriptSrcDirective(value)); > > Should we have a default case? Like logging to the console? Maybe... I haven't really thought about what to do for unsupported directives. My plan was just to ignore them.

Created attachment 82395 [details] Patch

Comment on attachment 82395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82395&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:130 > + emitDirective(String(name), String(value)); > + name.clear(); > + value.clear(); This still feels strange. Seems you should have an emitAndClear function to wrap this copy/paste. > Source/WebCore/page/ContentSecurityPolicy.h:49 > bool m_isEnabled; This name doesn't make much sense. Seems you could have parsed already and not have "enabled".

Created attachment 82402 [details] Patch

Comment on attachment 82402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82402&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:83 > + pos = parseDirective(pos, end); I think I would have designed this to return the name and value from this function and then call emitDirective from within this loop. That would get rid of the goto, and might make it more clear who is doing what. I think this is OK though. It's not immediately clear from reading that calling "parseDirective" will end up calling emitDirective. Actually, in what I proposed then the Vectors are managed by the outer loop and they auto-clear then the loop loops.

Comment on attachment 82402 [details] Patch Please consider my alternate design when landing.

Created attachment 82403 [details] Patch

Comment on attachment 82403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82403&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:87 > + parseDirective(pos, end, name, value); > + if (name.isEmpty()) > + continue; I would have probably had this return true/false for success. But this is good too.

Comment on attachment 82403 [details] Patch Yeah, it's a bit philosophical. Parsing can never "fail" per se. It's just that unnamed directives don't have any meaning, e.g., ";;;;" parses fine by has a bunch of unnamed directives.

Comment on attachment 82403 [details] Patch Clearing flags on attachment: 82403 Committed r78569: <http://trac.webkit.org/changeset/78569>

All reviewed patches have been landed. Closing bug.