Fix file access on Chrome for ChromeOS on Linux

Fix file access on Chrome for ChromeOS on Linux
so that we can open files in the user's Downloads
directory.

Shouldn't affect actual ChromeOS or other platforms.

BUG=chromium-os:29447
TEST=Ran on Linux, opened files from Downloads folder.


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

[Greg Spencer (Chromium), 2012-04-12 18:51:57]

willchan, I need an OWNERS review here too.

[zel, 2012-04-12 20:19:40]

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
net/url_request/url_request_file_job.cc:141: // If we're running Chrome for
ChromeOS on Linux, we want to allow files
if this is for debugging only on Linux machines with ChromeOS build. I would 

#if defined(OS_CHROMEOS)
#ifndef NDEBUG
...
#endif
#endif

[Greg Spencer (Chromium), 2012-04-12 20:26:58]

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
net/url_request/url_request_file_job.cc:141: // If we're running Chrome for
ChromeOS on Linux, we want to allow files
On 2012/04/12 20:19:40, zel wrote:
> if this is for debugging only on Linux machines with ChromeOS build. I would 
> 
> #if defined(OS_CHROMEOS)
> #ifndef NDEBUG
> ...
> #endif
> #endif

It's already inside of OS_CHROMEOS, and while I see what you're saying, I think
that making it available to release would be less annoying for a future
developer wondering why they can't access a file on their (release) build, even
though they can see it in the file selection dialog: they might spend time
trying to track it down as a bug.

[willchan no longer on Chromium, 2012-04-12 23:44:59]

I'm going to be OOO until Monday. Redirecting to eroman. But I'm annoyed by the
layering violation.

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/10068021/diff/1/net/url_request/url_request_fi...
net/url_request/url_request_file_job.cc:141: // If we're running Chrome for
ChromeOS on Linux, we want to allow files
On 2012/04/12 20:26:58, Greg Spencer (Chromium) wrote:
> On 2012/04/12 20:19:40, zel wrote:
> > if this is for debugging only on Linux machines with ChromeOS build. I would

> > 
> > #if defined(OS_CHROMEOS)
> > #ifndef NDEBUG
> > ...
> > #endif
> > #endif
> 
> It's already inside of OS_CHROMEOS, and while I see what you're saying, I
think
> that making it available to release would be less annoying for a future
> developer wondering why they can't access a file on their (release) build,
even
> though they can see it in the file selection dialog: they might spend time
> trying to track it down as a bug.

Ugh, I don't know why this whitelist was allowed in the first place. This stuff
is all layering violations. net/ shouldn't know anything about chromeos
filesystem kinda stuff. Is there any other way to accomplish it?

[eroman, 2012-04-13 00:25:23]

LGTM

[Greg Spencer (Chromium), 2012-04-13 19:17:09]

On 2012/04/12 23:44:59, willchan wrote:
> Ugh, I don't know why this whitelist was allowed in the first place. This
stuff
> is all layering violations. net/ shouldn't know anything about chromeos
> filesystem kinda stuff. Is there any other way to accomplish it?

It was put in over 18 months ago, so I'm guessing we weren't as focused on
layering violations then.  I agree that it's ugly.  In any case, I've filed
http://crosbug.com/29447 to address the layering problem.

I'm going to go ahead and commit this as-is, since my change didn't introduce
the layering violation, and it would be good to have the functionality I'm
adding to allow for easy debugging of UI issues.  My code can be moved along
with the rest of the whitelisting code when that issue is addressed.

[Greg Spencer (Chromium), 2012-04-14 00:45:30]

Zel, is this OK to go ahead with?

[zel, 2012-04-14 00:52:33]

no pressure form my side on this one, if you want to refactor it properly
go for it


On Fri, Apr 13, 2012 at 5:45 PM, <gspencer@chromium.org> wrote:

> Zel, is this OK to go ahead with?
>
>
http://codereview.chromium.**org/10068021/<http://codereview.chromium.org/100...
>

[willchan no longer on Chromium, 2012-04-14 16:21:23]

My minor preference is to fix this (it looks easy with NetworkDelegate). But you
already have approval from eroman, so go ahead and land if you'd like. You could
also land now and fix in a separate CL.

On 2012/04/14 00:52:33, zel wrote:
> no pressure form my side on this one, if you want to refactor it properly
> go for it
> 
> 
> On Fri, Apr 13, 2012 at 5:45 PM, <mailto:gspencer@chromium.org> wrote:
> 
> > Zel, is this OK to go ahead with?
> >
> >
>
http://codereview.chromium.**org/10068021/%3Chttp://codereview.chromium.org/1...>
> >

