[shell_util] Fall Back to Long Paths if Short Is Not Available

[shell_util] Fall Back to Long Paths if Short Is Not Available

If GetShortPathName() fails (and hence so does ShortNameFromPath()), fall
back to the long name.

It's not clear to me whether the difference between UNKNOWN and NOT_DEFAULT is meaningfully used anyway.  Maybe the code reviewer knows.  This change revises ProbeOpenCommandHandlers() so it will always return yes or no, no more maybe about it.

TBR=gab
BUG=543306
Committed: https://crrev.com/2d04ece803beb091016667fc0b3945c6e24e7373
Cr-Commit-Position: refs/heads/master@{#354396}

[Mark P, 2015-10-14 23:49:12]

mpearson@chromium.org changed reviewers:
+ gab@chromium.org

[Mark P, 2015-10-14 23:51:11]

gab@, please take a look. thanks, mark

Feel free to put it on the commit queue if it's fine without any modifications.

[gab, 2015-10-15 14:21:42]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2015-10-15 14:21:43]

I think this code could just use ProgramCompare instead of doing string
comparisons, @grt WDYT?

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
Value is only used inside if{} below, move it inline.

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1334: if
(!base::FilePath::CompareEqualIgnoreCase(short_path, app_path))
Actually, @grt: shouldn't this entire method use ProgramCompare instead and stop
being bothered by doing strcmp on short/long paths?

[Mark P, 2015-10-15 16:27:48]

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
On 2015/10/15 14:21:43, gab wrote:
> Value is only used inside if{} below, move it inline.

I don't know what you mean.  It's used in two spots below.  And I don't see how
to inline app_path in any case unless you want me to do something awkward like 
  ShortNameFromPath(chrome_exe, &app_path) ? app_path : chrome_exe.value()

That said, this point may be moot if grt@ agrees with you.

[gab, 2015-10-15 18:33:07]

Replying to comment.

@grt still looking for you advice on first comment. Thanks!

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
On 2015/10/15 16:27:48, Mark P wrote:
> On 2015/10/15 14:21:43, gab wrote:
> > Value is only used inside if{} below, move it inline.
> 
> I don't know what you mean.  It's used in two spots below.  And I don't see
how
> to inline app_path in any case unless you want me to do something awkward like

>   ShortNameFromPath(chrome_exe, &app_path) ? app_path : chrome_exe.value()

What I mean is you only need |short_app_path| in the if{} in the else{} you can
use |chrome_exe| directly.

> 
> That said, this point may be moot if grt@ agrees with you.

Right.

[Mark P, 2015-10-15 19:17:42]

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
On 2015/10/15 18:33:07, gab wrote:
> On 2015/10/15 16:27:48, Mark P wrote:
> > On 2015/10/15 14:21:43, gab wrote:
> > > Value is only used inside if{} below, move it inline.
> > 
> > I don't know what you mean.  It's used in two spots below.  And I don't see
> how
> > to inline app_path in any case unless you want me to do something awkward
like
> 
> >   ShortNameFromPath(chrome_exe, &app_path) ? app_path : chrome_exe.value()
> 
> What I mean is you only need |short_app_path| in the if{} in the else{} you
can
> use |chrome_exe| directly.

I don't understand.  Do you mean that new line 1338 should use
chrome_exe.value() instead of app_path?  If so, I don't necessarily agree; I
think it's possible that one ShortNameFromPath() succeeds and the other does
not.

> > 
> > That said, this point may be moot if grt@ agrees with you.
> 
> Right.

[Mark P, 2015-10-15 21:48:51]

gab@ apparently had no objections to this change, just a question about doing it
better (comparing programs instead of command lines) and doing it cleaner
(inline something).  He didn't mention any concerns about the issue in the
changelist description (changing some UNKNOWNS to DEFAULT/NOT_DEFAULT).

Given the lack of substantive issues raise, I'm submitting this to fix the
ongoing browser_tests problems on the tree.

Please improve the code in a later changelist.

--mark

[Mark P, 2015-10-15 21:49:11]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2015-10-15 21:50:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1402243003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1402243003/20001

[commit-bot: I haz the power, 2015-10-15 23:14:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-15 23:14:49]

