Implemented ShowOptionsWindow() for OS X (except the highlighting

Implemented ShowOptionsWindow() for OS X (except the highlighting
of the given group).

Refactored PreferencesWindowController a bit.

Made preferences_window_controller_unittest.mm use CocoaTest instead of CocoaTestHelper and reenabled disabled test which was crashing (likely due to interactions with CocoaTestHelper).

BUG=23073
TEST=manually,trybots

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

[akalin, 2009-11-13 09:55:16]

+thomasvl for review

How to test manually:
- Patch this change in, build, and run chromium with --enable-sync
- Pick the "Synchronize my bookmarks..." item from the wrench menu and enter
your login information.
- If you're syncing bookmarks successfully, the wrench menu item should now say
"Bookmarks synced..." and picking it should bring up the "Personal Stuff" pref
pane.  If a different pref pane is open, it should animate to "Personal Stuff". 
Otherwise, prefs are opened to "Personal Stuff" directly (no animatino).

[TVL, 2009-11-13 12:48:09]

lgtm

+ pinkerton for the app change and for ideas on the unittests question about
awakeFromNib.

http://codereview.chromium.org/391050/diff/1010/13
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/391050/diff/1010/13#newcode480
Line 480: - (IntegerPrefMember*)lastSelectedPage {
move the testing ones to a @impl block after this one, ie- @implementation
PreferencesWindowController(Testing)
...
@end

[pink (ping after 24hrs), 2009-11-13 15:32:29]

http://codereview.chromium.org/391050/diff/1010/11
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/391050/diff/1010/11#newcode701
Line 701: [prefsController_ showPreferences:sender];
why not do the switch before showing the window? If the window is already
visible, they'll still see the animation, but if it's not, then it just opens
directly to the right panel. Why slow the user down just so we can show a
transition?

http://codereview.chromium.org/391050/diff/1010/14
File chrome/browser/cocoa/preferences_window_controller_unittest.mm (right):

http://codereview.chromium.org/391050/diff/1010/14#newcode34
Line 34: class PrefsControllerTest : public PlatformTest {
make this a CocoaTest, CocoaTestHelper has been deprecated. I don't know if that
would help with your issues, though. Other window controller tests don't seem to
have any issues with loading from nibs.

[Nico, 2009-11-13 16:20:23]

drive-by

http://codereview.chromium.org/391050/diff/1010/13
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/391050/diff/1010/13#newcode535
Line 535: return OPTIONS_PAGE_GENERAL;
OPTIONS_PAGE_DEFAULT ?

http://codereview.chromium.org/391050/diff/1010/13#newcode545
Line 545: case OPTIONS_PAGE_GENERAL:
Add case OPTIONS_PAGE_DEFAULT:?

[akalin, 2009-11-13 21:05:38]

On 2009/11/13 12:48:09, TVL wrote:
> lgtm
> 
> http://codereview.chromium.org/391050/diff/1010/13#newcode480
> Line 480: - (IntegerPrefMember*)lastSelectedPage {
> move the testing ones to a @impl block after this one, ie- @implementation
> PreferencesWindowController(Testing)
> ...
> @end

Done.

On 2009/11/13 15:32:29, pink wrote:
> Line 701: [prefsController_ showPreferences:sender];
> why not do the switch before showing the window? If the window is already
> visible, they'll still see the animation, but if it's not, then it just opens
> directly to the right panel. Why slow the user down just so we can show a
> transition?

Done.

> http://codereview.chromium.org/391050/diff/1010/14#newcode34
> Line 34: class PrefsControllerTest : public PlatformTest {
> make this a CocoaTest, CocoaTestHelper has been deprecated. I don't know if
that
> would help with your issues, though. Other window controller tests don't seem
to
> have any issues with loading from nibs.

Done.  This, and moving stuff from the constructor to SetUp(), takes care of the
crashes.  Yay!

On 2009/11/13 16:20:23, Nico wrote:
> drive-by
> 
> http://codereview.chromium.org/391050/diff/1010/13#newcode535
> Line 535: return OPTIONS_PAGE_GENERAL;
> OPTIONS_PAGE_DEFAULT ?

I'd rather return OPTIONS_PAGE_GENERAL, which would give a more consistent error
case (since _DEFAULT would make it go to the last selected page, which could be
anything).

> http://codereview.chromium.org/391050/diff/1010/13#newcode545
> Line 545: case OPTIONS_PAGE_GENERAL:
> Add case OPTIONS_PAGE_DEFAULT:?

No need; page is put through normalizePage before being switched on, so
OPTIONS_PAGE_DEFAULT will never happen.

Please take another look!

[TVL, 2009-11-13 21:26:29]

still lgtm.

http://codereview.chromium.org/391050/diff/4003/6004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/391050/diff/4003/6004#newcode1709
Line 1709: default:
so the one advantage of listing the _DEFAULT value would be you could remove the
default: part and the compiler will warn if if there is an unhandled enum value.

[akalin, 2009-11-13 22:07:32]

On 2009/11/13 21:26:29, TVL wrote:
> Line 1709: default:
> so the one advantage of listing the _DEFAULT value would be you could remove
the
> default: part and the compiler will warn if if there is an unhandled enum
value.

Done, changed NOTIMPLEMENTED() to a LOG(DFATAL).

Also added more unittests.

pinkerton and thakis, go ahead and take another look!

[Nico, 2009-11-13 22:09:48]

Oh, I'm fine with whatever pink and thomas say :-)

On Fri, Nov 13, 2009 at 2:07 PM,  <akalin@chromium.org> wrote:
> On 2009/11/13 21:26:29, TVL wrote:
>>
>> Line 1709: default:
>> so the one advantage of listing the _DEFAULT value would be you could
>> remove
>
> the
>>
>> default: part and the compiler will warn if if there is an unhandled enum
>
> value.
>
> Done, changed NOTIMPLEMENTED() to a LOG(DFATAL).
>
> Also added more unittests.
>
> pinkerton and thakis, go ahead and take another look!
>
> http://codereview.chromium.org/391050
>