Respect the SaveZoneInformation policy when marking downloaded files as unsaf...

Respect the SaveZoneInformation policy when marking downloaded files as unsafe.

BUG=5719
TEST=Enable zone marking, download file, verify that it's marked unsafe. Disable zone marking, download file, verify that it is unmarked.

[Nico, 2010-02-08 21:39:33]

Passed the try servers. Cc'd people, can one of you take a look? CL's pretty
short.

[abarth-chromium, 2010-02-08 21:42:52]

I'll be able to look at this later this afternoon if someone doesn't get back to
you first.

[Catalin Patulea, 2010-02-08 21:43:36]

download_uitest.cc failed on Windows.. (it didn't find the zone info)

I'm not exactly sure why that is, perhaps the policy to not store it happens to
be set on the slaves, but I'm planning on looking at this a bit later and adding
tests for the policy both enabled and disabled.

[abarth-chromium, 2010-02-09 02:02:03]

This looks ok to me, but I'm not an expert on the COM part (which is likely the
tricky party).  Maybe ask nsylvain or cpu to review that part?

[Catalin Patulea, 2010-02-09 02:12:01]

I forgot to include a link to the build which thakis started for me and which
shows the failing tests:
http://build.chromium.org/buildbot/try-server/builders/win/builds/17818

On 2010/02/09 02:02:03, abarth wrote:
> tricky party).  Maybe ask nsylvain or cpu to review that part?
Ok, I will see if I can catch one of them on IRC.

I was giving more thought to the testing.. setting/unsetting the policy means
making registry changes on the slaves, most likely under HKEY_CURRENT_USER.
There is the potential for leaving the changes around if the test crashes
(unlikely, but not impossible). Is anything being done on the test bots to
restore the registry to a clean slate before starting tests?

[Glenn Wilson, 2010-02-09 02:24:49]

On 2010/02/09 02:12:01, Catalin Patulea wrote:
> I forgot to include a link to the build which thakis started for me and which
> shows the failing tests:
> http://build.chromium.org/buildbot/try-server/builders/win/builds/17818
> 
> On 2010/02/09 02:02:03, abarth wrote:
> > tricky party).  Maybe ask nsylvain or cpu to review that part?
> Ok, I will see if I can catch one of them on IRC.
> 
> I was giving more thought to the testing.. setting/unsetting the policy means
> making registry changes on the slaves, most likely under HKEY_CURRENT_USER.
> There is the potential for leaving the changes around if the test crashes
> (unlikely, but not impossible). Is anything being done on the test bots to
> restore the registry to a clean slate before starting tests?

I've had a patch I've been trying to land for a while that modifies registry
keys in HKCU and HKLM, but keeps failing (on XP only).  The trybots seem to
allow key accesses but the builders don't, so look out for that if you write a
unit test.

[Catalin Patulea, 2010-02-09 02:56:59]

On 2010/02/09 02:24:49, Glenn Wilson wrote:
> I've had a patch I've been trying to land for a while that modifies registry
Is that patch on codereview anywhere?

One way to look at this is that, since Chrome itself is no longer adding the
zone marker, we would actually be testing the OS, which is outside the scope of
the unittest. Chrome's responsibility to call SetSource, Save, etc could be
tested using a mock AttachmentServices. Does that sound okay? This avoids
registry modification issues, etc.

Any particularly good example of dependency injection I could take a look at?

[Glenn Wilson, 2010-02-09 15:15:59]

On 2010/02/09 02:56:59, Catalin Patulea wrote:
> On 2010/02/09 02:24:49, Glenn Wilson wrote:
> > I've had a patch I've been trying to land for a while that modifies registry
> Is that patch on codereview anywhere?

The fourth iteration is here:
http://codereview.chromium.org/576011/show

Sounds like you don't have to worry about this issue, though.

> 
> One way to look at this is that, since Chrome itself is no longer adding the
> zone marker, we would actually be testing the OS, which is outside the scope
of
> the unittest. Chrome's responsibility to call SetSource, Save, etc could be
> tested using a mock AttachmentServices. Does that sound okay? This avoids
> registry modification issues, etc.
> 
> Any particularly good example of dependency injection I could take a look at?