Patchset 2 (id:??) landed as
https://crrev.com/2d04ece803beb091016667fc0b3945c6e24e7373
Cr-Commit-Position: refs/heads/master@{#354396}

[gab, 2015-10-16 19:02:17]

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
On 2015/10/15 19:17:42, Mark P wrote:
> On 2015/10/15 18:33:07, gab wrote:
> > On 2015/10/15 16:27:48, Mark P wrote:
> > > On 2015/10/15 14:21:43, gab wrote:
> > > > Value is only used inside if{} below, move it inline.
> > > 
> > > I don't know what you mean.  It's used in two spots below.  And I don't
see
> > how
> > > to inline app_path in any case unless you want me to do something awkward
> like
> > 
> > >   ShortNameFromPath(chrome_exe, &app_path) ? app_path : chrome_exe.value()
> > 
> > What I mean is you only need |short_app_path| in the if{} in the else{} you
> can
> > use |chrome_exe| directly.
> 
> I don't understand.  Do you mean that new line 1338 should use
> chrome_exe.value() instead of app_path?  If so, I don't necessarily agree; I
> think it's possible that one ShortNameFromPath() succeeds and the other does
> not.

I mean you definitely don't need it if you're not using the short_path for the
command_line below. And if you are, sure getting chrome_exe's short_path can
also fail, but you can handle there (i.e. in previous code |short_app_path| is
not used until end of method -- only reason for previously having this code at
the top was the early return).

> 
> > > 
> > > That said, this point may be moot if grt@ agrees with you.
> > 
> > Right.
>

[gab, 2015-10-16 19:14:29]

On 2015/10/15 21:48:51, Mark P wrote:
> gab@ apparently had no objections to this change, just a question about doing
it
> better (comparing programs instead of command lines) and doing it cleaner
> (inline something).  He didn't mention any concerns about the issue in the
> changelist description (changing some UNKNOWNS to DEFAULT/NOT_DEFAULT).

Actually I'd missed this (and didn't think we were done?!) but yes that's
potentially an issue, ideally all errors resulting in uncertainties should
result in UNKNOWN as we otherwise may, e.g., prompt the user with a default
browser infobar every launch when they have Chrome as default (but fail to
detect that it is).

> 
> Given the lack of substantive issues raise, I'm submitting this to fix the
> ongoing browser_tests problems on the tree.

Hmmm okay, I would have appreciated a ping about this, I definitely hadn't
l.g.t.m.'ed with nits...

> 
> Please improve the code in a later changelist.

Since when is TBR + tell the owner to fix their code a thing we do? Improving
code health as we touch it is everyone's responsibility, IMO this CL only made
the code less readable.

I understand urgency to fix ongoing issues, but this was not raised at any point
on this CL before TBR'ing... Plus this code has been the same for 3+ years, I
don't understand this specific urgency...

Thanks for following up,
Gab

> 
> --mark

[gab, 2015-10-16 19:17:09]

On 2015/10/16 19:14:29, gab wrote:
> On 2015/10/15 21:48:51, Mark P wrote:
> > gab@ apparently had no objections to this change, just a question about
doing
> it
> > better (comparing programs instead of command lines) and doing it cleaner
> > (inline something).  He didn't mention any concerns about the issue in the
> > changelist description (changing some UNKNOWNS to DEFAULT/NOT_DEFAULT).
> 
> Actually I'd missed this (and didn't think we were done?!) but yes that's
> potentially an issue, ideally all errors resulting in uncertainties should
> result in UNKNOWN as we otherwise may, e.g., prompt the user with a default
> browser infobar every launch when they have Chrome as default (but fail to
> detect that it is).

FWIW, the suggestion to use ProgramCompare was also to make this more precise
and perhaps be able to get rid of UNKNOWN, but as-is this CL is not lgtm.

