Added AppleScript support

[pink, 2010-08-03 15:57:57]

Andy, can you give this CL a quick once-over? I've been staring at the multiple
iterations for a while now and I want to make sure I didn't miss anything.

[Andrew Bonventre (Bons), 2010-08-03 16:18:39]

No problem.

On Tue, Aug 3, 2010 at 8:57 AM,  <pinkerton@chromium.org> wrote:
> Andy, can you give this CL a quick once-over? I've been staring at the
> multiple
> iterations for a while now and I want to make sure I didn't miss anything.
>
> http://codereview.chromium.org/3046042/show
>

[Andrew Bonventre (Bons), 2010-08-03 18:08:23]

A lot of my comments are stylistic in nature (method naming to be more
consistent with current Cocoa APIs).

One thing I noticed a lack of scoped_nsobjects and the use of release in the
dealloc. Generally we try to stay away from release and use the scopers in place
of this.

Lastly, this is great work. The fact that I'm only nitpicking on the two above
issues is only a testament to the fact that there wasn't too much else to
complain about ;)

http://codereview.chromium.org/3046042/diff/13001/14002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3046042/diff/13001/14002#newcode7234
chrome/app/generated_resources.grd:7234: <message
name="IDS_GET_PROFILE_ERROR_APPLESCRIPT_MAC" desc="Error dialog title to be
displayed when profile fails to load.  Mac-only.">
Is 'Mac-only' needed for the translators in all these descriptions?

http://codereview.chromium.org/3046042/diff/13001/14003
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/3046042/diff/13001/14003#newcode1
chrome/browser/bookmarks/bookmark_model.h:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
We update the copyright year to the last year the file has been modified, so you
can just say Copyright (c) 2010 The Chromium....

http://codereview.chromium.org/3046042/diff/13001/14004
File chrome/browser/browser.h (right):

http://codereview.chromium.org/3046042/diff/13001/14004#newcode284
chrome/browser/browser.h:284: // The browser is in the process of closing?
No questions in comments. Will this always be true if the browser is attempting
to close? What constitutes 'attempting'?

http://codereview.chromium.org/3046042/diff/13001/14005
File chrome/browser/cocoa/applescript/bookmark_folder_applescript.h (right):

http://codereview.chromium.org/3046042/diff/13001/14005#newcode22
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:22: // Insert a
bookmark folder at the end.
Comments are preferred to be in the third person declarative, so method docs
should be something like "Inserts a bookmark folder at the end." Same is true
for the methods below.

Also, to be consistent with NSMutableArray, it seems like
- (void)addBookmarkFolder:(id)aBookmarkFolder is more appropriate.

http://codereview.chromium.org/3046042/diff/13001/14005#newcode28
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:28: -
(void)insertInBookmarkFolders:(id)aBookmarkFolder atIndex:(int)index;
insert in bookmark folders makes it seem like you're inserting the object within
the folders themselves, although I know your intention. I would rename to

insertBookmarkFolder and follow that convention below as well.

Also, prefer NSUInteger to int in these cases, since index should never be
negative. Here and below.

http://codereview.chromium.org/3046042/diff/13001/14005#newcode36
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:36: // Returns an
array of |BookmarkItemAppleScript*| of all the bookmark items
an _immutable_ array, which is important to note.

http://codereview.chromium.org/3046042/diff/13001/14006
File chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm (right):

http://codereview.chromium.org/3046042/diff/13001/14006#newcode13
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:13: #include
"chrome/browser/profile.h"
do you use Profile at all? is this include needed?

