Summary: | BlackBerry PlayBook doesn't sniff mime types | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | tabbott | ||||||||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | charles.wei, leo.yang, rakuco, rwlbuis, tonikitoo, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | Other | ||||||||||||||||
Bug Depends on: | 73791 | ||||||||||||||||
Bug Blocks: | 73144 | ||||||||||||||||
Attachments: |
|
Description
tabbott
2011-12-05 15:09:29 PST
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. *** |