> 
> > 
> > Given the lack of substantive issues raise, I'm submitting this to fix the
> > ongoing browser_tests problems on the tree.
> 
> Hmmm okay, I would have appreciated a ping about this, I definitely hadn't
> l.g.t.m.'ed with nits...
> 
> > 
> > Please improve the code in a later changelist.
> 
> Since when is TBR + tell the owner to fix their code a thing we do? Improving
> code health as we touch it is everyone's responsibility, IMO this CL only made
> the code less readable.
> 
> I understand urgency to fix ongoing issues, but this was not raised at any
point
> on this CL before TBR'ing... Plus this code has been the same for 3+ years, I
> don't understand this specific urgency...
> 
> Thanks for following up,
> Gab
> 
> > 
> > --mark

[Mark P, 2015-10-16 23:02:44]

On 2015/10/16 19:14:29, gab wrote:
> On 2015/10/15 21:48:51, Mark P wrote:
> > gab@ apparently had no objections to this change, just a question about
doing
> it
> > better (comparing programs instead of command lines) and doing it cleaner
> > (inline something).  He didn't mention any concerns about the issue in the
> > changelist description (changing some UNKNOWNS to DEFAULT/NOT_DEFAULT).
> 
> Actually I'd missed this (and didn't think we were done?!) but yes that's
> potentially an issue, ideally all errors resulting in uncertainties should
> result in UNKNOWN as we otherwise may, e.g., prompt the user with a default
> browser infobar every launch when they have Chrome as default (but fail to
> detect that it is).

Okay, to think more about this, it seems that previously we'd definitely
avoid showing a prompt if we failed to get the short name for any of the
programs associated with registered protocols or we failed to get the
short name of chrome.exe.  It sounds like there's no great reason to use the
short name in these comparisons and a more accepting manner (full name or
program comparison) would make the prompting more correct compared to the
intention (prompt if known not default).

Do you agree that it seems like this code and in general the program comparison
idea is more correct?

> > 
> > Given the lack of substantive issues raise, I'm submitting this to fix the
> > ongoing browser_tests problems on the tree.
> 
> Hmmm okay, I would have appreciated a ping about this, I definitely hadn't
> l.g.t.m.'ed with nits...
>
> > 
> > Please improve the code in a later changelist.
> 
> I understand urgency to fix ongoing issues, but this was not raised at any
point
> on this CL before TBR'ing... Plus this code has been the same for 3+ years, I
> don't understand this specific urgency...

Good points on all of the above.  I apologize; I realize I never conveyed at
all how urgent I thought this was.  A few of us thought this issue was the
reason
a whole test suite (browser_tests) was failing on a bot.  I can't believe I
never
mentioned that explicitly here!

I am led to believe that TBRing fixes to major breakages is acceptable as
sheriff
(as I was at the time).  But the way I handled it was inappropriate.

> Since when is TBR + tell the owner to fix their code a thing we do? Improving
> code health as we touch it is everyone's responsibility, IMO this CL only made
> the code less readable.

And, sheriff or not, telling someone (you) to improve someone else's changelist
(mine) later is wrong.  I should realized that I'd soon not be sheriff and not
dealing with fires, and be able to do it myself.

I'm write an improved changelist next week; hopefully grt@ will get back to us
soon
about the possible simplification.

--mark

> FWIW, the suggestion to use ProgramCompare was also to make this more precise
> and perhaps be able to get rid of UNKNOWN, but as-is this CL is not lgtm.

 
> Thanks for following up,
> Gab
> 
> > 
> > --mark

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
On 2015/10/16 19:02:17, gab wrote:
> On 2015/10/15 19:17:42, Mark P wrote:
> > On 2015/10/15 18:33:07, gab wrote:
> > > On 2015/10/15 16:27:48, Mark P wrote:
> > > > On 2015/10/15 14:21:43, gab wrote:
> > > > > Value is only used inside if{} below, move it inline.
> > > > 
> > > > I don't know what you mean.  It's used in two spots below.  And I don't
> see
> > > how
> > > > to inline app_path in any case unless you want me to do something
awkward
> > like
> > > 
> > > >   ShortNameFromPath(chrome_exe, &app_path) ? app_path :
chrome_exe.value()
> > > 
> > > What I mean is you only need |short_app_path| in the if{} in the else{}
you
> > can
> > > use |chrome_exe| directly.
> > 
> > I don't understand.  Do you mean that new line 1338 should use
> > chrome_exe.value() instead of app_path?  If so, I don't necessarily agree; I
> > think it's possible that one ShortNameFromPath() succeeds and the other does
> > not.
> 
> I mean you definitely don't need it if you're not using the short_path for the
> command_line below.

