Bug 24897 - Chromium's ResourceRequest and ResourceResponse structs need some new data members.
Summary: Chromium's ResourceRequest and ResourceResponse structs need some new data me...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-27 14:21 PDT by Michael Nordman
Modified: 2009-04-02 13:00 PDT (History)
2 users (show)

See Also:


Attachments
A simple patch (4.97 KB, patch)
2009-03-27 14:25 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
new patch snapshot #2 (5.33 KB, patch)
2009-03-27 15:55 PDT, Michael Nordman
fishd: review-
Details | Formatted Diff | Diff
take 3 (5.42 KB, patch)
2009-04-02 11:23 PDT, Michael Nordman
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2009-03-27 14:21:01 PDT
Chromium's ResponseRequest and ResourceResponse structs need some new data members... patch soon to follow.
Comment 1 Michael Nordman 2009-03-27 14:25:11 PDT
Created attachment 29024 [details]
A simple patch
Comment 2 Michael Nordman 2009-03-27 14:31:26 PDT
Oh... this has a companion CL in chromium...
http://codereview.chromium.org/9712/show
Comment 3 Darin Fisher (:fishd, Google) 2009-03-27 14:37:19 PDT
Can you add a comment here explaining why these members are needed?
Comment 4 Michael Nordman 2009-03-27 14:48:14 PDT
Sure...

These fields are needed to facilitate Chromium's implementation of the HTML5 ApplicationCache feature. We need to know what frame(or context) is doing the requesting, and from what cache the resulting resource was retrieved.
Comment 5 David Levin 2009-03-27 15:40:07 PDT
Misc style notes:

1. Don't fill in the requestee field unless only that person can do the review.
2. Do this "<set EMAIL_ADDRESS environment variable>" and fix that in the changelog.
3. Add some explanation of the change to the ChangeLog.
4. Add a link to the bug in the ChangeLog.
5. Remove WARNING: NO TEST CASES ADDED OR CHANGED from the change log and add a test or explain why there shouldn't be one.  (For example, if there is no change in functionality, then "No change in functionality, so no test." usually suffices.)
6. Format  C++ initializers like this:

MyClass::MyClass(Document* doc)
    : MySuperClass()
    , m_myMember(0)
    , m_doc(doc)
{
}

7. Function definitions: place each brace on its own line.
8. The indent size is 4 spaces.

Comment 6 Michael Nordman 2009-03-27 15:55:57 PDT
Created attachment 29030 [details]
new patch snapshot #2

Thnx David, new snapshot uploaded.
Comment 7 Darin Fisher (:fishd, Google) 2009-03-30 12:59:14 PDT
> These fields are needed to facilitate Chromium's implementation of the HTML5
> ApplicationCache feature. We need to know what frame(or context) is doing the
> requesting, and from what cache the resulting resource was retrieved.

How come the other ports don't need this?  I'm trying to understand why we need to be divergent here.
Comment 8 Michael Nordman 2009-03-30 13:39:19 PDT
> How come the other ports don't need this?  I'm trying to understand why we need
> to be divergent here.

AFAIK, no other port implements the AppCache functionality in the embedder side of things.
Please refer to the companion CL for chromium.
http://codereview.chromium.org/9712/show
Comment 9 Darin Fisher (:fishd, Google) 2009-03-31 16:18:28 PDT
Comment on attachment 29030 [details]
new patch snapshot #2

OK.  Thanks for adding the reference to the Chromium CL here.  That helps
others understand this design as well :)


>Index: WebCore/ChangeLog

>+        Chromium's ResourceRequest and ResourceResponse structs need some new data members.
>+
>+        These fields are needed to facilitate Chromium's implementation of the HTML5
>+        ApplicationCache feature. We need to know what frame(or context) is doing the

nit: Need a space after "frame"


>+        requesting, and from what cache the resulting resource was retrieved.
>+
>+        No change in functionality, so no tests.

Please include a bug link in your ChangeLog entry.


>Index: WebCore/platform/network/chromium/ResourceResponse.h

>+        // The id of the appcache this response was retrieved from,
>+        // or kNoAppCacheID (0)
>+        int64 m_appCacheID;

Where is kNoAppCacheID defined?  also, how come the contextID is 32-bits but
the CacheID is 64-bits?
Comment 10 Michael Nordman 2009-03-31 16:26:48 PDT
>+        // The id of the appcache this response was retrieved from,
>+        // or kNoAppCacheID (0)
>+        int64 m_appCacheID;

Where is kNoAppCacheID defined?  also, how come the contextID is 32-bits but
the CacheID is 64-bits?

The kNoAppCacheID constant is defined in chromium's webkit glue, webappcachecontext.h. This id corresponds with a sqlite rowid, its 64 bits to keep full fidelity with how sqlite rowids are defined.

>+ ChangeLog edits

WIll do... will upload a new patch with changelog mods once we gotten to final edits on the .h file.
Comment 11 Darin Fisher (:fishd, Google) 2009-03-31 17:04:18 PDT
> The kNoAppCacheID constant is defined in chromium's webkit glue,
> webappcachecontext.h. This id corresponds with a sqlite rowid, its 64 bits to
> keep full fidelity with how sqlite rowids are defined.

Ah, OK.  So that violates dependencies.  glue can depend on webcore, but webcore cannot depend directly on glue.  In other words, webcore (even the chromium port of webcore) needs to stand on its own.
Comment 12 Michael Nordman 2009-03-31 17:13:55 PDT
Yes, this is why the symbol kNoAppCacheID does not appear in the ctor of this struct. The value 0 is used directly.
Comment 13 Michael Nordman 2009-03-31 17:20:51 PDT
How 'bout I remove the reference to kNoAppCacheID in the comment.

I don't see how is this any different than having a requestorID data member that is initialized to zero?  What does requestorID mean in webcore (sans chromium) speak?

Comment 14 Michael Nordman 2009-04-02 11:23:43 PDT
Created attachment 29202 [details]
take 3

New patch uploaded
* added bug link the the changelog
* removed reference to kNoAppCacheID from comment
Comment 15 Darin Fisher (:fishd, Google) 2009-04-02 12:51:27 PDT
> I don't see how is this any different than having a requestorID data member
> that is initialized to zero?  What does requestorID mean in webcore (sans
> chromium) speak?

It is an identifier that the requestor is free to define.  What it means in Chrome-land doesn't matter.
Comment 16 Darin Fisher (:fishd, Google) 2009-04-02 12:59:12 PDT
Comment on attachment 29202 [details]
take 3

>+2009-03-27  michaeln  <michaeln@google.com>

For future changes, please be sure to have the REAL_NAME environment
variable defined so that the ChangeLog includes your full name.  It
is WebKit convention to do so.
Comment 17 Darin Fisher (:fishd, Google) 2009-04-02 13:00:37 PDT
Landed as:  http://trac.webkit.org/changeset/42180