Move Mac LaunchOnStartup enable/disable to File thread

Follow pattern used for Linux and Windows to move the appropriate bits of Mac code to the file thread.

BUG=59265
TEST=none

This patch also splits platform-specific LaunchOnStartup code to platform-specific files, eliminating #if blocks from the primary source file.

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

[Andrew T Wilson (Slow), 2010-12-03 02:06:59]

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:249: new
EnableLaunchOnStartupTask(this));
See my comment about moving this entire statement down into the
platform-specific files.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:299: AddPreferencesItem(menu);
One other way to do this is to define:

string16 GetPreferencesMenuLabel()

and implement this on various platforms.

Then this code becomes:

menu->AddItem(IDC_OPTIONS, GetPreferencesMenuLabel());

I don't actually know if that's any cleaner, though, so what you have here is
just fine too.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:56: : manager_(manager) {
I'm really nervous about passing the BackgroundModeManager in to the task - it's
not a thread-safe object, and I suspect the APIs we're calling (which access
prefs) will fail if called from the file thread.

On a related note, I don't think that the Task object should be the interface
point for cross-platform behavior. For one reason, you end up having to pollute
the object with things like manager_ which isn't needed across all platforms
(for that matter, the Tasks themselves aren't really needed on chromeos since we
don't do anything).

I think a cleaner way to go is to define to instance methods:
DisableLaunchOnStartup() and EnableLaunchOnStartup(), and you can implement
those on each platform. This will let you run platform specific code (like
IsLaunchOnStartupResetAllowed()) on the correct thread, and also let you omit
the Tasks entirely if the platform doesn't need them for some reason (like
chromeos). The downside is the PostTask call needs to be duplicated on each
platform, but I don't think that's a big deal.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_chromeos.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:10: }
This code should never be called, so we should put NOTREACHED() calls in all
these APIs. See also my previous note about not having Task objects at all on
chromeos.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_linux.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:17: #include
"base/nix/xdg_util.h"
I think all of these header files (other than unistd.h) should be sorted and put
in a single group. So:

#include "chrome/browser/background_mode_manager.h"

