Bug 73869 – BlackBerry PlayBook doesn't sniff mime types

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

Need to hook up the mime type sniffer.

Created attachment 118046 [details]
hook up MIMESniffer

[Rob Buis]

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

[Rob Buis]

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

[Rob Buis]

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
> [phone]  Tyler Abbott  <[e-mail]>

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

[Rob Buis]

Comment on attachment 118102 [details]
better commit message

LGTM

[WebKit Review Bot]

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.

[Leo Yang]

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.

[Eric Seidel (no email)]

Comment on attachment 118102 [details]
better commit message

He's not a committer, so this might as well be r-.

[Antonio Gomes]

(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.

[Rob Buis]

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

LGTM.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Charles Wei]

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