Bug 85766 – [Chromium] Move createLocalStorageNamespace to Platform.h

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

[Chromium] Move createLocalStorageNamespace to Platform.h

[Mark Pilgrim (Google)]

Created attachment 140459 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 140459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140459&action=review

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:-64
> -class WebStorageNamespace; // FIXME: Does this belong in platform?

Please move WebStorageNamespace.h and WebStorageArea.h at the same time.  It looks like there aren't any further dependencies, so that should work out fine.  We can then remove the FIXME comment because the answer seems to be "yes".

[Michael Nordman]

please wait till https://bugs.webkit.org/show_bug.cgi?id=85221 is committed prior to shuffling things around, i expect to commit tomorrow (been waiting on the m20 branch to be cut)

[Michael Nordman]

Just looked more closely at the patch, it doesn't actually resolve then linkage problem with the static StorageNamespace::create<<>> methods. I'm not sure shuffling the WebKit API method definitions to create WebStorageNamespace instances around needs to be done w/o actually solving the linkage problems.

[Adam Barth]

> I'm not sure shuffling the WebKit API method definitions to create WebStorageNamespace instances around needs to be done w/o actually solving the linkage problems.

We'll solve the linkage problems eventually.  This patch is just a step in the right direction.  Rather than solving this problem "depth first" by resolving each linkage problem, we're going "breath first" and refactoring the API to the point where we can fix these problems internally in WebKit.

[Michael Nordman]

fyi https://bugs.webkit.org/show_bug.cgi?id=85221 is landed, thnx for waiting

[Mark Pilgrim (Google)]

Created attachment 141477 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 141477 [details]
Patch

Also moves the other classes Adam wanted.

[Michael Nordman]

Comment on attachment 141477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141477&action=review

> Source/WebKit/chromium/public/WebStorageArea.h:31
> +#include "../../../Platform/chromium/public/WebStorageArea.h"

geez... look at that include path... is the plan to leave this files in WebKit/chromium/public or to switch consumers to include from Platform/chromium/public directly?

just curious

[James Robinson]

(In reply to comment #10)
> (From update of attachment 141477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141477&action=review
> 
> > Source/WebKit/chromium/public/WebStorageArea.h:31
> > +#include "../../../Platform/chromium/public/WebStorageArea.h"
> 
> geez... look at that include path... is the plan to leave this files in WebKit/chromium/public or to switch consumers to include from Platform/chromium/public directly?
> 
> just curious

We have two WebKit APIs - the client and the platform API.  If this header is logically part of the client API (i.e. it makes sense to talk about it in code that's otherwise using just the client API), then we leave this forwarding header in place.  If it's only used by code talking to the platform API, then we should update the chromium code to use the Platform location and then delete this header.  I don't want to force all chromium code currently using #includes from  WebKit/chromium/public/ to also have to #include stuff from Platform/... just to pick up types it wants to use.

[Adam Barth]

Comment on attachment 141477 [details]
Patch

LGTM.

[Adam Barth]

Comment on attachment 141477 [details]
Patch

It looks like Bug 85221 has landed, so we should be good to go here.

[WebKit Review Bot]

Comment on attachment 141477 [details]
Patch

Clearing flags on attachment: 141477

Committed r116812: <http://trac.webkit.org/changeset/116812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86313

[Gavin Peters]

Created attachment 141729 [details]
Patch

[Gavin Peters]

Mark, my apologies: webkit-patch spazziness led me to this bad upload.  I have obsoleted my upload, and unobsoleted yours.  I'm sorry for the confusion.

[Mark Pilgrim (Google)]

Created attachment 145797 [details]
Patch

[Mark Pilgrim (Google)]

Unbitrotting this patch after it was reviewed, committed, landed, mistakenly blamed for a downstream crash, rolled back, mistakenly obsoleted, and probably some other stuff I forgot.

[WebKit Review Bot]

Comment on attachment 145797 [details]
Patch

Clearing flags on attachment: 145797

Committed r119510: <http://trac.webkit.org/changeset/119510>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.