Bug 32336 - Update Chromium Loader to pass through the new ResourceType::TargetType
Summary: Update Chromium Loader to pass through the new ResourceType::TargetType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-09 12:02 PST by Mike Belshe
Modified: 2009-12-28 23:30 PST (History)
4 users (show)

See Also:


Attachments
patch (3.59 KB, patch)
2009-12-09 12:17 PST, Mike Belshe
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
followup patch to workaround horribly painful and broken chromium/webkit processes. (3.89 KB, patch)
2009-12-09 16:23 PST, Mike Belshe
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2009-12-09 12:02:07 PST
FrameLoaderClientImpl.cpp previously would override the resource type as a generic TargetIsSubresource when the target is not a main or subframe.  Update to only modify the type if the resource is a frame; otherwise, leave the setting as is, since the ResourceRequest now initializes and sets the resource type properly.
Comment 1 Mike Belshe 2009-12-09 12:17:19 PST
Created attachment 44554 [details]
patch
Comment 2 WebKit Review Bot 2009-12-09 12:19:26 PST
style-queue ran check-webkit-style on attachment 44554 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-09 13:24:27 PST
Comment on attachment 44554 [details]
patch

Looks sane.  Although you've already filled in the Reviewed by line, so I'm not sure what needs to be done for this patch if anything...
Comment 4 Darin Fisher (:fishd, Google) 2009-12-09 13:29:53 PST
Comment on attachment 44554 [details]
patch

> Index: WebKit/chromium/public/WebURLRequest.h

> -        TargetIsSubFrame,
> -        TargetIsSubResource,
> +        TargetIsSubframe,
> +        TargetIsSubresource,
> +        TargetIsStyleSheet,
> +        TargetIsScript,
> +        TargetIsFontResource,
> +        TargetIsImage,

Removal of webkit api is something we generally try to stage.
Otherwise, it puts burden on the person performing the webkit
update to land a two-sided patch :-(

I think I saw you send michaeln a patch for the Chromium side
of this, but in the future please preserve the old webkit api,
and then remove it in a subsequent patch once Chromium has
been updated.

Otherwise, r=me too.
Comment 5 Michael Nordman 2009-12-09 15:42:59 PST
> I think I saw you send michaeln a patch for the Chromium side
> of this, but in the future please preserve the old webkit api,
> and then remove it in a subsequent patch once Chromium has
> been updated.

Change looks great, having more nuanced info about what is being retrieved is really nice.

As mentioned in http://codereview.chromium.org/477008, can we stage these changes in without the need of the two-headed patch dance (you never know when the change that breaks 500 layout tests will just show up).
Comment 6 Mike Belshe 2009-12-09 16:23:47 PST
Created attachment 44579 [details]
followup patch to workaround horribly painful and broken chromium/webkit processes.
Comment 7 WebKit Review Bot 2009-12-09 16:26:27 PST
style-queue ran check-webkit-style on attachment 44579 [details] without any errors.
Comment 8 WebKit Commit Bot 2009-12-10 15:33:31 PST
Comment on attachment 44579 [details]
followup patch to workaround horribly painful and broken chromium/webkit processes.

Clearing flags on attachment: 44579

Committed r51967: <http://trac.webkit.org/changeset/51967>
Comment 9 Eric Seidel (no email) 2009-12-28 23:29:54 PST
I'm confused as to if this should still be open or not?
Comment 10 Eric Seidel (no email) 2009-12-28 23:30:26 PST
I believe this has been landed, so marking closed.  Please re-open if I'm wrong.