[Mac] Add per-plugin exceptions to content settings.

[Mac] Add per-plugin exceptions to content settings.

Screenshot: http://www.dropmocks.com/mXMd

I'm adding a subclass of NSArrayController, TableModelArrayController, that binds to a RemoveRowsTableModel that can use groups and displays them using group rows in an NSTableView. This cleans up SimpleContentExceptionsWindowController a lot, and the class could also be used for other table models that use groups (keyword editor and autofill).

XIB changes: In SimpleContentExceptionsWindow.xib, bind table view to TableModelArrayController instead of using the dataSource outlet. Buttons call actions on TableModelArrayController, and table view delegate also points to it.

BUG=39252
TEST=SimpleContentExceptionsWindowControllerTest.*:TableModelArrayControllerTest.*:PluginExceptionsTableModelTest.*

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

[Bernhard Bauer, 2010-09-10 16:28:19]

Please review. Thanks!

[Nico, 2010-09-10 16:33:34]

cc1objplus: warnings being treated as
errors/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:95:
warning: 'TableModelArrayControllerTest_CheckTitles_Test' has a field
'TableModelArrayControllerTest_CheckTitles_Test::<anonymous>' whose type uses
the anonymous
namespace/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:95:
warning: 'TableModelArrayControllerTest_CheckTitles_Test' has a base
'<unnamed>::TableModelArrayControllerTest' whose type uses the anonymous
namespace/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:101:
warning: 'TableModelArrayControllerTest_RemoveRows_Test' has a field
'TableModelArrayControllerTest_RemoveRows_Test::<anonymous>' whose type uses the
anonymous
namespace/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:101:
warning: 'TableModelArrayControllerTest_RemoveRows_Test' has a base
'<unnamed>::TableModelArrayControllerTest' whose type uses the anonymous
namespace/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:116:
warning: 'TableModelArrayControllerTest_RemoveAll_Test' has a field
'TableModelArrayControllerTest_RemoveAll_Test::<anonymous>' whose type uses the
anonymous
namespace/b/slave/mac/build/src/chrome/browser/cocoa/table_model_array_controller_unittest.mm:116:
warning: 'TableModelArrayControllerTest_RemoveAll_Test' has a base
'<unnamed>::TableModelArrayControllerTest' whose type uses the anonymous
namespace




