Add xdg mime support on Linux.

Add xdg mime support on Linux.

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

[Evan Stade, 2009-05-11 18:11:05]

http://codereview.chromium.org/113168/diff/141/142
File chrome/common/mime_icon.h (right):

http://codereview.chromium.org/113168/diff/141/142#newcode12
Line 12: /** Gets the mime type for a file. */
should be C++ style comments

Also, could you say how it is gotten (reading contents, reading filename, etc.)
and whether it touches the disk. And if you are making it so it can't handle
directories, note that too.

http://codereview.chromium.org/113168/diff/141/143
File chrome/common/mime_icon_linux.cc (right):

http://codereview.chromium.org/113168/diff/141/143#newcode455
Line 455: if (mime == kUnknownMimeType)
can you use else instead of this extra comparison?

http://codereview.chromium.org/113168/diff/141/143#newcode461
Line 461: /** Gets the file name for an icon given the mime type and icon size.
*/
//

http://codereview.chromium.org/113168/diff/141/143#newcode471
Line 471: const char *icon = xdg_mime_get_icon(mime_type.c_str());
* on left

http://codereview.chromium.org/113168/diff/141/143#newcode476
Line 476: // Try icon name similar than: text-plain
??

http://codereview.chromium.org/113168/diff/141/143#newcode479
Line 479: i = icon_name.find('/', i + 1))
need to use { } since for loop header is > 1 line. Also, the second line of the
loop header is indented one space too much, and there should be no space after
'for'.

http://codereview.chromium.org/113168/diff/141/143#newcode497
Line 497: if (!icon_file.empty())
this if statement is effectively ignored.

[Evan Stade, 2009-05-11 18:35:49]

I'd also put those mime_icon functions in a namespace (probably mime_icon::,
since it should match the filename, although GetFileMimeType seems more like a
mime_util::-esque function)

[Lei Zhang, 2009-05-11 18:39:12]

http://codereview.chromium.org/113168/diff/141/142
File chrome/common/mime_icon.h (right):

http://codereview.chromium.org/113168/diff/141/142#newcode12
Line 12: /** Gets the mime type for a file. */
On 2009/05/11 18:11:05, Evan Stade wrote:
> should be C++ style comments
> 
> Also, could you say how it is gotten (reading contents, reading filename,
etc.)
> and whether it touches the disk. And if you are making it so it can't handle
> directories, note that too.

ok.

http://codereview.chromium.org/113168/diff/141/143
File chrome/common/mime_icon_linux.cc (right):

http://codereview.chromium.org/113168/diff/141/143#newcode455
Line 455: if (mime == kUnknownMimeType)
On 2009/05/11 18:11:05, Evan Stade wrote:
> can you use else instead of this extra comparison?

I removed the GetFileInfo() altogether.

http://codereview.chromium.org/113168/diff/141/143#newcode461
Line 461: /** Gets the file name for an icon given the mime type and icon size.
*/
On 2009/05/11 18:11:05, Evan Stade wrote:
> //

Ok. I spent the last 3 years going the other way around.

http://codereview.chromium.org/113168/diff/141/143#newcode471
Line 471: const char *icon = xdg_mime_get_icon(mime_type.c_str());
On 2009/05/11 18:11:05, Evan Stade wrote:
> * on left

Done. (the rest of the file too)

http://codereview.chromium.org/113168/diff/141/143#newcode476
Line 476: // Try icon name similar than: text-plain
On 2009/05/11 18:11:05, Evan Stade wrote:
> ??

Original author wrote broken English? I updated the comment.

http://codereview.chromium.org/113168/diff/141/143#newcode479
Line 479: i = icon_name.find('/', i + 1))
On 2009/05/11 18:11:05, Evan Stade wrote:
> need to use { } since for loop header is > 1 line. Also, the second line of
the
> loop header is indented one space too much, and there should be no space after
> 'for'.

There should be a space after for, the indentation is correct.

http://codereview.chromium.org/113168/diff/141/143#newcode497
Line 497: if (!icon_file.empty())
On 2009/05/11 18:11:05, Evan Stade wrote:
> this if statement is effectively ignored.

