Bug 85766 - [Chromium] Move createLocalStorageNamespace to Platform.h
Summary: [Chromium] Move createLocalStorageNamespace to Platform.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
Depends on: 86313
Blocks: 82948
  Show dependency treegraph
 
Reported: 2012-05-06 19:49 PDT by Mark Pilgrim (Google)
Modified: 2012-06-05 11:54 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2012-05-06 19:50 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (18.58 KB, patch)
2012-05-11 12:43 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (90.22 KB, patch)
2012-05-14 07:55 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (17.00 KB, patch)
2012-06-05 08:00 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-05-06 19:49:38 PDT
[Chromium] Move createLocalStorageNamespace to Platform.h
Comment 1 Mark Pilgrim (Google) 2012-05-06 19:50:28 PDT
Created attachment 140459 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-06 19:51:47 PDT
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 3 Adam Barth 2012-05-06 20:42:59 PDT
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".
Comment 4 Michael Nordman 2012-05-07 13:24:50 PDT
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)
Comment 5 Michael Nordman 2012-05-07 14:26:29 PDT
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.
Comment 6 Adam Barth 2012-05-07 15:13:30 PDT
> 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.
Comment 7 Michael Nordman 2012-05-10 19:01:03 PDT
fyi https://bugs.webkit.org/show_bug.cgi?id=85221 is landed, thnx for waiting
Comment 8 Mark Pilgrim (Google) 2012-05-11 12:43:47 PDT
Created attachment 141477 [details]
Patch
Comment 9 Mark Pilgrim (Google) 2012-05-11 12:44:45 PDT
Comment on attachment 141477 [details]
Patch

Also moves the other classes Adam wanted.
Comment 10 Michael Nordman 2012-05-11 12:54:04 PDT
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
Comment 11 James Robinson 2012-05-11 12:56:36 PDT
(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 12 Adam Barth 2012-05-11 13:31:49 PDT
Comment on attachment 141477 [details]
Patch

LGTM.
Comment 13 Adam Barth 2012-05-11 13:32:23 PDT
Comment on attachment 141477 [details]
Patch

It looks like Bug 85221 has landed, so we should be good to go here.
Comment 14 WebKit Review Bot 2012-05-11 15:29:50 PDT
Comment on attachment 141477 [details]
Patch

Clearing flags on attachment: 141477

Committed r116812: <http://trac.webkit.org/changeset/116812>
Comment 15 WebKit Review Bot 2012-05-11 15:29:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2012-05-12 23:00:44 PDT
Re-opened since this is blocked by 86313
Comment 17 Gavin Peters 2012-05-14 07:55:17 PDT
Created attachment 141729 [details]
Patch
Comment 18 Gavin Peters 2012-05-14 07:57:43 PDT
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.
Comment 19 Mark Pilgrim (Google) 2012-06-05 08:00:31 PDT
Created attachment 145797 [details]
Patch
Comment 20 Mark Pilgrim (Google) 2012-06-05 08:03:13 PDT
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 21 WebKit Review Bot 2012-06-05 11:54:08 PDT
Comment on attachment 145797 [details]
Patch

Clearing flags on attachment: 145797

Committed r119510: <http://trac.webkit.org/changeset/119510>
Comment 22 WebKit Review Bot 2012-06-05 11:54:30 PDT
All reviewed patches have been landed.  Closing bug.