Place source URL of DownloadFile in extended attributes

Added functionality such that on Linux, after downloading a file,
the source URL and referrer URL are stored as extended file attributes.

Also see http://www.freedesktop.org/wiki/CommonExtendedAttributes

BUG=45903
TEST=content_unittests/file_metadata_unittest_linux.cc/FileMetadataLinuxTest

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

[cristian.patrasciuc, 2012-07-15 21:29:21]

Preliminary review request.

Besides possible code issues or style problems, some things that I'm not sure I
used correctly:
- The xattr.h header file. On my machine is in <sys/xattr.h>, on the man page it
says <attr/xattr.h>
- The macro that I used in base_file.cc to include my code might be too broad. I
could not test it this works or affects Android, ChromeOS, etc.
- The second unittest might not be needed. That scenario should not happen in
Chrome since we always set the attribute on a newly created/downloaded file.

Also, if you are not the correct reviewer please point me to  the right one.
Thanks.

[Randy Smith (Not in Mondays), 2012-07-15 21:33:45]

Ben, as downloads linux expert (:-}), would you be willing to take a look at
this CL?

[benjhayden, 2012-07-16 15:26:38]

On 2012/07/15 21:29:21, cristian.patrasciuc wrote:
> Preliminary review request.
> 
> Besides possible code issues or style problems, some things that I'm not sure
I
> used correctly:
> - The xattr.h header file. On my machine is in <sys/xattr.h>, on the man page
it
> says <attr/xattr.h>
> - The macro that I used in base_file.cc to include my code might be too broad.
I
> could not test it this works or affects Android, ChromeOS, etc.

I see /usr/include/sys/xattr.h on my linux box. I do not see attr/xattr.h.

I'm happy to rely on the trybots' verdict.

> - The second unittest might not be needed. That scenario should not happen in
> Chrome since we always set the attribute on a newly created/downloaded file.

Since it's a unit test and not a browser test, it should be fast enough that the
extra coverage is worth the fraction of a second. Also, this is a lower-level
library than chrome downloads, so it should not rely on behaviors of the
downloads system, especially behaviors that may change when downloads resumption
is done.

> 
> Also, if you are not the correct reviewer please point me to  the right one.
> Thanks.

Looks good pending trybots.

[benjhayden, 2012-07-16 17:32:10]

On 2012/07/16 15:26:38, benjhayden_chromium wrote:
> On 2012/07/15 21:29:21, cristian.patrasciuc wrote:
> > Preliminary review request.
> > 
> > Besides possible code issues or style problems, some things that I'm not
sure
> I
> > used correctly:
> > - The xattr.h header file. On my machine is in <sys/xattr.h>, on the man
page
> it
> > says <attr/xattr.h>
> > - The macro that I used in base_file.cc to include my code might be too
broad.
> I
> > could not test it this works or affects Android, ChromeOS, etc.
> 
> I see /usr/include/sys/xattr.h on my linux box. I do not see attr/xattr.h.
> 
> I'm happy to rely on the trybots' verdict.
> 
> > - The second unittest might not be needed. That scenario should not happen
in
> > Chrome since we always set the attribute on a newly created/downloaded file.
> 
> Since it's a unit test and not a browser test, it should be fast enough that
the
> extra coverage is worth the fraction of a second. Also, this is a lower-level
> library than chrome downloads, so it should not rely on behaviors of the
> downloads system, especially behaviors that may change when downloads
resumption
> is done.
> 
> > 
> > Also, if you are not the correct reviewer please point me to  the right one.
> > Thanks.
> 
> Looks good pending trybots.

I thought I could run the trybots for your cl directly, but it didn't work, so I
patched your CL into a fresh client and ran the bots from there:
https://chromiumcodereview.appspot.com/10790013

Looks like linux_rel and linux_chromeos have sys/xattr.h, but caught a
compile-time error.
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/3958...

Please also remove the blank lines at the ends of the files.

[benjhayden, 2012-07-16 20:24:21]

