Do not allow multiline input when naming bookmarks/folders on Mac

Do not allow multiline input when naming bookmark folders on Mac

- the respective nibs now use Single Line Mode for title entry
- BookmarkModel has been updated (with test cases) to strip extra whitespace for folder/bookmark titles.

BUG=100618
TEST=BookmarkModelTest.AddURLWithWhitespaceTitle, BookmarkModelTest.AddFolderWithWhitespaceTitle, BookmarkModelTest.SetTitleWithWhitespace

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

[KushalP, 2011-11-18 16:33:52]

The code in its current state compiles fine and the tests work locally.

However, when kicking off out/Debug/Chromium.app locally, the bug isn't actually
fixed ⊙﹏⊙

The nib in chrome/app/nib/BookmarkNameFolder.xib says that the file owner is
BookmarkNameFolderController, which these changes are made in.

What have I missed? And what are the tests missing?

[KushalP, 2011-11-18 17:14:31]

After a little thought, I think it may be because I'm only checking for "\n" and
there are likely to be other newline characters that I'm missing (unicode).

Hold off on providing me with the answer just yet if you have one at the ready
please!

[mrossetti, 2011-11-18 18:01:34]

Okay, no answer, just a hint: Have you considered making a change in the nib?

On 2011/11/18 17:14:31, KushalP wrote:
> After a little thought, I think it may be because I'm only checking for "\n"
and
> there are likely to be other newline characters that I'm missing (unicode).
> 
> Hold off on providing me with the answer just yet if you have one at the ready
> please!

[sky, 2011-11-18 19:01:18]

I've removed myself as a reviewer. Ilya or mrosetti should be good enough
(mrosetti is an OWNER here).

[Ilya Sherman, 2011-11-18 22:51:28]

http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm
(right):

http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm:114:
model->SetTitle(node_, base::SysNSStringToUTF16(clean_name));
Since I don't think our other platforms can display multi-line titles either, it
probably makes sense to do the cleanup within the model, in both SetTitle and
AddFolder.  You might then be able to use one of the methods in
base/string_util.h to clean up the whitespace, e.g. the CollapseWhitespace()
method.

[KushalP, 2011-11-19 11:24:38]

I thought about that, but then if you change it at the nib level it prevents
users from copy-pasting their preferred name into the box.

Rather than flat out preventing the copy-paste, I thought post-processing the
string input was kinder to the user.

On 2011/11/18 18:01:34, mrossetti wrote:
> Okay, no answer, just a hint: Have you considered making a change in the nib?

[KushalP, 2011-11-19 11:35:13]

Sure. I've got a working solution now for the cocoa version. The problem was
that unicode newlines weren't being removed.

On 2011/11/18 22:51:28, Ilya Sherman wrote:
>
http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
> File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm
> (right):
> 
>
http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
> chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm:114:
> model->SetTitle(node_, base::SysNSStringToUTF16(clean_name));
> Since I don't think our other platforms can display multi-line titles either,
it
> probably makes sense to do the cleanup within the model, in both SetTitle and
> AddFolder.  You might then be able to use one of the methods in
> base/string_util.h to clean up the whitespace, e.g. the CollapseWhitespace()
> method.

[KushalP, 2011-11-19 11:39:20]

Also, I didn't receive emails for the two replies from mrossetti and sky. I only
got an email for Ilya's response. Not sure what's going on there.

On 2011/11/19 11:35:13, KushalP wrote:
> Sure. I've got a working solution now for the cocoa version. The problem was
> that unicode newlines weren't being removed.
> 
> On 2011/11/18 22:51:28, Ilya Sherman wrote:
> >
>
http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
> > File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm
> > (right):
> > 
> >
>
http://codereview.chromium.org/8598015/diff/1/chrome/browser/ui/cocoa/bookmar...
> > chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm:114:
> > model->SetTitle(node_, base::SysNSStringToUTF16(clean_name));
> > Since I don't think our other platforms can display multi-line titles
either,
> it
> > probably makes sense to do the cleanup within the model, in both SetTitle
and
> > AddFolder.  You might then be able to use one of the methods in
> > base/string_util.h to clean up the whitespace, e.g. the CollapseWhitespace()
> > method.

