Bug 85766

Created attachment 140459 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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

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)

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.

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

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

Created attachment 141477 [details] Patch

Comment on attachment 141477 [details] Patch Also moves the other classes Adam wanted.

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

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

Comment on attachment 141477 [details] Patch LGTM.

Comment on attachment 141477 [details] Patch It looks like Bug 85221 has landed, so we should be good to go here.

Comment on attachment 141477 [details] Patch Clearing flags on attachment: 141477 Committed r116812: <http://trac.webkit.org/changeset/116812>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 86313

Created attachment 141729 [details] Patch

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.

Created attachment 145797 [details] Patch

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.

Comment on attachment 145797 [details] Patch Clearing flags on attachment: 145797 Committed r119510: <http://trac.webkit.org/changeset/119510>

All reviewed patches have been landed. Closing bug.