I hate how the system table group separator looks :-( I need to write a custom
cell that looks like the separator on windows at some point. But not much you
can do about that.

[Nico, 2010-09-10 16:47:09]

Fist pass…looks pretty nice!

I'm usually not a huge fan of bindings, but they seem like a good fit here.
(rsesek _is_ a fan, maybe he wants to comment on anything. He doesn't _have_ to,
though).

http://codereview.chromium.org/3327016/diff/1/6
File
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm
(right):

http://codereview.chromium.org/3327016/diff/1/6#newcode27
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:27:
-(TableModelArrayController*) arrayController {
space after -, no space after )

http://codereview.chromium.org/3327016/diff/1/7
File chrome/browser/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/7#newcode20
chrome/browser/cocoa/table_model_array_controller.h:20: class
RemoveRowsObserverBridge : public TableModelObserver {
can this move to the cc file?

http://codereview.chromium.org/3327016/diff/1/8
File chrome/browser/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/3327016/diff/1/8#newcode17
chrome/browser/cocoa/table_model_array_controller.mm:17:
fromControllerRows:(NSIndexSet*) rows;
no space after )

http://codereview.chromium.org/3327016/diff/1/8#newcode65
chrome/browser/cocoa/table_model_array_controller.mm:65: [NSNumber
numberWithBool:YES], @"_is_group_row",
use a static NSString* const for @"_is_group_row"? Also for the other constant
strings in this file

http://codereview.chromium.org/3327016/diff/1/8#newcode87
chrome/browser/cocoa/table_model_array_controller.mm:87: return [[[[self
arrangedObjects] objectAtIndex:row] objectForKey:@"_group_id"]
create a temp NSDictionary* var? still 2 lines, less weird line wrapping

http://codereview.chromium.org/3327016/diff/1/9
File chrome/browser/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/3327016/diff/1/9#newcode80
chrome/browser/cocoa/table_model_array_controller_unittest.mm:80:
*CommandLine::ForCurrentProcess() = command_line_;
eww! do have a ScopedFlagResetter or something like that? I know that something
like this exists in google3 at least.

(If not, what pattern do other tests that need to modify the command line use?
do they inject the "enable" bool and don't modify the command line, or do they
manually do the resetting?)

http://codereview.chromium.org/3327016/diff/1/9#newcode97
chrome/browser/cocoa/table_model_array_controller_unittest.mm:97:
EXPECT_NSEQ(@"(\n    FooPlugin,\n    \"[*.]example.com\",\n    BarPlugin,\n   
\"[*.]example.com\",\n    \"[*.]moose.org\"\n)",
80 cols. Maybe

EXPECT_NSEQ(@"(\n"
            @"    FooPlugin,\n"

etc (I'm not sure if you need to repeat the @ on every row. gcc doesn't need it,
but clang either choked on it or required it iirc. let me check that for you
real quick…no, clang doesn't care either. i guess repeating them is cleaner)

http://codereview.chromium.org/3327016/diff/1/9#newcode106
chrome/browser/cocoa/table_model_array_controller_unittest.mm:106:
EXPECT_NSEQ(@"(\n    BarPlugin,\n    \"[*.]example.com\",\n   
\"[*.]moose.org\"\n)",
same

http://codereview.chromium.org/3327016/diff/1/10
File chrome/browser/mock_plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/3327016/diff/1/10#newcode9
chrome/browser/mock_plugin_exceptions_table_model.cc:9: void
MockPluginExceptionsTableModel::set_plugins(
this looks like a pretty simple mock. can you mayhaps use googlemock (in
testing/gmock) instead?

(Also, the agile clowns call classes like this fakes and not mocks:
http://martinfowler.com/articles/mocksArentStubs.html I guess these are the
important things you come up with if you talk more than you code :-P)

[Robert Sesek, 2010-09-10 18:54:42]

Overall looks pretty good.  A bunch of nits, though.  One general comment on the
line wrapping form of:

- (void)      someLongMethod:(id)foo
withAReallyLongParamaterName:(int)bar
                     thatToo:(id)baz;

This river of whitespace is not the preferred style [1].  Just indent the next
line 4 spaces and vertically align the keywords, not the colons:

- (void)someLongMethod:(id)foo
    withAReallyLongParamaterName:(int)bar
    thatToo:(id)baz;

[1]
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho...

http://codereview.chromium.org/3327016/diff/1/4
File chrome/browser/cocoa/simple_content_exceptions_window_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/4#newcode22
chrome/browser/cocoa/simple_content_exceptions_window_controller.h:22: IBOutlet
TableModelArrayController* array_controller_;
naming: arrayController_

http://codereview.chromium.org/3327016/diff/1/6
File
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm
(right):

http://codereview.chromium.org/3327016/diff/1/6#newcode19
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:19:
@interface SimpleContentExceptionsWindowController(Testing)
nit: Space before '('

http://codereview.chromium.org/3327016/diff/1/6#newcode25
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:25:
@implementation SimpleContentExceptionsWindowController(Testing)
nit: Space before '('

http://codereview.chromium.org/3327016/diff/1/7
File chrome/browser/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/7#newcode36
chrome/browser/cocoa/table_model_array_controller.h:36: @interface
TableModelArrayController : NSArrayController<NSTableViewDelegate> {
This needs a top-level comment.  It should describe how it works, particularly
the difference between models that have groups and ones that don't.

http://codereview.chromium.org/3327016/diff/1/7#newcode43
chrome/browser/cocoa/table_model_array_controller.h:43: 
Designated initializer?

http://codereview.chromium.org/3327016/diff/1/8
File chrome/browser/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/3327016/diff/1/8#newcode11
chrome/browser/cocoa/table_model_array_controller.mm:11: @interface
TableModelArrayController(PrivateMethods)
Space before '('

http://codereview.chromium.org/3327016/diff/1/8#newcode16
chrome/browser/cocoa/table_model_array_controller.mm:16: - (void) 
setModelRows:(RemoveRowsTableModel::Rows*)model_rows
naming: modelRows

http://codereview.chromium.org/3327016/diff/1/8#newcode42
chrome/browser/cocoa/table_model_array_controller.mm:42: @implementation
TableModelArrayController
nit: Insert a line after this line.

http://codereview.chromium.org/3327016/diff/1/8#newcode65
chrome/browser/cocoa/table_model_array_controller.mm:65: [NSNumber
numberWithBool:YES], @"_is_group_row",
On 2010/09/10 16:47:10, Nico wrote:
> use a static NSString* const for @"_is_group_row"? Also for the other constant
> strings in this file

Definitely do this.

http://codereview.chromium.org/3327016/diff/1/8#newcode189
chrome/browser/cocoa/table_model_array_controller.mm:189: return
model_->CanRemoveRows(rows) ? YES : NO;
Don't need this ternary operation; just return the result.

http://codereview.chromium.org/3327016/diff/1/8#newcode205
chrome/browser/cocoa/table_model_array_controller.mm:205: - (NSIndexSet *)      
        tableView:(NSTableView *)tableView
nit: No space before '*'

http://codereview.chromium.org/3327016/diff/1/9
File chrome/browser/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/3327016/diff/1/9#newcode93
chrome/browser/cocoa/table_model_array_controller_unittest.mm:93: }
  // namespace

[Nico, 2010-09-10 21:11:56]

On Fri, Sep 10, 2010 at 11:54 AM,  <rsesek@chromium.org> wrote:
> Overall looks pretty good.  A bunch of nits, though.  One general comment on
> the
> line wrapping form of:
>
> - (void)      someLongMethod:(id)foo
> withAReallyLongParamaterName:(int)bar
>                     thatToo:(id)baz;
>
> This river of whitespace is not the preferred style [1].  Just indent the
> next
> line 4 spaces and vertically align the keywords, not the colons:
>
> - (void)someLongMethod:(id)foo
>    withAReallyLongParamaterName:(int)bar
>    thatToo:(id)baz;
>
> [1]
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho...

Didn't know that either. Thanks!

>
>
> http://codereview.chromium.org/3327016/diff/1/4
> File chrome/browser/cocoa/simple_content_exceptions_window_controller.h
> (right):
>
> http://codereview.chromium.org/3327016/diff/1/4#newcode22
> chrome/browser/cocoa/simple_content_exceptions_window_controller.h:22:
> IBOutlet TableModelArrayController* array_controller_;
> naming: arrayController_
>
> http://codereview.chromium.org/3327016/diff/1/6
> File
> chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm
> (right):
>
> http://codereview.chromium.org/3327016/diff/1/6#newcode19
>
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:19:
> @interface SimpleContentExceptionsWindowController(Testing)
> nit: Space before '('
>
> http://codereview.chromium.org/3327016/diff/1/6#newcode25
>
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:25:
> @implementation SimpleContentExceptionsWindowController(Testing)
> nit: Space before '('
>
> http://codereview.chromium.org/3327016/diff/1/7
> File chrome/browser/cocoa/table_model_array_controller.h (right):
>
> http://codereview.chromium.org/3327016/diff/1/7#newcode36
> chrome/browser/cocoa/table_model_array_controller.h:36: @interface
> TableModelArrayController : NSArrayController<NSTableViewDelegate> {
> This needs a top-level comment.  It should describe how it works,
> particularly the difference between models that have groups and ones
> that don't.
>
> http://codereview.chromium.org/3327016/diff/1/7#newcode43
> chrome/browser/cocoa/table_model_array_controller.h:43:
> Designated initializer?
>
> http://codereview.chromium.org/3327016/diff/1/8
> File chrome/browser/cocoa/table_model_array_controller.mm (right):
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode11
> chrome/browser/cocoa/table_model_array_controller.mm:11: @interface
> TableModelArrayController(PrivateMethods)
> Space before '('
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode16
> chrome/browser/cocoa/table_model_array_controller.mm:16: - (void)
> setModelRows:(RemoveRowsTableModel::Rows*)model_rows
> naming: modelRows
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode42
> chrome/browser/cocoa/table_model_array_controller.mm:42: @implementation
> TableModelArrayController
> nit: Insert a line after this line.
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode65
> chrome/browser/cocoa/table_model_array_controller.mm:65: [NSNumber
> numberWithBool:YES], @"_is_group_row",
> On 2010/09/10 16:47:10, Nico wrote:
>>
>> use a static NSString* const for @"_is_group_row"? Also for the other
>
> constant
>>
>> strings in this file
>
> Definitely do this.
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode189
> chrome/browser/cocoa/table_model_array_controller.mm:189: return
> model_->CanRemoveRows(rows) ? YES : NO;
> Don't need this ternary operation; just return the result.
>
> http://codereview.chromium.org/3327016/diff/1/8#newcode205
> chrome/browser/cocoa/table_model_array_controller.mm:205: - (NSIndexSet
> *)               tableView:(NSTableView *)tableView
> nit: No space before '*'
>
> http://codereview.chromium.org/3327016/diff/1/9
> File chrome/browser/cocoa/table_model_array_controller_unittest.mm
> (right):
>
> http://codereview.chromium.org/3327016/diff/1/9#newcode93
> chrome/browser/cocoa/table_model_array_controller_unittest.mm:93: }
>  // namespace
>
> http://codereview.chromium.org/3327016/show
>

[Bernhard Bauer, 2010-09-13 13:12:14]

On 2010/09/10 16:33:34, Nico wrote:
> cc1objplus: warnings being treated as
> errors

Hrm, why didn't I get these errors locally? I've got the same gcc version, just
with a different build number (5659 vs 5566).

Anyway, I fixed a bug in TableModelArrayController and cleaned it up a bit, so
you might want to take another look at that.

http://codereview.chromium.org/3327016/diff/1/4
File chrome/browser/cocoa/simple_content_exceptions_window_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/4#newcode22
chrome/browser/cocoa/simple_content_exceptions_window_controller.h:22: IBOutlet
TableModelArrayController* array_controller_;
On 2010/09/10 18:54:43, rsesek wrote:
> naming: arrayController_

Done.

http://codereview.chromium.org/3327016/diff/1/6
File
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm
(right):

http://codereview.chromium.org/3327016/diff/1/6#newcode19
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:19:
@interface SimpleContentExceptionsWindowController(Testing)
On 2010/09/10 18:54:43, rsesek wrote:
> nit: Space before '('

Done.

http://codereview.chromium.org/3327016/diff/1/6#newcode25
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:25:
@implementation SimpleContentExceptionsWindowController(Testing)
On 2010/09/10 18:54:43, rsesek wrote:
> nit: Space before '('

Done.

http://codereview.chromium.org/3327016/diff/1/6#newcode27
chrome/browser/cocoa/simple_content_exceptions_window_controller_unittest.mm:27:
-(TableModelArrayController*) arrayController {
On 2010/09/10 16:47:10, Nico wrote:
> space after -, no space after )

Done.

http://codereview.chromium.org/3327016/diff/1/7
File chrome/browser/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/7#newcode20
chrome/browser/cocoa/table_model_array_controller.h:20: class
RemoveRowsObserverBridge : public TableModelObserver {
On 2010/09/10 16:47:10, Nico wrote:
> can this move to the cc file?

No, because TableModelArrayController contains a
scoped_ptr<RemoveRowsObserverBridge>, so every class that owns a
TableModelArrayController (in this case, the unit test) needs to know the full
type of RemoveRowsObserverBridge.

http://codereview.chromium.org/3327016/diff/1/7#newcode36
chrome/browser/cocoa/table_model_array_controller.h:36: @interface
TableModelArrayController : NSArrayController<NSTableViewDelegate> {
On 2010/09/10 18:54:43, rsesek wrote:
> This needs a top-level comment.  It should describe how it works, particularly
> the difference between models that have groups and ones that don't.

OK, added a comment.

http://codereview.chromium.org/3327016/diff/1/7#newcode43
chrome/browser/cocoa/table_model_array_controller.h:43: 
On 2010/09/10 18:54:43, rsesek wrote:
> Designated initializer?

The designated intializer is the same one as for NSArrayController,
initWithContent:, but usually this class is instantiated from a XIB file anyway.
Should I add a comment explaining that?

http://codereview.chromium.org/3327016/diff/1/8
File chrome/browser/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/3327016/diff/1/8#newcode11
chrome/browser/cocoa/table_model_array_controller.mm:11: @interface
TableModelArrayController(PrivateMethods)
On 2010/09/10 18:54:43, rsesek wrote:
> Space before '('

Done.

http://codereview.chromium.org/3327016/diff/1/8#newcode16
chrome/browser/cocoa/table_model_array_controller.mm:16: - (void) 
setModelRows:(RemoveRowsTableModel::Rows*)model_rows
On 2010/09/10 18:54:43, rsesek wrote:
> naming: modelRows

Done.

http://codereview.chromium.org/3327016/diff/1/8#newcode17
chrome/browser/cocoa/table_model_array_controller.mm:17:
fromControllerRows:(NSIndexSet*) rows;
On 2010/09/10 16:47:10, Nico wrote:
> no space after )

Done.

http://codereview.chromium.org/3327016/diff/1/8#newcode42
chrome/browser/cocoa/table_model_array_controller.mm:42: @implementation
TableModelArrayController
On 2010/09/10 18:54:43, rsesek wrote:
> nit: Insert a line after this line.

Done.

http://codereview.chromium.org/3327016/diff/1/8#newcode65
chrome/browser/cocoa/table_model_array_controller.mm:65: [NSNumber
numberWithBool:YES], @"_is_group_row",
On 2010/09/10 18:54:43, rsesek wrote:
> On 2010/09/10 16:47:10, Nico wrote:
> > use a static NSString* const for @"_is_group_row"? Also for the other
constant
> > strings in this file
> 
> Definitely do this.

Done.

http://codereview.chromium.org/3327016/diff/1/8#newcode87
chrome/browser/cocoa/table_model_array_controller.mm:87: return [[[[self
arrangedObjects] objectAtIndex:row] objectForKey:@"_group_id"]
On 2010/09/10 16:47:10, Nico wrote:
> create a temp NSDictionary* var? still 2 lines, less weird line wrapping

Done.

http://codereview.chromium.org/3327016/diff/1/9
File chrome/browser/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/3327016/diff/1/9#newcode80
chrome/browser/cocoa/table_model_array_controller_unittest.mm:80:
*CommandLine::ForCurrentProcess() = command_line_;
On 2010/09/10 16:47:10, Nico wrote:
> eww! do have a ScopedFlagResetter or something like that? I know that
something
> like this exists in google3 at least.

I don't think so, although I probably could use an AutoReset<CommandLine> for
it.

> (If not, what pattern do other tests that need to modify the command line use?

> do they inject the "enable" bool and don't modify the command line, or do they
> manually do the resetting?)

All other tests I've seen do the same thing. 

What do you mean by injecting the "enable" bool?

http://codereview.chromium.org/3327016/diff/1/9#newcode93
chrome/browser/cocoa/table_model_array_controller_unittest.mm:93: }
On 2010/09/10 18:54:43, rsesek wrote:
>   // namespace

Done.

http://codereview.chromium.org/3327016/diff/1/9#newcode97
chrome/browser/cocoa/table_model_array_controller_unittest.mm:97:
EXPECT_NSEQ(@"(\n    FooPlugin,\n    \"[*.]example.com\",\n    BarPlugin,\n   
\"[*.]example.com\",\n    \"[*.]moose.org\"\n)",
On 2010/09/10 16:47:10, Nico wrote:
> 80 cols. Maybe
> 
> EXPECT_NSEQ(@"(\n"
>             @"    FooPlugin,\n"
> 
> etc (I'm not sure if you need to repeat the @ on every row. gcc doesn't need
it,
> but clang either choked on it or required it iirc. let me check that for you
> real quick…no, clang doesn't care either. i guess repeating them is cleaner)

Done.

http://codereview.chromium.org/3327016/diff/1/9#newcode106
chrome/browser/cocoa/table_model_array_controller_unittest.mm:106:
EXPECT_NSEQ(@"(\n    BarPlugin,\n    \"[*.]example.com\",\n   
\"[*.]moose.org\"\n)",
On 2010/09/10 16:47:10, Nico wrote:
> same

Done.

http://codereview.chromium.org/3327016/diff/1/10
File chrome/browser/mock_plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/3327016/diff/1/10#newcode9
chrome/browser/mock_plugin_exceptions_table_model.cc:9: void
MockPluginExceptionsTableModel::set_plugins(
On 2010/09/10 16:47:10, Nico wrote:
> this looks like a pretty simple mock. can you mayhaps use googlemock (in
> testing/gmock) instead?

Hm, I have to copy the plugin vector into an out parameter, and I don't think
GMock can do that. 

I'm only using a separate .cc file here to make sure that the header still
compiles by itself, otherwise I'd just have inlined these methods.

> (Also, the agile clowns call classes like this fakes and not mocks:
> http://martinfowler.com/articles/mocksArentStubs.html I guess these are the
> important things you come up with if you talk more than you code :-P)

Yeah, it's more of a stub than a mock (to use Fowler's terminology), but most of
the other test doubles in Chrome code are simply called mock, so I wanted to
name it in the same way.

[Nico, 2010-09-13 15:02:26]

On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> On 2010/09/10 16:33:34, Nico wrote:
> > cc1objplus: warnings being treated as
> > errors
> 
> Hrm, why didn't I get these errors locally? I've got the same gcc version,
just
> with a different build number (5659 vs 5566).

Not sure, I only saw them on the bots too. Anyway, I fixed them last friday
(cc'd you on that CL).

[Nico, 2010-09-13 15:04:58]

http://codereview.chromium.org/3327016/diff/1/7
File chrome/browser/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/7#newcode20
chrome/browser/cocoa/table_model_array_controller.h:20: class
RemoveRowsObserverBridge : public TableModelObserver {
On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> On 2010/09/10 16:47:10, Nico wrote:
> > can this move to the cc file?
> 
> No, because TableModelArrayController contains a
> scoped_ptr<RemoveRowsObserverBridge>, so every class that owns a
> TableModelArrayController (in this case, the unit test) needs to know the full
> type of RemoveRowsObserverBridge.

If I understand your reasoning correctly: This is wrong. A scoped_ptr needs only
a forward declaration of its pointee type, just like a normal pointer. The
reason for this is that the size of all pointers is equal (and a scoped_ptr
internally has only a pointer). If a class has a class-type member, the compiler
needs to know the members full type to know how big instances of the containing
class are, but for pointers that's not true.

[Nico, 2010-09-13 15:20:22]

http://codereview.chromium.org/3327016/diff/1/9
File chrome/browser/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/3327016/diff/1/9#newcode80
chrome/browser/cocoa/table_model_array_controller_unittest.mm:80:
*CommandLine::ForCurrentProcess() = command_line_;
On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> On 2010/09/10 16:47:10, Nico wrote:
> > eww! do have a ScopedFlagResetter or something like that? I know that
> something
> > like this exists in google3 at least.
> 
> I don't think so, although I probably could use an AutoReset<CommandLine> for
> it.
> 
> > (If not, what pattern do other tests that need to modify the command line
use?
> 
> > do they inject the "enable" bool and don't modify the command line, or do
they
> > manually do the resetting?)
> 
> All other tests I've seen do the same thing.

:-(

> What do you mean by injecting the "enable" bool?

In java, classes don't check flags directly but have a bool that's true if the
flag is on, and that flag is @injected by a guice binder that binds the correct
flag value. Google for "dependency injection", "guice", etc if this you think
this is interesting (hint: not really). But since c++ doesn't have a runtime
system, dependency injection needs to be done manually.

One example would be to give the class that checks for the flag a static
CommandLine* member, and a static SetCommandLineForTesting() function that sets
this, and then call that here with a local CommandLine object, instead of
overwriting the global command line object. (And if that static variable is
NULL, the class would look at the global object instead).

Makes sense?

[Robert Sesek, 2010-09-13 16:16:58]

LGTM with nits.

http://codereview.chromium.org/3327016/diff/1/7
File chrome/browser/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/3327016/diff/1/7#newcode43
chrome/browser/cocoa/table_model_array_controller.h:43: 
On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> On 2010/09/10 18:54:43, rsesek wrote:
> > Designated initializer?
> 
> The designated intializer is the same one as for NSArrayController,
> initWithContent:, but usually this class is instantiated from a XIB file
anyway.
> Should I add a comment explaining that?

Yes, just add the how-to-use information to the top-level comment.

http://codereview.chromium.org/3327016/diff/10002/11004
File chrome/browser/cocoa/simple_content_exceptions_window_controller.mm
(right):

http://codereview.chromium.org/3327016/diff/10002/11004#newcode65
chrome/browser/cocoa/simple_content_exceptions_window_controller.mm:65:
withColumns:columns
nit: Colon alignment is off by 1.

http://codereview.chromium.org/3327016/diff/10002/11007
File chrome/browser/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/3327016/diff/10002/11007#newcode97
chrome/browser/cocoa/table_model_array_controller.mm:97: -
(void)setModelRows:(RemoveRowsTableModel::Rows*)model_rows
naming: modelRows

http://codereview.chromium.org/3327016/diff/10002/11007#newcode129
chrome/browser/cocoa/table_model_array_controller.mm:129: [indexes
addIndex:i+offset];
Spaces around the '+'

[Bernhard Bauer, 2010-09-13 17:18:52]

On 2010/09/13 15:04:58, Nico wrote:
> http://codereview.chromium.org/3327016/diff/1/7
> File chrome/browser/cocoa/table_model_array_controller.h (right):
> 
> http://codereview.chromium.org/3327016/diff/1/7#newcode20
> chrome/browser/cocoa/table_model_array_controller.h:20: class
> RemoveRowsObserverBridge : public TableModelObserver {
> On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> > On 2010/09/10 16:47:10, Nico wrote:
> > > can this move to the cc file?
> > 
> > No, because TableModelArrayController contains a
> > scoped_ptr<RemoveRowsObserverBridge>, so every class that owns a
> > TableModelArrayController (in this case, the unit test) needs to know the
full
> > type of RemoveRowsObserverBridge.
> 
> If I understand your reasoning correctly: This is wrong. A scoped_ptr needs
only
> a forward declaration of its pointee type, just like a normal pointer. The
> reason for this is that the size of all pointers is equal (and a scoped_ptr
> internally has only a pointer). If a class has a class-type member, the
compiler
> needs to know the members full type to know how big instances of the
containing
> class are, but for pointers that's not true.

Hm. I remember getting an error the first time I tried it, but now it seems to
work. Oh well :-)

On 2010/09/13 15:20:22, Nico wrote:
> http://codereview.chromium.org/3327016/diff/1/9
> File chrome/browser/cocoa/table_model_array_controller_unittest.mm (right):
> 
> http://codereview.chromium.org/3327016/diff/1/9#newcode80
> chrome/browser/cocoa/table_model_array_controller_unittest.mm:80:
> *CommandLine::ForCurrentProcess() = command_line_;
> On 2010/09/13 13:12:14, Bernhard Bauer wrote:
> > On 2010/09/10 16:47:10, Nico wrote:
> > > eww! do have a ScopedFlagResetter or something like that? I know that
> > something
> > > like this exists in google3 at least.
> > 
> > I don't think so, although I probably could use an AutoReset<CommandLine>
for
> > it.
> > 
> > > (If not, what pattern do other tests that need to modify the command line
> use?
> > 
> > > do they inject the "enable" bool and don't modify the command line, or do
> they
> > > manually do the resetting?)
> > 
> > All other tests I've seen do the same thing.
> 
> :-(
> 
> > What do you mean by injecting the "enable" bool?
> 
> In java, classes don't check flags directly but have a bool that's true if the
> flag is on, and that flag is @injected by a guice binder that binds the
correct
> flag value. Google for "dependency injection", "guice", etc if this you think
> this is interesting (hint: not really). But since c++ doesn't have a runtime
> system, dependency injection needs to be done manually.
> 
> One example would be to give the class that checks for the flag a static
> CommandLine* member, and a static SetCommandLineForTesting() function that
sets
> this, and then call that here with a local CommandLine object, instead of
> overwriting the global command line object. (And if that static variable is
> NULL, the class would look at the global object instead).
> 
> Makes sense?

Yeah, it does. Would you prefer that? I'm just a little bit wary of adding too
much plumbing here, as the command line switch is only temporary until we have
it landed fully.

[Bernhard Bauer, 2010-09-13 17:20:38]

On 2010/09/13 16:16:58, rsesek wrote:
> Yes, just add the how-to-use information to the top-level comment.

ok, done.

http://codereview.chromium.org/3327016/diff/10002/11004
File chrome/browser/cocoa/simple_content_exceptions_window_controller.mm
(right):

http://codereview.chromium.org/3327016/diff/10002/11004#newcode65
chrome/browser/cocoa/simple_content_exceptions_window_controller.mm:65:
withColumns:columns
On 2010/09/13 16:16:58, rsesek wrote:
> nit: Colon alignment is off by 1.

Done.

http://codereview.chromium.org/3327016/diff/10002/11007
File chrome/browser/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/3327016/diff/10002/11007#newcode97
chrome/browser/cocoa/table_model_array_controller.mm:97: -
(void)setModelRows:(RemoveRowsTableModel::Rows*)model_rows
On 2010/09/13 16:16:58, rsesek wrote:
> naming: modelRows

Done.

http://codereview.chromium.org/3327016/diff/10002/11007#newcode129
chrome/browser/cocoa/table_model_array_controller.mm:129: [indexes
addIndex:i+offset];
On 2010/09/13 16:16:58, rsesek wrote:
> Spaces around the '+'

Done.

[Nico, 2010-09-13 17:21:35]

On Mon, Sep 13, 2010 at 10:18 AM,  <bauerb@chromium.org> wrote:
> On 2010/09/13 15:04:58, Nico wrote:
>>
>> http://codereview.chromium.org/3327016/diff/1/7
>> File chrome/browser/cocoa/table_model_array_controller.h (right):
>
>> http://codereview.chromium.org/3327016/diff/1/7#newcode20
>> chrome/browser/cocoa/table_model_array_controller.h:20: class
>> RemoveRowsObserverBridge : public TableModelObserver {
>> On 2010/09/13 13:12:14, Bernhard Bauer wrote:
>> > On 2010/09/10 16:47:10, Nico wrote:
>> > > can this move to the cc file?
>> >
>> > No, because TableModelArrayController contains a
>> > scoped_ptr<RemoveRowsObserverBridge>, so every class that owns a
>> > TableModelArrayController (in this case, the unit test) needs to know
>> > the
>
> full
>>
>> > type of RemoveRowsObserverBridge.
>
>> If I understand your reasoning correctly: This is wrong. A scoped_ptr
>> needs
>
> only
>>
>> a forward declaration of its pointee type, just like a normal pointer. The
>> reason for this is that the size of all pointers is equal (and a
>> scoped_ptr
>> internally has only a pointer). If a class has a class-type member, the
>
> compiler
>>
>> needs to know the members full type to know how big instances of the
>
> containing
>>
>> class are, but for pointers that's not true.
>
> Hm. I remember getting an error the first time I tried it, but now it seems
> to
> work. Oh well :-)
>
> On 2010/09/13 15:20:22, Nico wrote:
>>
>> http://codereview.chromium.org/3327016/diff/1/9
>> File chrome/browser/cocoa/table_model_array_controller_unittest.mm
>> (right):
>
>> http://codereview.chromium.org/3327016/diff/1/9#newcode80
>> chrome/browser/cocoa/table_model_array_controller_unittest.mm:80:
>> *CommandLine::ForCurrentProcess() = command_line_;
>> On 2010/09/13 13:12:14, Bernhard Bauer wrote:
>> > On 2010/09/10 16:47:10, Nico wrote:
>> > > eww! do have a ScopedFlagResetter or something like that? I know that
>> > something
>> > > like this exists in google3 at least.
>> >
>> > I don't think so, although I probably could use an
>> > AutoReset<CommandLine>
>
> for
>>
>> > it.
>> >
>> > > (If not, what pattern do other tests that need to modify the command
>> > > line
>> use?
>> >
>> > > do they inject the "enable" bool and don't modify the command line, or
>> > > do
>> they
>> > > manually do the resetting?)
>> >
>> > All other tests I've seen do the same thing.
>
>> :-(
>
>> > What do you mean by injecting the "enable" bool?
>
>> In java, classes don't check flags directly but have a bool that's true if
>> the
>> flag is on, and that flag is @injected by a guice binder that binds the
>
> correct
>>
>> flag value. Google for "dependency injection", "guice", etc if this you
>> think
>> this is interesting (hint: not really). But since c++ doesn't have a
>> runtime
>> system, dependency injection needs to be done manually.
>
>> One example would be to give the class that checks for the flag a static
>> CommandLine* member, and a static SetCommandLineForTesting() function that
>
> sets
>>
>> this, and then call that here with a local CommandLine object, instead of
>> overwriting the global command line object. (And if that static variable
>> is
>> NULL, the class would look at the global object instead).
>
>> Makes sense?
>
> Yeah, it does. Would you prefer that? I'm just a little bit wary of adding
> too
> much plumbing here, as the command line switch is only temporary until we
> have
> it landed fully.

I too would prefer a scoped commandline setter over injection here,
for the same reason (but the scoped setter would be useful in other
situations like these).

>
> http://codereview.chromium.org/3327016/show
>

[Bernhard Bauer, 2010-09-15 14:51:28]

On 2010/09/13 17:21:35, Nico wrote:
> I too would prefer a scoped commandline setter over injection here,
> for the same reason (but the scoped setter would be useful in other
> situations like these).

I used an AutoReset<CommandLine> instead of writing a separate class. Is that
okay?

[Nico, 2010-09-15 15:04:35]

LGTM