[KushalP, 2011-11-19 18:29:12]

Code updated and it's currently running through the trybots. Ready for a review.

[KushalP, 2011-11-20 00:31:15]

Just looked at the Win failure., I'm not entirely sure if that's me. The
whitespace inconsistency makes me think it is, but it's pretty late right now
and I'm not in a good state to check the logs/code to see what's wrong.

Will look tomorrow or later in the week when I get a chance.

[KushalP, 2011-11-21 16:00:40]

Just had a look at the Win failure.

I don't think that failure is due to me.

[mrossetti, 2011-11-21 18:24:20]

Did you _try_ changing the nib by turning on Uses Single Line Mode for the text
fields?

In my experiments (with the BookmarkEditor.xib), setting that mode 1) prevents
the manual entry of line breaks and 2) replaces line breaks with spaces when
pasted. Please let me know if your experimentation shows a different behavior.

There is one issue remaining, though: If the folder name _already_ has line
breaks in it then after changing the nib the name of the folder will appear
squished together (no spacing) in the edit dialog (but appears fine in the
bookmark bar with spacing where the line breaks would be).

[mrossetti, 2011-11-21 18:30:13]

One more question: Should we also be stripping CR/LFs from regular bookmarks,
too?

BTW, with my earlier comments I'm not suggesting that you don't also make the
changes to SetTitle (which look quite reasonable to me) but that you do make the
change to the several text fields for editing bookmark and folder names. This is
so that the user, when pasting, doesn't see one thing that then changes
magically after the OK button is pressed — they see the change immediately.

[KushalP, 2011-11-21 18:34:27]

On 2011/11/21 18:30:13, mrossetti wrote:
> One more question: Should we also be stripping CR/LFs from regular bookmarks,
> too?

I would assume so. Otherwise there would be differences in how users can
approach naming their bookmarks and folders. This may lead to confusion. It's
currently possible to add a multiline bookmark on the current stable Mac — just
tested.

> This is so that the user, when pasting, doesn't see one thing that then
changes
> magically after the OK button is pressed — they see the change immediately.

That makes a lot more sense. Thanks!

[KushalP, 2011-11-21 21:11:36]

On 2011/11/21 18:24:20, mrossetti wrote:
> Did you _try_ changing the nib by turning on Uses Single Line Mode for the
text
> fields?

Just tested it — takes a while to compile Chromium on my MBP.

If you toggle "Single line mode" on IB, you can't have newlines (expected) and
when you paste in a string with newlines, it converts them to spaces. Multiple
newlines are converted into multiple spaces.

My change to AddFolder and SetTitle will convert multiple spaces to single
spaces.

[Ilya Sherman, 2011-11-21 21:36:10]

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:310: // Remove line break whitespace
from Folder names.
Hmm, why do we only want to clean up /folder/ names?

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:18: #include "base/string_util.h"
nit: Please move this #include to the .cc file.

[KushalP, 2011-11-21 22:14:14]

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8598015/diff/7002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:310: // Remove line break whitespace
from Folder names.
On 2011/11/21 21:36:10, Ilya Sherman wrote:
> Hmm, why do we only want to clean up /folder/ names?

This change was made previous to the discussions with mrossetti. Updating now.

[KushalP, 2011-11-22 00:57:59]

@Ilya your previous comments (from Patch Set 2) have been handled in Patch Set
3.

It's currently running through the trybots.

