Add observers to ContentSettingsDialogController so that the dialog will show...

Add observers to ContentSettingsDialogController so that the dialog will show
live updates in response to sycronization.

BUG=41335
TEST=Open the 'Content settings' dialog on two separate instances of Chromium
when both are synced to the same account.  Change settings on computer A,
settings should auto-update on computer B.

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

[csilv, 2010-05-25 21:03:11]

+thakis for review.

[Nico, 2010-05-25 21:14:35]

Hrm. Something like this needs to be happen on linux and windows too. Is there
some way to do that without having to basically copy the code from this CL into
the other two OS's view classes? Maybe the hostcontentsettings map could be
pref-aware? or you could add a model class that does the settingValue_ setting
and checking; possibly talking to the UI through an inner Delegate class which
does the UI updating?

http://codereview.chromium.org/2063020/diff/1/2
File chrome/browser/cocoa/content_settings_dialog_controller.h (right):

http://codereview.chromium.org/2063020/diff/1/2#newcode29
chrome/browser/cocoa/content_settings_dialog_controller.h:29: BOOL
settingValue_;
Needs a comment

http://codereview.chromium.org/2063020/diff/1/3
File chrome/browser/cocoa/content_settings_dialog_controller.mm (right):

http://codereview.chromium.org/2063020/diff/1/3#newcode157
chrome/browser/cocoa/content_settings_dialog_controller.mm:157: PrefService*
prefs = profile->GetPrefs();
nit: use profile_

http://codereview.chromium.org/2063020/diff/1/3#newcode244
chrome/browser/cocoa/content_settings_dialog_controller.mm:244: settingValue_ =
YES;
Add a test that shows the settingValue_ stuff is necessary please.

[csilv, 2010-05-26 21:11:11]

HostContentSettings is already pref-aware.  So the pref notifications exist for
linux and windows to take advantage of, but changes here are really all about
connecting the pref notifications to Cocoa's key-value observing.  I don't think
there's going to be a completely cross platform way to handle notifications,
although there are probably methods to make the interface between pref
notifcations and cocoa kvo more seemless.

http://codereview.chromium.org/2063020/diff/1/2
File chrome/browser/cocoa/content_settings_dialog_controller.h (right):

http://codereview.chromium.org/2063020/diff/1/2#newcode29
chrome/browser/cocoa/content_settings_dialog_controller.h:29: BOOL
settingValue_;
On 2010/05/25 21:14:35, Nico wrote:
> Needs a comment

Changed the implementation a bit, removed this member, so no changes for this
file.

http://codereview.chromium.org/2063020/diff/1/3
File chrome/browser/cocoa/content_settings_dialog_controller.mm (right):

http://codereview.chromium.org/2063020/diff/1/3#newcode157
chrome/browser/cocoa/content_settings_dialog_controller.mm:157: PrefService*
prefs = profile->GetPrefs();
On 2010/05/25 21:14:35, Nico wrote:
> nit: use profile_

Done.

http://codereview.chromium.org/2063020/diff/1/3#newcode244
chrome/browser/cocoa/content_settings_dialog_controller.mm:244: settingValue_ =
YES;
On 2010/05/25 21:14:35, Nico wrote:
> Add a test that shows the settingValue_ stuff is necessary please.

I changed the implementation to use a stack based class for ignoring
notifications.  I believe this creates a cleaner solution than manually toggling
a boolean.

Could you clarify how I could write a test that shows that the notifications
need to be disabled?  The reason this is needed is that the 'set' accessor
methods change the pref setting, which causes the NotificationObserver to be
invoked.  So that would lead to an infinite loop.  I don't see a way around
that, from the NotificationObserver, we don't have information to indicate who
changed the value.

[Nico, 2010-05-27 17:30:55]

LG, but please add a test or two.

[csilv, 2010-06-02 22:41:34]

Hi Nico,
I've added additional unit tests to exercise each of the settings managed by
this dialog.

[csilv, 2010-06-07 18:57:55]

ping thakis: could you give this another review at your convenience?  thx.

[Nico, 2010-06-08 18:43:53]

LG.

Thanks for the thorough tests

http://codereview.chromium.org/2063020/diff/10001/11001
File chrome/browser/cocoa/content_settings_dialog_controller.h (right):

http://codereview.chromium.org/2063020/diff/10001/11001#newcode61
chrome/browser/cocoa/content_settings_dialog_controller.h:61: @property(assign,
nonatomic) NSInteger geolocationSettingIndex;
Can you put these into a TestingAPI category (other header files do this too,
grep for "TestingAPI")

http://codereview.chromium.org/2063020/diff/10001/11003
File chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm
(right):

http://codereview.chromium.org/2063020/diff/10001/11003#newcode53
chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm:53:
ContentSettingsDialogController* controller = GetController();
Since every test builds and destructs this, you could do this in SetUp() (and
the -close in TearDown()) if you want

http://codereview.chromium.org/2063020/diff/10001/11003#newcode55
chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm:55: //
Change setting, check dialog property
Comments should end with a "." (in the whole file)

[csilv, 2010-06-09 20:12:40]

http://codereview.chromium.org/2063020/diff/10001/11001
File chrome/browser/cocoa/content_settings_dialog_controller.h (right):

http://codereview.chromium.org/2063020/diff/10001/11001#newcode61
chrome/browser/cocoa/content_settings_dialog_controller.h:61: @property(assign,
nonatomic) NSInteger geolocationSettingIndex;
Ah, thanks for the tip... still getting up to speed on Chromium as you can tell.
 Done.

http://codereview.chromium.org/2063020/diff/10001/11003
File chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm
(right):

http://codereview.chromium.org/2063020/diff/10001/11003#newcode53
chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm:53:
ContentSettingsDialogController* controller = GetController();
On 2010/06/08 18:43:53, Nico wrote:
> Since every test builds and destructs this, you could do this in SetUp() (and
> the -close in TearDown()) if you want

Good idea.  Done.

http://codereview.chromium.org/2063020/diff/10001/11003#newcode55
chrome/browser/cocoa/content_settings_dialog_controller_unittest.mm:55: //
Change setting, check dialog property
On 2010/06/08 18:43:53, Nico wrote:
> Comments should end with a "." (in the whole file)

Done.

[csilv, 2010-06-16 18:04:04]

Hi Nico,
Hope you had a good wwdc.  When you get a chance, could you give this a look to
see if it's ready to submit?  thx!

[Nico, 2010-06-16 18:05:49]

Sent you a LG 8 days ago?

On Wed, Jun 16, 2010 at 11:04 AM, <csilv@chromium.org> wrote:

> Hi Nico,
> Hope you had a good wwdc.  When you get a chance, could you give this a
> look to
> see if it's ready to submit?  thx!
>
>
> http://codereview.chromium.org/2063020/show
>

[csilv, 2010-06-16 18:06:48]

Ack, you did.  Sorry.. submitting!