#include "base/environement.h"
#include "base/file_path.h"
...etc...

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_mac.mm (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:13: if
(!manager_->IsLaunchOnStartupResetAllowed())
See my previous notes - I don't think we can make these calls from the FILE
thread.

[Dmitry Titov, 2010-12-03 04:23:27]

Sorry, got srsly sidetracked today...

Drew said what I wanted to say about calling profile APIs on the wrong thread...
In Mac case that "allowed" bit setting may require a second task posted back to
the original thread? plus some code to make sure there are no race conditions?
Sounds a bit complicated.

Perhaps better is to not use profile API for storing this bit, but rather use
OSX CFPreferencesCopyAppValue/CFPreferencesSetAppValue API directly on the file
thread to do the same. This will also make manager_ not needed in the Task in
Mac case.

[Andrew T Wilson (Slow), 2010-12-03 18:45:18]

On 2010/12/03 04:23:27, Dmitry Titov wrote:
> Sorry, got srsly sidetracked today...
> 
> Drew said what I wanted to say about calling profile APIs on the wrong
thread...
> In Mac case that "allowed" bit setting may require a second task posted back
to
> the original thread? plus some code to make sure there are no race conditions?
> Sounds a bit complicated.
> 
> Perhaps better is to not use profile API for storing this bit, but rather use
> OSX CFPreferencesCopyAppValue/CFPreferencesSetAppValue API directly on the
file
> thread to do the same. This will also make manager_ not needed in the Task in
> Mac case.

I think you can do all of the "access the profile preferences" stuff on the UI
thread before posting the task on the File thread to create/delete the
LoginItem. So I don't think we should have to do this.

[Dmitry Titov, 2010-12-03 19:11:44]

When it tries to set the Chrome as login item, it may bail if the check says
user already set it as such. Then it should not set the preference. So
setting the bit depends on result of operation on the file thread. So it's
either ship the result back using second Task (and it's race-prone) or just
set the bit right there on file thread. Makes sense?

On Fri, Dec 3, 2010 at 10:45 AM, <atwilson@chromium.org> wrote:

> On 2010/12/03 04:23:27, Dmitry Titov wrote:
>
>> Sorry, got srsly sidetracked today...
>>
>
>  Drew said what I wanted to say about calling profile APIs on the wrong
>>
> thread...
>
>> In Mac case that "allowed" bit setting may require a second task posted
>> back
>>
> to
>
>> the original thread? plus some code to make sure there are no race
>> conditions?
>> Sounds a bit complicated.
>>
>
>  Perhaps better is to not use profile API for storing this bit, but rather
>> use
>> OSX CFPreferencesCopyAppValue/CFPreferencesSetAppValue API directly on the
>>
> file
>
>> thread to do the same. This will also make manager_ not needed in the Task
>> in
>> Mac case.
>>
>
> I think you can do all of the "access the profile preferences" stuff on the
> UI
> thread before posting the task on the File thread to create/delete the
> LoginItem. So I don't think we should have to do this.
>
>
>
> http://codereview.chromium.org/5368002/
>

[Andrew T Wilson (Slow), 2010-12-03 19:13:52]

Ah, yeah, I see what you're saying now.

-atw

On Fri, Dec 3, 2010 at 11:11 AM, Dmitry Titov <dimich@chromium.org> wrote:

> When it tries to set the Chrome as login item, it may bail if the check
> says user already set it as such. Then it should not set the preference. So
> setting the bit depends on result of operation on the file thread. So it's
> either ship the result back using second Task (and it's race-prone) or just
> set the bit right there on file thread. Makes sense?
>
>
> On Fri, Dec 3, 2010 at 10:45 AM, <atwilson@chromium.org> wrote:
>
>> On 2010/12/03 04:23:27, Dmitry Titov wrote:
>>
>>> Sorry, got srsly sidetracked today...
>>>
>>
>>  Drew said what I wanted to say about calling profile APIs on the wrong
>>>
>> thread...
>>
>>> In Mac case that "allowed" bit setting may require a second task posted
>>> back
>>>
>> to
>>
>>> the original thread? plus some code to make sure there are no race
>>> conditions?
>>> Sounds a bit complicated.
>>>
>>
>>  Perhaps better is to not use profile API for storing this bit, but rather
>>> use
>>> OSX CFPreferencesCopyAppValue/CFPreferencesSetAppValue API directly on
>>> the
>>>
>> file
>>
>>> thread to do the same. This will also make manager_ not needed in the
>>> Task in
>>> Mac case.
>>>
>>
>> I think you can do all of the "access the profile preferences" stuff on
>> the UI
>> thread before posting the task on the File thread to create/delete the
>> LoginItem. So I don't think we should have to do this.
>>
>>
>>
>> http://codereview.chromium.org/5368002/
>>
>
>

[The wrong rickcam account, 2010-12-04 02:08:04]

Please take another look.  I've refactored differently as suggested by Drew, and
switched to using CFPreferencesSetAppValue et al as suggested by Dmitry.

I'm still doing countless iterations of logging out and back in to ensure that
what I saw on the Mac makes sense, but I believe that this change is ready to
go.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:249: new
EnableLaunchOnStartupTask(this));
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> See my comment about moving this entire statement down into the
> platform-specific files.

Done.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:299: AddPreferencesItem(menu);
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> One other way to do this is to define:
> 
> string16 GetPreferencesMenuLabel()
> 
> and implement this on various platforms.
> 
> Then this code becomes:
> 
> menu->AddItem(IDC_OPTIONS, GetPreferencesMenuLabel());
> 
> I don't actually know if that's any cleaner, though, so what you have here is
> just fine too.

Done.  I'd argue that yours is cleaner.  It makes the API about data rather than
side-effect.  More RESTful :-)

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:56: : manager_(manager) {
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> I'm really nervous about passing the BackgroundModeManager in to the task -
it's
> not a thread-safe object, and I suspect the APIs we're calling (which access
> prefs) will fail if called from the file thread.
> 
> On a related note, I don't think that the Task object should be the interface
> point for cross-platform behavior. For one reason, you end up having to
pollute
> the object with things like manager_ which isn't needed across all platforms
> (for that matter, the Tasks themselves aren't really needed on chromeos since
we
> don't do anything).
> 
> I think a cleaner way to go is to define to instance methods:
> DisableLaunchOnStartup() and EnableLaunchOnStartup(), and you can implement
> those on each platform. This will let you run platform specific code (like
> IsLaunchOnStartupResetAllowed()) on the correct thread, and also let you omit
> the Tasks entirely if the platform doesn't need them for some reason (like
> chromeos). The downside is the PostTask call needs to be duplicated on each
> platform, but I don't think that's a big deal.

Done.  I've pushed the EnableLaunchOnStartup definition down into the
platform-specific files, and then reorganized and cleaned-up those files.  A
positive side-effect is that I was also able to move the Task class definitions
out of the header file.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_chromeos.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:10: }
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> This code should never be called, so we should put NOTREACHED() calls in all
> these APIs. See also my previous note about not having Task objects at all on
> chromeos.

