Share code between Mac and Linux in ResourceBundle.

Share code between Mac and Linux in ResourceBundle.

BUG=25891
TEST=None

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

[TVL, 2009-11-25 14:09:10]

lgtm

+tony for the linux side of things.

http://codereview.chromium.org/442002/diff/1/3
File app/resource_bundle_linux.cc (right):

http://codereview.chromium.org/442002/diff/1/3#newcode62
app/resource_bundle_linux.cc:62: for (GdkPixbufMap::iterator i =
gdk_pixbufs_.begin();
too bad we can't share the dtor since they are so close.

[Mark Mentovai, 2009-11-25 15:11:29]

I would like to see LoadResources and the destructor shared.

Because the two are not entirely identical, you'll need to factor out the bits
that get the paths to the two .paks into platform-specific functions.

In order to get the destructor to work, one approach is to make the _posix
version define ResourceBundlePOSIX, and then have ResourceBundle extend that
class with the correct implementations on Mac and Linux.  There are other
similar ways that you can achieve this too.

http://codereview.chromium.org/442002/diff/1/5
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/1/5#newcode1
app/resource_bundle_posix.cc:1: // Copyright (c) 2006-2009 The Chromium Authors.
All rights reserved.
Just 2009

http://codereview.chromium.org/442002/diff/1/5#newcode2
app/resource_bundle_posix.cc:2: // Use of this source code is governed by a
BSD-style license that can be
svn propset svn:eol-style LF app/resource_bundle_posix.cc

[hayato, 2009-11-27 05:53:47]

Thank you for the review.

I shared dtor and LoadResources as you suggested.

As for dtor, I chose a lazy, but simple way, using '#ifdef' directive.

As for LoadResoruces, I made two static  functions to get the paths to the two
.paks and used them from LoadResources.

Though I verified the change didn't break build and the test passed, is there
any way to verity this change won't break anything?

http://codereview.chromium.org/442002/diff/1/3
File app/resource_bundle_linux.cc (right):

http://codereview.chromium.org/442002/diff/1/3#newcode62
app/resource_bundle_linux.cc:62: for (GdkPixbufMap::iterator i =
gdk_pixbufs_.begin();
agree. I shared the dtor.

On 2009/11/25 14:09:10, TVL wrote:
> too bad we can't share the dtor since they are so close.

http://codereview.chromium.org/442002/diff/1/5
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/1/5#newcode1
app/resource_bundle_posix.cc:1: // Copyright (c) 2006-2009 The Chromium Authors.
All rights reserved.
On 2009/11/25 15:11:29, Mark Mentovai wrote:
> Just 2009

Done.

http://codereview.chromium.org/442002/diff/1/5#newcode2
app/resource_bundle_posix.cc:2: // Use of this source code is governed by a
BSD-style license that can be
On 2009/11/25 15:11:29, Mark Mentovai wrote:
> svn propset svn:eol-style LF app/resource_bundle_posix.cc

Oh. Sorry. I am using git and I used 'git cl upload' to upload this patch to
codereview. But it doesn't seem that 'git cl upload' command includes necessary
'svn:eol-style LF' in the sending patch even if [auto-props] is set to in
~/.subversion/config.

Currently, there is no way to set svn's property when uploading the patch for
codereview if we use git. Hmm.
I've filed the bug as http://code.google.com/p/chromium/issues/detail?id=28844

I guess that when I commit the patch, there should be no problem because 'git cl
dcommit' uses 'git svn dcommit', which takes care of  'eol-style LF' correctly.

From the next time, I'll use 'svn' instead of 'git' if a change contains new
file until the issue is solved.
Sorry for the inconvenience.

[Mark Mentovai, 2009-11-27 17:07:22]

I think we'll be all set if you make these changes.  This is some nice cleanup.

If you've let the try servers chew on this change and all of the tests came back
as green on all three platforms, that's a pretty good test.  You might also want
to do local Mac and Linux builds if you're able to make sure that the
application still functions properly, but I think that for a change like this
one, if anything was out of place, the try servers would fail lots of tests.

No sweat on the LF thing, if git does the right thing.  I'm accustomed to seeing
svn:eol-style set on added files in Rietveld, and it's an easy-ish thing to miss
so I'll mention it when I notice it's missing.  If the git workflow already
handles this when adding files to svn, then it's fine.  I have no way of knowing
who's using git and who's using svn.

http://codereview.chromium.org/442002/diff/6001/6004
File app/resource_bundle_linux.cc (right):

http://codereview.chromium.org/442002/diff/6001/6004#newcode70
app/resource_bundle_linux.cc:70: PathService::Get(base::DIR_EXE,
&resources_file_path);
After getting DIR_EXE, you probably want to test:

  if (resources_file_path.empty())
    return resources_file_path;

similar to what's done in ResourceBundle::GetLocaleFilePath.

http://codereview.chromium.org/442002/diff/6001/6006
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/6001/6006#newcode57
app/resource_bundle_posix.cc:57: // If for some reason we were unable to load a
resource dll, return an empty
This comment came from the Windows implementation, and it's been wrong for Mac
and Linux ever since.  There is no resource dll, there's a pak file.  Now is an
excellent time to fix this comment.

http://codereview.chromium.org/442002/diff/6001/6006#newcode76
app/resource_bundle_posix.cc:76: string16 msg(reinterpret_cast<const
char16*>(data.data()),
Before this line, do:

  DCHECK_EQ(data.length() % 2, 0);

to ensure that the data might plausibly be UTF-16-encoded.

http://codereview.chromium.org/442002/diff/6001/6006#newcode82
app/resource_bundle_posix.cc:82: #if defined(MAC_OSX)
This is the wrong #ifdef.  You want OS_MACOSX.  See build/build_config.h.

http://codereview.chromium.org/442002/diff/6001/6006#newcode83
app/resource_bundle_posix.cc:83: DLOG_IF(WARNING, pref_locale.size() != 0)
For tests like this, I think:

  DLOG_IF(WARNING, !pref_locale.empty())

is clearer than testing .size() != 0.

http://codereview.chromium.org/442002/diff/6001/6006#newcode87
app/resource_bundle_posix.cc:87: DCHECK(resources_data_ == NULL) << "resource
data already loaded!";
Rather than DCHECK(this == that), use DCHECK_EQ(this, that).  But in this case,
DCHECK(!resources_data_) is all that you need.  Same on line 93.

http://codereview.chromium.org/442002/diff/6001/6006#newcode89
app/resource_bundle_posix.cc:89: DCHECK(!resources_file_path.empty()) << "chrome
pak file not found";
For the DCHECK messages in this function, we have:

"resource data"
"chrome pak" (twice)
"locale data"
"locale pak"

Inconsistent terminology.  I'd rewrite these to be...

"chrome.pak already loaded"
"chrome.pak not found"
"failed to load chrome.pak"
"locale.pak already loaded"
"failed to load locale.pak"

without the exclamation points.  Let's save a couple of bytes in our string
data.

http://codereview.chromium.org/442002/diff/6001/6006#newcode96
app/resource_bundle_posix.cc:96: // It's possible that there are no locale dlls
found, in which case we just
This comment refers to DLLs, but there are no DLLs.

[TVL, 2009-11-28 01:22:20]

http://codereview.chromium.org/442002/diff/6001/6003
File app/resource_bundle.h (right):

http://codereview.chromium.org/442002/diff/6001/6003#newcode166
app/resource_bundle.h:166: #if defined(USE_X11)
shouldn't this is defined(OS_LINUX)

http://codereview.chromium.org/442002/diff/6001/6006
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/6001/6006#newcode17
app/resource_bundle_posix.cc:17: bool success=
resources_pak->Load(resources_pak_path);
space before equals

http://codereview.chromium.org/442002/diff/6001/6006#newcode29
app/resource_bundle_posix.cc:29: #if defined(USE_X11)
defined(OS_LINUX)

[hayato, 2009-11-30 10:46:07]

On 2009/11/27 17:07:22, Mark Mentovai wrote:
> I think we'll be all set if you make these changes.  This is some nice
cleanup.
> 
> If you've let the try servers chew on this change and all of the tests came
back
> as green on all three platforms, that's a pretty good test.  You might also
want
> to do local Mac and Linux builds if you're able to make sure that the
> application still functions properly, but I think that for a change like this
> one, if anything was out of place, the try servers would fail lots of tests.

Thank you. I appreciate it. I used try servers to verity all tests were green on
three platforms. I also did build on local Mac and Linux and verified that the
application still functions correctly and locale.pak was successfully found on
both platforms.


> No sweat on the LF thing, if git does the right thing.  I'm accustomed to
seeing
> svn:eol-style set on added files in Rietveld, and it's an easy-ish thing to
miss
> so I'll mention it when I notice it's missing.  If the git workflow already
> handles this when adding files to svn, then it's fine.  I have no way of
knowing
> who's using git and who's using svn.


Thank you again for letting me know it.
I couldn't find a workaround yet. I asked my neighbor who is using git and he
said he used 'gcl' instead of 'git cl' if a change contains a new file because
of the issue.  I'll follow his advise from next time until the issue is solved.
When submitting using 'git cl dcommit', there should be set 'svn:eol-style'
according to the document. So I'll try it after LGTM and check 'svn:eol-style'
will be set correctly.

http://codereview.chromium.org/442002/diff/6001/6003
File app/resource_bundle.h (right):

http://codereview.chromium.org/442002/diff/6001/6003#newcode166
app/resource_bundle.h:166: #if defined(USE_X11)
On 2009/11/28 01:22:20, TVL wrote:
> shouldn't this is defined(OS_LINUX)
> 

Done.

http://codereview.chromium.org/442002/diff/6001/6004
File app/resource_bundle_linux.cc (right):

http://codereview.chromium.org/442002/diff/6001/6004#newcode70
app/resource_bundle_linux.cc:70: PathService::Get(base::DIR_EXE,
&resources_file_path);
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> After getting DIR_EXE, you probably want to test:
> 
>   if (resources_file_path.empty())
>     return resources_file_path;
> 
> similar to what's done in ResourceBundle::GetLocaleFilePath.

Done.

http://codereview.chromium.org/442002/diff/6001/6006
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/6001/6006#newcode17
app/resource_bundle_posix.cc:17: bool success=
resources_pak->Load(resources_pak_path);
On 2009/11/28 01:22:20, TVL wrote:
> space before equals

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode29
app/resource_bundle_posix.cc:29: #if defined(USE_X11)
On 2009/11/28 01:22:20, TVL wrote:
> defined(OS_LINUX)

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode57
app/resource_bundle_posix.cc:57: // If for some reason we were unable to load a
resource dll, return an empty
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> This comment came from the Windows implementation, and it's been wrong for Mac
> and Linux ever since.  There is no resource dll, there's a pak file.  Now is
an
> excellent time to fix this comment.

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode76
app/resource_bundle_posix.cc:76: string16 msg(reinterpret_cast<const
char16*>(data.data()),
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> Before this line, do:
> 
>   DCHECK_EQ(data.length() % 2, 0);
> 
> to ensure that the data might plausibly be UTF-16-encoded.

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode82
app/resource_bundle_posix.cc:82: #if defined(MAC_OSX)
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> This is the wrong #ifdef.  You want OS_MACOSX.  See build/build_config.h.

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode83
app/resource_bundle_posix.cc:83: DLOG_IF(WARNING, pref_locale.size() != 0)
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> For tests like this, I think:
> 
>   DLOG_IF(WARNING, !pref_locale.empty())
> 
> is clearer than testing .size() != 0.

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode87
app/resource_bundle_posix.cc:87: DCHECK(resources_data_ == NULL) << "resource
data already loaded!";
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> Rather than DCHECK(this == that), use DCHECK_EQ(this, that).  But in this
case,
> DCHECK(!resources_data_) is all that you need.  Same on line 93.

Done.

http://codereview.chromium.org/442002/diff/6001/6006#newcode89
app/resource_bundle_posix.cc:89: DCHECK(!resources_file_path.empty()) << "chrome
pak file not found";
Done. Thank you!

On 2009/11/27 17:07:22, Mark Mentovai wrote:
> For the DCHECK messages in this function, we have:
> 
> "resource data"
> "chrome pak" (twice)
> "locale data"
> "locale pak"
> 
> Inconsistent terminology.  I'd rewrite these to be...
> 
> "chrome.pak already loaded"
> "chrome.pak not found"
> "failed to load chrome.pak"
> "locale.pak already loaded"
> "failed to load locale.pak"
> 
> without the exclamation points.  Let's save a couple of bytes in our string
> data.

http://codereview.chromium.org/442002/diff/6001/6006#newcode96
app/resource_bundle_posix.cc:96: // It's possible that there are no locale dlls
found, in which case we just
On 2009/11/27 17:07:22, Mark Mentovai wrote:
> This comment refers to DLLs, but there are no DLLs.

Done.

[Mark Mentovai, 2009-11-30 14:08:34]

LGTM

http://codereview.chromium.org/442002/diff/13001/13006
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/13001/13006#newcode83
app/resource_bundle_posix.cc:83: #if defined(OS_MACOSX)
This, without the #ifdef, should be in the Mac version of
ResourceBundle::GetLocaleFilePath, replacing the awkward comment there.

http://codereview.chromium.org/442002/diff/13001/13006#newcode97
app/resource_bundle_posix.cc:97: // It's possible that there are no locale.pak
found, in which case we just
Simplify and revise this comment:

    // It's possible that there is no locale.pak.

but this comment is a little confusing to me.  The Mac version of
GetLocaleFilePath checks for locale.pak and will return an empty path if there
is no such file.  But the Linux implementation doesn't do that: it'll return a
path that might not refer to a real file.

That discrepancy makes me think that maybe we just ought to leave this comment
out?

[hayato, 2009-12-01 06:03:41]

Thank you fro the review.

http://codereview.chromium.org/442002/diff/13001/13006
File app/resource_bundle_posix.cc (right):

http://codereview.chromium.org/442002/diff/13001/13006#newcode83
app/resource_bundle_posix.cc:83: #if defined(OS_MACOSX)
On 2009/11/30 14:08:34, Mark Mentovai wrote:
> This, without the #ifdef, should be in the Mac version of
> ResourceBundle::GetLocaleFilePath, replacing the awkward comment there.

Done. Thank you!

http://codereview.chromium.org/442002/diff/13001/13006#newcode97
app/resource_bundle_posix.cc:97: // It's possible that there are no locale.pak
found, in which case we just
Okay. It might be better that we get rid of the discrepancy between Mac and
LInux on ResourceBundle::GetLocaleFilePath.

I made GetLocaleFilePath in Linux version check explicitly that the filepath
really exists. 

On 2009/11/30 14:08:34, Mark Mentovai wrote:
> Simplify and revise this comment:
> 
>     // It's possible that there is no locale.pak.
> 
> but this comment is a little confusing to me.  The Mac version of
> GetLocaleFilePath checks for locale.pak and will return an empty path if there
> is no such file.  But the Linux implementation doesn't do that: it'll return a
> path that might not refer to a real file.
> 
> That discrepancy makes me think that maybe we just ought to leave this comment
> out?

[hayato, 2009-12-02 02:39:04]

I'll land this change soon because this is LGTMed.
If you have any concerns, please let me know.

Thank you for the review again.