Fix http://crbug.com/13971: OSX: Text copied from Omnibox is styled

Fix http://crbug.com/13971:	OSX: Text copied from Omnibox is styled

Provide a custom field editor for the location bar that overrides cut/copy to only write plain text to the clipboard.

BUG=13971
TEST=Type a URL in the Omnibox, select all and copy url, open Textedit and paste the url into it.  Expected results: url should be unstyled previous behavior: styled text (as it appeared in the Omnibox was pasted)

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

[jeremy, 2009-06-12 22:37:18]

Sending to shess for a preliminary look. Unit test will (hopefully) be included
in final patch.

[Scott Hess - ex-Googler, 2009-06-12 23:04:27]

Overall, I think I'm +1 on your approach, with nits.

http://codereview.chromium.org/126075/diff/1/3
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/126075/diff/1/3#newcode15
Line 15: @class LocationBarFieldEditor;
Not needed if you have the @interface in here, but ...

http://codereview.chromium.org/126075/diff/1/3#newcode30
Line 30: 
It might be reasonable to put this @interface over in the .mm file.  There's no
particular reason to expose this stuff to anyone else.

http://codereview.chromium.org/126075/diff/1/3#newcode41
Line 41: LocationBarFieldEditor* locationBarFieldEditor_;  // strong
scoped_nsobject<>?

http://codereview.chromium.org/126075/diff/1/4
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/126075/diff/1/4#newcode31
Line 31: [super writeSelectionToPasteboard:pb type:NSStringPboardType];
No need to use super, here, just use self.

Could you replace all three lines with:

[self writeSelectionToPasteboard:pb
                  types:[NSArray arrayWithObject:NSStringPboardType]];

http://codereview.chromium.org/126075/diff/1/4#newcode36
Line 36: [super delete:nil];
Again, no need to call super, here.

http://codereview.chromium.org/126075/diff/1/4#newcode158
Line 158: return nil;
I know this is micro-optimizing, but would it make sense to create
locationBarFieldEditor_ lazily, rather than at startup?

[Scott Hess - ex-Googler, 2009-06-12 23:07:29]

Just a note for the ages: -[NSTextFieldCell setUpFieldEditorAttributes:] may
also be able to manage this.  Since we don't currently have a custom cell,
there's no convenient place to hang that, though, so it's probably a non-starter
until I start landing the keyword and hint support.

On 2009/06/12 23:04:27, shess wrote:
> Overall, I think I'm +1 on your approach, with nits.
> 
> http://codereview.chromium.org/126075/diff/1/3
> File chrome/browser/cocoa/toolbar_controller.h (right):
> 
> http://codereview.chromium.org/126075/diff/1/3#newcode15
> Line 15: @class LocationBarFieldEditor;
> Not needed if you have the @interface in here, but ...
> 
> http://codereview.chromium.org/126075/diff/1/3#newcode30
> Line 30: 
> It might be reasonable to put this @interface over in the .mm file.  There's
no
> particular reason to expose this stuff to anyone else.
> 
> http://codereview.chromium.org/126075/diff/1/3#newcode41
> Line 41: LocationBarFieldEditor* locationBarFieldEditor_;  // strong
> scoped_nsobject<>?
> 
> http://codereview.chromium.org/126075/diff/1/4
> File chrome/browser/cocoa/toolbar_controller.mm (right):
> 
> http://codereview.chromium.org/126075/diff/1/4#newcode31
> Line 31: [super writeSelectionToPasteboard:pb type:NSStringPboardType];
> No need to use super, here, just use self.
> 
> Could you replace all three lines with:
> 
> [self writeSelectionToPasteboard:pb
>                   types:[NSArray arrayWithObject:NSStringPboardType]];
> 
> http://codereview.chromium.org/126075/diff/1/4#newcode36
> Line 36: [super delete:nil];
> Again, no need to call super, here.
> 
> http://codereview.chromium.org/126075/diff/1/4#newcode158
> Line 158: return nil;
> I know this is micro-optimizing, but would it make sense to create
> locationBarFieldEditor_ lazily, rather than at startup?

[jeremy, 2009-06-13 00:02:59]

Now with unit tests.

All comments addressed.

http://codereview.chromium.org/126075/diff/1/3
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/126075/diff/1/3#newcode15
Line 15: @class LocationBarFieldEditor;
On 2009/06/12 23:04:27, shess wrote:
> Not needed if you have the @interface in here, but ...

Done.

http://codereview.chromium.org/126075/diff/1/3#newcode30
Line 30: 
Need it here for testing :(

http://codereview.chromium.org/126075/diff/1/3#newcode41
Line 41: LocationBarFieldEditor* locationBarFieldEditor_;  // strong
This is an objective-c object, I'm afraid of getting bad behavior due to lazy
destruction of this object.

http://codereview.chromium.org/126075/diff/1/4
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/126075/diff/1/4#newcode31
Line 31: [super writeSelectionToPasteboard:pb type:NSStringPboardType];
No, the first line clears the clipboard :)

http://codereview.chromium.org/126075/diff/1/4#newcode36
Line 36: [super delete:nil];
On 2009/06/12 23:04:27, shess wrote:
> Again, no need to call super, here.

Done.

http://codereview.chromium.org/126075/diff/1/4#newcode158
Line 158: return nil;
Apparently not, see: http://www.cocoadev.com/index.pl?CreatingCustomFieldEditor

[Scott Hess - ex-Googler, 2009-06-13 01:10:24]

