Bug 28960 – [Chromium] Need to add manifestUrl to ResourceResponse.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Nordman]

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.

[Michael Nordman]

Created attachment 39027 [details]
simple patch

Also fixes a NPE in our v8 bindings.

[Mark Rowe (bdash)]

These seem like two unrelated changes.

[Michael Nordman]

(In reply to comment #2)
> These seem like two unrelated changes.

I guess the common thread is chrome/appcache.

[Eric Seidel (no email)]

How do other implementations solve the same "problem"?

[Michael Nordman]

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.

[Michael Nordman]

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.

[Michael Nordman]

fyi: here's the chrome-side of this
http://codereview.chromium.org/201026/show

[Darin Fisher (:fishd, Google)]

Comment on attachment 39027 [details]
simple patch

> Index: WebCore/ChangeLog
...
> [phone]  Michael Nordman  <[e-mail]>
> +
> +        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.

[Michael Nordman]

Created attachment 39127 [details]
simple patch 2

(OOPS!)

Also made changes the other existing getters/setters to match naming and style.

[Michael Nordman]

The matching change in chrome has been updated as well...
http://codereview.chromium.org/201026/show

[Darin Fisher (:fishd, Google)]

If I understand correctly, the DEPRECATED members exist so we can avoid a two-sided landing.  Hence, I went ahead and set commit-queue+.

[Eric Seidel (no email)]

Comment on attachment 39127 [details]
simple patch 2

Clearing flags on attachment: 39127

Committed r48109: <http://trac.webkit.org/changeset/48109>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

Created attachment 39202 [details]
house cleaning

Remove a few deprecated methods.

[Michael Nordman]

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

[Eric Seidel (no email)]

Bitten by bug 28230.

[Eric Seidel (no email)]

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+

[Michael Nordman]

(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!

[Eric Seidel (no email)]

Comment on attachment 39202 [details]
house cleaning

Clearing flags on attachment: 39202

Committed r48175: <http://trac.webkit.org/changeset/48175>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.