[protector] Support for collapsing multiple changes into a single one.

[protector] Support for collapsing multiple changes into a single one.

Each collapsible change has a key (domain). Subsequent changes with the same key are collapsed into CompositeSettingsChange.


BUG=114298
TEST=functional:protector.py; browser_tests:ProtectorServiceTest.*; unit_tests:CompositeSettingsChangeTest.*


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

[Ivan Korotkov, 2012-03-30 11:34:38]

A big CL for a big weekend :)
PTAL!

[Ivan Korotkov, 2012-04-04 11:00:11]

Friendly ping

[Ivan Korotkov, 2012-04-06 08:48:56]

Friendly ping. It's been merged! Brand new and fresh!

[whywhat, 2012-04-06 16:28:32]

I would suggest you to find one more reviewer for this change and the other
changes you do for protector as I'm not sure I'll be able to do reviews in a
timely manner and a second owner (one in each MTV and MSK timezone) would be
nice to have. You would get more support when there're suspicions against it
working properly.
And don't scare me with "big review for big weekend" anymore :)

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:24: CompositeSettingsChange*
composite_change = new CompositeSettingsChange;
nit: I think we should use () always for constructor calls.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
Maybe this property should be returned by clearer boolean method? I.e.
IsCollapsible or smth like that?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.h:95: // Returns an empty string if
there is no specific representaion for new
s/representaion/representation
The function returns a pair, it can't return an empty string.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.h:101: // a single
MulipleSettingChange instance. Empty keywords are not considered
When do we have an empty keyword? When we don't know that the new setting is?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/composite_settings_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:22:
changes_.push_back(other);
Check for NULL?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:35:
changes_[i]->Apply(browser);
I think we should be careful about applying/discarding changes that interfer
(i.e. when different changes touch the same settings/backup). Do you think such
a thing might happen?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:91: //
CompositeSettingsChange should never be put inside another one.
Maybe this means the class hierarchy is wrong?
Maybe there should be CompositeSettingsChange and ComposableSettingChange and
MergeWith should receive Composable only?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/composite_settings_change.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.h:16: class
CompositeSettingsChange : public BaseSettingChange {
Comment about the class?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.h:38:
ScopedVector<BaseSettingChange> changes_;
Member comments, please.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/default_search_provider_change.cc (left):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/default_search_provider_change.cc:151: 
Could you, please, leave the blank line?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/default_search_provider_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/default_search_provider_change.cc:348: // like
"google.com" in that case (same in other changes), so that all google
using .com in this case could be confusing ("I thought I was using google.ru"
(c) user), maybe just Google?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:35: new_item.change.reset(change);
Maybe not do it here since we call release later in true branch of the if?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:42: Item* merger_item =
FindItemToMergeWith(change);
merger_item -> item_to_merge_with?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:45:
merger_item->change.release()->MergeWith(new_item.change.release());
Would split this into two statements with a comment why each release is needed.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:139: item->is_merging = false;
why do we have to false the fields here?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:169: : is_merging(false),
show_when_merged(false) {
each field on a separate line, please.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:182: // return item.change.get()
== other_;
remove the commented line?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:74: // Pair of error and
corresponding change instance. linked_ptr is used because
It's not a pair anymore.
BTW, maybe Items should be std::vector<linked_ptr<Item>> so change here and
error could be just pointers or scoped_ptrs?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:83: bool is_merging;
Why merging, not merged as in comment?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:86: bool show_when_merged;
This makes three states represented with two booleans.
Maybe use an enum with three constants?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:113: // Returns an Item instance
whose change can be merged with |change|, if any.
What if there're multiple items that can be merged?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/session_startup_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:29: const size_t
kNamePriority = 50U;
Maybe we should put the priorities in a single file so it's easy to figure out
which change has a greater one?
Otherwise I have to look up all priorities when adding a new change type to see
if the priority for it is correctly aligned with the others.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:61: GURL GetFirstNewURL()
const;
What if the first new URL is the same as the old one?
I.e. third party adds their homepage as the last one?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:156: string16 first_url =
UTF8ToUTF16(GetFirstNewURL().host());
so is it first_url or first_url_host?

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:178: GURL
SessionStartupChange::GetFirstNewURL() const {
I guess it could return .host() since it's what every caller is calling
afterwards.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/settings_change_global_error.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/settings_change_global_error.cc:168: // The bubble may
be closed as the result of RemoveFromProfile call.
Could you add more details on when RemoveFromProfile is called so that this
situation occurs?

[Ivan Korotkov, 2012-04-09 11:51:43]

Sure, sorry for scaring you :) I'll add sky@ for future reviews.

[Ivan Korotkov, 2012-04-09 14:46:46]

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:24: CompositeSettingsChange*
composite_change = new CompositeSettingsChange;
On 2012/04/06 16:28:32, whywhat wrote:
> nit: I think we should use () always for constructor calls.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
On 2012/04/06 16:28:32, whywhat wrote:
> Maybe this property should be returned by clearer boolean method? I.e.
> IsCollapsible or smth like that?

In most cases, the IsCollapsible will have the same code as GetApplyKeyword,
just with an .empty() check on end.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.h:95: // Returns an empty string if
there is no specific representaion for new
On 2012/04/06 16:28:32, whywhat wrote:
> s/representaion/representation
> The function returns a pair, it can't return an empty string.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.h:101: // a single
MulipleSettingChange instance. Empty keywords are not considered
On 2012/04/06 16:28:32, whywhat wrote:
> When do we have an empty keyword? When we don't know that the new setting is?

For settings which are not clear how to merge with others, like backup invalid
or startup changed to NTP.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/composite_settings_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:22:
changes_.push_back(other);
On 2012/04/06 16:28:32, whywhat wrote:
> Check for NULL?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:35:
changes_[i]->Apply(browser);
On 2012/04/06 16:28:32, whywhat wrote:
> I think we should be careful about applying/discarding changes that interfer
> (i.e. when different changes touch the same settings/backup). Do you think
such
> a thing might happen?

It shouldn't with the current *Change classes. The only change interfering with
others is BackupInvalid but it should never be added together with other Prefs
changes.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.cc:91: //
CompositeSettingsChange should never be put inside another one.
On 2012/04/06 16:28:32, whywhat wrote:
> Maybe this means the class hierarchy is wrong?
> Maybe there should be CompositeSettingsChange and ComposableSettingChange and
> MergeWith should receive Composable only?

I guess you're right but that sounds a little too much heavyweight for now :)

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/composite_settings_change.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.h:16: class
CompositeSettingsChange : public BaseSettingChange {
On 2012/04/06 16:28:32, whywhat wrote:
> Comment about the class?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/composite_settings_change.h:38:
ScopedVector<BaseSettingChange> changes_;
On 2012/04/06 16:28:32, whywhat wrote:
> Member comments, please.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/default_search_provider_change.cc (left):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/default_search_provider_change.cc:151: 
On 2012/04/06 16:28:32, whywhat wrote:
> Could you, please, leave the blank line?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/default_search_provider_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/default_search_provider_change.cc:348: // like
"google.com" in that case (same in other changes), so that all google
On 2012/04/06 16:28:32, whywhat wrote:
> using .com in this case could be confusing ("I thought I was using google.ru"
> (c) user), maybe just Google?

This "keyword" is only used for comparing changes to decide which should be
merged, user-visible name is returned by GetApplyDisplayName.

It's probably easier to fix the merging algorithm in ProtectorService to merge
changes with "google.*" keywords together.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:35: new_item.change.reset(change);
On 2012/04/06 16:28:32, whywhat wrote:
> Maybe not do it here since we call release later in true branch of the if?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:42: Item* merger_item =
FindItemToMergeWith(change);
On 2012/04/06 16:28:32, whywhat wrote:
> merger_item -> item_to_merge_with?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:45:
merger_item->change.release()->MergeWith(new_item.change.release());
On 2012/04/06 16:28:32, whywhat wrote:
> Would split this into two statements with a comment why each release is
needed.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:139: item->is_merging = false;
On 2012/04/06 16:28:32, whywhat wrote:
> why do we have to false the fields here?

So that when this error is really removed from profile, this path is not
executed.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:169: : is_merging(false),
show_when_merged(false) {
On 2012/04/06 16:28:32, whywhat wrote:
> each field on a separate line, please.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.cc:182: // return item.change.get()
== other_;
On 2012/04/06 16:28:32, whywhat wrote:
> remove the commented line?

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:74: // Pair of error and
corresponding change instance. linked_ptr is used because
On 2012/04/06 16:28:32, whywhat wrote:
> It's not a pair anymore.
> BTW, maybe Items should be std::vector<linked_ptr<Item>> so change here and
> error could be just pointers or scoped_ptrs? 

Fixed comment.
It's easier actually to work with vector<Item> than with
vector<linked_ptr<Item>> and the performance difference shouldn't be big here.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:83: bool is_merging;
On 2012/04/06 16:28:32, whywhat wrote:
> Why merging, not merged as in comment?

Fixed

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:86: bool show_when_merged;
On 2012/04/06 16:28:32, whywhat wrote:
> This makes three states represented with two booleans.
> Maybe use an enum with three constants?

Hm, it doesn't look like a tri-state flag to merge into a single enum, I think.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:113: // Returns an Item instance
whose change can be merged with |change|, if any.
On 2012/04/06 16:28:32, whywhat wrote:
> What if there're multiple items that can be merged?

Merging strategy is (and should be) transitive so it shouldn't happen :)

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/session_startup_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:29: const size_t
kNamePriority = 50U;
On 2012/04/06 16:28:32, whywhat wrote:
> Maybe we should put the priorities in a single file so it's easy to figure out
> which change has a greater one?
> Otherwise I have to look up all priorities when adding a new change type to
see
> if the priority for it is correctly aligned with the others.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:61: GURL GetFirstNewURL()
const;
On 2012/04/06 16:28:32, whywhat wrote:
> What if the first new URL is the same as the old one?
> I.e. third party adds their homepage as the last one?

I considered that a non-common use case but you're right, ideally we should
compare new and old URLs and pick the first in new that wasn't present in old.
Added a TODO.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:156: string16 first_url =
UTF8ToUTF16(GetFirstNewURL().host());
On 2012/04/06 16:28:32, whywhat wrote:
> so is it first_url or first_url_host?

Fixed.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/session_startup_change.cc:178: GURL
SessionStartupChange::GetFirstNewURL() const {
On 2012/04/06 16:28:32, whywhat wrote:
> I guess it could return .host() since it's what every caller is calling
> afterwards.

Done.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/settings_change_global_error.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/settings_change_global_error.cc:168: // The bubble may
be closed as the result of RemoveFromProfile call.
On 2012/04/06 16:28:32, whywhat wrote:
> Could you add more details on when RemoveFromProfile is called so that this
> situation occurs?

Done.

[commit-bot: I haz the power, 2012-04-09 16:53:44]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[whywhat, 2012-04-10 09:55:44]

LGTM

I'd also expect all your longer-than-one-word replies to make it into comments
above the code questioned.

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> On 2012/04/06 16:28:32, whywhat wrote:
> > Maybe this property should be returned by clearer boolean method? I.e.
> > IsCollapsible or smth like that?
> 
> In most cases, the IsCollapsible will have the same code as GetApplyKeyword,
> just with an .empty() check on end.

So? I see nothing wrong with that :)

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:86: bool show_when_merged;
On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> On 2012/04/06 16:28:32, whywhat wrote:
> > This makes three states represented with two booleans.
> > Maybe use an enum with three constants?
> 
> Hm, it doesn't look like a tri-state flag to merge into a single enum, I
think.

Both booleans above define three states out of possible four as you state above
(show_when_merged is meaningful only when is_merged is true)

[Ivan Korotkov, 2012-04-10 11:19:49]

Added comments in those places.

Thanks for a big review, won't scare you with such ones anymore :)

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
On 2012/04/10 09:55:44, whywhat wrote:
> On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> > On 2012/04/06 16:28:32, whywhat wrote:
> > > Maybe this property should be returned by clearer boolean method? I.e.
> > > IsCollapsible or smth like that?
> > 
> > In most cases, the IsCollapsible will have the same code as GetApplyKeyword,
> > just with an .empty() check on end.
> 
> So? I see nothing wrong with that :)

Er, well, code duplication is bad isn't it? :)
If in future there happen to be changes that have no specific keyword but still
can be merged into a "whatever-change" and, OTOH, changes that absolutely cannot
be merged, I'll add something like CanBeMerged().

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/protector_service.h (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/protector_service.h:86: bool show_when_merged;
On 2012/04/10 09:55:44, whywhat wrote:
> On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> > On 2012/04/06 16:28:32, whywhat wrote:
> > > This makes three states represented with two booleans.
> > > Maybe use an enum with three constants?
> > 
> > Hm, it doesn't look like a tri-state flag to merge into a single enum, I
> think.
> 
> Both booleans above define three states out of possible four as you state
above
> (show_when_merged is meaningful only when is_merged is true)

I meant that, enum would be good if those were 3 distinct states but (single,
merged and merged,needs show) don't look like distinct states, 2 of them are
nearly the same apart from the show bit.

[whywhat, 2012-04-10 11:22:07]

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
On 2012/04/10 11:19:49, Ivan Korotkov wrote:
> On 2012/04/10 09:55:44, whywhat wrote:
> > On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> > > On 2012/04/06 16:28:32, whywhat wrote:
> > > > Maybe this property should be returned by clearer boolean method? I.e.
> > > > IsCollapsible or smth like that?
> > > 
> > > In most cases, the IsCollapsible will have the same code as
GetApplyKeyword,
> > > just with an .empty() check on end.
> > 
> > So? I see nothing wrong with that :)
> 
> Er, well, code duplication is bad isn't it? :)
> If in future there happen to be changes that have no specific keyword but
still
> can be merged into a "whatever-change" and, OTOH, changes that absolutely
cannot
> be merged, I'll add something like CanBeMerged().

There would be none if IsCollapsible() would be implemented like { return
!GetApplyKeyword().empty() } and the nuance of empty keyword meaning something
special would be hidden.

[Ivan Korotkov, 2012-04-10 11:33:17]

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
File chrome/browser/protector/base_setting_change.cc (right):

https://chromiumcodereview.appspot.com/9968007/diff/14001/chrome/browser/prot...
chrome/browser/protector/base_setting_change.cc:57: return std::string();
On 2012/04/10 11:22:08, whywhat wrote:
> On 2012/04/10 11:19:49, Ivan Korotkov wrote:
> > On 2012/04/10 09:55:44, whywhat wrote:
> > > On 2012/04/09 14:46:47, Ivan Korotkov wrote:
> > > > On 2012/04/06 16:28:32, whywhat wrote:
> > > > > Maybe this property should be returned by clearer boolean method? I.e.
> > > > > IsCollapsible or smth like that?
> > > > 
> > > > In most cases, the IsCollapsible will have the same code as
> GetApplyKeyword,
> > > > just with an .empty() check on end.
> > > 
> > > So? I see nothing wrong with that :)
> > 
> > Er, well, code duplication is bad isn't it? :)
> > If in future there happen to be changes that have no specific keyword but
> still
> > can be merged into a "whatever-change" and, OTOH, changes that absolutely
> cannot
> > be merged, I'll add something like CanBeMerged().
> 
> There would be none if IsCollapsible() would be implemented like { return
> !GetApplyKeyword().empty() } and the nuance of empty keyword meaning something
> special would be hidden.

Ok, you've convinced me :)

[commit-bot: I haz the power, 2012-04-11 12:19:36]

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

[commit-bot: I haz the power, 2012-04-11 12:19:44]

Presubmit check for 9968007-26002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  chrome/browser/protector/composite_settings_change_unittest.cc

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2012-04-11 12:24:35]

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

[commit-bot: I haz the power, 2012-04-11 13:00:07]

Try job failure for 9968007-18007 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-04-11 14:09:10]

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

[commit-bot: I haz the power, 2012-04-11 15:44:52]

Change committed as 131773