webkit: Merge blob and fileapi into one build target 'storage'

webkit: Merge blob and fileapi into one build target 'webkit_storage'

Replace build targets 'blob' and 'fileapi' with newly introduced 'webkit_storage'
Introduce a new directory webkit/storage to host webkit_storage.gypi and webkit_storage_export.h
Replace BLOB_EXPORT and FILEAPI_EXPORT with WEBKIT_STORAGE_EXPORT

BUG=155242
TEST=build
TBR=darin@chromium.org, piman@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162679

[hashimoto, 2012-10-11 10:26:25]

Please review

[kinuko, 2012-10-11 12:36:50]

Thanks for making this change!  Please resolve the try failures (you may need
clobber?).

As for the target/directory naming I defer to Darin/Dirk.

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
I think we'd better name this 'webkit_storage'

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
webkit/storage/webkit_storage.gypi:32: 'sources': [
Can we add a TODO comment to note we're importing more storage files?  Naming
blob+fileapi 'storage' may sound a bit weird (if we're only combining them we
can probably just name it 'fileapi')

[hashimoto, 2012-10-11 13:22:45]

On 2012/10/11 12:36:50, kinuko wrote:
> Thanks for making this change!  Please resolve the try failures (you may need
> clobber?).
Bots are failing because DumpRenderTree (under third_party/WebKit/Tools) is
still depending on the old 'blob' target.
Looks like we need to make changes for WebKit at the same time.
> 
> As for the target/directory naming I defer to Darin/Dirk.
> 
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> File webkit/storage/webkit_storage.gypi (right):
> 
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
> I think we'd better name this 'webkit_storage'
> 
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> webkit/storage/webkit_storage.gypi:32: 'sources': [
> Can we add a TODO comment to note we're importing more storage files?  Naming
> blob+fileapi 'storage' may sound a bit weird (if we're only combining them we
> can probably just name it 'fileapi')

[kinuko, 2012-10-12 03:32:45]

On 2012/10/11 13:22:45, hashimoto wrote:
> On 2012/10/11 12:36:50, kinuko wrote:
> > Thanks for making this change!  Please resolve the try failures (you may
need
> > clobber?).
> Bots are failing because DumpRenderTree (under third_party/WebKit/Tools) is
> still depending on the old 'blob' target.
> Looks like we need to make changes for WebKit at the same time.

I think we can first make a change that leaves an empty blob target (with no
sources), change WebKit's gyp and remove the empty target.

> > As for the target/directory naming I defer to Darin/Dirk.
> > 
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > File webkit/storage/webkit_storage.gypi (right):
> > 
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
> > I think we'd better name this 'webkit_storage'
> > 
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > webkit/storage/webkit_storage.gypi:32: 'sources': [
> > Can we add a TODO comment to note we're importing more storage files? 
Naming
> > blob+fileapi 'storage' may sound a bit weird (if we're only combining them
we
> > can probably just name it 'fileapi')

[kinuko, 2012-10-12 03:38:18]

On 2012/10/12 03:32:45, kinuko wrote:
> On 2012/10/11 13:22:45, hashimoto wrote:
> > On 2012/10/11 12:36:50, kinuko wrote:
> > > Thanks for making this change!  Please resolve the try failures (you may
> need
> > > clobber?).
> > Bots are failing because DumpRenderTree (under third_party/WebKit/Tools) is
> > still depending on the old 'blob' target.
> > Looks like we need to make changes for WebKit at the same time.
> 
> I think we can first make a change that leaves an empty blob target (with no
> sources), change WebKit's gyp and remove the empty target.

Wait... can you check why we specifically need the blob target there?
Isn't webkit_support.gyp:webkit_support enough?

> > > As for the target/directory naming I defer to Darin/Dirk.
> > > 
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > File webkit/storage/webkit_storage.gypi (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
> > > I think we'd better name this 'webkit_storage'
> > > 
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > webkit/storage/webkit_storage.gypi:32: 'sources': [
> > > Can we add a TODO comment to note we're importing more storage files? 
> Naming
> > > blob+fileapi 'storage' may sound a bit weird (if we're only combining them
> we
> > > can probably just name it 'fileapi')

[hashimoto, 2012-10-12 03:54:08]

On 2012/10/12 03:38:18, kinuko wrote:
> On 2012/10/12 03:32:45, kinuko wrote:
> > On 2012/10/11 13:22:45, hashimoto wrote:
> > > On 2012/10/11 12:36:50, kinuko wrote:
> > > > Thanks for making this change!  Please resolve the try failures (you may
> > need
> > > > clobber?).
> > > Bots are failing because DumpRenderTree (under third_party/WebKit/Tools)
is
> > > still depending on the old 'blob' target.
> > > Looks like we need to make changes for WebKit at the same time.
> > 
> > I think we can first make a change that leaves an empty blob target (with no
> > sources), change WebKit's gyp and remove the empty target.
> 
> Wait... can you check why we specifically need the blob target there?
> Isn't webkit_support.gyp:webkit_support enough?
Removed the 'blob' line from DumpRenderTree.gyp locally and succeeded clobber
build.
It looks safe to stop depending on 'blob' from DumpRenderTree.
> 
> > > > As for the target/directory naming I defer to Darin/Dirk.
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > File webkit/storage/webkit_storage.gypi (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
> > > > I think we'd better name this 'webkit_storage'
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > webkit/storage/webkit_storage.gypi:32: 'sources': [
> > > > Can we add a TODO comment to note we're importing more storage files? 
> > Naming
> > > > blob+fileapi 'storage' may sound a bit weird (if we're only combining
them
> > we
> > > > can probably just name it 'fileapi')

[kinuko, 2012-10-12 04:29:44]

On 2012/10/12 03:54:08, hashimoto wrote:
> On 2012/10/12 03:38:18, kinuko wrote:
> > On 2012/10/12 03:32:45, kinuko wrote:
> > > On 2012/10/11 13:22:45, hashimoto wrote:
> > > > On 2012/10/11 12:36:50, kinuko wrote:
> > > > > Thanks for making this change!  Please resolve the try failures (you
may
> > > need
> > > > > clobber?).
> > > > Bots are failing because DumpRenderTree (under third_party/WebKit/Tools)
> is
> > > > still depending on the old 'blob' target.
> > > > Looks like we need to make changes for WebKit at the same time.
> > > 
> > > I think we can first make a change that leaves an empty blob target (with
no
> > > sources), change WebKit's gyp and remove the empty target.
> > 
> > Wait... can you check why we specifically need the blob target there?
> > Isn't webkit_support.gyp:webkit_support enough?
> Removed the 'blob' line from DumpRenderTree.gyp locally and succeeded clobber
> build.
> It looks safe to stop depending on 'blob' from DumpRenderTree.

Running WebKit try. Let's see if we can remove it:
https://bugs.webkit.org/show_bug.cgi?id=99132

> > > > > As for the target/directory naming I defer to Darin/Dirk.
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > > File webkit/storage/webkit_storage.gypi (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > > webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
> > > > > I think we'd better name this 'webkit_storage'
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
> > > > > webkit/storage/webkit_storage.gypi:32: 'sources': [
> > > > > Can we add a TODO comment to note we're importing more storage files? 
> > > Naming
> > > > > blob+fileapi 'storage' may sound a bit weird (if we're only combining
> them
> > > we
> > > > > can probably just name it 'fileapi')

[hashimoto, 2012-10-12 06:45:27]

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
webkit/storage/webkit_storage.gypi:17: 'target_name': 'storage',
On 2012/10/11 12:36:51, kinuko wrote:
> I think we'd better name this 'webkit_storage'

Done.

http://codereview.chromium.org/11103031/diff/5066/webkit/storage/webkit_stora...
webkit/storage/webkit_storage.gypi:32: 'sources': [
On 2012/10/11 12:36:51, kinuko wrote:
> Can we add a TODO comment to note we're importing more storage files?  Naming
> blob+fileapi 'storage' may sound a bit weird (if we're only combining them we
> can probably just name it 'fileapi')

Done.

[marja, 2012-10-12 11:14:42]

http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_stor...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_stor...
webkit/storage/webkit_storage.gypi:17: 'target_name': 'webkit_storage',
It's slightly confusing though that you're renaming fileapi to webkit_storage,
because there is dom_storage, too.

[kinuko, 2012-10-12 11:32:14]

http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_stor...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_stor...
webkit/storage/webkit_storage.gypi:17: 'target_name': 'webkit_storage',
On 2012/10/12 11:14:42, marja wrote:
> It's slightly confusing though that you're renaming fileapi to webkit_storage,
> because there is dom_storage, too.

That's true, but our plan is going to include dom_storage into this one too
(that's why we have TODO comment in line 33).

If it looks too weird we can keep the target name webkit_fileapi or something
limited to blob+fileapi for now.

[darin (slow to review), 2012-10-12 21:32:42]

On Fri, Oct 12, 2012 at 4:32 AM, <kinuko@chromium.org> wrote:

>
> http://codereview.chromium.**org/11103031/diff/24001/**
>
webkit/storage/webkit_storage.**gypi<http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_storage.gypi>
> File webkit/storage/webkit_storage.**gypi (right):
>
> http://codereview.chromium.**org/11103031/diff/24001/**
>
webkit/storage/webkit_storage.**gypi#newcode17<http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_storage.gypi#newcode17>
> webkit/storage/webkit_storage.**gypi:17: 'target_name': 'webkit_storage',
> On 2012/10/12 11:14:42, marja wrote:
>
>> It's slightly confusing though that you're renaming fileapi to
>>
> webkit_storage,
>
>> because there is dom_storage, too.
>>
>
> That's true, but our plan is going to include dom_storage into this one
> too (that's why we have TODO comment in line 33).
>
> If it looks too weird we can keep the target name webkit_fileapi or
> something limited to blob+fileapi for now.
>
>
Might as well pick the long term name now.   I assume you plan to merge in
other modules shortly, right? :-)

-Darin



>
http://codereview.chromium.**org/11103031/<http://codereview.chromium.org/111...
>

[kinuko, 2012-10-13 01:24:06]

On 2012/10/12 21:32:42, darin wrote:
> On Fri, Oct 12, 2012 at 4:32 AM, <mailto:kinuko@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/11103031/diff/24001/**
> >
>
webkit/storage/webkit_storage.**gypi<http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_storage.gypi>
> > File webkit/storage/webkit_storage.**gypi (right):
> >
> > http://codereview.chromium.**org/11103031/diff/24001/**
> >
>
webkit/storage/webkit_storage.**gypi#newcode17<http://codereview.chromium.org/11103031/diff/24001/webkit/storage/webkit_storage.gypi#newcode17>
> > webkit/storage/webkit_storage.**gypi:17: 'target_name': 'webkit_storage',
> > On 2012/10/12 11:14:42, marja wrote:
> >
> >> It's slightly confusing though that you're renaming fileapi to
> >>
> > webkit_storage,
> >
> >> because there is dom_storage, too.
> >>
> >
> > That's true, but our plan is going to include dom_storage into this one
> > too (that's why we have TODO comment in line 33).
> >
> > If it looks too weird we can keep the target name webkit_fileapi or
> > something limited to blob+fileapi for now.
> >
> >
> Might as well pick the long term name now.   I assume you plan to merge in
> other modules shortly, right? :-)

Yes, we plan to do so.

> -Darin
> 
> 
> 
> >
>
http://codereview.chromium.**org/11103031/%3Chttp://codereview.chromium.org/1...>
> >

[hashimoto, 2012-10-15 06:12:35]

Dirk and Darin,
Do the name of the new target and the addition of webkit/storage directory look
good to you?

[Dirk Pranke, 2012-10-15 18:48:12]

On 2012/10/15 06:12:35, hashimoto wrote:
> Dirk and Darin,
> Do the name of the new target and the addition of webkit/storage directory
look
> good to you?

Looks great to me.

[kinuko, 2012-10-16 04:45:49]

lgtm

[hashimoto, 2012-10-16 06:10:26]

Darin,
Does this change look good to you?

[hashimoto, 2012-10-17 06:03:06]

tony,
Could you review this change as a src/webkit owner?

[tony, 2012-10-17 16:58:09]

LGTM with one small suggestion.  If it doesn't work, then this is fine too.

http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
webkit/storage/webkit_storage.gypi:34: '../blob/blob_data.cc',
Would it be possible to keep a gypi file in each subdirectory that only defines
a variable that lists all the files in that directory?  E.g., there would be a
webkit_blob.gypi and a webkit_fileapi.gypi.  webkit_storage.gypi would include
the other files and sources would reference the variable from the other gypi
files.

The benefit is that it would be easy to find the .gypi file to add a new to
(it's not obvious that files in webkit/fileapi are listed in webkit/storage).

[hashimoto, 2012-10-18 05:16:06]

New patch set addressing tony's suggestion.
PTAL

http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
File webkit/storage/webkit_storage.gypi (right):

http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
webkit/storage/webkit_storage.gypi:34: '../blob/blob_data.cc',
On 2012/10/17 16:58:10, tony wrote:
> Would it be possible to keep a gypi file in each subdirectory that only
defines
> a variable that lists all the files in that directory?  E.g., there would be a
> webkit_blob.gypi and a webkit_fileapi.gypi.  webkit_storage.gypi would include
> the other files and sources would reference the variable from the other gypi
> files.
> 
> The benefit is that it would be easy to find the .gypi file to add a new to
> (it's not obvious that files in webkit/fileapi are listed in webkit/storage).

Done.

[kinuko, 2012-10-18 07:03:18]

On 2012/10/18 05:16:06, hashimoto wrote:
> New patch set addressing tony's suggestion.
> PTAL
> 
>
http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
> File webkit/storage/webkit_storage.gypi (right):
> 
>
http://codereview.chromium.org/11103031/diff/27002/webkit/storage/webkit_stor...
> webkit/storage/webkit_storage.gypi:34: '../blob/blob_data.cc',
> On 2012/10/17 16:58:10, tony wrote:
> > Would it be possible to keep a gypi file in each subdirectory that only
> defines
> > a variable that lists all the files in that directory?  E.g., there would be
a
> > webkit_blob.gypi and a webkit_fileapi.gypi.  webkit_storage.gypi would
include
> > the other files and sources would reference the variable from the other gypi
> > files.
> > 
> > The benefit is that it would be easy to find the .gypi file to add a new to
> > (it's not obvious that files in webkit/fileapi are listed in
webkit/storage).
> 
> Done.

Looks nicer to me (lgtm++)

[hashimoto, 2012-10-18 07:06:39]

TBR-ing darin@ for chrome/ and content/, piman@ for ui/compositor.

[commit-bot: I haz the power, 2012-10-18 07:07:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11103031/46003

[commit-bot: I haz the power, 2012-10-18 09:09:12]

Change committed as 162679