Remove static PACKAGE_NAME variable and refactor getRawAcceptableDirectories method.

Remove static PACKAGE_NAME variable and refactor
getRawAcceptableDirectories method.

The use of PACKAGE_NAME variable is unnecessary which can be avoided, hence refactoring using passing context down till getRawAcceptableDirectories() and using getPackageName() method directly.

BUG=None.

Committed: https://crrev.com/11c47071c45eb89f78dcd95a8c00f83feeaceafd
Cr-Commit-Position: refs/heads/master@{#294826}

[deepak.db, 2014-09-11 06:03:13]

deepak.db@samsung.com changed reviewers:
+ andrewhayden@chromium.org, wajahat.s@samsung.com, yfriedman@chromium.org

[deepak.db, 2014-09-11 06:03:14]

Please Review, Thanks!

[deepak.db, 2014-09-11 06:46:03]

deepak.db@samsung.com changed reviewers:
+ benm@chromium.org

[deepak.db, 2014-09-11 06:46:04]

Please Review

[Andrew Hayden (chromium.org), 2014-09-11 09:43:34]

On 2014/09/11 06:46:04, deepak.db wrote:
> Please Review

Some background:
The original intention of "getRawAcceptableDirectories" is to allow us to
hard-code legacy paths that are often found "in the wild" that may now be
aliases to different *canonical* paths; additionally, they allow us to access
paths that may be different under specific conditions such as when running under
test harnesses. The code that calls this - filePathAcceptable(...) - has a
comment that explains this in some detail. In the real world, test harnesses may
result in "/mnt/sdcard/" pointing to almost anywhere, but the call to
canonicalize() will resolve to the actual runtime location (e.g., a temporary
directory on a build bot, etc). In this sense, using /data/data here is correct
because it is also such a path; but whereas /sdcard/ and /mnt/sdcard/ exist
independently of Chromium, the application cache directory does not. In this
sense, it seems reasonable to use PathUtils here, but to be completely honest I
don't remember the exact test problems we ran into with /sdcard/ and
/mnt/sdcard/, so it may truly be that the hardcoded path is required here as
well; I didn't add the code for allowing the cache directory, so I can't say for
sure. I'd ping the author of this change:
https://codereview.chromium.org/467173002/


That said, the use of a static variable PACKAGE_NAME here is definitely
ill-advised and I'm strongly in favor of the rewiring you have done to pass the
Context object down. I've made a note in the source review for this variable
(https://codereview.chromium.org/467173002/) that in my opinion this static
construct should not have been used to begin with.

To summarize: I'd actually leave the /data/data as it is unless the other author
can confirm that it's fine to use PathUtils, which should ensure that everything
works right even under test harnesses; but I'd keep the work you've done to hand
the Context object down, e.g. line 368 becomes:
  result.add("/data/data/" + context.getPackageName + "/cache/");
The system should take care of canonicalizing this to the same place that
PathUtils.getCacheDirectory() would return.

To be completely honest, in this case using PathUtils might actually be ok -
I've forgotten the *exact* behavior we saw under test that required
getRawAcceptableDirectories+canonicalize.

[deepak.db, 2014-09-11 11:25:29]

Thanks Andrew for review, I have incorporated the changes as per suggestions
PTAL!

On 2014/09/11 09:43:34, Andrew Hayden wrote:
> On 2014/09/11 06:46:04, deepak.db wrote:
> > Please Review
> 
> Some background:
> The original intention of "getRawAcceptableDirectories" is to allow us to
> hard-code legacy paths that are often found "in the wild" that may now be
> aliases to different *canonical* paths; additionally, they allow us to access
> paths that may be different under specific conditions such as when running
under
> test harnesses. The code that calls this - filePathAcceptable(...) - has a
> comment that explains this in some detail. In the real world, test harnesses
may
> result in "/mnt/sdcard/" pointing to almost anywhere, but the call to
> canonicalize() will resolve to the actual runtime location (e.g., a temporary
> directory on a build bot, etc). In this sense, using /data/data here is
correct
> because it is also such a path; but whereas /sdcard/ and /mnt/sdcard/ exist
> independently of Chromium, the application cache directory does not. In this
> sense, it seems reasonable to use PathUtils here, but to be completely honest
I
> don't remember the exact test problems we ran into with /sdcard/ and
> /mnt/sdcard/, so it may truly be that the hardcoded path is required here as
> well; I didn't add the code for allowing the cache directory, so I can't say
for
> sure. I'd ping the author of this change:
> https://codereview.chromium.org/467173002/
> 

> 
> That said, the use of a static variable PACKAGE_NAME here is definitely
> ill-advised and I'm strongly in favor of the rewiring you have done to pass
the
> Context object down. I've made a note in the source review for this variable
> (https://codereview.chromium.org/467173002/) that in my opinion this static
> construct should not have been used to begin with.
> 
> To summarize: I'd actually leave the /data/data as it is unless the other
author
> can confirm that it's fine to use PathUtils, which should ensure that
everything
> works right even under test harnesses; but I'd keep the work you've done to
hand
> the Context object down, e.g. line 368 becomes:
>   result.add("/data/data/" + context.getPackageName + "/cache/");
> The system should take care of canonicalizing this to the same place that
> PathUtils.getCacheDirectory() would return.
> 
> To be completely honest, in this case using PathUtils might actually be ok -
> I've forgotten the *exact* behavior we saw under test that required
> getRawAcceptableDirectories+canonicalize.

[Andrew Hayden (chromium.org), 2014-09-11 11:28:16]

LGTM and thank you for the cleanup.

[deepak.db, 2014-09-12 13:28:27]

deepak.db@samsung.com changed reviewers:
+ tedchoc@chromium.org

[deepak.db, 2014-09-12 13:28:28]

Thanks Andrew.
+tedchoc
Please Review, need another LGTM to land this patch.

[Ted C, 2014-09-12 15:35:01]

lgtm

[deepak.db, 2014-09-13 09:35:53]

The CQ bit was checked by deepak.db@samsung.com

[commit-bot: I haz the power, 2014-09-13 09:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561743003/40001

[commit-bot: I haz the power, 2014-09-13 11:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-13 11:30:49]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Andrew Hayden (chromium.org), 2014-09-15 08:17:55]

On 2014/09/13 11:30:49, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

Looks like you'll need to add an implementation for
MockContext.getPackageName().

[deepak.db, 2014-09-15 14:02:23]

Adding implementation for getPackageName in MediaResourceGetterTest class

On 2014/09/15 08:17:55, Andrew Hayden wrote:
> On 2014/09/13 11:30:49, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_dbg_tests_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> 
> Looks like you'll need to add an implementation for
> MockContext.getPackageName().

[Andrew Hayden (chromium.org), 2014-09-15 14:09:00]

still LGTM

[deepak.db, 2014-09-15 14:09:33]

The CQ bit was checked by deepak.db@samsung.com

[commit-bot: I haz the power, 2014-09-15 14:12:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561743003/60001

[commit-bot: I haz the power, 2014-09-15 15:15:30]

Committed patchset #4 (id:60001) as 97c2116c9ea189370514d46ced5ab331f49686a6

[commit-bot: I haz the power, 2014-09-15 15:20:03]

Patchset 4 (id:??) landed as
https://crrev.com/11c47071c45eb89f78dcd95a8c00f83feeaceafd
Cr-Commit-Position: refs/heads/master@{#294826}

[deepak.db, 2014-11-19 06:19:54]

Patchset #5 (id:80001) has been deleted