Bug 80430

Summary: [BlackBerry] Set correct ResourceRequest target type.
Product: WebKit Reporter: Lyon Chen <liachen>
Component: WebKit BlackBerryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: japhet, rwlbuis, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Other   
Attachments:
Description Flags
Patch for 80430
rwlbuis: review-
Updated patch based on rwlbuis's comments. none

Description Lyon Chen 2012-03-06 10:44:11 PST
Right now ResourceRequests loaded by appCache are not set with correct target type. Also Worker related ResourceRequest are not correctly set for BlackBerry platform.
Comment 1 Lyon Chen 2012-03-06 11:00:26 PST
Created attachment 130406 [details]
Patch for 80430

Patch for 80430.
Comment 2 Rob Buis 2012-03-06 14:26:31 PST
Comment on attachment 130406 [details]
Patch for 80430

View in context: https://bugs.webkit.org/attachment.cgi?id=130406&action=review

The HashMap needs to be static for sure.

> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:92
> +    MimeTypeResourceRequestTypeMap* map = 0;

You need to make it static, otherwise you keep allocating it each time the function is called.

> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:114
> +            // map->add(String(""), ResourceRequest::TargetIsMedia);

Maybe better not include the commented three lines.

> Source/WebKit/blackberry/ChangeLog:8
> +        Removed unused code in dispatchWillSendRequest(), they are too late!

Can you explain what and why it is too late?
Comment 3 Lyon Chen 2012-03-06 18:13:05 PST
(In reply to comment #2)
> The HashMap needs to be static for sure.
> 
> You need to make it static, otherwise you keep allocating it each time the function is called.

Yes, this is an error, will fix.

> 
> Maybe better not include the commented three lines.
> 

Will remove them.

> Can you explain what and why it is too late?

In FrameLoaderClientBlackBerry::dispatchWillSendRequest() request.initializePlatformRequest() is called before the target type setting codes. And the target type is only referred to by us in initializePlatformRequest(), so it doesn't make sense to change the target type after it is used already.
Comment 4 Lyon Chen 2012-03-06 18:30:51 PST
Created attachment 130512 [details]
Updated patch based on rwlbuis's comments.
Comment 5 Rob Buis 2012-03-06 19:08:04 PST
Comment on attachment 130512 [details]
Updated patch based on rwlbuis's comments.

LGTM.
Comment 6 WebKit Review Bot 2012-03-06 23:25:14 PST
Comment on attachment 130512 [details]
Updated patch based on rwlbuis's comments.

Clearing flags on attachment: 130512

Committed r110023: <http://trac.webkit.org/changeset/110023>
Comment 7 WebKit Review Bot 2012-03-06 23:25:20 PST
All reviewed patches have been landed.  Closing bug.