[Ilya Sherman, 2011-11-22 01:18:16]

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model.cc:309: // Remove extra whitespace from
Folder/Bookmark names.
nit: No need to capitalize "folder" or "bookmark" here.

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:265:
TEST_F(BookmarkModelTest, AddFolderWithNewLine) {
There are a bunch more things that could be tested:

(1) Adding URLs (regular bookmarks) rather than folders.
(2) Renaming the bookmark/folder after it has been added.
(3) Other whitespace: leading whitespace, trailing whitespace, multiple
consecutive spaces, tab characters, etc.

Rather than create lots of test functions for these, I would recommend creating
a single "TitleWithWhitespace" (or something like that) test.  One common
pattern in tests with lots of niggly sub-cases like this is to create an array
mapping inputs to expectations, e.g.

struct {
  string16 input_title;
  string16 expected_title;
} test_cases[] = {
  {"foobar", "foobar"},
  {"foo\nbar", "foo bar"},
  {"foo\n\nbar", "foo bar"},
  /* Add more test cases here */
};

and then iterate over that array (if you do this, the ARRAYSIZE_UNSAFE macro
will probably be helpful).

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:275:
ASSERT_EQ(BookmarkNode::FOLDER, new_node->type());
nit: I know the style is wrong in other tests in this file, but these should all
be EXPECT_EQ rather than ASSERT_EQ.  ASSERT* should only be used when EXPECT*
won't suffice, e.g. if the test would crash were the assertion to fail.

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:279: new_node->id() !=
model_.synced_node()->id());
nit: No need for this EXPECT_TRUE stmt in all of the added tests -- it's already
tested for generic bookmark additions, and it's not so relevant to the newline
behavior.

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm
(right):

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.mm:93: 
nit: Since you're not making any other changes in this file, please revert this
change.

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller_unittest.mm
(right):

http://codereview.chromium.org/8598015/diff/15002/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller_unittest.mm:191:
}
It looks to me like this is simply testing the changes to the model.  If so,
let's just test those with the other set of unit tests -- no need to test the
same thing twice.

[KushalP, 2011-11-22 18:52:15]

New Patch Set uploaded and it's currently running through the trybots. Ready to
review when you're ready.

@Ilya I've covered all of your nits/changes in this Patch Set.

[KushalP, 2011-11-22 19:27:25]

Just rebased (no real changes to patch sets) to LKGR. Hopefully that will fix
the gl_context_mac.cc missing error that the Mac bot is showing.

[mrossetti, 2011-11-22 19:33:33]

On 2011/11/22 19:27:25, KushalP wrote:
> Just rebased (no real changes to patch sets) to LKGR. Hopefully that will fix
> the gl_context_mac.cc missing error that the Mac bot is showing.

FYI, there was a thread on this earlier:
http://groups.google.com/a/chromium.org/group/chromium-dev/t/fb7cfbd703a09701

In the meantime, I'll review.

[Ilya Sherman, 2011-11-22 21:00:35]

Bookmarks model changes l g t m (with a nit).  For the Xib changes, have you
resolved the platform target warning?

http://codereview.chromium.org/8598015/diff/26001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/8598015/diff/26001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:71: };
nit: Since you're declaring this outside of the test functions, please rename
this to something more specific, e.g. |whitespace_test_cases|

[mrossetti, 2011-11-22 21:29:18]

LGTM, one the bots are clear.

Note: I'm fine with the nib file platform warning. The "uses single line mode"
setting will just have no effect under 10.5 but is valuable under 10.6+ to
eliminate some user UI surprise.

[KushalP, 2011-11-22 21:43:58]

On 2011/11/22 19:33:33, mrossetti wrote:
> FYI, there was a thread on this earlier:
> http://groups.google.com/a/chromium.org/group/chromium-dev/t/fb7cfbd703a09701

Yeah, I saw that when @rsleevi posted it. Wasn't sure whether it was something
that's been happening since the weekend or if it stopped. Just in case, thought
a rebase against LKGR would mean the bots might force a clobber.

[KushalP, 2011-11-22 21:48:22]

On 2011/11/22 21:00:35, Ilya Sherman wrote:
> Bookmarks model changes l g t m (with a nit).  For the Xib changes, have you
> resolved the platform target warning?

No. I don't think I can stop the platform warning from appearing. Although I
could implement something for 10.5 machines which listened to key events on the
NSTextField and ran CollapseWhitespace() on it. Should I do this?