Please see https://chromiumcodereview.appspot.com/10790013 patch set 3 for one
way to avoid the compile error. I'm not sure if it's the best way.
With that fixed, it looks like DCHECK(referrer.valid()) fails on linux_rel for
most downloads tests.
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/3960...
You can probably fix this with if (referrer.valid())
SetExtendedFileAttribute(...);

[benjhayden, 2012-07-17 17:22:15]

Looks like this will also fail on ext file systems unless they're mounted with
-o user_xattr.
http://archlinux.2023198.n4.nabble.com/Cannot-set-xattr-td2038548.html
This happens on my work station, so it is likely to happen on other engineers'
work stations as well.
Perhaps the test should either mount a disk image or disable itself if the
filesystem does not support user_xattr?

[cristian.patrasciuc, 2012-07-17 18:04:50]

Is there a standard way to disable a test programatically?

Regarding the new lines at the end of the files, I added them because the Lint
column here on the code review site told me to do so.

I will upload a new patch set tomorrow once I'll figure out how to disable the
test automatically if extended attributes are not supported.

Thanks.

[benjhayden, 2012-07-17 18:49:51]

On 2012/07/17 18:04:50, cristian.patrasciuc wrote:
> Is there a standard way to disable a test programatically?
> 
> Regarding the new lines at the end of the files, I added them because the Lint
> column here on the code review site told me to do so.

That's interesting. I've always ignored that column because it's always shown
either 0 errors or ? errors.
I don't see anything in the style guide about blank lines at ends of files, so I
suppose you can keep them if you want.

> 
> I will upload a new patch set tomorrow once I'll figure out how to disable the
> test automatically if extended attributes are not supported.
> 
> Thanks.

Usually we disable tests on whole platforms using #if defined(OS_FOO), but since
we want to be more specific and dynamic here, I think you can just do this:
TEST_F(FileMetadataLinuxTest, CheckMetadataSetCorrectly) {
  if (IsXAttrDisabled()) {
    LOG(WARNING) << "Not testing file_metadata::AddOriginMetadataToFile()
because xattrs are not supported on this filesystem";
    return;
  }
  ...
}

[Randy Smith (Not in Mondays), 2012-07-18 18:12:38]

On 2012/07/17 18:49:51, benjhayden_chromium wrote:
> On 2012/07/17 18:04:50, cristian.patrasciuc wrote:
> > Is there a standard way to disable a test programatically?
> > 
> > Regarding the new lines at the end of the files, I added them because the
Lint
> > column here on the code review site told me to do so.
> 
> That's interesting. I've always ignored that column because it's always shown
> either 0 errors or ? errors.
> I don't see anything in the style guide about blank lines at ends of files, so
I
> suppose you can keep them if you want.

The style guide says to minimize vertical whitespace:

"This is more a principle than a rule: don't use blank lines when you don't have
to. In particular, don't put more than one or two blank lines between functions,
resist starting functions with a blank line, don't end functions with a blank
line, and be discriminating with your use of blank lines inside functions.

The basic principle is: The more code that fits on one screen, the easier it is
to follow and understand the control flow of the program. Of course, readability
can suffer from code being too dense as well as too spread out, so use your
judgement. But in general, minimize use of vertical whitespace."

> 
> > 
> > I will upload a new patch set tomorrow once I'll figure out how to disable
the
> > test automatically if extended attributes are not supported.
> > 
> > Thanks.
> 
> Usually we disable tests on whole platforms using #if defined(OS_FOO), but
since
> we want to be more specific and dynamic here, I think you can just do this:
> TEST_F(FileMetadataLinuxTest, CheckMetadataSetCorrectly) {
>   if (IsXAttrDisabled()) {
>     LOG(WARNING) << "Not testing file_metadata::AddOriginMetadataToFile()
> because xattrs are not supported on this filesystem";
>     return;
>   }
>   ...
> }

[cristian.patrasciuc, 2012-07-19 17:20:16]

I uploaded a new patch that should fix the above mentioned issues.

[benjhayden, 2012-07-19 19:56:02]

On 2012/07/19 17:20:16, cristian.patrasciuc wrote:
> I uploaded a new patch that should fix the above mentioned issues.

