Bug 73869

Created attachment 118046 [details] hook up MIMESniffer

Comment on attachment 118046 [details] hook up MIMESniffer View in context: https://bugs.webkit.org/attachment.cgi?id=118046&action=review This really needs a ChangeLog! Looks quite good otherwise. > Source/WebCore/platform/network/MIMESniffing.cpp:28 > +using std::strlen; This should not be needed. Also it may be better to do this in a seperate bug. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:364 > + if (pos != -1) { pos variable can be introduced in the if > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:374 > + const char* type = sniffer.sniff(buf, len); Can be included in if. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:376 > + m_sniffedMimeType = WTF::String(type); Wrong indentation.

Created attachment 118092 [details] cleaned up some style, added changelog entry

Comment on attachment 118092 [details] cleaned up some style, added changelog entry View in context: https://bugs.webkit.org/attachment.cgi?id=118092&action=review The code looks great to me, but ChangeLog does not follow the required format. Probably last round but r1 for now. > Source/WebCore/ChangeLog:19 > + The ChangeLog is not quite right, it goes: bug title bug url reviewed by Description of the solution Tests, or why there are no tests.

Created attachment 118096 [details] correct changelog format

Created attachment 118099 [details] previous patch introduced unwanted changes

Comment on attachment 118099 [details] previous patch introduced unwanted changes View in context: https://bugs.webkit.org/attachment.cgi?id=118099&action=review > Source/WebCore/ChangeLog:1 > +2011-12-06 Tyler Abbott <tabbott@rim.com> needs an empty line before the bug title > Source/WebCore/ChangeLog:7 > + Hook up MIMESniffing. Override Content-Types will I'd say "Hook up MIMESniffing for the BlackBerry port.

Created attachment 118102 [details] better commit message

Comment on attachment 118102 [details] better commit message LGTM

Comment on attachment 118102 [details] better commit message Rejecting attachment 118102 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ED at 346. Hunk #5 FAILED at 551. 5 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/platform/network/blackberry/NetworkJob.cpp.rej patching file Source/WebCore/platform/network/blackberry/NetworkJob.h Hunk #1 FAILED at 152. Hunk #2 FAILED at 177. 2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/network/blackberry/NetworkJob.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Rob Buis', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10736865

(In reply to comment #10) > (From update of attachment 118102 [details]) > Rejecting attachment 118102 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > Last 500 characters of output: > ED at 346. > Hunk #5 FAILED at 551. > 5 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/platform/network/blackberry/NetworkJob.cpp.rej > patching file Source/WebCore/platform/network/blackberry/NetworkJob.h > Hunk #1 FAILED at 152. > Hunk #2 FAILED at 177. > 2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/network/blackberry/NetworkJob.h.rej > > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Rob Buis', u'--force']" exit_code: 1 > > Full output: http://queues.webkit.org/results/10736865 Looks like these files haven't been pushed upstream yet. Thanks for the review Rob.

A nit: remove WTF:: before String because it's unnecessary and you need to keep the same style with other usage of String in the same file.

Comment on attachment 118102 [details] better commit message He's not a committer, so this might as well be r-.

(In reply to comment #13) > (From update of attachment 118102 [details]) > He's not a committer, so this might as well be r-. tyler, please move on here :-)

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 118102 [details] [details]) > > He's not a committer, so this might as well be r-. > > tyler, please move on here :-) This is on my TODO list :-)

Created attachment 131105 [details] The old patch was stale, here is a fresh one.

Comment on attachment 131105 [details] The old patch was stale, here is a fresh one. LGTM.

Comment on attachment 131105 [details] The old patch was stale, here is a fresh one. Clearing flags on attachment: 131105 Committed r110361: <http://trac.webkit.org/changeset/110361>

All reviewed patches have been landed. Closing bug.

*** Bug 75340 has been marked as a duplicate of this bug. ***