Clean up ifdef around FilePath creation(webkit)

Clean up ifdef around FilePath creation(webkit)

Splitting from the original patch here, crrev.com/14942008
to only include 'webkit' specific changes

BUG=102853
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203002

[kinuko, 2013-05-16 05:20:25]

Looks plausible, I'd like defer this to NPAPI owners.

[Bernhard Bauer, 2013-05-16 06:48:05]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
I think MimeUtil::GetMimeTypeFromExtensionHelper() should also be updated,
otherwise we have a SysWideToNativeMB conversion here on Windows that we don't
undo.

[mrunal, 2013-05-17 00:16:00]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
I don't think it will call SysWideToNativeMB for Windows since there is a
different version of FromUTF8Unsafe() (file_path.cc:line 622) for Windows which
would get called here.

[Bernhard Bauer, 2013-05-17 07:04:44]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
On 2013/05/17 00:16:00, mrunal wrote:
> I don't think it will call SysWideToNativeMB for Windows since there is a
> different version of FromUTF8Unsafe() (file_path.cc:line 622) for Windows
which
> would get called here.

Oh, right! But what about Linux then?

Basically, MimeUtil uses the same #ifdef that we have here, just with a
FilePath::StringType instead of a FilePath (and in reverse), so if this is the
right thing to do here and we want to get the same string back, we should also
change that.

[mrunal, 2013-05-20 23:48:06]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
> Basically, MimeUtil uses the same #ifdef that we have here, just with a
> FilePath::StringType instead of a FilePath (and in reverse), so if this is the
> right thing to do here and we want to get the same string back, we should also
> change that.
Specifically what do you want me to change? Are you suggesting I change
MimeUtil::GetMimeTypeFromExtensionHelper() by substituting
WideToUTF8(SysNativeMBToWide(ext)) for OS_POSIX?
I'm not sure if that's the right thing to do as not all the functions calling
GetMimeTypeFromFile() are/will be calling FromUTF8Unsafe(). As per the bug
description it sounded this change won't be any worse than the current #ifdef

[Bernhard Bauer, 2013-05-21 17:31:01]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
On 2013/05/20 23:48:06, mrunal wrote:
> > Basically, MimeUtil uses the same #ifdef that we have here, just with a
> > FilePath::StringType instead of a FilePath (and in reverse), so if this is
the
> > right thing to do here and we want to get the same string back, we should
also
> > change that.
> Specifically what do you want me to change? Are you suggesting I change
> MimeUtil::GetMimeTypeFromExtensionHelper() by substituting
> WideToUTF8(SysNativeMBToWide(ext)) for OS_POSIX?

No, only for defined(OS_POSIX) && !(defined(OS_MACOSX) || defined(OS_CHROMEOS)),
just like FilePath::AsUTF8Unsafe does. In fact, it might be the right thing to
just use FilePath::AsUTF8Unsafe instead (as you can see, this is very subtle to
get right).

> I'm not sure if that's the right thing to do as not all the functions calling
> GetMimeTypeFromFile() are/will be calling FromUTF8Unsafe().

Well, all the functions that construct a FilePath from a UTF8 string are (or at
least they should be, and that's what you're changing here).

> As per the bug
> description it sounded this change won't be any worse than the current #ifdef

Yes, that's why I'm asking you to make the change in more places. Basically,
we're making the assumption that on Linux, file paths are in the native
encoding. That assumption isn't always correct, but if we make it, we should
make it everywhere in the code instead of just in some places.

In particular, if we use the same reversible mapping from FilePath to UTF8
string everywhere, in places like here it doesn't make a difference whether our
assumption that all file paths are native encoding is correct, because we'll get
back the same string in any case!

[mrunal, 2013-05-22 01:55:39]

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
File webkit/plugins/npapi/plugin_stream.cc (right):

https://codereview.chromium.org/14680012/diff/1/webkit/plugins/npapi/plugin_s...
webkit/plugins/npapi/plugin_stream.cc:87: if (net::GetMimeTypeFromFile(path,
&temp_mime_type))
Hi Bernhard,
Thanks for the clarification. I will update the
MimeUtil::GetMimeTypeFromExtensionHelper() with your suggested changes.

[mrunal, 2013-05-28 18:17:11]

PTAL

[Bernhard Bauer, 2013-05-28 21:26:14]

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc#new...
net/base/mime_util.cc:228: const string& ext_narrow_str =
WideToUTF8(base::SysNativeMBToWide(ext));
Why don't you just use FilePath::AsUTF8Unsafe? There is really no need to have
this piece of logic in multiple places, particularly if it's as subtle as this.

[mrunal, 2013-05-28 21:38:47]

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc#new...
net/base/mime_util.cc:228: const string& ext_narrow_str =
WideToUTF8(base::SysNativeMBToWide(ext));
On 2013/05/28 21:26:14, Bernhard Bauer wrote:
> Why don't you just use FilePath::AsUTF8Unsafe? 
Because FilePath::AsUTF8Unsafe() is not a static function like FromUTF8Unsafe().
It's a member function. We are dealing with base::FilePath::StringType here.

[Bernhard Bauer, 2013-05-28 22:04:00]

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc#new...
net/base/mime_util.cc:228: const string& ext_narrow_str =
WideToUTF8(base::SysNativeMBToWide(ext));
On 2013/05/28 21:38:47, mrunal wrote:
> On 2013/05/28 21:26:14, Bernhard Bauer wrote:
> > Why don't you just use FilePath::AsUTF8Unsafe? 
> Because FilePath::AsUTF8Unsafe() is not a static function like
FromUTF8Unsafe().
> It's a member function. We are dealing with base::FilePath::StringType here.

Right, just construct a new FilePath (which copies the string), then call
AsUTF8Unsafe(). Having to go through a FilePath object here is somewhat
unfortunate, but then so is having to call an ...Unsafe() method in the first
place.

[mrunal, 2013-05-28 22:41:02]

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/14680012/diff/11001/net/base/mime_util.cc#new...
net/base/mime_util.cc:228: const string& ext_narrow_str =
WideToUTF8(base::SysNativeMBToWide(ext));
> Right, just construct a new FilePath (which copies the string), then call
> AsUTF8Unsafe(). Having to go through a FilePath object here is somewhat
> unfortunate, but then so is having to call an ...Unsafe() method in the first
> place.
Yeah that will be bit ugly but I get your point now. I will upload another
patch.

[mrunal, 2013-05-28 22:52:14]

PTAL

[jam, 2013-05-28 23:16:20]

bauerb is in webkit/plugins/npapi/OWNERS, removing myself

[Bernhard Bauer, 2013-05-28 23:26:41]

https://codereview.chromium.org/14680012/diff/20001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/14680012/diff/20001/net/base/mime_util.cc#new...
net/base/mime_util.cc:223: const string& ext_narrow_str =
path_ext.AsUTF8Unsafe();
You should not take a reference here! The returned value might be a temporary,
in which case it will be destroyed right after this line, so the reference will
become invalid.

[mrunal, 2013-05-28 23:44:50]

> You should not take a reference here! The returned value might be a temporary,
> in which case it will be destroyed right after this line, so the reference
will
> become invalid.

Good catch! I have uploaded new patch.

[Bernhard Bauer, 2013-05-29 07:28:58]

LGTM!

[cbentzel, 2013-05-29 11:08:06]

LGTM for net

[commit-bot: I haz the power, 2013-05-29 19:00:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/14680012/26001

[commit-bot: I haz the power, 2013-05-29 23:16:51]

Change committed as 203002