Fails on the linux_rel bot:
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/4059...
http://codereview.chromium.org/10790013/

[cristian.patrasciuc, 2012-07-19 21:11:51]

I didn't read carefully the man page of setxattr in case of disabled extended
attributes. It's the 'errno' variable that is set to ENOTSUP, not the return
value of the setxattr call.

I think that file systems on which the tests are run don't support xattr. The
last patch should fix this.

[cristian.patrasciuc, 2012-07-19 21:25:06]

I also have to specify a namespace for the test attribute, otherwise it will
always fail.

[benjhayden, 2012-07-20 17:20:04]

A couple nits. The trybots are almost done and looking good so far.
http://codereview.chromium.org/10790013/

http://codereview.chromium.org/10784007/diff/25007/content/browser/file_metad...
File content/browser/file_metadata_unittest_linux.cc (right):

http://codereview.chromium.org/10784007/diff/25007/content/browser/file_metad...
content/browser/file_metadata_unittest_linux.cc:88: ScopedTempDir temp_dir_;
Please keep member variables private and expose them with accessors as
necessary.

http://codereview.chromium.org/10784007/diff/25007/content/browser/file_metad...
content/browser/file_metadata_unittest_linux.cc:97: LOG(INFO) << "Test is
skipped because extended attributes are not "
Please put this message just once in SetUp instead of in each TEST_F.

[cristian.patrasciuc, 2012-07-23 09:29:58]

I added a new patch with the changes you requested.

[benjhayden, 2012-07-23 17:04:44]

lgtm

[commit-bot: I haz the power, 2012-07-23 17:04:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10784007...

[commit-bot: I haz the power, 2012-07-23 17:04:58]

Presubmit check for 10784007-18003 failed and returned exit status 1.

Running presubmit commit checks ...

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

Presubmit checks took 1.1s to calculate.

[benjhayden, 2012-07-23 17:08:49]

Darin, can we have a rubberstamp?

On 2012/07/23 17:04:58, I haz the power (commit-bot) wrote:
> Presubmit check for 10784007-18003 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     content
>     content/browser
> 
> Presubmit checks took 1.1s to calculate.

[darin (slow to review), 2012-07-24 05:00:04]

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
File content/browser/file_metadata_linux.h (right):

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
content/browser/file_metadata_linux.h:13: namespace file_metadata {
nit: why is this in content/browser/ and not content/browser/download/?

nit: why the namespace?  namespaces are supposed to match directory names (note:
we have a lot of violations of this rule in the source tree).

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
content/browser/file_metadata_linux.h:19: const char kSourceURLAttrName[] =
"user.xdg.origin.url";
nit: just declare these here, define them in the .cc file, and  export them
instead?  that way you don't end up duplicating these into each translation
unit.  sure, the linker will probably eliminate duplicate symbols, but why add
more work for the linker? :-)

[cristian.patrasciuc, 2012-07-24 11:20:42]

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
File content/browser/file_metadata_linux.h (right):

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
content/browser/file_metadata_linux.h:13: namespace file_metadata {
There is already an implementation for Mac OS, under
content/browser/file_metadata_mac.h/mm. I decided to put the linux
implementation in the same folder and to use the same namespace (i.e.
file_metadata). If it's required I can move them (both linux and mac
implementations) under content/browser/download in a separate CL. They are only
used in content/browser/download/base_file.cc so there should be no problem.

https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/fi...
content/browser/file_metadata_linux.h:19: const char kSourceURLAttrName[] =
"user.xdg.origin.url";
On 2012/07/24 05:00:04, darin wrote:
> nit: just declare these here, define them in the .cc file, and  export them
> instead?  that way you don't end up duplicating these into each translation
> unit.  sure, the linker will probably eliminate duplicate symbols, but why add
> more work for the linker? :-)

Done.

[darin (slow to review), 2012-07-24 20:18:44]

On Tue, Jul 24, 2012 at 4:20 AM, <cristian.patrasciuc@gmail.com> wrote:

