Bug 24897 – Chromium's ResourceRequest and ResourceResponse structs need some new data members.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Michael Nordman]

Chromium's ResponseRequest and ResourceResponse structs need some new data members... patch soon to follow.

[Michael Nordman]

Created attachment 29024 [details]
A simple patch

[Michael Nordman]

Oh... this has a companion CL in chromium...
http://codereview.chromium.org/9712/show

[Darin Fisher (:fishd, Google)]

Can you add a comment here explaining why these members are needed?

[Michael Nordman]

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.

[David Levin]

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.

[Michael Nordman]

Created attachment 29030 [details]
new patch snapshot #2

Thnx David, new snapshot uploaded.

[Darin Fisher (:fishd, Google)]

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

[Michael Nordman]

> 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

[Darin Fisher (:fishd, Google)]

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?

[Michael Nordman]

>+        // 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.

[Darin Fisher (:fishd, Google)]

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

[Michael Nordman]

Yes, this is why the symbol kNoAppCacheID does not appear in the ctor of this struct. The value 0 is used directly.

[Michael Nordman]

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?

[Michael Nordman]

Created attachment 29202 [details]
take 3

New patch uploaded
* added bug link the the changelog
* removed reference to kNoAppCacheID from comment

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 29202 [details]
take 3

>[phone]  michaeln  <[e-mail]>

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.

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42180