http://codereview.chromium.org/8598015/diff/26001/chrome/browser/bookmarks/bo...
> File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):
> 
>
http://codereview.chromium.org/8598015/diff/26001/chrome/browser/bookmarks/bo...
> chrome/browser/bookmarks/bookmark_model_unittest.cc:71: };
> nit: Since you're declaring this outside of the test functions, please rename
> this to something more specific, e.g. |whitespace_test_cases|

Working on it now. Will be in the next Patch Set in ~5mins.

[Ilya Sherman, 2011-11-22 21:54:02]

On 2011/11/22 21:48:22, KushalP wrote:
> On 2011/11/22 21:00:35, Ilya Sherman wrote:
> > Bookmarks model changes l g t m (with a nit).  For the Xib changes, have you
> > resolved the platform target warning?
> 
> No. I don't think I can stop the platform warning from appearing. Although I
> could implement something for 10.5 machines which listened to key events on
the
> NSTextField and ran CollapseWhitespace() on it. Should I do this?

It's certainly not worth implementing something custom for 10.5.  It would be
good if we could suppress the warning from the build output, though, since we
know we don't care about it.

[Ilya Sherman, 2011-11-22 22:00:01]

Nico (thakis) suggested on IRC setting the single-line flag from code rather
than in the .xib file, so that we can avoid adding a warning.

[KushalP, 2011-11-23 01:02:21]

From discussions on IRC, this was chosen to be the preferred way to pull off the
above:

// Check if NSTextField supports the method. This check is in place as
// only 10.6 and greater support the inner method.
if ([nameTextField_ respondsToSelector:@selector(setUsesSingleLineMode)]) {
  // Enforce single-line folder/bookmark naming.
  [nameTextField_ setUsesSingleLineMode:YES];
}

However, adding the above to line 107 of
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm produces the
following error:

make: ***
[out/Debug/obj.target/browser/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller.o]
Error 1
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm:111:5: error:
instance method '-setUsesSingleLineMode:' not found
      (return type defaults to 'id') [-Werror]
    [nameTextField_ setUsesSingleLineMode:YES];

This is in the class where a NSTextField is exposed. In the other two, the text
fields are directly synthesised to NSStrings. How should I proceed?

[mrossetti, 2011-11-23 04:22:18]

On 2011/11/23 01:02:21, KushalP wrote:
> From discussions on IRC, this was chosen to be the preferred way to pull off
the
> above:
> 
> // Check if NSTextField supports the method. This check is in place as
> // only 10.6 and greater support the inner method.
> if ([nameTextField_ respondsToSelector:@selector(setUsesSingleLineMode)]) {
>   // Enforce single-line folder/bookmark naming.
>   [nameTextField_ setUsesSingleLineMode:YES];
> }
> 
> However, adding the above to line 107 of
> chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm produces the
> following error:
> 
> make: ***
>
[out/Debug/obj.target/browser/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller.o]
> Error 1
> chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm:111:5: error:
> instance method '-setUsesSingleLineMode:' not found
>       (return type defaults to 'id') [-Werror]
>     [nameTextField_ setUsesSingleLineMode:YES];
> 
> This is in the class where a NSTextField is exposed. In the other two, the
text
> fields are directly synthesised to NSStrings. How should I proceed?

Sigh. There is no way. At this point I'd suggest backing off from trying to set
-[setSingleLineMode:] in code or in nibs and just go with the model changes.
It'll be better than the current situation.

Since that code has already been removed consider this an LGTM with the revert
of the nibs and removal of the attempt to setUsesSingleLineMode: calls.

[KushalP, 2011-11-23 09:42:15]

On 2011/11/23 04:22:18, mrossetti wrote:
> Sigh. There is no way. At this point I'd suggest backing off from trying to
set
> -[setSingleLineMode:] in code or in nibs and just go with the model changes.
> It'll be better than the current situation.
> 
> Since that code has already been removed consider this an LGTM with the revert
> of the nibs and removal of the attempt to setUsesSingleLineMode: calls.

