WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
179063
Add a FileSystem namespace to FileSystem.cpp
https://bugs.webkit.org/show_bug.cgi?id=179063
Summary
Add a FileSystem namespace to FileSystem.cpp
Christopher Reid
Reported
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.
Attachments
Patch
(346.01 KB, patch)
2017-10-31 11:34 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch
(344.62 KB, patch)
2017-10-31 12:45 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch
(331.57 KB, patch)
2017-10-31 13:04 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
patch
(305.08 KB, patch)
2017-10-31 14:19 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch
(300.57 KB, patch)
2017-10-31 15:15 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch
(300.55 KB, patch)
2017-10-31 16:18 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch
(324.38 KB, patch)
2017-11-01 00:52 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Patch for landing
(307.67 KB, patch)
2017-11-02 15:53 PDT
,
Christopher Reid
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Christopher Reid
Comment 1
2017-10-31 11:34:27 PDT
Created
attachment 325463
[details]
Patch
Christopher Reid
Comment 2
2017-10-31 12:45:33 PDT
Created
attachment 325473
[details]
Patch Missed a namespace in FileSystemCoca.mm
Christopher Reid
Comment 3
2017-10-31 13:04:23 PDT
Created
attachment 325479
[details]
Patch
Christopher Reid
Comment 4
2017-10-31 14:19:40 PDT
Created
attachment 325493
[details]
patch
Christopher Reid
Comment 5
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.
Christopher Reid
Comment 6
2017-10-31 16:18:00 PDT
Created
attachment 325513
[details]
Patch Fixing an enum using two FileSystem namespaces
Christopher Reid
Comment 7
2017-11-01 00:52:16 PDT
Created
attachment 325555
[details]
Patch This should fix the remaining mac build issues
Darin Adler
Comment 8
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.
Christopher Reid
Comment 9
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.
Christopher Reid
Comment 10
2017-11-02 15:53:02 PDT
Created
attachment 325786
[details]
Patch for landing
WebKit Commit Bot
Comment 11
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
>
WebKit Commit Bot
Comment 12
2017-11-02 19:33:32 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 13
2017-11-15 12:29:43 PST
<
rdar://problem/35567651
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug