Some Chrome Frame cleanup:...

Some Chrome Frame cleanup:

1) Rearrange the chrome_frame.gyp file to:
  a) Split out most of CF into a static lib to make writing unit tests easier (not having to re-include source files).
  b) Remove most of the un-needed xulrunner-sdk includes.

2) Move all OBJECT_ENTRY_AUTO macros to chrome_tab.cc since they don't work without extra work when residing in a lib.

3) Rename npchrome_tab.dll to npchrome_frame.dll.

BUG=none
TEST=none



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

[slightlyoff, 2010-01-04 22:34:19]

LGTM, with nit

http://codereview.chromium.org/523040/diff/1/13
File chrome_frame/test_utils.cc (right):

http://codereview.chromium.org/523040/diff/1/13#newcode23
chrome_frame/test_utils.cc:23: const wchar_t kChromeFrameDllName[] =
L"npchrome_frame.dll";
you declare it const but not static in a section labeled "Statics".

[amit, 2010-01-04 23:20:53]

Looks pretty good. Please change npchrome_frame to chrome_frame.

http://codereview.chromium.org/523040/diff/1/18
File chrome/installer/util/util_constants.cc (right):

http://codereview.chromium.org/523040/diff/1/18#newcode116
chrome/installer/util/util_constants.cc:116: const wchar_t kChromeFrameDll[] =
L"npchrome_frame.dll";
please remove the 'np' from the front :)
It should be chrome_frame.dll

http://codereview.chromium.org/523040/diff/1/15
File chrome_frame/FILES (right):

http://codereview.chromium.org/523040/diff/1/15#newcode1
chrome_frame/FILES:1: servers\npchrome_frame.dll
npchrome_frame.dll -> chrome_frame.dll

http://codereview.chromium.org/523040/diff/1/4
File chrome_frame/SYMBOLS (right):

http://codereview.chromium.org/523040/diff/1/4#newcode1
chrome_frame/SYMBOLS:1: servers\npchrome_frame.pdb
ditto

http://codereview.chromium.org/523040/diff/1/3
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/523040/diff/1/3#newcode130
chrome_frame/chrome_frame.gyp:130: 'test/util_unittests.cc',
nice!

http://codereview.chromium.org/523040/diff/1/12
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/523040/diff/1/12#newcode313
chrome_frame/test/html_util_unittests.cc:313: TEST(HttpUtils,
HasFrameBustingHeader) {
cool

[Paweł Hajdan Jr., 2010-01-05 06:38:33]

Drive-by with a testing nit.

http://codereview.chromium.org/523040/diff/1/12
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/523040/diff/1/12#newcode314
chrome_frame/test/html_util_unittests.cc:314: // Simple negative cases.
Please use EXPECT_ for all the test cases in this function. This way you can
test more in a single step in case of failures.

[robertshield, 2010-01-05 18:24:35]

Thanks for your comments!

Main thing is that browsers that load certain kinds of plugins require said
plugins to be named np<something>.dll, so I can't change the name very easily.

Also updated to merge with some recent-ish chrome.gyp refactoring.

http://codereview.chromium.org/523040/diff/1/18
File chrome/installer/util/util_constants.cc (right):

http://codereview.chromium.org/523040/diff/1/18#newcode116
chrome/installer/util/util_constants.cc:116: const wchar_t kChromeFrameDll[] =
L"npchrome_frame.dll";
On 2010/01/04 23:20:54, amit wrote:
> please remove the 'np' from the front :)
> It should be chrome_frame.dll

As discussed, we need the 'np'.

http://codereview.chromium.org/523040/diff/1/15
File chrome_frame/FILES (right):

http://codereview.chromium.org/523040/diff/1/15#newcode1
chrome_frame/FILES:1: servers\npchrome_frame.dll
On 2010/01/04 23:20:54, amit wrote:
> npchrome_frame.dll -> chrome_frame.dll

ditto

http://codereview.chromium.org/523040/diff/1/4
File chrome_frame/SYMBOLS (right):

http://codereview.chromium.org/523040/diff/1/4#newcode1
chrome_frame/SYMBOLS:1: servers\npchrome_frame.pdb
On 2010/01/04 23:20:54, amit wrote:
> ditto

ditto

http://codereview.chromium.org/523040/diff/1/3
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/523040/diff/1/3#newcode130
chrome_frame/chrome_frame.gyp:130: 'test/util_unittests.cc',
On 2010/01/04 23:20:54, amit wrote:
> nice!

Thanks!

http://codereview.chromium.org/523040/diff/1/12
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/523040/diff/1/12#newcode313
chrome_frame/test/html_util_unittests.cc:313: TEST(HttpUtils,
HasFrameBustingHeader) {
On 2010/01/04 23:20:54, amit wrote:
> cool

Done.

http://codereview.chromium.org/523040/diff/1/12#newcode314
chrome_frame/test/html_util_unittests.cc:314: // Simple negative cases.
On 2010/01/05 06:38:33, Paweł Hajdan Jr. wrote:
> Please use EXPECT_ for all the test cases in this function. This way you can
> test more in a single step in case of failures.

I've changed this test to use the EXPECT macros, thanks!

http://codereview.chromium.org/523040/diff/1/13
File chrome_frame/test_utils.cc (right):

http://codereview.chromium.org/523040/diff/1/13#newcode23
chrome_frame/test_utils.cc:23: const wchar_t kChromeFrameDllName[] =
L"npchrome_frame.dll";
On 2010/01/04 22:34:19, slightlyoff wrote:
> you declare it const but not static in a section labeled "Statics".

sorry, should have placed this before the comment. fixed.

[Paweł Hajdan Jr., 2010-01-05 18:34:27]

The code I commented about in drive-by comment LGTM. Thanks.

[amit, 2010-01-05 18:42:20]

lgtm

On Tue, Jan 5, 2010 at 10:34 AM,  <phajdan.jr@chromium.org> wrote:
> The code I commented about in drive-by comment LGTM. Thanks.
>
> http://codereview.chromium.org/523040
>