Contributed by Eriq Augustine <eriq.augustine@gmail.com>

Ensure one radio button is always checked in extension context menus.

This patch changes the behavior on all systems to match the behavior on Linux
ie. there is always exactly one item selected in a run of radio items.

This patch includes a unit test (ExtensionMenuManagerTest.SanitizeRadioButtons).

BUG=49808
TEST=On any OS but Linux, load an extension that has a context menu without
items selected by defuat. One item shuold be selected, but none are.

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

[clintstaley, 2011-12-14 23:39:48]

"so that and run" => "so that a run" ??  In comments in extension_menu_manager.h

Thanks for including all our names on the authors, Eriq, but it should probably
be just you, since you did all the work here :) and we wouldn't know how to fix
your stuff.

One question: this seems like a post-facto repair on the radio buttons.  There
was no way to make them behave correctly on Linux by some inherent change?

[Aaron Boodman, 2011-12-15 00:43:21]

+asargent (original author of this code)

Hooray unit tests.

It looks like ExtensionMenuManager already tries to preserve this invariant in
some cases (e.g., ::RadioItemSelected, which is called for the Add() case and
the Execute() case). 

It seems like it would be more consistent to do something like that in the
update and remove cases, rather than the post-facto fix up.

What do you think, Antony?

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_context_menu_api.cc (right):

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_context_menu_api.cc:356: if
(manager->RemoveContextMenuItem(id)) {
We usually prefer the opposite pattern:

if (!somethingThatCanFail())
  return false;

finalCleanupStuff();
return true;

[Aaron Boodman, 2011-12-15 00:44:01]

Also, you will need to submit a CLA if you haven't done that already:

http://code.google.com/legal/individual-cla-v1.0.html

[Aaron Boodman, 2011-12-15 00:53:55]

Some style nits if we end up going with this solution.

I'll leave the bigger picture to Antony.

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.cc:464: void
ExtensionMenuManager::SanitizeRadioButtons() {
You could pass in the extension ID to cut down on the work being done here.

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.cc:482:
ExtensionMenuItem::List::const_iterator radio_run_ittr;
s/ittr/iter/g ?

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.cc:482:
ExtensionMenuItem::List::const_iterator radio_run_ittr;
Better to declare this right about the loop where it is used for clarity.

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_menu_manager_unittest.cc (right):

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager_unittest.cc:555: 
Remove extra blank lines. There should be exactly one '\n' at the end of the
file.

[asargent_no_longer_on_chrome, 2011-12-15 18:38:39]

>It looks like ExtensionMenuManager already tries to preserve this 
> invariant in some cases (e.g., ::RadioItemSelected, which is 
> called for the Add() case and the Execute() case). 
>
> It seems like it would be more consistent to do something like 
> that in the update and remove cases, rather than the post-facto 
> fix up.
>
> What do you think, Antony?

In private email I had suggested the approach of a sanitize method. Seeing the
code, I agree that it seems slightly funny to rely on a consumer of the
ExtensionMenuManager to tell it when to enforce what essentially should be a
correctness invariant it maintains internally. We could make the sanitize method
private, and call it from within the various ExtensionMenuManager methods that
change state like AddContextItem, AddChildItem, ChangeParent, and
RemoveContextMenuItem. This would leave the one case of how to handle updates
properly - right now in response to API calls we can end up changing the
type/selectedness of an item by just getting a raw pointer to an
ExtensionMenuItem and calling methods on it, without notifying
ExtensionMenuManager. Maybe for that case we'd want to add an "ItemUpdated"
method on ExtensionMenuManager to notify it that we modified an item.

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_menu_manager.h (right):

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.h:253: // Sanitize radio
buttons so that and run of radio buttons does not
typo: "and run" -> "a run"

http://codereview.chromium.org/8935016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.h:269: // Recursivly looks for
a run of radio items. If on is found that
typo: "Recursivly" -> "Recursively"
typo: "If on" -> "If one"

[asargent_no_longer_on_chrome, 2011-12-15 18:42:12]

Oh one nit about the CL description: the typical style is:

"One line (<80 chars) overview description.

Zero or more paragraphs with extra comments.

BUG=(bug number)
TEST=(test instructions)"

So in your case the first line of the description should be something along the
lines of "Ensure one radio button is always checked in extension context menus".
And in the extra comments, you can skip mentioning the bug number since that's
already in the BUG= line.

[Eriq, 2011-12-15 19:07:42]

On 2011/12/15 18:38:39, Antony Sargent wrote:
> We could make the sanitize method
> private, and call it from within the various ExtensionMenuManager methods that
> change state like AddContextItem, AddChildItem, ChangeParent, and
> RemoveContextMenuItem. This would leave the one case of how to handle updates
> properly - right now in response to API calls we can end up changing the
> type/selectedness of an item by just getting a raw pointer to an
> ExtensionMenuItem and calling methods on it, without notifying
> ExtensionMenuManager. Maybe for that case we'd want to add an "ItemUpdated"
> method on ExtensionMenuManager to notify it that we modified an item.

If we do this, then we can make a much simpler sanitize method that just 
looks around that specific item. (Similar to the behavior of 
::RadioItemSelected).

[asargent_no_longer_on_chrome, 2011-12-15 19:34:33]

> If we do this, then we can make a much simpler sanitize method that just 
> looks around that specific item. (Similar to the behavior of 
> ::RadioItemSelected).

Yep, that's what I was thinking. Let's go that direction.

[Eriq, 2012-01-05 23:20:37]

Ok. Changed it as per Antony's suggestion.

[asargent_no_longer_on_chrome, 2012-01-06 21:17:42]

Just a few minor issues.

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.cc:148: }
nit: other code uses the optional style practice of omitting braces for
single-line bodies of if statements, so yours should follow that too (here and
in your other edits in the file). E.g.

if (condition)
  foo();
else
  bar();

instead of:

if (condition) {
  foo();
} else {
  bar();
}

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.cc:472: if ((*i)->type() ==
ExtensionMenuItem::RADIO) {
nit: you could improve readability slightly by making an early out for the
non-radio case. E.g.

while (i != item_list.end()) {
  if ((*i)->type() != ExtensionMenuItem::RADIO) {
    ++i;
    break;
  }
  ... rest of code, now indented one level less ...
}

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.cc:490: // check the first
radio item in the list (i).
nit: this comment isn't quite right. It could be something like: "Reset the last
checked item we found to checked. If we didn't find any checked items, set the
first one to checked."

Alternatively, you could just modify the comment several lines above to be
something like "Uncheck any checked radio items in the run, and at the end reset
the appropriate one to checked."

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_menu_manager.h (right):

http://codereview.chromium.org/8935016/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.h:274: void
SanitizeRadioList(const ExtensionMenuItem::List& item_list);
Since you potentially modify some of |item_list|'s members by calling SetChecked
on them, you should pass a "ExtensionMenuItem::List*" instead of "const
ExtensionMenuItem::List&" to indicate to callers that this method modifies its
contents.

[eaugusti, 2012-01-07 02:14:40]

> SanitizeRadioList(const ExtensionMenuItem::List& item_list);
> Since you potentially modify some of |item_list|'s members by calling
SetChecked
> on them, you should pass a "ExtensionMenuItem::List*" instead of "const
> ExtensionMenuItem::List&" to indicate to callers that this method modifies its
> contents.

Using "ExtensionMenuItem::List*" instead of "const ExtensionMenuItem::List&" 
would force me to directly reference children_ instead of calling the accessor
since children() returns a "const ExtensionMenuItem::List&". I am a little 
reserved about referencing the children directly instead of calling the
accessor,
but if you are okay with it then I am.

[asargent_no_longer_on_chrome, 2012-01-11 01:09:07]

LGTM

> Using "ExtensionMenuItem::List*" instead of "const ExtensionMenuItem::List&" 
> would force me to directly reference children_ instead of calling the accessor
> since children() returns a "const ExtensionMenuItem::List&". I am a little 
> reserved about referencing the children directly instead of calling the
> accessor,
> but if you are okay with it then I am.

That's a good point - I think it's actually better to leave it they way you had
it before. (I think I had been mistakenly thinking that the typedef for
ExtensionMenuItem::List was std::vector<const ExtensionMenuItem*>, but it is
just
std::vector<ExtensionMenuItem*>).

[Eriq, 2012-01-11 01:35:18]

Ok, changed "ExtensionMenuItem::List*" back to 
"const ExtensionMenuItem::List&".

[asargent_no_longer_on_chrome, 2012-01-11 18:02:52]

Great - do you need me to land this for you, or does the commit queue work?

[Eriq, 2012-01-11 19:32:13]

I think I need you to land it for me.

Thanks.

[commit-bot: I haz the power, 2012-01-11 20:56:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Eriq.Augustine@gmail.com/8935016/27001

[commit-bot: I haz the power, 2012-01-11 22:01:09]

Change committed as 117301