Ah, now I understand.

> And if you are, sure getting chrome_exe's short_path can
> also fail, but you can handle there (i.e. in previous code |short_app_path| is
> not used until end of method -- only reason for previously having this code at
> the top was the early return).
> 
> > 
> > > > 
> > > > That said, this point may be moot if grt@ agrees with you.
> > > 
> > > Right.
> > 
>

[grt (UTC plus 2), 2015-10-18 01:10:46]

in my primitive tests, GetShortPathName works fine on XP. can you point me to
logs that show the failure mode for this particular use in shell_util.cc?

[Mark P, 2015-10-18 04:42:02]

On Sat, Oct 17, 2015 at 6:10 PM, <grt@chromium.org> wrote:

> in my primitive tests, GetShortPathName works fine on XP. can you point me
> to
> logs that show the failure mode for this particular use in shell_util.cc?
>

Here are a bunch of logs with the error message:
Error getting short (8.3) path for ...
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

--mark



>
> https://codereview.chromium.org/1402243003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[grt (UTC plus 2), 2015-10-18 13:49:46]

On 2015/10/18 04:42:02, Mark P wrote:
> On Sat, Oct 17, 2015 at 6:10 PM, <mailto:grt@chromium.org> wrote:
> 
> > in my primitive tests, GetShortPathName works fine on XP. can you point me
> > to
> > logs that show the failure mode for this particular use in shell_util.cc?
> >
> 
> Here are a bunch of logs with the error message:
> Error getting short (8.3) path for ...
>
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...
> 
> --mark
> 
> 
> 
> >
> > https://codereview.chromium.org/1402243003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Apologies if I'm asking stupid questions here. That log isn't using \\?\ paths.
Is this change really related to the bug? Is this change addressing something
that was causing test failures, or just logging noise? Does the file in question
(in the log you pointed me to) really exist on disk?

I'm trying to understand what, if anything, is broken and being fixed by this
change. If GetShortBlahBlah isn't behaving as we expect, maybe other things are
broken, too. Thanks.

[gab, 2015-10-19 18:56:20]

On 2015/10/16 23:02:44, Mark P wrote:
> On 2015/10/16 19:14:29, gab wrote:
> > On 2015/10/15 21:48:51, Mark P wrote:
> > > gab@ apparently had no objections to this change, just a question about
> doing
> > it
> > > better (comparing programs instead of command lines) and doing it cleaner
> > > (inline something).  He didn't mention any concerns about the issue in the
> > > changelist description (changing some UNKNOWNS to DEFAULT/NOT_DEFAULT).
> > 
> > Actually I'd missed this (and didn't think we were done?!) but yes that's
> > potentially an issue, ideally all errors resulting in uncertainties should
> > result in UNKNOWN as we otherwise may, e.g., prompt the user with a default
> > browser infobar every launch when they have Chrome as default (but fail to
> > detect that it is).
> 
> Okay, to think more about this, it seems that previously we'd definitely
> avoid showing a prompt if we failed to get the short name for any of the
> programs associated with registered protocols or we failed to get the
> short name of chrome.exe.  It sounds like there's no great reason to use the
> short name in these comparisons and a more accepting manner (full name or
> program comparison) would make the prompting more correct compared to the
> intention (prompt if known not default).
> 
> Do you agree that it seems like this code and in general the program
comparison
> idea is more correct?

I think so, but was asking grt's opinion in my first comment on this CL as he's
more closely familiar with the low-level details of ProgramCompare.