How so? icon_file can be empty.

[Evan Stade, 2009-05-11 18:51:17]

http://codereview.chromium.org/113168/diff/141/143
File chrome/common/mime_icon_linux.cc (right):

http://codereview.chromium.org/113168/diff/141/143#newcode479
Line 479: i = icon_name.find('/', i + 1))
On 2009/05/11 18:39:12, Lei Zhang wrote:
> On 2009/05/11 18:11:05, Evan Stade wrote:
> > need to use { } since for loop header is > 1 line. Also, the second line of
> the
> > loop header is indented one space too much, and there should be no space
after
> > 'for'.
> 
> There should be a space after for, the indentation is correct.

you are so right

http://codereview.chromium.org/113168/diff/141/143#newcode497
Line 497: if (!icon_file.empty())
On 2009/05/11 18:39:12, Lei Zhang wrote:
> On 2009/05/11 18:11:05, Evan Stade wrote:
> > this if statement is effectively ignored.
> 
> How so? icon_file can be empty.

nevermind, I'm blind.

http://codereview.chromium.org/113168/diff/189/1075#newcode24
Line 24: 
punctuation (period at end)

http://codereview.chromium.org/113168/diff/189/1075#newcode61
Line 61: int min_size;
Comments go like this.

http://codereview.chromium.org/113168/diff/189/1075#newcode90
Line 90: ~IconTheme() {
delete[]

[Evan Stade, 2009-05-11 18:52:05]

lgtm

[Evan Martin, 2009-05-11 21:15:46]

http://codereview.chromium.org/113168/diff/265/288
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113168/diff/265/288#newcode447
Line 447: 'third_party/xdg_mime/xdgmime.c',
Should this be its own library?  It seems it shouldn't be built at all on
non-Linux.

http://codereview.chromium.org/113168/diff/265/266
File chrome/common/mime_util.h (right):

http://codereview.chromium.org/113168/diff/265/266#newcode15
Line 15: // have to exist. Please not because it doesn't touch the disk, this
does not
s/not/note/

http://codereview.chromium.org/113168/diff/265/266#newcode17
Line 17: std::string GetFileMimeType(FilePath file_path);
Since this is Linux-specific (and it doesn't work with a real path anyway)
perhaps this should just take a std::string?

http://codereview.chromium.org/113168/diff/265/266#newcode19
Line 19: // Gets the file name for an icon given the mime type and icon size.
"size" is unclear -- pixels, bytes, something else?

http://codereview.chromium.org/113168/diff/265/266#newcode20
Line 20: FilePath GetMimeIcon(const std::string mime_type, const unsigned int
size);
const std::string&
We don't use unsigned either.  Maybe size_t if it's really a size?

http://codereview.chromium.org/113168/diff/265/267
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/265/267#newcode9
Line 9: #include <cstdlib>
Should this be grouped with the C++ headers?

http://codereview.chromium.org/113168/diff/265/267#newcode22
Line 22: static const int kUpdateInterval = 5;
Comment on this?  What is it, what are its units?

http://codereview.chromium.org/113168/diff/265/267#newcode25
Line 25: static std::map<std::string, int>* icon_dirs = NULL;
"static" unnecessary in an anonymous namespace.  (above and below a few times as
well.)

http://codereview.chromium.org/113168/diff/265/267#newcode38
Line 38: static time_t last_check_time = 0;
This feels like a ton of static variables.  Maybe they belong in a class that we
then have one singleton of?

http://codereview.chromium.org/113168/diff/265/267#newcode41
Line 41: class IconTheme {
Comments above this and SubDirInfo as to what they are.

http://codereview.chromium.org/113168/diff/265/267#newcode57
Line 57: int size;
All member variables need comments.

http://codereview.chromium.org/113168/diff/265/267#newcode70
Line 70: std::map<std::string, int>::iterator iter;
weird tabbing.

http://codereview.chromium.org/113168/diff/265/267#newcode93
Line 93: static std::string ReadLine(FILE* fp) {
Comments above all member functions.  Should these be only declared here, then
defined outside the class declaration?

http://codereview.chromium.org/113168/diff/265/267#newcode99
Line 99: while ((ch = fgetc(fp)) != EOF) {
Why not use fgets?  Fewer syscalls.

http://codereview.chromium.org/113168/diff/265/282
File chrome/third_party/xdg_mime/README.chromium (right):

http://codereview.chromium.org/113168/diff/265/282#newcode6
Line 6: on 2009/05/11. This version contains some bugfixes to the original code.
Maybe include the SVN revision number?

http://codereview.chromium.org/113168/diff/265/269
File chrome/third_party/xdg_mime/compile.patch (right):

http://codereview.chromium.org/113168/diff/265/269#newcode7
Line 7: +#define HAVE_MMAP 1
Shouldn't this go into whichever config.h this file includes?

[Evan Stade, 2009-05-11 22:51:20]

http://codereview.chromium.org/113168/diff/265/266
File chrome/common/mime_util.h (right):

http://codereview.chromium.org/113168/diff/265/266#newcode19
Line 19: // Gets the file name for an icon given the mime type and icon size.
can you document what happens when there's no icon found, or not found in the
right size, or whatever?

[Lei Zhang, 2009-05-11 23:45:42]

I reorganized the mime util code, apologies for making it difficult to diff
between patch sets.

http://codereview.chromium.org/113168/diff/265/288
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113168/diff/265/288#newcode447
Line 447: 'third_party/xdg_mime/xdgmime.c',
On 2009/05/11 21:15:46, Evan Martin wrote:
> Should this be its own library?  It seems it shouldn't be built at all on
> non-Linux.

Done. I think I got the gyp syntax right.

http://codereview.chromium.org/113168/diff/265/266
File chrome/common/mime_util.h (right):

http://codereview.chromium.org/113168/diff/265/266#newcode15
Line 15: // have to exist. Please not because it doesn't touch the disk, this
does not
On 2009/05/11 21:15:46, Evan Martin wrote:
> s/not/note/

Fixed.

http://codereview.chromium.org/113168/diff/265/266#newcode17
Line 17: std::string GetFileMimeType(FilePath file_path);
On 2009/05/11 21:15:46, Evan Martin wrote:
> Since this is Linux-specific (and it doesn't work with a real path anyway)
> perhaps this should just take a std::string?

Sure, in an earlier version of the patch it used to do something with the
FilePath.

http://codereview.chromium.org/113168/diff/265/266#newcode19
Line 19: // Gets the file name for an icon given the mime type and icon size.
On 2009/05/11 21:15:46, Evan Martin wrote:
> "size" is unclear -- pixels, bytes, something else?

Done and done below too.

http://codereview.chromium.org/113168/diff/265/266#newcode20
Line 20: FilePath GetMimeIcon(const std::string mime_type, const unsigned int
size);
On 2009/05/11 21:15:46, Evan Martin wrote:
> const std::string&
> We don't use unsigned either.  Maybe size_t if it's really a size?

size_t it is.

http://codereview.chromium.org/113168/diff/265/267
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/265/267#newcode9
Line 9: #include <cstdlib>
On 2009/05/11 21:15:46, Evan Martin wrote:
> Should this be grouped with the C++ headers?

Yes, done.

http://codereview.chromium.org/113168/diff/265/267#newcode22
Line 22: static const int kUpdateInterval = 5;
On 2009/05/11 21:15:46, Evan Martin wrote:
> Comment on this?  What is it, what are its units?

Done.

http://codereview.chromium.org/113168/diff/265/267#newcode25
Line 25: static std::map<std::string, int>* icon_dirs = NULL;
On 2009/05/11 21:15:46, Evan Martin wrote:
> "static" unnecessary in an anonymous namespace.  (above and below a few times
as
> well.)

Removed except for LoadTheme().

http://codereview.chromium.org/113168/diff/265/267#newcode38
Line 38: static time_t last_check_time = 0;
On 2009/05/11 21:15:46, Evan Martin wrote:
> This feels like a ton of static variables.  Maybe they belong in a class that
we
> then have one singleton of?

I added a TODO.

http://codereview.chromium.org/113168/diff/265/267#newcode41
Line 41: class IconTheme {
On 2009/05/11 21:15:46, Evan Martin wrote:
> Comments above this and SubDirInfo as to what they are.

Ok.

http://codereview.chromium.org/113168/diff/265/267#newcode57
Line 57: int size;
On 2009/05/11 21:15:46, Evan Martin wrote:
> All member variables need comments.

Done.

http://codereview.chromium.org/113168/diff/265/267#newcode70
Line 70: std::map<std::string, int>::iterator iter;
On 2009/05/11 21:15:46, Evan Martin wrote:
> weird tabbing.

Fixed.

http://codereview.chromium.org/113168/diff/265/267#newcode93
Line 93: static std::string ReadLine(FILE* fp) {
On 2009/05/11 21:15:46, Evan Martin wrote:
> Comments above all member functions.  Should these be only declared here, then
> defined outside the class declaration?

A lot of these should be private, and not used outside anywhere. I made it so
and arranged the functions alphabetically.

http://codereview.chromium.org/113168/diff/265/267#newcode99
Line 99: while ((ch = fgetc(fp)) != EOF) {
On 2009/05/11 21:15:46, Evan Martin wrote:
> Why not use fgets?  Fewer syscalls.

Done. Can you read over this carefully and make sure I did it right?

http://codereview.chromium.org/113168/diff/265/267#newcode241
Line 241: std::string GetIconPath(const std::string &icon_name,
FYI, I FilePathed several functions.

http://codereview.chromium.org/113168/diff/265/282
File chrome/third_party/xdg_mime/README.chromium (right):

http://codereview.chromium.org/113168/diff/265/282#newcode6
Line 6: on 2009/05/11. This version contains some bugfixes to the original code.
On 2009/05/11 21:15:46, Evan Martin wrote:
> Maybe include the SVN revision number?

Done.

http://codereview.chromium.org/113168/diff/265/269
File chrome/third_party/xdg_mime/compile.patch (right):

http://codereview.chromium.org/113168/diff/265/269#newcode7
Line 7: +#define HAVE_MMAP 1
On 2009/05/11 21:15:46, Evan Martin wrote:
> Shouldn't this go into whichever config.h this file includes?

We'd have to create a config.h and define HAVE_CONFIG_H. This seemed simpler.

[Lei Zhang, 2009-05-11 23:50:26]

On 2009/05/11 22:51:20, Evan Stade wrote:
> http://codereview.chromium.org/113168/diff/265/266
> File chrome/common/mime_util.h (right):
> 
> http://codereview.chromium.org/113168/diff/265/266#newcode19
> Line 19: // Gets the file name for an icon given the mime type and icon size.
> can you document what happens when there's no icon found, or not found in the
> right size, or whatever?

This will be in patch set 9. I forgot I still got another round of commenting to
go through.

[Evan Martin, 2009-05-12 00:04:16]

High-level comments: this seems to a bunch of grunging around on the file
system, but it's not clear to me when that happens.  Can the caller control
this?  Does it happen on the proper thread?

Most classes we write are of the form:

class Foo {
  void bar();
};

void Foo::bar() {
}

We make a small exception if the class is small (like fits on a screen or two). 
I think yours is large.

http://codereview.chromium.org/113168/diff/298/299
File chrome/common/mime_util.h (right):

http://codereview.chromium.org/113168/diff/298/299#newcode19
Line 19: // Gets the file name for an icon given the mime type and icon pixel
size.
Still not sure what that last param is.  Width in pixels?  Width*height?

http://codereview.chromium.org/113168/diff/298/299#newcode20
Line 20: FilePath GetMimeIcon(const std::string& mime_type, const size_t size);
const unnecessary for size_t

http://codereview.chromium.org/113168/diff/298/300
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/298/300#newcode22
Line 22: // TODO(thestig) put all of these globals in a Singleton?
For example, they're all going to show up as leaks in Valgrind.  If you intend
to leave these as globals they should at least be g_foo_bar.

I really think you ought to fix this now rather than TODOing it because people
rarely get to TODOs.  If you *promise* your very next CL will clean this up, I
may be lenient.  :)

http://codereview.chromium.org/113168/diff/298/300#newcode69
Line 69: int threshold;  // Maximum difference from the desired sized. 2 by
default.
typo: "sized"

http://codereview.chromium.org/113168/diff/298/300#newcode82
Line 82: LOG(INFO) << "Trying find theme in " << theme_path.value();
Once you've got this code working, I suggest maybe removing the extraneous
logging calls, as they mask logging of other information...

http://codereview.chromium.org/113168/diff/298/300#newcode88
Line 88: if (!LoadIndexTheme(theme_index)) return;
"return" on next line

http://codereview.chromium.org/113168/diff/298/300#newcode105
Line 105: std::map<std::string, int>::iterator subdir_iter;
You should declare variables as they're used -- dir_iter doesn't seem to ever be
used in this function?

http://codereview.chromium.org/113168/diff/298/300#newcode115
Line 115: if (!icon_path.empty()) return icon_path;
"return" on its own line.  (Does the linter not complain about this?)

http://codereview.chromium.org/113168/diff/298/300#newcode125
Line 125: int d = abs(MatchesSize(info, size));
Avoid single-letter variables as much as possible.  I suggest "delta" for this
one and something like "min_delta_so_far" or whatever for other one.

http://codereview.chromium.org/113168/diff/298/300#newcode135
Line 135: if (!icon_path.empty() || !inherits || inherits_ == "") return
icon_path;
return on its own line

http://codereview.chromium.org/113168/diff/298/300#newcode151
Line 151: delete theme;
You can use a scoped_ptr to avoid this kind of logic.  This would just be
"theme.reset()" and the end of hte function is "theme.release()"

http://codereview.chromium.org/113168/diff/298/300#newcode160
Line 160: FilePath GetIconPathUnderSubdir(const std::string& icon_name,
comments on all member functions.  e.g. from the function name it appears this
is "return (icon_name + '/' + subdir)"?

http://codereview.chromium.org/113168/diff/298/300#newcode168
Line 168: // LOG(INFO) << "Searching " << icon_path.value();
You can delete commented-out code.

http://codereview.chromium.org/113168/diff/298/300#newcode251
Line 251: int MatchesSize(SubDirInfo* info, int size) {
defintely comments on all of these functions.  :)

http://codereview.chromium.org/113168/diff/298/300#newcode276
Line 276: char * ch;
this is no longer a char, but a pointer to the line or something.

http://codereview.chromium.org/113168/diff/298/300#newcode278
Line 278: const size_t BUFFER_SIZE = 100;
kBufferSize

http://codereview.chromium.org/113168/diff/298/300#newcode322
Line 322: SubDirInfo* current_info_;  // Pointer into |info_array_|.
This comment describes what it *is* (a pointer) but not what it's used for.

http://codereview.chromium.org/113168/diff/298/300#newcode326
Line 326: void TryAddIconDir(const FilePath& dir) {
comments!

http://codereview.chromium.org/113168/diff/298/300#newcode366
Line 366: default_themes[2] = IconTheme::LoadTheme("crystalsvg");   // KDE3
comment seems weird in the gnome branch (?)

http://codereview.chromium.org/113168/diff/298/300#newcode398
Line 398: if (!file_util::DirectoryExists(dir)) return;
return on its own line

http://codereview.chromium.org/113168/diff/298/300#newcode411
Line 411: local_data_dir = local_data_dir.AppendASCII(".local");
Is this part of the spec?  I'm a bit surprised.

http://codereview.chromium.org/113168/diff/298/300#newcode449
Line 449: if (now > last_check_time + kUpdateInterval) {
empty code?

[Evan Stade, 2009-05-12 00:08:02]

On 2009/05/12 00:04:16, Evan Martin wrote:
> Does it happen on the proper thread?

It might be a good idea to note in the header which calls access the disk, but
the sole user of this class (AFAIK) will be IconLoader.cc, which is executed on
the file thread. So, yes.

[Evan Stade, 2009-05-12 01:41:21]

porting code for style sucks huh?

http://codereview.chromium.org/113168/diff/298/300
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/298/300#newcode27
Line 27: // Store icon dirs and their mtime.
use English. (I know you didn't write it originally)

http://codereview.chromium.org/113168/diff/298/300#newcode45
Line 45: // Example themes on GNOME includes 'Human' and 'Mist'.
typo: include

http://codereview.chromium.org/113168/diff/298/300#newcode259
Line 259: ? (size - info->min_size) : (size - info->max_size);
aren't operators supposed to go on the end of the (previous) line? Also, { }
around the branches (since else is 2 lines). Same below.

http://codereview.chromium.org/113168/diff/298/300#newcode276
Line 276: char * ch;
you never read |ch|, can you just get rid of it?

http://codereview.chromium.org/113168/diff/298/300#newcode327
Line 327: if (!file_util::DirectoryExists(dir)) return;
return own line

[Lei Zhang, 2009-05-12 02:36:22]

http://codereview.chromium.org/113168/diff/298/299
File chrome/common/mime_util.h (right):

http://codereview.chromium.org/113168/diff/298/299#newcode19
Line 19: // Gets the file name for an icon given the mime type and icon pixel
size.
On 2009/05/12 00:04:16, Evan Martin wrote:
> Still not sure what that last param is.  Width in pixels?  Width*height?

I further explained what |size| is.

http://codereview.chromium.org/113168/diff/298/299#newcode20
Line 20: FilePath GetMimeIcon(const std::string& mime_type, const size_t size);
On 2009/05/12 00:04:16, Evan Martin wrote:
> const unnecessary for size_t

Ok.

http://codereview.chromium.org/113168/diff/298/300
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/298/300#newcode22
Line 22: // TODO(thestig) put all of these globals in a Singleton?
On 2009/05/12 00:04:16, Evan Martin wrote:
> For example, they're all going to show up as leaks in Valgrind.  If you intend
> to leave these as globals they should at least be g_foo_bar.
> 
> I really think you ought to fix this now rather than TODOing it because people
> rarely get to TODOs.  If you *promise* your very next CL will clean this up, I
> may be lenient.  :)

Ok. Done. Though I think the singleton code looks terrible.

http://codereview.chromium.org/113168/diff/298/300#newcode69
Line 69: int threshold;  // Maximum difference from the desired sized. 2 by
default.
On 2009/05/12 00:04:16, Evan Martin wrote:
> typo: "sized"

Fixed.

http://codereview.chromium.org/113168/diff/298/300#newcode82
Line 82: LOG(INFO) << "Trying find theme in " << theme_path.value();
On 2009/05/12 00:04:16, Evan Martin wrote:
> Once you've got this code working, I suggest maybe removing the extraneous
> logging calls, as they mask logging of other information...

The code is working. It would be nice to keep them around for debugging in the
future. For now I just deleted them. I wish there was a lower logging level than
INFO, or if we had logging channels.

http://codereview.chromium.org/113168/diff/298/300#newcode105
Line 105: std::map<std::string, int>::iterator subdir_iter;
On 2009/05/12 00:04:16, Evan Martin wrote:
> You should declare variables as they're used -- dir_iter doesn't seem to ever
be
> used in this function?

Removed.

http://codereview.chromium.org/113168/diff/298/300#newcode115
Line 115: if (!icon_path.empty()) return icon_path;
On 2009/05/12 00:04:16, Evan Martin wrote:
> "return" on its own line.  (Does the linter not complain about this?)

No it doesn't. All instances fixed.

http://codereview.chromium.org/113168/diff/298/300#newcode125
Line 125: int d = abs(MatchesSize(info, size));
On 2009/05/12 00:04:16, Evan Martin wrote:
> Avoid single-letter variables as much as possible.  I suggest "delta" for this
> one and something like "min_delta_so_far" or whatever for other one.

s/delta/min_delta_seen/
s/d/delta/

http://codereview.chromium.org/113168/diff/298/300#newcode151
Line 151: delete theme;
On 2009/05/12 00:04:16, Evan Martin wrote:
> You can use a scoped_ptr to avoid this kind of logic.  This would just be
> "theme.reset()" and the end of hte function is "theme.release()"

Done. Can you double check? I haven't used scoped_ptr much in the past.

http://codereview.chromium.org/113168/diff/298/300#newcode168
Line 168: // LOG(INFO) << "Searching " << icon_path.value();
On 2009/05/12 00:04:16, Evan Martin wrote:
> You can delete commented-out code.

Ok.

http://codereview.chromium.org/113168/diff/298/300#newcode276
Line 276: char * ch;
On 2009/05/12 00:04:16, Evan Martin wrote:
> this is no longer a char, but a pointer to the line or something.

Actually we don't need it anymore. Removed.

http://codereview.chromium.org/113168/diff/298/300#newcode278
Line 278: const size_t BUFFER_SIZE = 100;
On 2009/05/12 00:04:16, Evan Martin wrote:
> kBufferSize

Done.

http://codereview.chromium.org/113168/diff/298/300#newcode322
Line 322: SubDirInfo* current_info_;  // Pointer into |info_array_|.
On 2009/05/12 00:04:16, Evan Martin wrote:
> This comment describes what it *is* (a pointer) but not what it's used for.

Come to think of it, it's only used in one function, and we don't need to save
it between calls. I renamed it current_info. It is pretty much just a pointer.

http://codereview.chromium.org/113168/diff/298/300#newcode326
Line 326: void TryAddIconDir(const FilePath& dir) {
On 2009/05/12 00:04:16, Evan Martin wrote:
> comments!

Next change set...

http://codereview.chromium.org/113168/diff/298/300#newcode366
Line 366: default_themes[2] = IconTheme::LoadTheme("crystalsvg");   // KDE3
On 2009/05/12 00:04:16, Evan Martin wrote:
> comment seems weird in the gnome branch (?)

What do you mean?

http://codereview.chromium.org/113168/diff/298/300#newcode411
Line 411: local_data_dir = local_data_dir.AppendASCII(".local");
On 2009/05/12 00:04:16, Evan Martin wrote:
> Is this part of the spec?  I'm a bit surprised.

Yes, this is the fallback. Per the spec: "$XDG_DATA_HOME defines the base
directory relative to which user specific data files should be stored. If
$XDG_DATA_HOME is either not set or empty, a default equal to $HOME/.local/share
should be used."

http://codereview.chromium.org/113168/diff/298/300#newcode449
Line 449: if (now > last_check_time + kUpdateInterval) {
On 2009/05/12 00:04:16, Evan Martin wrote:
> empty code?

There should be something there per the theme spec. Want me to comment out this
entire block for now?

[Evan Martin, 2009-05-12 23:49:53]

I'm kinda astounded at how much code we need to get icons right.  Nice work. 
LGTM (self-evident nits below)

http://codereview.chromium.org/113168/diff/1194/420
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/1194/420#newcode327
Line 327: char buffer[100];
[kBufferSize] ?

http://codereview.chromium.org/113168/diff/1194/420#newcode330
Line 330: char end = result[result.length() - 1];
Will this do the wrong thing if you read an empty line?  (i.e. result.length()
== 0)

http://codereview.chromium.org/113168/diff/1194/420#newcode421
Line 421: // Per xdg theme spec, we should check the icon directories ever so
often for
typo: "ever"

http://codereview.chromium.org/113168/diff/1194/420#newcode427
Line 427: time_t last_check_time =
Singleton<MimeUtilConstants>::get()->last_check_time;
You could do

MimeUtilConstants* constants = Singleton<MimeUtilConstants>::get();
to save typing below.

http://codereview.chromium.org/113168/diff/1194/420#newcode440
Line 440: // TODO(thestig): something changed. start over. Upstream fix to
Google
not quite sure what "upstream" means here

[Lei Zhang, 2009-05-13 06:28:03]

Ya, turns out implementing the spec takes a bit more work than the pseudo code
given. We still need to work on this a bit. Avi pointed out to me/estade that
this code should go in base/platform_mime_util_linux.cc, so we'll work on it a
bit more tomorrow.

http://codereview.chromium.org/113168/diff/1194/420
File chrome/common/mime_util_linux.cc (right):

http://codereview.chromium.org/113168/diff/1194/420#newcode327
Line 327: char buffer[100];
On 2009/05/12 23:49:53, Evan Martin wrote:
> [kBufferSize] ?

Right.

http://codereview.chromium.org/113168/diff/1194/420#newcode330
Line 330: char end = result[result.length() - 1];
On 2009/05/12 23:49:53, Evan Martin wrote:
> Will this do the wrong thing if you read an empty line?  (i.e. result.length()
> == 0)

Ok, I checked for length == 0 first.

http://codereview.chromium.org/113168/diff/1194/420#newcode421
Line 421: // Per xdg theme spec, we should check the icon directories ever so
often for
On 2009/05/12 23:49:53, Evan Martin wrote:
> typo: "ever"

Fixed.

http://codereview.chromium.org/113168/diff/1194/420#newcode427
Line 427: time_t last_check_time =
Singleton<MimeUtilConstants>::get()->last_check_time;
On 2009/05/12 23:49:53, Evan Martin wrote:
> You could do
> 
> MimeUtilConstants* constants = Singleton<MimeUtilConstants>::get();
> to save typing below.

Done. Thx.

http://codereview.chromium.org/113168/diff/1194/420#newcode440
Line 440: // TODO(thestig): something changed. start over. Upstream fix to
Google
On 2009/05/12 23:49:53, Evan Martin wrote:
> not quite sure what "upstream" means here

Give back to the project this code came from.

[Evan Stade, 2009-05-14 21:43:35]

ping

[Lei Zhang, 2009-05-14 23:23:07]

On 2009/05/14 21:43:35, Evan Stade wrote:
> ping

pong. patch set 13 moved everything into base, where we can't create a separate
target in the gyp file because we'll get a base -> xdg_mime -> base circular
dependency.

I'm using the code in net in
PlatformMimeUtil::GetPlatformMimeTypeFromExtension(). I also added a
NOTIMPLEMENTED() for PlatformMimeUtil::GetPreferredExtensionForMimeType() in
case anyone calls it.

[Evan Stade, 2009-05-14 23:28:51]

On 2009/05/14 23:23:07, Lei Zhang wrote:
> On 2009/05/14 21:43:35, Evan Stade wrote:
> > ping
> 
> pong. patch set 13 moved everything into base, where we can't create a
separate
> target in the gyp file because we'll get a base -> xdg_mime -> base circular
> dependency.

cripes

> I'm using the code in net in
> PlatformMimeUtil::GetPlatformMimeTypeFromExtension(). I also added a
> NOTIMPLEMENTED() for PlatformMimeUtil::GetPreferredExtensionForMimeType() in
> case anyone calls it.

heh, one more NOTIMPLEMENTED() for Evan M. to squash later on...

[Lei Zhang, 2009-05-14 23:38:32]

On 2009/05/14 23:28:51, Evan Stade wrote:
> On 2009/05/14 23:23:07, Lei Zhang wrote:
> > pong. patch set 13 moved everything into base, where we can't create a
> separate
> > target in the gyp file because we'll get a base -> xdg_mime -> base circular
> > dependency.
> 
> cripes

Except that didn't work. I added a rule to exclude it from os != linux in patch
set 14. Let's see what the try server says.

[Lei Zhang, 2009-05-15 21:30:27]

On 2009/05/14 23:38:32, Lei Zhang wrote:
> On 2009/05/14 23:28:51, Evan Stade wrote:
> > On 2009/05/14 23:23:07, Lei Zhang wrote:
> > > pong. patch set 13 moved everything into base, where we can't create a
> > separate
> > > target in the gyp file because we'll get a base -> xdg_mime -> base
circular
> > > dependency.
> > 
> > cripes
> 
> Except that didn't work. I added a rule to exclude it from os != linux in
patch
> set 14. Let's see what the try server says.

try server was happy...
mr. evmar, one more glance just to make sure we're happy with having it in base
+ base/third_party ?

[Evan Martin, 2009-05-15 22:55:49]

LGTM rubber stamp