http://codereview.chromium.org/3046042/diff/13001/14006#newcode25
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:25: if
(node->is_folder()) {
to avoid excessive nesting, you could simply say
if (!node->is_folder())
  continue;

and then have the block below be on the same level.

http://codereview.chromium.org/3046042/diff/13001/14006#newcode44
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:44: if (!model)
{
no need for curly brackety thingies since it's a one-liner.

http://codereview.chromium.org/3046042/diff/13001/14006#newcode59
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:59: // This
method gets called when a new bookmark folder is created so
You say this above as well. Do you mean that the method is normally called after
a new bookmark folder object is created and then added to the model?

It seems like you can combine a lot of these two types of functions. For the
instance of simply adding a folder to the end (in the above method), why not
just pass the object to this function with the index being one more than the
last item?

http://codereview.chromium.org/3046042/diff/13001/14007
File chrome/browser/cocoa/applescript/bookmark_item_applescript.h (right):

http://codereview.chromium.org/3046042/diff/13001/14007#newcode10
chrome/browser/cocoa/applescript/bookmark_item_applescript.h:10: #include
"base/scoped_nsobject.h"
i don't see any scopers in here.

http://codereview.chromium.org/3046042/diff/13001/14007#newcode19
chrome/browser/cocoa/applescript/bookmark_item_applescript.h:19: NSString*
tempURL_;
this should be a scoped_nsobject it looks like since you release it within the
dealloc method.

http://codereview.chromium.org/3046042/diff/13001/14008
File chrome/browser/cocoa/applescript/bookmark_item_applescript.mm (right):

http://codereview.chromium.org/3046042/diff/13001/14008#newcode28
chrome/browser/cocoa/applescript/bookmark_item_applescript.mm:28: [tempURL_
release];
red flag. we try to avoid release if at all possible. see my comment in the
header about placing this within a scoped_nsobject.

http://codereview.chromium.org/3046042/diff/13001/14010
File chrome/browser/cocoa/applescript/bookmark_node_applescript.mm (right):

http://codereview.chromium.org/3046042/diff/13001/14010#newcode14
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:14: #include
"chrome/browser/profile_manager.h"
I think you only need profile.h in this case, but I could be mistaken.

http://codereview.chromium.org/3046042/diff/13001/14010#newcode17
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:17: @interface
BookmarkNodeAppleScript()
label this as (Private) instead of empty.

http://codereview.chromium.org/3046042/diff/13001/14010#newcode43
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:43: [tempTitle_
release];
another red flag. place tempTitle in a scoper.

http://codereview.chromium.org/3046042/diff/13001/14010#newcode50
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:50: [self
release];
have you tested this? you're calling release before even having a chance to call
init.

http://codereview.chromium.org/3046042/diff/13001/14010#newcode61
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:61:
scoped_nsobject<NSNumber>  numID(
extra space between <NSNumber> and numID

http://codereview.chromium.org/3046042/diff/13001/14011
File chrome/browser/cocoa/applescript/browsercrapplication+applescript.h
(right):

http://codereview.chromium.org/3046042/diff/13001/14011#newcode23
chrome/browser/cocoa/applescript/browsercrapplication+applescript.h:23: //
Insert a window at the beginning.
beginning? this seems to contradict the behavior of the bookmark interface which
always inserts at the end unless an index is provided.

http://codereview.chromium.org/3046042/diff/13001/14011#newcode29
chrome/browser/cocoa/applescript/browsercrapplication+applescript.h:29: -
(void)insertInAppleScriptWindows:(WindowAppleScript*)aWindow
same comments as with the bookmark stuff.

http://codereview.chromium.org/3046042/diff/13001/14016
File chrome/browser/cocoa/applescript/element_applescript.m (right):

http://codereview.chromium.org/3046042/diff/13001/14016#newcode1
chrome/browser/cocoa/applescript/element_applescript.m:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
this file should have the .mm extension.

http://codereview.chromium.org/3046042/diff/13001/14016#newcode13
chrome/browser/cocoa/applescript/element_applescript.m:13: // calling
objectSpecifier asks an object to return an object specifier
Capitalize.

http://codereview.chromium.org/3046042/diff/13001/14016#newcode32
chrome/browser/cocoa/applescript/element_applescript.m:32: [uniqueID_ release];
scopers?

http://codereview.chromium.org/3046042/diff/13001/14026
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/3046042/diff/13001/14026#newcode1
chrome/chrome_exe.gypi:1: # Copyright (c) 2009-2010 The Chromium Authors. All
rights reserved.
2010 only.

[shreyas, 2010-08-03 19:12:37]

http://codereview.chromium.org/3046042/diff/13001/14002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3046042/diff/13001/14002#newcode7234
chrome/app/generated_resources.grd:7234: <message
name="IDS_GET_PROFILE_ERROR_APPLESCRIPT_MAC" desc="Error dialog title to be
displayed when profile fails to load.  Mac-only.">
Many of the other string that are "mac-only" were using this, i wanted to be
consistent.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> Is 'Mac-only' needed for the translators in all these descriptions?

http://codereview.chromium.org/3046042/diff/13001/14004
File chrome/browser/browser.h (right):

http://codereview.chromium.org/3046042/diff/13001/14004#newcode284
chrome/browser/browser.h:284: // The browser is in the process of closing?
i think ill just remove the comment, its well explained in the comment above the
instance variable.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> No questions in comments. Will this always be true if the browser is
attempting
> to close? What constitutes 'attempting'?

http://codereview.chromium.org/3046042/diff/13001/14005
File chrome/browser/cocoa/applescript/bookmark_folder_applescript.h (right):

http://codereview.chromium.org/3046042/diff/13001/14005#newcode22
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:22: // Insert a
bookmark folder at the end.
Sadly this is one of those methods that is used by AppleScript runtime directly
and cannot be renamed.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> Comments are preferred to be in the third person declarative, so method docs
> should be something like "Inserts a bookmark folder at the end." Same is true
> for the methods below.
> 
> Also, to be consistent with NSMutableArray, it seems like
> - (void)addBookmarkFolder:(id)aBookmarkFolder is more appropriate.

http://codereview.chromium.org/3046042/diff/13001/14005#newcode28
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:28: -
(void)insertInBookmarkFolders:(id)aBookmarkFolder atIndex:(int)index;
Same comment as above, indexes can never be negative, i have mentioned this in
the comment
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> insert in bookmark folders makes it seem like you're inserting the object
within
> the folders themselves, although I know your intention. I would rename to
> 
> insertBookmarkFolder and follow that convention below as well.
> 
> Also, prefer NSUInteger to int in these cases, since index should never be
> negative. Here and below.

http://codereview.chromium.org/3046042/diff/13001/14005#newcode36
chrome/browser/cocoa/applescript/bookmark_folder_applescript.h:36: // Returns an
array of |BookmarkItemAppleScript*| of all the bookmark items
Actually the internal implementation returns an NSMutableArray, but doesn't the
return type specify this?
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> an _immutable_ array, which is important to note.

http://codereview.chromium.org/3046042/diff/13001/14006
File chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm (right):

http://codereview.chromium.org/3046042/diff/13001/14006#newcode13
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:13: #include
"chrome/browser/profile.h"
Changed/Deleted.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> do you use Profile at all? is this include needed?

http://codereview.chromium.org/3046042/diff/13001/14006#newcode59
chrome/browser/cocoa/applescript/bookmark_folder_applescript.mm:59: // This
method gets called when a new bookmark folder is created so
That was how it was initially, but i have mentioned in the header that perform
bound checking before calling directly, 1 more than end is out of bounds.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> You say this above as well. Do you mean that the method is normally called
after
> a new bookmark folder object is created and then added to the model?
> 
> It seems like you can combine a lot of these two types of functions. For the
> instance of simply adding a folder to the end (in the above method), why not
> just pass the object to this function with the index being one more than the
> last item?

http://codereview.chromium.org/3046042/diff/13001/14010
File chrome/browser/cocoa/applescript/bookmark_node_applescript.mm (right):

http://codereview.chromium.org/3046042/diff/13001/14010#newcode17
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:17: @interface
BookmarkNodeAppleScript()
u cant do that since @property should be in a class extension and not in a
category
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> label this as (Private) instead of empty.

http://codereview.chromium.org/3046042/diff/13001/14010#newcode50
chrome/browser/cocoa/applescript/bookmark_node_applescript.mm:50: [self
release];
Yes i have tested this, and it is pretty standard convention to release an
object if something goes wrong during the init process.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> have you tested this? you're calling release before even having a chance to
call
> init.

http://codereview.chromium.org/3046042/diff/13001/14011
File chrome/browser/cocoa/applescript/browsercrapplication+applescript.h
(right):

http://codereview.chromium.org/3046042/diff/13001/14011#newcode23
chrome/browser/cocoa/applescript/browsercrapplication+applescript.h:23: //
Insert a window at the beginning.
If you look at the UI, when u add a bookmark its always added at the end,
whereas a new window is created in front of the present one, tried to mimic
this.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> beginning? this seems to contradict the behavior of the bookmark interface
which
> always inserts at the end unless an index is provided.

http://codereview.chromium.org/3046042/diff/13001/14016
File chrome/browser/cocoa/applescript/element_applescript.m (right):

http://codereview.chromium.org/3046042/diff/13001/14016#newcode1
chrome/browser/cocoa/applescript/element_applescript.m:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
why? i dont use any objective-c++ only objective-c
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> this file should have the .mm extension.

http://codereview.chromium.org/3046042/diff/13001/14016#newcode32
chrome/browser/cocoa/applescript/element_applescript.m:32: [uniqueID_ release];
i could add scoped_nsobect but i again would have to add the setters and getters
and deal with self assignment, retain counts n all that, y not let the compiler
synthesize it, in trying to save a dealloc methods we end up with lot of getters
and setters, final call is yours.
On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> scopers?

[Andrew Bonventre (Bons), 2010-08-03 21:15:42]

LGTM

http://codereview.chromium.org/3046042/diff/13001/14016
File chrome/browser/cocoa/applescript/element_applescript.m (right):

http://codereview.chromium.org/3046042/diff/13001/14016#newcode1
chrome/browser/cocoa/applescript/element_applescript.m:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
On 2010/08/03 19:12:37, shreyas wrote:
> why? i dont use any objective-c++ only objective-c
> On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> > this file should have the .mm extension.
> 
> 

Just convention. We don't have any other .m's within chrome except from
third-party stuff.

http://codereview.chromium.org/3046042/diff/13001/14016#newcode32
chrome/browser/cocoa/applescript/element_applescript.m:32: [uniqueID_ release];
On 2010/08/03 19:12:37, shreyas wrote:
> i could add scoped_nsobect but i again would have to add the setters and
getters
> and deal with self assignment, retain counts n all that, y not let the
compiler
> synthesize it, in trying to save a dealloc methods we end up with lot of
getters
> and setters, final call is yours.
> On 2010/08/03 18:08:23, Andrew Bonventre (Bons) wrote:
> > scopers?
> 
> 

No problem. Spoke with pink about this and non-scopers are fine with me.