Bug 179063

Summary: Add a FileSystem namespace to FileSystem.cpp
Product: WebKit Reporter: Christopher Reid <chris.reid>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bfulgham, cdumez, commit-queue, darin, don.olmstead, ggaren, mmaxfield, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Christopher Reid 2017-10-31 11:21:42 PDT
I am planning on moving FileSystem to PAL soon. Before work is done to move it, I'm working on cleaning up and updating the code.
The functions of filesystem are global to the WebCore namespace. Adding a FileSystem namespace will help avoid naming collisions with these functions.
Comment 1 Christopher Reid 2017-10-31 11:34:27 PDT
Created attachment 325463 [details]
Patch
Comment 2 Christopher Reid 2017-10-31 12:45:33 PDT
Created attachment 325473 [details]
Patch

Missed a namespace in FileSystemCoca.mm
Comment 3 Christopher Reid 2017-10-31 13:04:23 PDT
Created attachment 325479 [details]
Patch
Comment 4 Christopher Reid 2017-10-31 14:19:40 PDT
Created attachment 325493 [details]
patch
Comment 5 Christopher Reid 2017-10-31 15:15:48 PDT
Created attachment 325504 [details]
Patch

Fixing some cases where FileSystem:: was wrongly added to functions with the same name as FileSystem functions.
Comment 6 Christopher Reid 2017-10-31 16:18:00 PDT
Created attachment 325513 [details]
Patch

Fixing an enum using two FileSystem namespaces
Comment 7 Christopher Reid 2017-11-01 00:52:16 PDT
Created attachment 325555 [details]
Patch

This should fix the remaining mac build issues
Comment 8 Darin Adler 2017-11-01 21:23:38 PDT
Comment on attachment 325555 [details]
Patch

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

I’m not thrilled by this change; I don’t really know what specific name conflicts this is preventing. But I guess it’s OK.

> Source/WebCore/ChangeLog:12
> +        * Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp:

The file list in this change log is a mess. For example, it’s not great to just leave in all sorts of erroneous "deleted" lines for functions moved inside a namespace. The point of the change log generation tool is to start the change log for you. It’s not great to just leave all the lines in there without looking them over. For a global replace like this, might even want to leave out all the function names.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:469
> +    Vector<String> entries = WebCore::FileSystem::listDirectory(directory, ASCIILiteral("*"));

Do we really need the WebCore prefix here? This code is in a namespace nested inside the WebCore namespace. I’m surprised this is required.
Comment 9 Christopher Reid 2017-11-02 13:56:22 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 325555 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325555&action=review
> 
> I’m not thrilled by this change; I don’t really know what specific name
> conflicts this is preventing. But I guess it’s OK.
> 
> > Source/WebCore/ChangeLog:12
> > +        * Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp:
> 
> The file list in this change log is a mess. For example, it’s not great to
> just leave in all sorts of erroneous "deleted" lines for functions moved
> inside a namespace. The point of the change log generation tool is to start
> the change log for you. It’s not great to just leave all the lines in there
> without looking them over. For a global replace like this, might even want
> to leave out all the function names.

That's good to know, I'll clean up the ChangeLogs and I can take out all the touched function names. 

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:469
> > +    Vector<String> entries = WebCore::FileSystem::listDirectory(directory, ASCIILiteral("*"));
> 
> Do we really need the WebCore prefix here? This code is in a namespace
> nested inside the WebCore namespace. I’m surprised this is required.

It actually looks fine without that WebCore namespace, I'll take it out.
Comment 10 Christopher Reid 2017-11-02 15:53:02 PDT
Created attachment 325786 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-11-02 19:33:30 PDT
Comment on attachment 325786 [details]
Patch for landing

Clearing flags on attachment: 325786

Committed r224371: <https://trac.webkit.org/changeset/224371>
Comment 12 WebKit Commit Bot 2017-11-02 19:33:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-11-15 12:29:43 PST
<rdar://problem/35567651>