Bug 73869

Summary: BlackBerry PlayBook doesn't sniff mime types
Product: WebKit Reporter: tabbott
Component: Page LoadingAssignee: 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 Flags
hook up MIMESniffer
rwlbuis: review-
cleaned up some style, added changelog entry
rwlbuis: review-
correct changelog format
none
previous patch introduced unwanted changes
none
better commit message
eric: review-, webkit.review.bot: commit-queue-
The old patch was stale, here is a fresh one. none

Description tabbott 2011-12-05 15:09:29 PST
Need to hook up the mime type sniffer.
Comment 1 tabbott 2011-12-06 07:12:56 PST
Created attachment 118046 [details]
hook up MIMESniffer
Comment 2 Rob Buis 2011-12-06 08:05:38 PST
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.
Comment 3 tabbott 2011-12-06 12:36:15 PST
Created attachment 118092 [details]
cleaned up some style, added changelog entry
Comment 4 Rob Buis 2011-12-06 12:41:53 PST
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.
Comment 5 tabbott 2011-12-06 12:51:59 PST
Created attachment 118096 [details]
correct changelog format
Comment 6 tabbott 2011-12-06 12:55:32 PST
Created attachment 118099 [details]
previous patch introduced unwanted changes
Comment 7 Rob Buis 2011-12-06 13:09:11 PST
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.
Comment 8 tabbott 2011-12-06 13:36:44 PST
Created attachment 118102 [details]
better commit message
Comment 9 Rob Buis 2011-12-06 13:38:07 PST
Comment on attachment 118102 [details]
better commit message

LGTM
Comment 10 WebKit Review Bot 2011-12-06 14:39:07 PST
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
Comment 11 tabbott 2011-12-06 14:52:44 PST
(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.
Comment 12 Leo Yang 2011-12-06 16:34:35 PST
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 13 Eric Seidel (no email) 2011-12-21 15:06:05 PST
Comment on attachment 118102 [details]
better commit message

He's not a committer, so this might as well be r-.
Comment 14 Antonio Gomes 2012-02-05 21:44:24 PST
(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 :-)
Comment 15 tabbott 2012-02-06 07:17:21 PST
(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 :-)
Comment 16 tabbott 2012-03-09 14:12:32 PST
Created attachment 131105 [details]
The old patch was stale, here is a fresh one.
Comment 17 Rob Buis 2012-03-09 14:24:20 PST
Comment on attachment 131105 [details]
The old patch was stale, here is a fresh one.

LGTM.
Comment 18 WebKit Review Bot 2012-03-09 18:56:09 PST
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>
Comment 19 WebKit Review Bot 2012-03-09 18:56:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Charles Wei 2012-03-26 23:39:02 PDT
*** Bug 75340 has been marked as a duplicate of this bug. ***