Install from disk image: Create persistent Dock icon

Install from disk image: Create persistent Dock icon.

When installing from the disk image, Chrome now places itself in the Dock,
as though the user had placed it there as a "persistent" tile. Users can reach
this feature by dragging an icon directly to the Dock or by choosing "Keep
in Dock" from an icon's contextual menu.

BUG=87623
TEST=Install from disk image. A new persistent Dock tile should be created
     under the conditions specified in the comment for install_from_dmg.mm
     AddDockIcon. If any changes are made to the Dock, the Dock should be
     restarted. The installed application should launch immediately following
     this.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91156

[Robert Sesek, 2011-06-29 18:26:42]

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
File chrome/browser/mac/install_from_dmg.mm (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:459: LOG(ERROR) << "Too many items in the
Dock";
srsly?

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:463: NSString* installed_path_dock =
[installed_path stringByAppendingString:@"/"];
I think I understand why this is necessary, but please comment.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:468: for (int index = 0; index < count;
++index) {
Pinkerton in the past has told me to use fast enumeration and keep a counter
instead of doing this. Take that as you will.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:471: // If the Dock already contains a
reference to the newly-installed
nit: don't hyphenate compound adjectives if the left component ends in 'ly';
here and other uses of newly-installed

[you've corrected my use of 'we' so many times I don't feel guilty :D]

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:499: if (app_index < 0) {
This case is confusing me; shouldn't app_index be set via line 480?

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:520: for (int index = 0; index < count;
++index) {
Same comment about fast enum.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.cc
File chrome/browser/mac/launchd.cc (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.cc#n...
chrome/browser/mac/launchd.cc:28: ScopedLaunchData
job_label_launchd(launch_data_new_string(job_label.c_str()));
The memory management here warrants a comment, I think.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.h
File chrome/browser/mac/launchd.h (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.h#ne...
chrome/browser/mac/launchd.h:16: int StartJob(const std::string& job_label);
You don't use these. Is there a point to landing them?

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/scoped_launc...
File chrome/browser/mac/scoped_launch_data.h (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/scoped_launc...
chrome/browser/mac/scoped_launch_data.h:19: typedef launch_data_t element_type;
What's this used for?

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/scoped_launc...
chrome/browser/mac/scoped_launch_data.h:55: launch_data_t temp = that.object_;
std::swap()?

[Mark Mentovai, 2011-06-29 19:29:29]

OK, ready.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
File chrome/browser/mac/install_from_dmg.mm (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:457: int count =
static_cast<int>([persistent_app_paths count]);
I only had “count” in a separate variable to eliminate some casts in the for
loops. Since I’ve changed the types around a little bit, this is no longer
necessary, so I got rid of it. I didn’t like it anyway. It seemed kind of
fragile to track the count separately. Ah, ah, ah.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:459: LOG(ERROR) << "Too many items in the
Dock";
rsesek wrote:
> srsly?

Only because I was being defensive about casting away signedness. I had used a
signed int so that I could use -1 as my special “didn’t find it” sentinel. I
changed strategy, switching to just using NSUInteger everywhere, with a special
kNotFound constant and == comparisons instead of < 0 comparisons.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:468: for (int index = 0; index < count;
++index) {
rsesek wrote:
> Pinkerton in the past has told me to use fast enumeration and keep a counter
> instead of doing this. Take that as you will.

I like fast enumeration and used it in PersistentAppPaths, but I’m going to veto
this suggestion. I think it’s more fragile to have to track two things
separately than to track one things and base the second strictly on the first.
What if someone adds a “continue” in the future?

It’s not like this is really performance-critical code.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:471: // If the Dock already contains a
reference to the newly-installed
rsesek wrote:
> nit: don't hyphenate compound adjectives if the left component ends in 'ly';
> here and other uses of newly-installed
> 
> [you've corrected my use of 'we' so many times I don't feel guilty :D]

:)

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/install_from...
chrome/browser/mac/install_from_dmg.mm:499: if (app_index < 0) {
rsesek wrote:
> This case is confusing me; shouldn't app_index be set via line 480?

480 is only effective if the Dock already has a reference to the application on
the disk image. 480 covers the case where the user drags the icon into the Dock
from the dmg volume window in the Finder and then clicked it. I don’t want to
leave any icons pointing to the disk image around.

Normal users don’t do silly things like that. Think like a new user for a
second.

I’ve expanded the comment at 480 to explain this better.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.h
File chrome/browser/mac/launchd.h (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/launchd.h#ne...
chrome/browser/mac/launchd.h:16: int StartJob(const std::string& job_label);
rsesek wrote:
> You don't use these. Is there a point to landing them?

No, I was going to take them out anyway, but I figured I’d let you comment
because you got upset last time I uploaded a new change set in the middle of
your review.

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/scoped_launc...
File chrome/browser/mac/scoped_launch_data.h (right):

http://codereview.chromium.org/7273052/diff/1/chrome/browser/mac/scoped_launc...
chrome/browser/mac/scoped_launch_data.h:19: typedef launch_data_t element_type;
rsesek wrote:
> What's this used for?

All of the scopers have it.

It‘s only helpful if the scoper itself winds up inside a template, but it’s
handy in those cases.

[Robert Sesek, 2011-06-30 15:49:52]

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
File chrome/browser/mac/install_from_dmg.mm (right):

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
chrome/browser/mac/install_from_dmg.mm:486: const NSUInteger kNotFound = -1;
Why not use NSNotFound?

[Mark Mentovai, 2011-06-30 15:53:58]

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
File chrome/browser/mac/install_from_dmg.mm (right):

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
chrome/browser/mac/install_from_dmg.mm:486: const NSUInteger kNotFound = -1;
rsesek wrote:
> Why not use NSNotFound?

Because that’s LONG_MAX or 0x7fffffff (32-bit).

[Robert Sesek, 2011-06-30 15:54:34]

LGTM

[Mark Mentovai, 2011-06-30 16:02:41]

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
File chrome/browser/mac/install_from_dmg.mm (right):

http://codereview.chromium.org/7273052/diff/11008/chrome/browser/mac/install_...
chrome/browser/mac/install_from_dmg.mm:486: const NSUInteger kNotFound = -1;
I wrote:
> rsesek wrote:
> > Why not use NSNotFound?
> 
> Because that’s LONG_MAX or 0x7fffffff (32-bit).

But that doesn’t matter, I guess I just had -1 in mind as my magic value. I’ll
use NSNotFound.