Done.  Eliminate this routine outright a la your previous note.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_linux.cc (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:17: #include
"base/nix/xdg_util.h"
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> I think all of these header files (other than unistd.h) should be sorted and
put
> in a single group. So:
> 
> #include "chrome/browser/background_mode_manager.h"
> 
> #include "base/environement.h"
> #include "base/file_path.h"
> ...etc...

Done.

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_mac.mm (right):

http://codereview.chromium.org/5368002/diff/14001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:13: if
(!manager_->IsLaunchOnStartupResetAllowed())
On 2010/12/03 02:06:59, Andrew T Wilson wrote:
> See my previous notes - I don't think we can make these calls from the FILE
> thread.

Done.  I've switched to use the family of routines that Dmitry suggested.

[Andrew T Wilson (Slow), 2010-12-04 18:49:48]

This is much cleaner after this refactoring - thanks for tackling this!

I have have a few nits below, and one serious concern about our use of
CFPreferences. If we decide to go back to storing that "can reset login item"
flag in preferences, we should chat about whether it should properly live in the
per-profile user preferences, or in the browser local state, because I'm not
positive that the previous version was entirely correct.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:11: #include "base/task.h"
Remove this #include.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_chromeos.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:6: #include
"grit/generated_resources.h"
Blank line between "#include 'chrome/browser/background_mode_manager.h" and
other includes.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:8: void
BackgroundModeManager::EnableLaunchOnStartup(bool should_launch) {
I'm torn about whether we should put a NOT_REACHED in here.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_linux.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:15: #include
"chrome/app/chrome_command_ids.h"
I don't think this is needed any more (chrome_command_ids)

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:28: virtual void Run();
BTW, one thing I sometimes do for these private classes is just put the
implementation inline, especially since there's only one method. It's OK as-is
though.

One other question - should these be put in an anonymous namespace?

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:73: LOG(WARNING) << "Failed to
deregister launch on login.";
I might add a NOT_REACHED here as I think we'd like to know if this is routinely
getting hit, whereas most people never look at their logs.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_mac.mm (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:8: #include
"chrome/app/chrome_command_ids.h"
can you remove chrome_command_ids.h?

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:29: Boolean
keyExistsAndHasValidFormat;  // ignored
should be hacker_style, not camelCase?

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:30: if
(CFPreferencesGetAppBooleanValue(kLaunchOnStartupResetAllowedPrefsKey,
I did some quick googling, and I didn't see anything that implies that these
CFPreferences APIs are thread safe, so I'm not positive it's OK to call them
from the file thread. I'd want you to run this by some of the mac folks
(pinkerton?) first - maybe we just want to make a new rule that these APIs can
only be called from the FILE thread, and create a wrapper function in mac_util
that ensures that the right thread is being used.

Failing that, I don't think it's the end of the world to send a message back to
the UI thread to set the "launchonstartupresetallowed" value in the profile
preferences.

Also, "Hey, aren't you inverting the sense of this check here?" :)

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_win.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_win.cc:13: #include
"chrome/app/chrome_command_ids.h"
Remove chrome_command_ids?

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_win.cc:77: LOG(WARNING) << "Failed to
register launch on login.";
NOT_REACHED here also?

[Andrew T Wilson (Slow), 2010-12-04 19:30:48]

Hmmm, one more thought about the NOT_REACHED suggestion - this might break unit
tests on chromeos (I don't think it will, but maybe), so if you add this make
sure to rerun through the chromeos trybots.

[The wrong rickcam account, 2010-12-07 22:59:57]

Please take another look:
 - header cleanup
 - additional anonymous namespaces
 - Replaced several LOGs with NOTREACHED.

NOTE: This keeps the CFPreferences calls following avi@'s assurance that they
are thread-safe.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:11: #include "base/task.h"
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> Remove this #include.

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_chromeos.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:6: #include
"grit/generated_resources.h"
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> Blank line between "#include 'chrome/browser/background_mode_manager.h" and
> other includes.

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_chromeos.cc:8: void
BackgroundModeManager::EnableLaunchOnStartup(bool should_launch) {
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> I'm torn about whether we should put a NOT_REACHED in here.

Done.

I thought about it and decided against with the idea that it would put
(unnecessary??) burden on BackgroundModeManager to avoid calling
EnableLaunchOnStartup at all.

Now that we've dug in a bit more on the details of NOTREACHED -- it's a DCHECK
-- I'm going to say that Yes, we should add it here.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_linux.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:15: #include
"chrome/app/chrome_command_ids.h"
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> I don't think this is needed any more (chrome_command_ids)

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:28: virtual void Run();
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> BTW, one thing I sometimes do for these private classes is just put the
> implementation inline, especially since there's only one method. It's OK as-is
> though.
> 
> One other question - should these be put in an anonymous namespace?

Two-part answer:

(1) I'd like to leave the definitions as is.  When there's only one member
function AND that definition is concise, I will also sometimes do this.  If
either one is meaty, I like to keep them separate.  In this case, there's the
additional bit where I think that it's a Good Thing to have Disable... and
Enable... follow the same patterns.  EnableLaunchOnStartupTask::Run, with a
30-line body, tips the scale for me.

(2) Yes.  I sprinked anonymous namespaces around the Task class definitions
throughout background_mode_manager*.*, expanding existing anonymous namespace
regions where it seemed appropriate.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_linux.cc:73: LOG(WARNING) << "Failed to
deregister launch on login.";
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> I might add a NOT_REACHED here as I think we'd like to know if this is
routinely
> getting hit, whereas most people never look at their logs.

Done.  I replaced LOG(...) with NOTREACHED() throughout
background_mode_manager*.*

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_mac.mm (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:8: #include
"chrome/app/chrome_command_ids.h"
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> can you remove chrome_command_ids.h?

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:29: Boolean
keyExistsAndHasValidFormat;  // ignored
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> should be hacker_style, not camelCase?

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_mac.mm:30: if
(CFPreferencesGetAppBooleanValue(kLaunchOnStartupResetAllowedPrefsKey,
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> I did some quick googling, and I didn't see anything that implies that these
> CFPreferences APIs are thread safe, so I'm not positive it's OK to call them
> from the file thread. I'd want you to run this by some of the mac folks
> (pinkerton?) first - maybe we just want to make a new rule that these APIs can
> only be called from the FILE thread, and create a wrapper function in mac_util
> that ensures that the right thread is being used.
> 
> Failing that, I don't think it's the end of the world to send a message back
to
> the UI thread to set the "launchonstartupresetallowed" value in the profile
> preferences.
> 
> Also, "Hey, aren't you inverting the sense of this check here?" :)

I've fixed the inverted test.

Based on email from avi@ affirming that they are thread-safe, and further
discussion with you about our options, I will leave this code using
CFPreferences in place.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_win.cc (right):

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_win.cc:13: #include
"chrome/app/chrome_command_ids.h"
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> Remove chrome_command_ids?

Done.

http://codereview.chromium.org/5368002/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_win.cc:77: LOG(WARNING) << "Failed to
register launch on login.";
On 2010/12/04 18:49:48, Andrew T Wilson wrote:
> NOT_REACHED here also?

Done.  Also got the one 23-lines earlier as part of a general sweep.

[Andrew T Wilson (Slow), 2010-12-08 01:53:30]

LGTM with one more required cleanup and one question:

1) Remove prefs::kLaunchOnStartupResetAllowed in pref_names.h/cc
2) Make sure the CFPreference stuff is really per-executable so we don't break
multi-channel support.