> 
> > > 
> > > Given the lack of substantive issues raise, I'm submitting this to fix the
> > > ongoing browser_tests problems on the tree.
> > 
> > Hmmm okay, I would have appreciated a ping about this, I definitely hadn't
> > l.g.t.m.'ed with nits...
> >
> > > 
> > > Please improve the code in a later changelist.
> > 
> > I understand urgency to fix ongoing issues, but this was not raised at any
> point
> > on this CL before TBR'ing... Plus this code has been the same for 3+ years,
I
> > don't understand this specific urgency...
> 
> Good points on all of the above.  I apologize; I realize I never conveyed at
> all how urgent I thought this was.  A few of us thought this issue was the
> reason
> a whole test suite (browser_tests) was failing on a bot.  I can't believe I
> never
> mentioned that explicitly here!
> 
> I am led to believe that TBRing fixes to major breakages is acceptable as
> sheriff
> (as I was at the time).  But the way I handled it was inappropriate.
> 
> > Since when is TBR + tell the owner to fix their code a thing we do?
Improving
> > code health as we touch it is everyone's responsibility, IMO this CL only
made
> > the code less readable.
> 
> And, sheriff or not, telling someone (you) to improve someone else's
changelist
> (mine) later is wrong.  I should realized that I'd soon not be sheriff and not
> dealing with fires, and be able to do it myself.
> 
> I'm write an improved changelist next week; hopefully grt@ will get back to us
> soon
> about the possible simplification.

Okay thank you, hadn't realized this was part of a sheriff stint either
(definitely puts everything in a different perspective). Will wait for your next
CL (or revert of this one pending your ongoing discussion with grt).

> 
> --mark
> 
> > FWIW, the suggestion to use ProgramCompare was also to make this more
precise
> > and perhaps be able to get rid of UNKNOWN, but as-is this CL is not lgtm.
> 
>  
> > Thanks for following up,
> > Gab
> > 
> > > 
> > > --mark
> 
>
https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
> File chrome/installer/util/shell_util.cc (right):
> 
>
https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
> chrome/installer/util/shell_util.cc:1313: app_path = chrome_exe.value();
> On 2015/10/16 19:02:17, gab wrote:
> > On 2015/10/15 19:17:42, Mark P wrote:
> > > On 2015/10/15 18:33:07, gab wrote:
> > > > On 2015/10/15 16:27:48, Mark P wrote:
> > > > > On 2015/10/15 14:21:43, gab wrote:
> > > > > > Value is only used inside if{} below, move it inline.
> > > > > 
> > > > > I don't know what you mean.  It's used in two spots below.  And I
don't
> > see
> > > > how
> > > > > to inline app_path in any case unless you want me to do something
> awkward
> > > like
> > > > 
> > > > >   ShortNameFromPath(chrome_exe, &app_path) ? app_path :
> chrome_exe.value()
> > > > 
> > > > What I mean is you only need |short_app_path| in the if{} in the else{}
> you
> > > can
> > > > use |chrome_exe| directly.
> > > 
> > > I don't understand.  Do you mean that new line 1338 should use
> > > chrome_exe.value() instead of app_path?  If so, I don't necessarily agree;
I
> > > think it's possible that one ShortNameFromPath() succeeds and the other
does
> > > not.
> > 
> > I mean you definitely don't need it if you're not using the short_path for
the
> > command_line below.
> 
> Ah, now I understand.
> 
> > And if you are, sure getting chrome_exe's short_path can
> > also fail, but you can handle there (i.e. in previous code |short_app_path|
is
> > not used until end of method -- only reason for previously having this code
at
> > the top was the early return).
> > 
> > > 
> > > > > 
> > > > > That said, this point may be moot if grt@ agrees with you.
> > > > 
> > > > Right.
> > > 
> >

[grt (UTC plus 2), 2015-10-19 19:30:39]

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/1402243003/diff/20001/chrome/installer/util/s...
chrome/installer/util/shell_util.cc:1334: if
(!base::FilePath::CompareEqualIgnoreCase(short_path, app_path))
On 2015/10/15 14:21:43, gab wrote:
> Actually, @grt: shouldn't this entire method use ProgramCompare instead and
stop
> being bothered by doing strcmp on short/long paths?

Might be a good idea, although the condition leading to the benign error message
on the bot will still be there: GetShortPathName and ProgramCompare both work
best on files that exist/are accessible. The errors on the bot indicate that a
portion of the path passed in to ShortNameFromPath are inaccessible/don't exist.
This should be investigated, as it doesn't make sense. Is there a permissions
problem on the bot?