Add initial xdg-settings patch to xdg-utils.

Add initial xdg-settings patch to xdg-utils.


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

[Michael Moss, 2009-06-30 22:44:02]

http://codereview.chromium.org/151109/diff/1/7
File third_party/xdg-utils/README.google (right):

http://codereview.chromium.org/151109/diff/1/7#newcode1
Line 1: This directory contains a copy of xdg-utils obtained from CVS on
2009-06-29,
Can you note the specific revision?

http://codereview.chromium.org/151109/diff/1/3
File third_party/xdg-utils/scripts/xdg-settings (right):

http://codereview.chromium.org/151109/diff/1/3#newcode362
Line 362: *text/html*)
Technically, this could match stuff other than text/html. I'm not too worried
about it in practice, but if you want to make it a bit more robust, you should
split $MIME on semi-colons, then test each MimeType individually for text/html.

http://codereview.chromium.org/151109/diff/1/3#newcode367
Line 367: ;;
Did you intend to do something here?

http://codereview.chromium.org/151109/diff/1/3#newcode370
Line 370: if (grep -v ^MimeType= "$apps/$1"; echo "MimeType=text/html;$MIME") >
"$apps/$1.new"; then
This test is always true as long as the echo succeeds and the "new" file is
writable. Is that what you intended? Maybe you want "&&" after the grep.

Also, you should probably use mktemp(1) to create files, rather than just adding
".new" to existing filenames.

http://codereview.chromium.org/151109/diff/1/3#newcode579
Line 579: done
I thinks this can just be 'readlink -f $browser'.

http://codereview.chromium.org/151109/diff/1/3#newcode608
Line 608: generic)
Is this ever hit?

[Mike Mammarella [gmail], 2009-06-30 23:42:09]

http://codereview.chromium.org/151109/diff/1/7
File third_party/xdg-utils/README.google (right):

http://codereview.chromium.org/151109/diff/1/7#newcode1
Line 1: This directory contains a copy of xdg-utils obtained from CVS on
2009-06-29,
On 2009/06/30 22:44:02, mmoss wrote:
> Can you note the specific revision?

Unfortunately, CVS doesn't have a global revision number. Each file has its own,
which is why I list only the date.

http://codereview.chromium.org/151109/diff/1/3
File third_party/xdg-utils/scripts/xdg-settings (right):

http://codereview.chromium.org/151109/diff/1/3#newcode362
Line 362: *text/html*)
On 2009/06/30 22:44:02, mmoss wrote:
> Technically, this could match stuff other than text/html. I'm not too worried
> about it in practice, but if you want to make it a bit more robust, you should
> split $MIME on semi-colons, then test each MimeType individually for
text/html.

Changed to:
text/html;*|*;text/html;*|*;text/html;|*;text/html)

http://codereview.chromium.org/151109/diff/1/3#newcode367
Line 367: ;;
On 2009/06/30 22:44:02, mmoss wrote:
> Did you intend to do something here?

Removed.

http://codereview.chromium.org/151109/diff/1/3#newcode370
Line 370: if (grep -v ^MimeType= "$apps/$1"; echo "MimeType=text/html;$MIME") >
"$apps/$1.new"; then
On 2009/06/30 22:44:02, mmoss wrote:
> This test is always true as long as the echo succeeds and the "new" file is
> writable. Is that what you intended? Maybe you want "&&" after the grep.
> 
> Also, you should probably use mktemp(1) to create files, rather than just
adding
> ".new" to existing filenames.

Reworked somewhat clever file rewrite logic. (It did, in fact, mean to only test
whether the file was writable, or rather, was written. But that wasn't obvious.)

As for appending .new, it was following the example in make_default_kde in
xdg-mime.in which does the same. Should I change it, or leave it consistent?

http://codereview.chromium.org/151109/diff/1/3#newcode579
Line 579: done
On 2009/06/30 22:44:02, mmoss wrote:
> I thinks this can just be 'readlink -f $browser'.

Done.

http://codereview.chromium.org/151109/diff/1/3#newcode608
Line 608: generic)
On 2009/06/30 22:44:02, mmoss wrote:
> Is this ever hit?

No, but detectDE comes from xdg-utils-common.in, and xdg-open.in (for instance)
seems to suggest that "generic" is at least an intended possible value for $DE.
So I handle it here as well. I could remove it though, along with
dispatch_generic() above. Preference?

[Michael Moss, 2009-06-30 23:51:16]

http://codereview.chromium.org/151109/diff/1/3
File third_party/xdg-utils/scripts/xdg-settings (right):

http://codereview.chromium.org/151109/diff/1/3#newcode370
Line 370: if (grep -v ^MimeType= "$apps/$1"; echo "MimeType=text/html;$MIME") >
"$apps/$1.new"; then
> As for appending .new, it was following the example in make_default_kde in
> xdg-mime.in which does the same. Should I change it, or leave it consistent?

Yeah, they shouldn't do that, especially if they're not testing if the file
exists first (and then what would they do?). Better to guarantee uniqueness from
the start with mktemp.

http://codereview.chromium.org/151109/diff/1/3#newcode608
Line 608: generic)
> seems to suggest that "generic" is at least an intended possible value for
$DE.

OK, leave it then.

[Mike Mammarella [gmail], 2009-07-01 00:14:04]

On 2009/06/30 23:51:16, mmoss wrote:
> http://codereview.chromium.org/151109/diff/1/3
> File third_party/xdg-utils/scripts/xdg-settings (right):
> 
> http://codereview.chromium.org/151109/diff/1/3#newcode370
> Line 370: if (grep -v ^MimeType= "$apps/$1"; echo "MimeType=text/html;$MIME")
>
> "$apps/$1.new"; then
> > As for appending .new, it was following the example in make_default_kde in
> > xdg-mime.in which does the same. Should I change it, or leave it consistent?
> 
> Yeah, they shouldn't do that, especially if they're not testing if the file
> exists first (and then what would they do?). Better to guarantee uniqueness
from
> the start with mktemp.

Changed.

> http://codereview.chromium.org/151109/diff/1/3#newcode608
> Line 608: generic)
> > seems to suggest that "generic" is at least an intended possible value for
> $DE.
> 
> OK, leave it then.

[Michael Moss, 2009-07-01 18:36:43]

LGTM with one last nit (no need to resend for review after that change).

http://codereview.chromium.org/151109/diff/1023/1026
File third_party/xdg-utils/scripts/xdg-settings.in (right):

http://codereview.chromium.org/151109/diff/1023/1026#newcode68
Line 68: temp="`mktemp $apps/$1.XXXXXXXX`" || return
Sorry to be nitpicky still, but you might want to use just six placeholders for
the temp file name (XXXXXX). I ran into an issue once where some version of
mktemp (on a recent distro) failed because I had tried to use fewer than six.
I'm not sure if it will also fail with more than six, but I am pretty sure that
six is always safe.

[Mike Mammarella [gmail], 2009-07-01 19:54:20]

http://codereview.chromium.org/151109/diff/1023/1026
File third_party/xdg-utils/scripts/xdg-settings.in (right):

http://codereview.chromium.org/151109/diff/1023/1026#newcode68
Line 68: temp="`mktemp $apps/$1.XXXXXXXX`" || return
On 2009/07/01 18:36:43, mmoss wrote:
> Sorry to be nitpicky still, but you might want to use just six placeholders
for
> the temp file name (XXXXXX). I ran into an issue once where some version of
> mktemp (on a recent distro) failed because I had tried to use fewer than six.
> I'm not sure if it will also fail with more than six, but I am pretty sure
that
> six is always safe.

Done.