Fix PathServiceTest.Get on iOS.

Fix PathServiceTest.Get on iOS.

DIR_USER_DESKTOP does not apply on iOS, so exclude it from the set of paths that
are tested.

BUG=b/151050


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

[rohitrao (ping after 24h), 2012-09-20 18:03:00]

+gab for review

+brettw for OWNERS approval

[gab, 2012-09-20 18:24:55]

lgtm, but I have a question: shouldn't the implementation of DIR_USER_DESKTOP in
base_paths_mac.mm then return false automatically on iOS (i.e. is there a point
querying NSDesktopDirectory there)?

[rohitrao (ping after 24h), 2012-09-20 18:35:34]

[-brettw]

Changed.  That mirrors the Android code better.

+mento for review, since this is no longer quite as trivial a change.

[gab, 2012-09-20 18:43:48]

Ideally imo you would introduce base_paths_ios.mm and a PathProviderIOS method
for path_provider.cc, this is what Android and other platforms are doing. I'm
not sure what's the convention for iOS though (i.e. is there even such thing as
_ios files?!).

PathProviders can be chained (i.e. its a linked list of them in
path_provider.cc), so that you could chain the iOS one in front of the Mac one
to still fallback to the Mac one for paths with a common implementation on
both... just a thought as I just finished refactoring a chunk of this in
http://codereview.chromium.org/10964007/.

Cheers,
Gab

[rohitrao (ping after 24h), 2012-09-20 18:58:56]

Can two chained PathProviders both register to handle the same keys?  The
Android path provider seems to have its own key space.  In this case, I just
want to override DIR_USER_DESKTOP and a couple other keys, not create
iOS-specific keys in a separate key space.  I don't think there's any point in
forking off a PathProviderIOS if it simply duplicates 90% of the code from
PathProviderMac; a couple of #ifdefs seems better in that case.

This is probably worth following up on, but I think that should be in a separate
CL.  My immediate goal is to get the iOS build green again.

[gab, 2012-09-21 13:48:19]

On 2012/09/20 18:58:56, rohitrao wrote:
> Can two chained PathProviders both register to handle the same keys?  The
> Android path provider seems to have its own key space.  In this case, I just
> want to override DIR_USER_DESKTOP and a couple other keys, not create
> iOS-specific keys in a separate key space.  I don't think there's any point in
> forking off a PathProviderIOS if it simply duplicates 90% of the code from
> PathProviderMac; a couple of #ifdefs seems better in that case.

If you look in path_provider.cc, PathProviders are chained in such a way that if
the first one can't handle the request, it will try the next PathProvider, and
so on... This is already the case for the platform-specific PathProviders (e.g.
PathProviderMac) that are chained to PathProvider (the generic provider in
base_paths.cc). What I was suggesting is that you add PathProviderIOS (which
handles only the paths that are different) and chain it to PathProviderMac to
let Mac handle the common paths that PathProviderIOS doesn't handle.

In your case however it might be more complex than that since the OS-specific
implementation is to return false for DIR_USER_DESKTOP (but returning false
would mean you want the next handler to handle it...). So this is probably
overkill... anyways, I was just putting it out there for design consideration in
the future if you realize you have too many iOS-specific ifdefs in
base_paths_mac.mm.

> 
> This is probably worth following up on, but I think that should be in a
separate
> CL.  My immediate goal is to get the iOS build green again.

Yep, definitely not something that should go in this patch just to fix the iOS
build.

Cheers,
Gab

[lliabraa, 2012-09-25 14:34:52]

@mento - can you please take a look at this? The ios build has been broken for
days.

On 2012/09/21 13:48:19, gab wrote:
> On 2012/09/20 18:58:56, rohitrao wrote:
> > Can two chained PathProviders both register to handle the same keys?  The
> > Android path provider seems to have its own key space.  In this case, I just
> > want to override DIR_USER_DESKTOP and a couple other keys, not create
> > iOS-specific keys in a separate key space.  I don't think there's any point
in
> > forking off a PathProviderIOS if it simply duplicates 90% of the code from
> > PathProviderMac; a couple of #ifdefs seems better in that case.
> 
> If you look in path_provider.cc, PathProviders are chained in such a way that
if
> the first one can't handle the request, it will try the next PathProvider, and
> so on... This is already the case for the platform-specific PathProviders
(e.g.
> PathProviderMac) that are chained to PathProvider (the generic provider in
> base_paths.cc). What I was suggesting is that you add PathProviderIOS (which
> handles only the paths that are different) and chain it to PathProviderMac to
> let Mac handle the common paths that PathProviderIOS doesn't handle.
> 
> In your case however it might be more complex than that since the OS-specific
> implementation is to return false for DIR_USER_DESKTOP (but returning false
> would mean you want the next handler to handle it...). So this is probably
> overkill... anyways, I was just putting it out there for design consideration
in
> the future if you realize you have too many iOS-specific ifdefs in
> base_paths_mac.mm.
> 
> > 
> > This is probably worth following up on, but I think that should be in a
> separate
> > CL.  My immediate goal is to get the iOS build green again.
> 
> Yep, definitely not something that should go in this patch just to fix the iOS
> build.
> 
> Cheers,
> Gab

[Mark Mentovai, 2012-09-25 14:39:51]

LGTM

[commit-bot: I haz the power, 2012-09-25 14:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rohitrao@chromium.org/10970019/6

[commit-bot: I haz the power, 2012-09-25 18:16:26]

Change committed as 158607