Bug 28960 - [Chromium] Need to add manifestUrl to ResourceResponse.
Summary: [Chromium] Need to add manifestUrl to ResourceResponse.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-03 18:28 PDT by Michael Nordman
Modified: 2009-09-08 12:28 PDT (History)
2 users (show)

See Also:


Attachments
simple patch (2.73 KB, patch)
2009-09-03 18:30 PDT, Michael Nordman
fishd: review-
Details | Formatted Diff | Diff
simple patch 2 (7.35 KB, patch)
2009-09-06 16:00 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
house cleaning (1.86 KB, patch)
2009-09-08 12:13 PDT, Michael Nordman
no flags 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-09-03 18:28:28 PDT
We need to add a manifestUrl data member to ResourceResponse. Only needs to be populated for 'main' resource responses. This is needed for use to detect 'foreign' entries early on and to restart the navigation at that time.
Comment 1 Michael Nordman 2009-09-03 18:30:14 PDT
Created attachment 39027 [details]
simple patch

Also fixes a NPE in our v8 bindings.
Comment 2 Mark Rowe (bdash) 2009-09-03 18:46:11 PDT
These seem like two unrelated changes.
Comment 3 Michael Nordman 2009-09-03 18:49:18 PDT
(In reply to comment #2)
> These seem like two unrelated changes.

I guess the common thread is chrome/appcache.
Comment 4 Eric Seidel (no email) 2009-09-04 00:16:49 PDT
How do other implementations solve the same "problem"?
Comment 5 Michael Nordman 2009-09-04 01:23:39 PDT
I presume your wondering about webcore's impl... see ApplicationCacheHost::selectCacheWithManifest and ApplicationCacheGroup::selectCache. These methods detect 'foreign' entries and as a side effect may restart the frame navigation.
Comment 6 Michael Nordman 2009-09-04 01:27:29 PDT
Also see WebAppplicationCacheHostImpl::selectCacheWithManifest() in chrome code.

We solve the "problem" in the same way. We just get the manifestUrl the response was loaded from in a different way.
Comment 7 Michael Nordman 2009-09-04 14:52:28 PDT
fyi: here's the chrome-side of this
http://codereview.chromium.org/201026/show
Comment 8 Darin Fisher (:fishd, Google) 2009-09-04 22:08:26 PDT
Comment on attachment 39027 [details]
simple patch

> Index: WebCore/ChangeLog
...
> +2009-08-31  Michael Nordman  <michaeln@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Chromium appcache modifications.
> +
> +        No new tests.

nit: please include a link to this bug report.


> Index: WebCore/bindings/v8/custom/V8DOMApplicationCacheCustom.cpp
...
> +    if (EventListener* listener = appcache->getAttributeEventListener(toEventID(name)))
> +       return eventListenerToV8Object(listener);

nit: indentation should be 4 white spaces


> Index: WebCore/platform/network/chromium/ResourceResponse.h
...
> +        const KURL& getAppCacheManifestURL() const { return m_appCacheManifestURL; }

getters should not start with "get"... just call this appCacheManifestURL.
Comment 9 Michael Nordman 2009-09-06 16:00:43 PDT
Created attachment 39127 [details]
simple patch 2

(OOPS!)

Also made changes the other existing getters/setters to match naming and style.
Comment 10 Michael Nordman 2009-09-06 16:04:37 PDT
The matching change in chrome has been updated as well...
http://codereview.chromium.org/201026/show
Comment 11 Darin Fisher (:fishd, Google) 2009-09-07 00:28:23 PDT
If I understand correctly, the DEPRECATED members exist so we can avoid a two-sided landing.  Hence, I went ahead and set commit-queue+.
Comment 12 Eric Seidel (no email) 2009-09-07 00:35:27 PDT
Comment on attachment 39127 [details]
simple patch 2

Clearing flags on attachment: 39127

Committed r48109: <http://trac.webkit.org/changeset/48109>
Comment 13 Eric Seidel (no email) 2009-09-07 00:35:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Michael Nordman 2009-09-08 12:13:25 PDT
Created attachment 39202 [details]
house cleaning

Remove a few deprecated methods.
Comment 15 Michael Nordman 2009-09-08 12:15:54 PDT
Resetting status to UNCONFIRMED so the new house cleaning patch is seen.

(Not sure if thats actually necessary or not for the patch to be picked up by std queries for things todo?)
Comment 16 Eric Seidel (no email) 2009-09-08 12:17:56 PDT
Bitten by bug 28230.
Comment 17 Eric Seidel (no email) 2009-09-08 12:19:05 PDT
Comment on attachment 39202 [details]
house cleaning

Given no indication otherwise, I assume this patch can be landed w/o coordination with Chromium.  r+/cq+
Comment 18 Michael Nordman 2009-09-08 12:20:44 PDT
(In reply to comment #17)
> (From update of attachment 39202 [details])
> Given no indication otherwise, I assume this patch can be landed w/o
> coordination with Chromium.  r+/cq+

Yes, thank you!
Comment 19 Eric Seidel (no email) 2009-09-08 12:28:54 PDT
Comment on attachment 39202 [details]
house cleaning

Clearing flags on attachment: 39202

Committed r48175: <http://trac.webkit.org/changeset/48175>
Comment 20 Eric Seidel (no email) 2009-09-08 12:28:59 PDT
All reviewed patches have been landed.  Closing bug.