>
> https://chromiumcodereview.**appspot.com/10784007/diff/**
>
18003/content/browser/file_**metadata_linux.h<https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/file_metadata_linux.h>
> File content/browser/file_metadata_**linux.h (right):
>
> https://chromiumcodereview.**appspot.com/10784007/diff/**
>
18003/content/browser/file_**metadata_linux.h#newcode13<https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/file_metadata_linux.h#newcode13>
> content/browser/file_metadata_**linux.h:13: namespace file_metadata {
> There is already an implementation for Mac OS, under
> content/browser/file_metadata_**mac.h/mm. I decided to put the linux
> implementation in the same folder and to use the same namespace (i.e.
> file_metadata). If it's required I can move them (both linux and mac
> implementations) under content/browser/download in a separate CL. They
> are only used in content/browser/download/base_**file.cc so there should
> be no problem.
>
> Yes, I think they should be moved.  You can move the Mac version as a
separate CL if you prefer.



>
> https://chromiumcodereview.**appspot.com/10784007/diff/**
>
18003/content/browser/file_**metadata_linux.h#newcode19<https://chromiumcodereview.appspot.com/10784007/diff/18003/content/browser/file_metadata_linux.h#newcode19>
> content/browser/file_metadata_**linux.h:19: const char
> kSourceURLAttrName[] = "user.xdg.origin.url";
> On 2012/07/24 05:00:04, darin wrote:
>
>> nit: just declare these here, define them in the .cc file, and  export
>>
> them
>
>> instead?  that way you don't end up duplicating these into each
>>
> translation
>
>> unit.  sure, the linker will probably eliminate duplicate symbols, but
>>
> why add
>
>> more work for the linker? :-)
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/10784007/<https://chromiumcodereview...
>

[cristian.patrasciuc, 2012-07-25 10:42:09]

I moved the files to content/browser/download. For the MAC-related ones I'll
create a separate CL after this one will be committed.

[darin (slow to review), 2012-07-25 18:25:18]

https://chromiumcodereview.appspot.com/10784007/diff/28001/content/browser/do...
File content/browser/download/file_metadata_linux.h (right):

https://chromiumcodereview.appspot.com/10784007/diff/28001/content/browser/do...
content/browser/download/file_metadata_linux.h:13: namespace file_metadata {
nit: this shouldn't be in its own namespace:

From the style guide:
"With named namespaces, choose the name based on the project, and possibly its
path."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

[cristian.patrasciuc, 2012-07-26 10:06:43]

Since there is no content::download namespace yet and the classes from
content/download are either in no namespace or in the "content" namespace, I
chose to put my function in the "content" namespace.

[darin (slow to review), 2012-07-26 18:47:00]

LGTM

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
File content/browser/download/file_metadata_unittest_linux.cc (right):

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
content/browser/download/file_metadata_unittest_linux.cc:22: namespace {
you can also put the test in the content namespace so you don't need to write
content:: in front of content bits.

namespace content {
namespace {

...

}  // namespace
}  // namespace content

[commit-bot: I haz the power, 2012-07-26 21:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10784007...

[commit-bot: I haz the power, 2012-07-26 21:47:29]

Try job failure for 10784007-29007 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-07-27 06:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10784007...

[commit-bot: I haz the power, 2012-07-27 08:03:56]

Change committed as 148729

[Paweł Hajdan Jr., 2012-08-29 16:17:14]

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
File content/browser/download/file_metadata_linux.cc (right):

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
content/browser/download/file_metadata_linux.cc:25: DPLOG(ERROR)
How about returning bool success value, and letting the caller handle it?

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
content/browser/download/file_metadata_linux.cc:32:
DCHECK(file_util::PathIsWritable(file));
Same here. You should return bool, and return false when path is not writable.

Same with "silent" error handling below. The rationale is that now this became a
part of the API, and once this has more callers, the problem becomes harder to
repair.

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
File content/browser/download/file_metadata_unittest_linux.cc (right):

https://chromiumcodereview.appspot.com/10784007/diff/29007/content/browser/do...
content/browser/download/file_metadata_unittest_linux.cc:117: if
(!is_xattr_supported()) return;
nit: Each statement should be on its own line. Please move the return to new
line, same below.