Closed Bug 1428543 Opened 6 years ago Closed 6 years ago

Modify nsIFile for preparing GetNativePath removal

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files)

      No description provided.
(In reply to Nathan Froyd [:froydnj] from bug 685236 comment #67)
> We tend to discourage namespaces underneath mozilla::, and the usual
> mozilla::filesystem::Path::value_type feels like a mouthful.  Do you think
> it'd be reasonable to just have mozilla::Path, or were you trying to save
> that name for a more fully-featured Path class?
> 
> WDYT about calling Path PathTraits or similar, since there's no actual data
> storage in this class?

The naming scheme is taken from C++17 std::filesystem::path, although most std::filesystem features are not implemented yet.

I tried mozilla::Path first, but it caused name conflicts due to mozilla::gfx::Path :(
Do you know a good way round?

> I realize Path::value_type is carried over here from nsTString.  But I don't
> know that it's gaining us anything having it here as a public API, or that
> it's a reasonable name for a public API in this case.
> 
> WDYT about making value_type private to Path, and then requiring people to
> use Path::string_type::value_type if they need to?  I realize that's more
> typing, but I don't expect it to be used very much, and I think it's
> slightly more obvious what's being referenced in that case.

Actually I took it from std::filesystem::path::value_type.

> Referencing nsTString and similar from MFBT is not really allowed; you're
> depending on users including some string header prior to this, which is not
> the typical way we structure headers in Gecko.  Why are we putting this in
> MFBT rather than in XPCOM?

I would like to use Path::value_type from Moz2D, where XPCOM is not allowed either. Where is the best place for such things?

(In reply to Nathan Froyd [:froydnj] from bug 685236 comment #68)
> Comment on attachment 8939827 [details]
> Bug 685236 - Implement FileDescriptorFile::GetPersistentDescriptor.
> 
> It looks like this is primarily used to replace GetNativePath usages in
> subsequent patches.
> 
> 1) Why is this a reasonable transformation?  (Are we sure that a "persistent
> descriptor" is really what we want?)
> 2) Why are we not simply using GetPersistentDescriptor everywhere that we
> were using GetNativePath?
> 
> The code of the patch is obviously OK, but I am unsure of the semantics that
> we're trying to implement here...

I'm using GetPersistentDescriptor based on the interface description:
>     *  Accessor to a null terminated string which will specify
>     *  the file in a persistent manner for disk storage.

Although the GetPersistentDescriptor result happens to be a UTF-8 path on Windows and a native path on other platforms, I would not like to depend on the implementation detail. It should be considered as an opaque identifier. So I'm using it when unique identifiers are necessary (for example, hash keys), but I'm not using it to open files on the filesystem. It will not work on Windows anyway.

(In reply to Nathan Froyd [:froydnj] from bug 685236 comment #69)
> Comment on attachment 8939826 [details]
> Bug 685236 - Add nsIFile::DisplayPath.
> 
> `DisplayPath` reads ambiguously to me: is the method displaying a path, or
> is it returning a path for display?  Maybe PathForHumanConsumption? :)  (Not
> a great name, better suggestions welcome!)
> 
> I am a little concerned that we are adding more API surface to nsIFile. 
> Would it not be better to add a standalone function here that we implement
> differently for Windows and Unix?

Unfortunately, nsLocalFile is not the only nsIFile implementation. DisplayPath must be an nsIFile method due to FileDescriptorFile :(

Alternatively, we can (ab)use GetPersistentDescriptor to implement DisplayPath despite that the interface description says "DO NOT TRY TO INTERPRET IT AS HUMAN READABLE TEXT!". WDYT?
Flags: needinfo?(nfroyd)
Changes:
* Moved string type aliases from Path.h to nsIFile.h.
* Renamed DisplayPath to HumanReadablePath.
* Implemented HumanReadablePath using GetPersistentDescriptor.
(In reply to Masatoshi Kimura [:emk] from comment #1)
> (In reply to Nathan Froyd [:froydnj] from bug 685236 comment #67)
> > We tend to discourage namespaces underneath mozilla::, and the usual
> > mozilla::filesystem::Path::value_type feels like a mouthful.  Do you think
> > it'd be reasonable to just have mozilla::Path, or were you trying to save
> > that name for a more fully-featured Path class?
> > 
> > WDYT about calling Path PathTraits or similar, since there's no actual data
> > storage in this class?
> 
> The naming scheme is taken from C++17 std::filesystem::path, although most
> std::filesystem features are not implemented yet.

Ah, I see, I was unaware of std::filesystem::path.

> I tried mozilla::Path first, but it caused name conflicts due to
> mozilla::gfx::Path :(
> Do you know a good way round?

Other than fixing all the bare Path references to use full qualification to avoid conflicts, no, not really. =/  Calling it FSPath conflicts with OS X (I think?) and MozFSPath is an awkward name.

> > I realize Path::value_type is carried over here from nsTString.  But I don't
> > know that it's gaining us anything having it here as a public API, or that
> > it's a reasonable name for a public API in this case.
> > 
> > WDYT about making value_type private to Path, and then requiring people to
> > use Path::string_type::value_type if they need to?  I realize that's more
> > typing, but I don't expect it to be used very much, and I think it's
> > slightly more obvious what's being referenced in that case.
> 
> Actually I took it from std::filesystem::path::value_type.

I see, maybe consistency with the standard library here is good.  A comment about Path's purpose and consistency with the standard library would be good.

> > Referencing nsTString and similar from MFBT is not really allowed; you're
> > depending on users including some string header prior to this, which is not
> > the typical way we structure headers in Gecko.  Why are we putting this in
> > MFBT rather than in XPCOM?
> 
> I would like to use Path::value_type from Moz2D, where XPCOM is not allowed
> either. Where is the best place for such things?

MFBT, I guess.  I suppose you can put nsTString and similar behind #ifdef MOZILLA_INTERNAL_API.

> (In reply to Nathan Froyd [:froydnj] from bug 685236 comment #69)
> > Comment on attachment 8939826 [details]
> > Bug 685236 - Add nsIFile::DisplayPath.
> > 
> > `DisplayPath` reads ambiguously to me: is the method displaying a path, or
> > is it returning a path for display?  Maybe PathForHumanConsumption? :)  (Not
> > a great name, better suggestions welcome!)
> > 
> > I am a little concerned that we are adding more API surface to nsIFile. 
> > Would it not be better to add a standalone function here that we implement
> > differently for Windows and Unix?
> 
> Unfortunately, nsLocalFile is not the only nsIFile implementation.
> DisplayPath must be an nsIFile method due to FileDescriptorFile :(

Ugh, ugh.  What if we defined GetNativePath something like:

[notxpcom,noscript] PlatformPath getNativePath();

with PlatformPath being nsString for Windows and nsCString everywhere else?  Would that be acceptable?

> Alternatively, we can (ab)use GetPersistentDescriptor to implement
> DisplayPath despite that the interface description says "DO NOT TRY TO
> INTERPRET IT AS HUMAN READABLE TEXT!". WDYT?

Let's not do this.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> MFBT, I guess.  I suppose you can put nsTString and similar behind #ifdef
> MOZILLA_INTERNAL_API.

I moved nsString aliases to nsIFile.

> > Unfortunately, nsLocalFile is not the only nsIFile implementation.
> > DisplayPath must be an nsIFile method due to FileDescriptorFile :(
> 
> Ugh, ugh.  What if we defined GetNativePath something like:
> 
> [notxpcom,noscript] PlatformPath getNativePath();
> 
> with PlatformPath being nsString for Windows and nsCString everywhere else? 
> Would that be acceptable?

It is what the current nativePath() is doing (although the names are a bit different). The purpose of DisplayPath() is to provide a char* representation on all platforms, because our printf-like functions do not support widechar parameters.

Or is this a request to rename nativePath() and PathString?

> > Alternatively, we can (ab)use GetPersistentDescriptor to implement
> > DisplayPath despite that the interface description says "DO NOT TRY TO
> > INTERPRET IT AS HUMAN READABLE TEXT!". WDYT?
> 
> Let's not do this.

OK, I'll update the patch accordingly.
I changed the implementation of HumanReadablePath().
Comment on attachment 8940686 [details]
Bug 1428543 - Add mozilla::filesystem::Path and use it in nsIFile.

https://reviewboard.mozilla.org/r/210942/#review221422

My apologies for taking so long to review this.  Thank you for the changes you made here.

::: xpcom/io/nsIFile.idl:14
(Diff revision 2)
> +using PathString = nsTString<filesystem::Path::value_type>;
> +using PathSubstring = nsTSubstring<filesystem::Path::value_type>;

I think you need an `#include "nsStringFwd.h"` here, so `nsIFile.h` isn't going to cargo-cult includes from other files.
Attachment #8940686 - Flags: review?(nfroyd) → review+
Comment on attachment 8940687 [details]
Bug 1428543 - Implement FileDescriptorFile::GetPersistentDescriptor.

https://reviewboard.mozilla.org/r/210944/#review221426
Attachment #8940687 - Flags: review?(nfroyd) → review+
Comment on attachment 8940688 [details]
Bug 1428543 - Add nsIFile::HumanReadablePath.

https://reviewboard.mozilla.org/r/210940/#review221430

::: xpcom/io/nsIFile.idl:265
(Diff revision 2)
> -    [notxpcom,nostdcall] PathString nativePath();
> +    [notxpcom,nostdcall,must_use] PathString nativePath();
>  %{C++
>      nsresult GetNativePath(nsACString& aPath);
> +    /*
> +     * Returns a human-readable path string.
> +     * The result is deliberately quoted to prevent abuse.

I understand what you are trying to do here, but I wonder whether users of this method will be surprised by the extra quoting.  Are you willing to try it without the extra quoting and see if people abuse it?
Attachment #8940688 - Flags: review?(nfroyd) → review+
Comment on attachment 8940686 [details]
Bug 1428543 - Add mozilla::filesystem::Path and use it in nsIFile.

https://reviewboard.mozilla.org/r/210942/#review221422

> I think you need an `#include "nsStringFwd.h"` here, so `nsIFile.h` isn't going to cargo-cult includes from other files.

Added the #include.
Comment on attachment 8940688 [details]
Bug 1428543 - Add nsIFile::HumanReadablePath.

https://reviewboard.mozilla.org/r/210940/#review221430

> I understand what you are trying to do here, but I wonder whether users of this method will be surprised by the extra quoting.  Are you willing to try it without the extra quoting and see if people abuse it?

OK. Hopefully people won't start breaking non-ASCII paths on Windows...
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/92b84f3e9049
Add mozilla::filesystem::Path and use it in nsIFile. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a7b0d7e12748
Implement FileDescriptorFile::GetPersistentDescriptor. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a610685bb09d
Add nsIFile::HumanReadablePath. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/92b84f3e9049
https://hg.mozilla.org/mozilla-central/rev/a7b0d7e12748
https://hg.mozilla.org/mozilla-central/rev/a610685bb09d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: