Made sync code build and pass unit tests on OS X.

Made sync code build and pass unit tests on OS X.

Major changes:
- Moved sync_setup_{flow,wizard} to sync directory.
- Made browser_with_test_window_test compile on non-Windows platform.
- Moved localized contents font util functions to app/.

BUG=23073
TEST=trybot

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

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

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

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

[akalin, 2009-10-15 01:51:01]

Adding munjal and nick as reviewers.

[akalin, 2009-10-15 05:02:55]

On 2009/10/15 01:51:01, akalin wrote:
> Adding munjal and nick as reviewers.

Looks like the trybots are happy with this change, although I had to re-disable
a test for Linux that I enabled for non-Windows as part of this CL.  I will look
at it when I get back into town.

[Munjal (Google), 2009-10-15 18:00:12]

I think we should get someone to reviews the changes to browser window stuff
unless Nick is familiar with that stuff.

I am also not sure if we are okay with moving files under sync/views/ to
directly under sync/. Isn't there some GYP magic we can do to include them on
MAC too?

http://codereview.chromium.org/279004/diff/8020/7027
File chrome/browser/sync/sync_setup_flow.h (right):

http://codereview.chromium.org/279004/diff/8020/7027#newcode8
Line 8: #define CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_FLOW_H_
Remove VIEWS from these 2 lines.

http://codereview.chromium.org/279004/diff/8020/7027#newcode187
Line 187: #endif  // CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_FLOW_H_
Remove VIEWS

http://codereview.chromium.org/279004/diff/8020/7029
File chrome/browser/sync/sync_setup_wizard.h (right):

http://codereview.chromium.org/279004/diff/8020/7029#newcode6
Line 6: #define CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_WIZARD_H_
Remove VIEWS from these 2 lines.

http://codereview.chromium.org/279004/diff/8020/7029#newcode51
Line 51: #endif  // CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_WIZARD_H_
Remove VIEWS

http://codereview.chromium.org/279004/diff/8020/7031
File chrome/browser/sync/util/user_settings_unittest.cc (right):

