Bug 28003 – Cleanup: ResourceResponse should be a class, not a struct

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Michael Nordman]

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;

[Michael Nordman]

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 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)]

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

[Alexey Proskuryakov]

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

[Adam Barth]

Re-assigning to fishd per Comment #2.

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

This hasn't been landed yet.

[Eric Seidel (no email)]

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]

(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)]

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 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]

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

[Michael Nordman]

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)]

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]

(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)]

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)]

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

[Eric Seidel (no email)]

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]

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