Wince :(

But it's so close to being implemented that I can touch it. Is this what "the
mac gods" have stated would be preferred over a better experience for users on
10.6+ because of a build warning appearing?

[Ilya Sherman, 2011-11-23 10:21:27]

On 2011/11/23 01:02:21, KushalP wrote:
> From discussions on IRC, this was chosen to be the preferred way to pull off
the
> above:
> 
> // Check if NSTextField supports the method. This check is in place as
> // only 10.6 and greater support the inner method.
> if ([nameTextField_ respondsToSelector:@selector(setUsesSingleLineMode)]) {
>   // Enforce single-line folder/bookmark naming.
>   [nameTextField_ setUsesSingleLineMode:YES];
> }

What if you cast |nameTextField_| to an |id| first?  You already have the
respondsToSelector if-stmt guard, so you know it's safe to invoke the selector;
at this point, it's just a game of convincing the compiler, right?

[KushalP, 2011-11-23 10:24:09]

On 2011/11/23 10:21:27, Ilya Sherman wrote:
> What if you cast |nameTextField_| to an |id| first?  You already have the
> respondsToSelector if-stmt guard, so you know it's safe to invoke the
selector;
> at this point, it's just a game of convincing the compiler, right?

I'll try that after lunch.

[KushalP, 2011-11-23 10:43:21]

Just had a quick look through the docs. It appears that NSCell (and children)
have the setUsesSingleLineMode method. NSTextField does not.

I can still compile this if you want me to try and see what casting to id shows?

NSCell:
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Applica...

I could change the field to be an NSTextFieldCell?

[Ilya Sherman, 2011-11-23 19:41:22]

On 2011/11/23 10:43:21, KushalP wrote:
> Just had a quick look through the docs. It appears that NSCell (and children)
> have the setUsesSingleLineMode method. NSTextField does not.

In that case, I think you want to call the methods on [nameTextField_ cell]
rather than on nameTextField_ itself.

> I can still compile this if you want me to try and see what casting to id
shows?

Well, if the text field itself does not respond to the selector, you'll never
make it into the if stmt in the first place, so appeasing the compiler won't
help too much ;)   But you might still need a casting trick like this once you
update the code to call into the cell, since this API is 10.6+ only.

[KushalP, 2011-11-24 00:58:15]

Things that were tried and haven't worked:

// calling against id
id nameCell = [nameTextField_ cell]

// using categories to define the methods in place
@interface NSTextFieldCell (multilinebookmarks)

// wrapping with extra parens
[((id)[nameTextField_ cell]) setUsesSingleLineMode:YES];

If you have any further ideas, keep 'em coming! There MUST be a way to get this
working.

[Ilya Sherman, 2011-11-24 01:15:50]

On 2011/11/24 00:58:15, KushalP wrote:
> Things that were tried and haven't worked:
> 
> // calling against id
> id nameCell = [nameTextField_ cell]
> 
> // using categories to define the methods in place
> @interface NSTextFieldCell (multilinebookmarks)

What goes wrong when you try to use a category?  For example, this code compiles
just fine, though there is obviously no implementation of "asdfoasdghasiodhf"
anywhere: http://pastebin.com/iHdzpRDJ

[KushalP, 2011-11-24 18:00:33]

Tried categories again today. A clobber got it working! :D

However, I'm on Snow Leopard which means I'm running the 10.5 SDK. So I haven't
been able to test whether the UI updates as expected.

Other than setting GYP to use 10.6, is there some other less time-intensive way
to check this works in the UI?

Also, as stated previously, this only works for one of the UI elements because
the other 2 are synthesised to strings.

[KushalP, 2011-11-28 16:52:54]

Looks like my change in Patch Set 8 is in the wrong place.

Running this through gdb and setting a breakpoint on setUsesSingleLineMode
doesn't show up with anything when trying to add a folder from the BMB.

Will look into this more later tonight or tomorrow.

[Ilya Sherman, 2011-11-28 23:56:29]