http://codereview.chromium.org/279004/diff/8020/7031#newcode21
Line 21: std::string PathStringToUTF8Quick(const PathString& wide) {
Didn't we have a similar function somewhere else too? I think we can safely add
this function besides PathStringToUTF8 instead of duplicating it everywhere it's
needed.

[ncarter (slow), 2009-10-15 18:24:02]

I'm familiar with the window.h / window.cc changes and that looks fine.  The
browser window with test code is less familiar to me.

In addition to Munjal's observations, some small things:

http://codereview.chromium.org/279004/diff/8020/7021
File app/app.gyp (right):

http://codereview.chromium.org/279004/diff/8020/7021#newcode101
Line 101: 'gfx/font_util.cc',
Please sort (w > u)

http://codereview.chromium.org/279004/diff/8020/7033
File chrome/chrome.gyp (right):

http://codereview.chromium.org/279004/diff/8020/7033#newcode4608
Line 4608: # TODO(akalin): Figure out why this unittest fails on linux.
Consider filing a bug.

http://codereview.chromium.org/279004/diff/8020/7033#newcode6703
Line 6703: 'browser/sync/util/data_encryption_unittest.cc',
On windows at least, the paradigm of include-then-exclude with a "sources!" rule
is nice because it means the files still are included in project-wide grep.

[akalin, 2009-10-15 21:16:19]

On 2009/10/15 18:00:12, munjal wrote:
> I am also not sure if we are okay with moving files under sync/views/ to
> directly under sync/. Isn't there some GYP magic we can do to include them on
> MAC too?

It is my impression that everything under views/ is Windows-specific, and so
chrome/browser/views/ is also by extension, since it uses stuff on views/. 
Also, sync_setup_{flow,wizard} don't strike me as views-related classes.

> 
> http://codereview.chromium.org/279004/diff/8020/7027
> File chrome/browser/sync/sync_setup_flow.h (right):
> 
> http://codereview.chromium.org/279004/diff/8020/7027#newcode8
> Line 8: #define CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_FLOW_H_
> Remove VIEWS from these 2 lines.
> http://codereview.chromium.org/279004/diff/8020/7027#newcode187
> Line 187: #endif  // CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_FLOW_H_
> Remove VIEWS
> http://codereview.chromium.org/279004/diff/8020/7029
> File chrome/browser/sync/sync_setup_wizard.h (right):
> 
> http://codereview.chromium.org/279004/diff/8020/7029#newcode6
> Line 6: #define CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_WIZARD_H_
> Remove VIEWS from these 2 lines.
> http://codereview.chromium.org/279004/diff/8020/7029#newcode51
> Line 51: #endif  // CHROME_BROWSER_VIEWS_SYNC_SYNC_SETUP_WIZARD_H_
> Remove VIEWS

Done.

> http://codereview.chromium.org/279004/diff/8020/7031#newcode21
> Line 21: std::string PathStringToUTF8Quick(const PathString& wide) {
> Didn't we have a similar function somewhere else too? I think we can safely
add
> this function besides PathStringToUTF8 instead of duplicating it everywhere
it's
> needed.

Yeah, there was UTF8ToPathStringQuick.  Moved both to
character_set_converters.h.

Please take another look.

[akalin, 2009-10-15 21:18:20]

On 2009/10/15 18:24:02, nick wrote:
> I'm familiar with the window.h / window.cc changes and that looks fine.  The
> browser window with test code is less familiar to me.

I will add sky@ for the browser window test code.

> http://codereview.chromium.org/279004/diff/8020/7021#newcode101
> Line 101: 'gfx/font_util.cc',
> Please sort (w > u)

Done.

> http://codereview.chromium.org/279004/diff/8020/7033
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/279004/diff/8020/7033#newcode4608
> Line 4608: # TODO(akalin): Figure out why this unittest fails on linux.
> Consider filing a bug.

Will do.

> http://codereview.chromium.org/279004/diff/8020/7033#newcode6703
> Line 6703: 'browser/sync/util/data_encryption_unittest.cc',
> On windows at least, the paradigm of include-then-exclude with a "sources!"
rule
> is nice because it means the files still are included in project-wide grep.

Okay, changed it back and added an exclusion for non-Windows.

Please take another look.

[akalin, 2009-10-15 21:19:27]

On 2009/10/15 21:18:20, akalin wrote:
> 
> I will add sky@ for the browser window test code.
> 

+sky for changes to test_browser_window.h and browser_with_test_window_test.h.

[sky, 2009-10-15 21:25:44]

As long as all tests still run, LGTM.

[akalin, 2009-10-15 21:37:47]

On 2009/10/15 21:25:44, sky wrote:
> As long as all tests still run, LGTM.

The latest patch merged with head (picking up the Linux sync port related
changes) and it looks like the trybots are still happy.

[ncarter (slow), 2009-10-15 21:45:23]

LGTM!

http://codereview.chromium.org/279004/diff/3040/2046
File chrome/browser/sync/sync_setup_wizard.h (right):

http://codereview.chromium.org/279004/diff/3040/2046#newcode66
Line 66: 
Only one newline at the end of the file is necessary.

[ncarter (slow), 2009-10-15 21:58:37]

Note that Tim just checked in some significant changes to the wizard, so make
sure you double check that your patch applies correctly (not sure how git mv
interacts with updates coming from upstream.  svn fails pretty hard in my
experience.)

[akalin, 2009-10-15 23:24:25]

On 2009/10/15 21:45:23, nick wrote:
> LGTM!
> 
http://codereview.chromium.org/279004/diff/3040/2046#newcode66
> Line 66: 
> Only one newline at the end of the file is necessary.

Lint wants that newline there, for some reason.

On 2009/10/15 21:58:37, nick wrote:
> Note that Tim just checked in some significant changes to the wizard, so make
> sure you double check that your patch applies correctly (not sure how git mv
> interacts with updates coming from upstream.  svn fails pretty hard in my
> experience.)

I merged with head to pick up Tim's changes and it went smoothly.  I will check
in as soon as munjal LGTMs and the trybots turn green. :)

[Munjal (Google), 2009-10-15 23:28:15]

LGTM.

[akalin (wrong akalin), 2009-10-16 01:02:45]

I had to add a few calls to UTF16ToWide() to get Tim's latest changes
to compile for OS X.  But the trybots are now happy so I will submit.

On Thu, Oct 15, 2009 at 1:28 PM,  <munjal@chromium.org> wrote:
> LGTM.
>
> http://codereview.chromium.org/279004
>



-- 
Frederick Akalin
Software Engineer

[ncarter (slow), 2009-10-16 01:05:17]

Cool.

On Thu, Oct 15, 2009 at 6:02 PM, Fred Akalin <akalin@google.com> wrote:

> I had to add a few calls to UTF16ToWide() to get Tim's latest changes
> to compile for OS X.  But the trybots are now happy so I will submit.
>
> On Thu, Oct 15, 2009 at 1:28 PM,  <munjal@chromium.org> wrote:
> > LGTM.
> >
> > http://codereview.chromium.org/279004
> >
>
>
>
> --
> Frederick Akalin
> Software Engineer
>

[Evan Stade, 2009-10-19 20:09:21]

http://codereview.chromium.org/279004/diff/14018/14035
File chrome/chrome.gyp (right):

http://codereview.chromium.org/279004/diff/14018/14035#newcode4638
Line 4638: # TODO(akalin): Figure out why this unittest fails on linux.
this is NOT how you disable unittests. Use the DISABLED_ macro.