Bug 28003

Summary:

Cleanup: ResourceResponse should be a class, not a struct

Product:

WebKit

Reporter:

Michael Nordman <michaeln>

Component:

WebCore Misc.

Assignee:

Darin Fisher (:fishd, Google) <fishd>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

abarth, ap, eric, fishd

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

30637, 30670

Bug Blocks:

Attachments:

Description	Flags
struct -> class patch	eric: review-, fishd: commit-queue-

Michael Nordman

Reported 2009-08-04 15:42:55 PDT

This 'struct' really should be a 'class'. Here's how things are defined for various ports... C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\cf\ResourceRequest.h(37): struct ResourceRequest : ResourceRequestBase { C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\curl\ResourceRequest.h(36): struct ResourceRequest : ResourceRequestBase { C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\qt\ResourceRequest.h(38): struct ResourceRequest : ResourceRequestBase { C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\soup\ResourceRequest.h(36): struct ResourceRequest : ResourceRequestBase { C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\chromium\ResourceRequest.h(38): struct ResourceRequest : public ResourceRequestBase { C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\mac\ResourceRequest.h(41): class ResourceRequest : public ResourceRequestBase { Here's how many of the forward declarations are declared... C:\chrome\1\src\third_party\WebKit\WebCore\history\HistoryItem.h(53):struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\inspector\InspectorController.h(67):struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\inspector\InspectorResource.h(54): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\appcache\ApplicationCache.h(45):struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\appcache\ApplicationCacheHost.h(51): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\DocumentThreadableLoader.h(42): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\FrameLoaderClient.h(65): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\MainResourceLoader.h(43): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\SubresourceLoader.h(36): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\SubresourceLoaderClient.h(36):struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\ThreadableLoader.h(41): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\loader\WorkerThreadableLoader.h(50): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\cf\ResourceRequestCFNet.h(33): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\ResourceHandleClient.h(53): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\platform\network\ResourceRequestBase.h(49): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\platform\CrossThreadCopier.h(44): struct ResourceRequest; C:\chrome\1\src\third_party\WebKit\WebCore\xml\XMLHttpRequest.h(37):struct ResourceRequest;

Attachments
struct -> class patch (12.85 KB, patch) 2009-08-05 15:15 PDT, Michael Nordman	eric: review- fishd: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Michael Nordman

Comment 1 2009-08-05 15:15:18 PDT

Created attachment 34184 [details] struct -> class patch Not much here, just some house cleaning. I think I've got all of them in the webkit repository. There are a few in chrome's repository to get as well once this lands and rolls into view for us there.

Darin Fisher (:fishd, Google)

Comment 2 2009-08-05 21:55:19 PDT

Comment on attachment 34184 [details] struct -> class patch commit-queue- because I need to land this in conjunction with a chromium patch.

Eric Seidel (no email)

Comment 3 2009-08-05 22:09:34 PDT

I feel like we've changed *this particular class* back and forth like 3 times now, no?

Alexey Proskuryakov

Comment 4 2009-08-06 13:35:37 PDT

Several people landed build fixes for their ports without looking at the bigger picture, yes. This should resolve the problem for real.

Adam Barth

Comment 5 2009-08-07 18:53:39 PDT

Re-assigning to fishd per Comment #2.

Eric Seidel (no email)

Comment 6 2009-08-21 14:56:55 PDT

It's been 2 weeks, just wanted to check if this should still be open.

Alexey Proskuryakov

Comment 7 2009-08-21 16:04:58 PDT

This hasn't been landed yet.

Eric Seidel (no email)

Comment 8 2009-09-01 03:07:19 PDT

Just checking in again, seeing this is still in the to-be-committed list after another week and a half. :) Really not trying to be a pest...

Michael Nordman

Comment 9 2009-09-05 08:13:31 PDT

(In reply to comment #8) > Just checking in again, seeing this is still in the to-be-committed list after > another week and a half. :) Really not trying to be a pest... We're holding off until the chromium port is landed in webkit's svn to avoid the dreaded two-sided patch landing stunt maneuver.

Eric Seidel (no email)

Comment 10 2009-09-18 12:07:27 PDT

I suspect that this patch will be long out of date by the time that happens. We should probably mark this r- once this no longer applies and you can post a new patch once you're ready to land.

Eric Seidel (no email)

Comment 11 2009-09-18 12:12:00 PDT

Comment on attachment 34184 [details] struct -> class patch Yeah: 1 out of 1 hunk FAILED -- saving rejects to file WebCore/loader/DocumentThreadableLoader.h.rej I'm happy to r+ a patch once you're closer to ready to land. As is, seems silly to leave this in the queue.

Alexey Proskuryakov

Comment 12 2009-09-18 12:50:21 PDT

How much trouble will it be if this is landed separately, with Chromium changes to follow? It's been a long wait.

Michael Nordman

Comment 13 2009-09-18 13:04:44 PDT

Is there any pressing need to get this simple cosmetic change landed? Is having this sit in the Q really an issue? To answer your question, it's no more trouble than the average webkit patch that breaks our build for some relatively simple reason... our canary goes red shortly after things land, and the poor soul serving as webkit gardener has to figure out why... and when rolling DEPS (switching what revision we build the mainline against) to the revision with the breaking change, be sure to commit the chrome-side changes with that DEPS roll. That is the so called two-sided patch process. Now if there's nothing else snaking thru the system to resolve, its not a big deal. But if at the same time we're working thru some other build issues (internal or webkit related which can pop up at any time)... it can be not a lot of fun. We're working on getting the chromium port setup as a first class citizen which will make the dreaded two-sided patch a thing of the past (i hope).

Eric Seidel (no email)

Comment 14 2009-09-18 13:15:22 PDT

I don't think there is any rush on this patch, no. I don't think that it makes much sense for patches which do not apply to sit r+'d in the to-be-committed list. :) I try and look through the to-be-committed list about once a week and make sure that patches from 3rd parties aren't just rotting there waiting for commit, or that bugs aren't sitting open which have already been landed, etc. Having un-actionable patches there is just a distraction.

Michael Nordman

Comment 15 2009-09-18 13:24:15 PDT

(In reply to comment #14) > I don't think there is any rush on this patch, no. > > I don't think that it makes much sense for patches which do not apply to sit > r+'d in the to-be-committed list. :) I try and look through the > to-be-committed list about once a week and make sure that patches from 3rd > parties aren't just rotting there waiting for commit, or that bugs aren't > sitting open which have already been landed, etc. Having un-actionable patches > there is just a distraction. So w're ok to have this sit around with r- on it for a bit?

Eric Seidel (no email)

Comment 16 2009-09-18 13:32:31 PDT

There are lots and lots and lots of open bugs in bugzilla with r-'d patches on them. :) No one will notice.

Eric Seidel (no email)

Comment 17 2009-10-21 14:41:33 PDT

Sad that this is still in purgatory. bug 30637 was a dupe.

Eric Seidel (no email)

Comment 18 2009-11-08 10:39:25 PST

This bug is being slowly done by other bugs anyway, it's soon going to be uneeded. bug 30670 is the latest one.

Michael Nordman

Comment 19 2009-11-18 15:33:40 PST

Looks like this has been taken care of in a piecemeal fashion. Marking as RESOLVED/INVALID.

Note You need to log in before you can comment on or make changes to this bug.