On 2011/11/24 18:00:33, KushalP wrote:
> Tried categories again today. A clobber got it working! :D

Hooray!

> However, I'm on Snow Leopard which means I'm running the 10.5 SDK. So I
haven't
> been able to test whether the UI updates as expected.
> 
> Other than setting GYP to use 10.6, is there some other less time-intensive
way
> to check this works in the UI?

If you're on Snow Leopard -- rather than Leopard -- then at runtime you should
be using the 10.6 SDK.  The GYP setting should only matter for whether or not
the code compiles, not whether it runs correctly.

For debugging, I might recommend creating a small test project that has a single
NSTextField view.  First, set the project to target the 10.6 SDK, and get things
compiling and behaving correctly, without worrying about categories or other
promises to the compiler.  Once you get that working, set the project target to
10.5 SDK, and add categories, etc. until everything is compiling and behaving
correctly again.  Then, once you get that working, apply the same solution
within the Chromium code base.  That should let you be able to quickly test your
changes, without needing to recompile the Chromium source each time while
prototyping/experimenting.

[KushalP, 2011-11-29 18:37:04]

On 2011/11/28 23:56:29, Ilya Sherman wrote:
> For debugging, I might recommend creating a small test project that has a
single
> NSTextField view.  First, set the project to target the 10.6 SDK, and get
things
> compiling and behaving correctly, without worrying about categories or other
> promises to the compiler.  Once you get that working, set the project target
to
> 10.5 SDK, and add categories, etc. until everything is compiling and behaving
> correctly again.  Then, once you get that working, apply the same solution
> within the Chromium code base.  That should let you be able to quickly test
your
> changes, without needing to recompile the Chromium source each time while
> prototyping/experimenting.

Just did the above and got a small project working against 10.5 using the exact
same category code as used in the above Patch Set.

Will have another look through the nibs and make sure that I'm calling it on the
correct IB field. So close to getting this fixed that I can taste it! :D

[KushalP, 2011-11-30 00:51:42]

Patch Set 10 now prevents folders from having multiline input in the most
obvious ways that I've found:

- Right-click on BMB and "Add Folder"
- via Bookmark Bubble, using "Choose Another Folder" or the "Edit" button

Things that need to be done:

There's a common bit of code that should be in a header file. It is used by
files in chrome/browser/ui/cocoa/bookmarks/ but could be useful to others. My
original thought was to pull this out and leave it in a header file called
"bookmark_cell_single_line.h" in chrome/browser/ui/cocoa/bookmarks/. Is this
okay, or should it go somewhere higher up the order?

[Ilya Sherman, 2011-11-30 06:09:27]

On 2011/11/30 00:51:42, KushalP wrote:
> Patch Set 10 now prevents folders from having multiline input in the most
> obvious ways that I've found:
> 
> - Right-click on BMB and "Add Folder"
> - via Bookmark Bubble, using "Choose Another Folder" or the "Edit" button
> 
> Things that need to be done:
> 
> There's a common bit of code that should be in a header file. It is used by
> files in chrome/browser/ui/cocoa/bookmarks/ but could be useful to others. My
> original thought was to pull this out and leave it in a header file called
> "bookmark_cell_single_line.h" in chrome/browser/ui/cocoa/bookmarks/. Is this
> okay, or should it go somewhere higher up the order?

We generally try to only pull functions up on an as-needed basis, so I would
leave it in bookmarks/ for now.

[Ilya Sherman, 2011-11-30 06:09:38]

http://codereview.chromium.org/8598015/diff/50004/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller.mm
(right):

http://codereview.chromium.org/8598015/diff/50004/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller.mm:572: NSCell
*folderCell = [folderTreeView_ preparedCellAtColumn:0 row:row];
nit: "NSCell *" -> "NSCell* "

[KushalP, 2011-11-30 12:55:21]

Updated for nits and is currently running through the trybots.

[commit-bot: I haz the power, 2011-12-01 02:28:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8598015/54002

[commit-bot: I haz the power, 2011-12-01 04:34:05]

Change committed as 112400