[Greg Spencer (Chromium), 2012-04-18 22:35:08]

On 2012/04/14 16:21:23, willchan wrote:
> My minor preference is to fix this (it looks easy with NetworkDelegate). But
you
> already have approval from eroman, so go ahead and land if you'd like. You
could
> also land now and fix in a separate CL.

I decided to just fix it in place.  PTAL.

[achuithb, 2012-04-19 20:05:56]

Sorry about the layering mess. This was my starter bug at Google.

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:7: #include
"base/chromeos/chromeos_version.h"
Shouldn't this be under #if defined(OS_CHROMEOS)?

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:312: static const char* const
kLocalAccessWhiteList[] = {
Could you please add a comment here stating that we disallow access to all files
except ones under these white-listed directories on chromeos devices?

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:325: const char* home =
getenv("HOME");
I think you should just return true here. We only care about the white list on
the device.

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate....
net/base/network_delegate.h:204: 
nit:unnecessary newline?

[Greg Spencer (Chromium), 2012-04-19 20:51:17]

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:7: #include
"base/chromeos/chromeos_version.h"
On 2012/04/19 20:05:56, achuith.bhandarkar wrote:
> Shouldn't this be under #if defined(OS_CHROMEOS)?

Whoops.  Yep. Done.

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:312: static const char* const
kLocalAccessWhiteList[] = {
On 2012/04/19 20:05:56, achuith.bhandarkar wrote:
> Could you please add a comment here stating that we disallow access to all
files
> except ones under these white-listed directories on chromeos devices?

Done.

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:325: const char* home =
getenv("HOME");
On 2012/04/19 20:05:56, achuith.bhandarkar wrote:
> I think you should just return true here. We only care about the white list on
> the device.

Done.  I do worry a bit that making it act differently than the "real thing"
might delay finding a bug in new code until it runs on the device.  But then
none of the other directories in the whitelist are even mapped properly when
running on Linux anyhow, so I guess it doesn't matter.

[willchan no longer on Chromium, 2012-04-19 21:12:24]

You rule for fixing it the right way!

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:85: virtual bool
CanAccessFile(const net::URLRequest* request,
const net::URLRequest& instead. The other places here where we used pointers
instead of const references were mistakes IMO.

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate.cc
File net/base/network_delegate.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate....
net/base/network_delegate.cc:114: const FilePath& path){
Missing a whitespace before the '{'

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.cc:345: if
(URLRequestJobManager::GetInstance()->file_access_allowed())
From what I can tell, all this code was added purely for chromeos. Can you just
kill it all? I think that the only code we should have in net/ is the code in
URLRequestFileJob to look for
URLRequestContext=>NetworkDelegate=>CanAccessFile() and the member function in
NetworkDelegate. URLRequest and URLRequestJobManager should have none of this
code. ChromeNetworkDelegate can have it, and for CrOs tests, it can call a class
static member function in ChromeNetworkDelegate instead of on URLRequest to
allow access to all files.

willchan@penguin:/usr/local/google/chromium1/src$ git gs enable_file_access
net/url_request/url_request.cc:341: 
URLRequestJobManager::GetInstance()->set_enable_file_access(true);
net/url_request/url_request.cc:346:  return
URLRequestJobManager::GetInstance()->enable_file_access();
net/url_request/url_request_job_manager.cc:268:      enable_file_access_(false)
{
net/url_request/url_request_job_manager.h:68:  void set_enable_file_access(bool
enable) { enable_file_access_ = enable; }
net/url_request/url_request_job_manager.h:69:  bool enable_file_access() const {
return enable_file_access_; }
net/url_request/url_request_job_manager.h:111:  bool enable_file_access_;

willchan@penguin:/usr/local/google/chromium1/src$ git gs "AllowFileAccess()"
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:341:   
net::URLRequest::AllowFileAccess();
net/proxy/proxy_script_fetcher_impl_unittest.cc:128:   
URLRequest::AllowFileAccess();
net/url_request/url_request.cc:340:void URLRequest::AllowFileAccess() {
net/url_request/url_request.h:306:  static void AllowFileAccess();
net/url_request/url_request_unittest.cc:415:    URLRequest::AllowFileAccess();

willchan@penguin:/usr/local/google/chromium1/src$ git gs "IsFileAccessAllowed()"
net/url_request/url_request.cc:345:bool URLRequest::IsFileAccessAllowed() {
net/url_request/url_request.h:307:  static bool IsFileAccessAllowed();
net/url_request/url_request_file_job.cc:136:  if
(URLRequest::IsFileAccessAllowed()) {  // for tests.

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.h:312: bool IsFileAccessAllowed(const FilePath&
path);
This is only used by the URLRequestFileJob, right? Can you move the checking of
the NetworkDelegate to URLRequestFileJob? Also, this should be a const member
function.

[Greg Spencer (Chromium), 2012-04-20 00:05:53]

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/12005/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:85: virtual bool
CanAccessFile(const net::URLRequest* request,
On 2012/04/19 21:12:24, willchan wrote:
> const net::URLRequest& instead. The other places here where we used pointers
> instead of const references were mistakes IMO.

Done.  I fixed the other places too (turns out there were only two that took
const URLRequest* anyhow).

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate.cc
File net/base/network_delegate.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/base/network_delegate....
net/base/network_delegate.cc:114: const FilePath& path){
On 2012/04/19 21:12:24, willchan wrote:
> Missing a whitespace before the '{'

Done.

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.cc:345: if
(URLRequestJobManager::GetInstance()->file_access_allowed())
On 2012/04/19 21:12:24, willchan wrote:
> From what I can tell, all this code was added purely for chromeos. Can you
just
> kill it all? I think that the only code we should have in net/ is the code in
> URLRequestFileJob to look for
> URLRequestContext=>NetworkDelegate=>CanAccessFile() and the member function in
> NetworkDelegate. URLRequest and URLRequestJobManager should have none of this
> code. ChromeNetworkDelegate can have it, and for CrOs tests, it can call a
class
> static member function in ChromeNetworkDelegate instead of on URLRequest to
> allow access to all files.

Yeah, you're right.  I've moved almost all of it either into the
URLRequestFileJob or into the ChromeNetworkDelegate.  I had to leave a static on
URLRequestFileJob to override the delegate because there is one platform test in
net/ that relies on having access to file:// URLs
(proxy_script_fetcher_impl_unittest.cc) and since it's in net/, it can't call a
ChromeNetworkDelegate function, but I think I've made it as minimal as possible.

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.h:312: bool IsFileAccessAllowed(const FilePath&
path);
On 2012/04/19 21:12:24, willchan wrote:
> This is only used by the URLRequestFileJob, right? Can you move the checking
of
> the NetworkDelegate to URLRequestFileJob? Also, this should be a const member
> function.

Done.  I originally missed how to get at the network delegate from the
URLRequestFileJob, or I would have done this before.

[willchan no longer on Chromium, 2012-04-20 00:14:03]

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.cc:345: if
(URLRequestJobManager::GetInstance()->file_access_allowed())
On 2012/04/20 00:05:54, Greg Spencer (Chromium) wrote:
> On 2012/04/19 21:12:24, willchan wrote:
> > From what I can tell, all this code was added purely for chromeos. Can you
> just
> > kill it all? I think that the only code we should have in net/ is the code
in
> > URLRequestFileJob to look for
> > URLRequestContext=>NetworkDelegate=>CanAccessFile() and the member function
in
> > NetworkDelegate. URLRequest and URLRequestJobManager should have none of
this
> > code. ChromeNetworkDelegate can have it, and for CrOs tests, it can call a
> class
> > static member function in ChromeNetworkDelegate instead of on URLRequest to
> > allow access to all files.
> 
> Yeah, you're right.  I've moved almost all of it either into the
> URLRequestFileJob or into the ChromeNetworkDelegate.  I had to leave a static
on
> URLRequestFileJob to override the delegate because there is one platform test
in
> net/ that relies on having access to file:// URLs
> (proxy_script_fetcher_impl_unittest.cc) and since it's in net/, it can't call
a
> ChromeNetworkDelegate function, but I think I've made it as minimal as
possible.

Can you get rid of that and just set a NetworkDelegate subclass?

http://codereview.chromium.org/10068021/diff/22003/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/22003/net/base/network_delegate....
net/base/network_delegate.h:86: bool NotifyFileAccessRequested(const URLRequest&
request,
Anything that's got a bool return value probably should be named different from
Notify. You don't have to fix the other ones (unless you're really nice!) but
can you rename this? To avoid the naming conflict with the virtual private
member functions, I'd name them FooInternal() or something.

[Greg Spencer (Chromium), 2012-04-20 19:21:28]

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10068021/diff/12005/net/url_request/url_reques...
net/url_request/url_request.cc:345: if
(URLRequestJobManager::GetInstance()->file_access_allowed())
On 2012/04/20 00:14:03, willchan wrote:
> Can you get rid of that and just set a NetworkDelegate subclass?

Duh, that would be better. :-) Done.

http://codereview.chromium.org/10068021/diff/22003/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/22003/net/base/network_delegate....
net/base/network_delegate.h:86: bool NotifyFileAccessRequested(const URLRequest&
request,
On 2012/04/20 00:14:03, willchan wrote:
> Anything that's got a bool return value probably should be named different
from
> Notify. You don't have to fix the other ones (unless you're really nice!) but
> can you rename this? To avoid the naming conflict with the virtual private
> member functions, I'd name them FooInternal() or something.

Done.  I renamed the cookie related ones too. (Renaming is easy in Eclipse, when
it works.)

[Greg Spencer (Chromium), 2012-05-02 02:52:23]

Friendly ping...

[willchan no longer on Chromium, 2012-05-02 05:23:56]

lgtm

[willchan no longer on Chromium, 2012-05-02 05:24:27]

On 2012/05/02 05:23:56, willchan wrote:
> lgtm

Sorry for the delay, feel free to ping me sooner next time, 1 week is atrocious
response time. I usually shoot for 24h.

[achuithb, 2012-05-02 07:20:57]

lgtm

[Greg Spencer (Chromium), 2012-05-02 17:11:58]

On 2012/05/02 05:24:27, willchan wrote:
> On 2012/05/02 05:23:56, willchan wrote:
> > lgtm
> 
> Sorry for the delay, feel free to ping me sooner next time, 1 week is
atrocious
> response time. I usually shoot for 24h.

No worries, the only reason I pinged you so late was that I forgot about this
change too. :-)

[commit-bot: I haz the power, 2012-05-02 20:25:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10068021/54002

[commit-bot: I haz the power, 2012-05-02 20:26:08]

Presubmit check for 10068021-54002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit
    content

Presubmit checks took 2.7s to calculate.

[Greg Spencer (Chromium), 2012-05-02 20:46:52]

Darin,

I need an OWNERS review for the following files:

content/shell/shell_network_delegate.cc
content/shell/shell_network_delegate.h
webkit/tools/test_shell/simple_resource_loader_bridge.cc

Basically, the changes in these files are just because they use an API that had
some functions renamed.

[Greg Spencer (Chromium), 2012-05-03 17:35:15]

Since these are such rote changes, I'm going to TBR the last OWNERs review.  Let
me know if you want additional changes and I'll make them.

[darin (slow to review), 2012-05-04 06:49:31]

LGTM for webkit/ changes, and it's always OK to TBR such trivial changes.  But,
I do find something else about this change somewhat awkward:

http://codereview.chromium.org/10068021/diff/54002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/54002/net/base/network_delegate....
net/base/network_delegate.h:188: virtual bool CanGetCookiesInternal(const
URLRequest& request,
Why are you using the "Internal" suffix here?  It seems like subclasses are
expected to implement these methods, and yet it seems suboptimal for subclasses
to implement internal methods.  Such a suffix seems to suggest something private
or internal to the class that defines them.

Also, this Internal naming isn't consistent with the rest of the NetworkDelegate
interface, which maps concrete NotifyFoo methods to OnFoo virtual methods. 
Should those instead be virtual NofifyFooInternal methods?  Or, should these be
OnCanGetCookies, OnCanSetCookie and OnCanAccessFile instead?

[Greg Spencer (Chromium), 2012-05-05 02:46:19]

http://codereview.chromium.org/10068021/diff/54002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10068021/diff/54002/net/base/network_delegate....
net/base/network_delegate.h:188: virtual bool CanGetCookiesInternal(const
URLRequest& request,
On 2012/05/04 06:49:31, darin wrote:
> Why are you using the "Internal" suffix here?  It seems like subclasses are
> expected to implement these methods, and yet it seems suboptimal for
subclasses
> to implement internal methods.  Such a suffix seems to suggest something
private
> or internal to the class that defines them.
> 
> Also, this Internal naming isn't consistent with the rest of the
NetworkDelegate
> interface, which maps concrete NotifyFoo methods to OnFoo virtual methods. 
> Should those instead be virtual NofifyFooInternal methods?  Or, should these
be
> OnCanGetCookies, OnCanSetCookie and OnCanAccessFile instead?

Well, the "Internal" was a suggestion from another reviewer,  but I think you're
right: OnCanGetCookies, etc. is better.  Luckily I forgot to check the commit
box, so I've fixed this.

[commit-bot: I haz the power, 2012-05-05 02:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10068021/65002

[commit-bot: I haz the power, 2012-05-05 03:03:26]

Try job failure for 10068021-65002 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-05 16:40:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10068021/74002

[commit-bot: I haz the power, 2012-05-05 19:41:10]

Change committed as 135553