http://codereview.chromium.org/126075/diff/1/3
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/126075/diff/1/3#newcode41
Line 41: LocationBarFieldEditor* locationBarFieldEditor_;  // strong
On 2009/06/13 00:03:00, jeremy wrote:
> This is an objective-c object, I'm afraid of getting bad behavior due to lazy
> destruction of this object.

Not sure what you mean.  Won't be lazy, the object will be sent -release in
-dealloc.  You just won't have to write the code.

http://codereview.chromium.org/126075/diff/1/4
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/126075/diff/1/4#newcode158
Line 158: return nil;
On 2009/06/13 00:03:00, jeremy wrote:
> Apparently not, see:
http://www.cocoadev.com/index.pl?CreatingCustomFieldEditor

I don't mean creating a one-off, I mean doing the +alloc/-init here, and then
keep that copy cached.  One less potentially complicated object created at
startup.

http://codereview.chromium.org/126075/diff/1007/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126075/diff/1007/7#newcode743
Line 743: // for the specific object.
Comment references tab controller, code references toolbar controller.

http://codereview.chromium.org/126075/diff/1007/8
File chrome/browser/cocoa/location_bar_fieldeditor_unittest.mm (right):

http://codereview.chromium.org/126075/diff/1007/8#newcode59
Line 59: ClearPasteboard();
60% of respondents prefer PlatformTest::TearDown() as the last thing rather than
the first.  It may not matter, here, I think your +pasteboardWithName: call is
probably returning a global rather than something autoreleased.  But probably
good hygiene.

http://codereview.chromium.org/126075/diff/1007/8#newcode78
Line 78: AssertNoRichTextOnClipboard();
Suggest you change this pattern to:
    ASSERT_TRUE(NoRichTextOnClipboard());
That way failures will log more relevant output.  Likewise for
ClipboardContainsText().

http://codereview.chromium.org/126075/diff/1007/8#newcode82
Line 82: [field_editor.get() performCopy:pb];
Not sure I'm understanding why you don't just call -copy: and -cut:.  As-is, you
test that your helper methods work right, but you don't test that your field
editor intercepts and redirects the original NSResponder calls appropriately.

Except that then you'd be screwing with the main pasteboard.  Sigh.  Well,
probably not worth overriding +generalPasteboard via +poseAsClass:.  Which I see
is being used by webkit or someone (I think that's your mmentovai comment).  Am
I rambling?

http://codereview.chromium.org/126075/diff/1007/8#newcode93
Line 93: ASSERT_TRUE([[field_editor.get() textStorage] length] == 0);
ASSERT_EQ() here.

[Scott Hess - ex-Googler, 2009-06-13 01:20:59]

http://codereview.chromium.org/126075/diff/1007/8
File chrome/browser/cocoa/location_bar_fieldeditor_unittest.mm (right):

http://codereview.chromium.org/126075/diff/1007/8#newcode22
Line 22: return [NSPasteboard pasteboardWithName:pasteboard_name];
Just wondering if you could have pb_ = [NSPasteboard pasteboardWithUniqueName];
in SetUp()?  And [pb_ releaseGlobally]; in TearDown().

On the one hand, maybe this is just being excessive.  On the other hand, won't
the current code spin off uncollected pasteboards on the build bots?

[jeremy, 2009-06-15 16:43:33]

http://codereview.chromium.org/126075/diff/1007/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126075/diff/1007/7#newcode743
Line 743: // for the specific object.
On 2009/06/13 01:10:24, shess wrote:
> Comment references tab controller, code references toolbar controller.

Done.

http://codereview.chromium.org/126075/diff/1007/8
File chrome/browser/cocoa/location_bar_fieldeditor_unittest.mm (right):

http://codereview.chromium.org/126075/diff/1007/8#newcode22
Line 22: return [NSPasteboard pasteboardWithName:pasteboard_name];
Thanks Scott!
done

http://codereview.chromium.org/126075/diff/1007/8#newcode59
Line 59: ClearPasteboard();
On 2009/06/13 01:10:24, shess wrote:
> 60% of respondents prefer PlatformTest::TearDown() as the last thing rather
than
> the first.  It may not matter, here, I think your +pasteboardWithName: call is
> probably returning a global rather than something autoreleased.  But probably
> good hygiene.

Done.

http://codereview.chromium.org/126075/diff/1007/8#newcode78
Line 78: AssertNoRichTextOnClipboard();
On 2009/06/13 01:10:24, shess wrote:
> Suggest you change this pattern to:
>     ASSERT_TRUE(NoRichTextOnClipboard());
> That way failures will log more relevant output.  Likewise for
> ClipboardContainsText().

Done.

http://codereview.chromium.org/126075/diff/1007/8#newcode93
Line 93: ASSERT_TRUE([[field_editor.get() textStorage] length] == 0);
On 2009/06/13 01:10:24, shess wrote:
> ASSERT_EQ() here.

Done.

[Scott Hess - ex-Googler, 2009-06-15 18:22:29]

LGTM, with style nit.

http://codereview.chromium.org/126075/diff/1012/1014
File chrome/browser/cocoa/location_bar_fieldeditor_unittest.mm (right):

http://codereview.chromium.org/126075/diff/1012/1014#newcode87
Line 87: ASSERT_EQ([[field_editor.get() textStorage] length], (NSUInteger)0);
I assume the cast is because of a type mismatch?  I don't want to be the bearer
of this news, but ... static_cast<NSUInteger>(...).  Sigh.  I hates it.

Alternate, try 0U.  [At least in my browser, that sucks - 'U' after the zero
character, so that it's an unsigned constant in the first place.]