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: P2    
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-

Description Michael Nordman 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;
Comment 1 Michael Nordman 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.
Comment 2 Darin Fisher (:fishd, Google) 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.
Comment 3 Eric Seidel (no email) 2009-08-05 22:09:34 PDT
I feel like we've changed *this particular class* back and forth like 3 times now, no?
Comment 4 Alexey Proskuryakov 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.
Comment 5 Adam Barth 2009-08-07 18:53:39 PDT
Re-assigning to fishd per Comment #2.
Comment 6 Eric Seidel (no email) 2009-08-21 14:56:55 PDT
It's been 2 weeks, just wanted to check if this should still be open.
Comment 7 Alexey Proskuryakov 2009-08-21 16:04:58 PDT
This hasn't been landed yet.
Comment 8 Eric Seidel (no email) 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...
Comment 9 Michael Nordman 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.
Comment 10 Eric Seidel (no email) 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.
Comment 11 Eric Seidel (no email) 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.
Comment 12 Alexey Proskuryakov 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.
Comment 13 Michael Nordman 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).
Comment 14 Eric Seidel (no email) 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.
Comment 15 Michael Nordman 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?
Comment 16 Eric Seidel (no email) 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.
Comment 17 Eric Seidel (no email) 2009-10-21 14:41:33 PDT
Sad that this is still in purgatory.  bug 30637 was a dupe.
Comment 18 Eric Seidel (no email) 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.
Comment 19 Michael Nordman 2009-11-18 15:33:40 PST
Looks like this has been taken care of in a piecemeal fashion. Marking as RESOLVED/INVALID.