ProtectorService supports multiple change instances.

ProtectorService supports multiple change instances.


BUG=None
TEST=ProtectorServiceTest.*, protector.py


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

[Ivan Korotkov, 2012-02-29 16:57:40]

https://chromiumcodereview.appspot.com/9500020/diff/1/chrome/browser/protecto...
File chrome/browser/protector/settings_change_global_error.cc (right):

https://chromiumcodereview.appspot.com/9500020/diff/1/chrome/browser/protecto...
chrome/browser/protector/settings_change_global_error.cc:169: #if
!defined(TOOLKIT_GTK)
Disregard that, please (comes from a different CL).

[Ivan Korotkov, 2012-02-29 17:27:28]

PTAL
This is an architectural CL that make protector -> change and protector -> error
relations one-to-many.

[whywhat, 2012-02-29 18:55:14]

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:70: linked_ptr<BaseSettingChange>
change;
Something makes me think you need a map actually.

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:75: class MatchItemByChange {
Why can't you define the classes in the cc file?

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:90: class MatchItemByError {
Or maybe two maps :)

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/protector_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service_browsertest.cc:37: 
Default arguments are forbidden by the style guide afair. Remove the blank line.

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service_browsertest.cc:51: bool
IsGlobalErrorActive(BaseSettingChange* change = NULL) {
Ditto.

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/settings_change_global_error.h (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/settings_change_global_error.h:110: static
std::bitset<IDC_SHOW_SETTINGS_CHANGE_LAST -
Using statis non-elementary types members is also forbidden. Could be a pointer
to bitset though.
Will static bitset work with multiple-profiles?

[Ivan Korotkov, 2012-02-29 19:23:44]

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:70: linked_ptr<BaseSettingChange>
change;
On 2012/02/29 18:55:14, whywhat wrote:
> Something makes me think you need a map actually.

As discussed, it's not really needed because cross-lookups are pretty rare.

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:75: class MatchItemByChange {
On 2012/02/29 18:55:14, whywhat wrote:
> Why can't you define the classes in the cc file?

Done :(

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service.h:90: class MatchItemByError {
On 2012/02/29 18:55:14, whywhat wrote:
> Or maybe two maps :)

Same here.

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/protector_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service_browsertest.cc:37: 
On 2012/02/29 18:55:14, whywhat wrote:
> Default arguments are forbidden by the style guide afair. Remove the blank
line.

Done :((

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/protector_service_browsertest.cc:51: bool
IsGlobalErrorActive(BaseSettingChange* change = NULL) {
On 2012/02/29 18:55:14, whywhat wrote:
> Ditto.

Done :(((

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
File chrome/browser/protector/settings_change_global_error.h (right):

https://chromiumcodereview.appspot.com/9500020/diff/3001/chrome/browser/prote...
chrome/browser/protector/settings_change_global_error.h:110: static
std::bitset<IDC_SHOW_SETTINGS_CHANGE_LAST -
On 2012/02/29 18:55:14, whywhat wrote:
> Using statis non-elementary types members is also forbidden. Could be a
pointer
> to bitset though.
> Will static bitset work with multiple-profiles?

As discussed, it's used application-wide for vectors/maps.

[whywhat, 2012-03-01 19:19:28]

lgtm

[Ivan Korotkov, 2012-03-01 19:24:00]

Yay!

[commit-bot: I haz the power, 2012-03-01 19:24:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/9500020/9002

[commit-bot: I haz the power, 2012-03-01 21:17:53]

Change committed as 124482