Bug 80430 - [BlackBerry] Set correct ResourceRequest target type.
Summary: [BlackBerry] Set correct ResourceRequest target type.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-06 10:44 PST by Lyon Chen
Modified: 2012-03-06 23:25 PST (History)
4 users (show)

See Also:


Attachments
Patch for 80430 (12.15 KB, patch)
2012-03-06 11:00 PST, Lyon Chen
rwlbuis: review-
Details | Formatted Diff | Diff
Updated patch based on rwlbuis's comments. (12.15 KB, patch)
2012-03-06 18:30 PST, Lyon Chen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.