Views: Rebuild wrench menu if error is added or removed

Views: Rebuild wrench menu if error is added or removed

This CL rebuilds the wrench menu model if a new GlobalError object is added or removed.

BUG=96608
TEST=


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

[sky, 2011-10-07 19:40:17]

http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
chrome/browser/ui/views/toolbar_view.cc:342: if (!is_wrench_menu_model_valid_) {
Is this really enough? What happens if a global error is removed while the menu
is showing?

Also, I'm fine with us creating and destroying the menu each time through.
Unless there is a reason not to.

[sail, 2011-10-07 20:42:08]

http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
chrome/browser/ui/views/toolbar_view.cc:342: if (!is_wrench_menu_model_valid_) {
On 2011/10/07 19:40:17, sky wrote:
> Is this really enough? What happens if a global error is removed while the
menu
> is showing?

I believe if the bookmark model changes while the menu is open we close the
menu. Should I do the same thing here?

> Also, I'm fine with us creating and destroying the menu each time through.
> Unless there is a reason not to.

Yea, that might be even simpler. When would I destroy the menu though?

[sky, 2011-10-07 21:46:09]

On 2011/10/07 20:42:08, sail wrote:
>
http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
> File chrome/browser/ui/views/toolbar_view.cc (right):
> 
>
http://codereview.chromium.org/8201006/diff/1/chrome/browser/ui/views/toolbar...
> chrome/browser/ui/views/toolbar_view.cc:342: if (!is_wrench_menu_model_valid_)
{
> On 2011/10/07 19:40:17, sky wrote:
> > Is this really enough? What happens if a global error is removed while the
> menu
> > is showing?
> 
> I believe if the bookmark model changes while the menu is open we close the
> menu. Should I do the same thing here?

That's ok for the bookmark model as in theory only the user should have
initiated the change so that we should really never get into the situation of
the menu showing and the model changing (sync and extensions may break that
assumption, but I'm ignoring those). Global errors don't feel the same, in so
far as my understanding is they can randomly happen without user interaction.
That said, if you think they are infrequent enough than closing the menu works
for me.

> > Also, I'm fine with us creating and destroying the menu each time through.
> > Unless there is a reason not to.
> 
> Yea, that might be even simpler. When would I destroy the menu though?

After the run loop returns?

  -Scott

[sail, 2011-10-10 16:42:07]

Fixed to always build the menu each time. Also closes the menu if an error is
added or removed.
Please take another look. Thanks.

[sky, 2011-10-10 17:28:53]

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.h (left):

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.h:206: scoped_ptr<WrenchMenu> wrench_menu_;
Keep this one as a member, otherwise we can run into weird deletion ordering and
stack unwinding problems (see note in MenuRunner for more details).

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:573:
Source<Profile>(browser_->profile()));
Do we need original profile here, or is profile() ok?

[sail, 2011-10-10 17:47:01]

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.h (left):

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.h:206: scoped_ptr<WrenchMenu> wrench_menu_;
On 2011/10/10 17:28:53, sky wrote:
> Keep this one as a member, otherwise we can run into weird deletion ordering
and
> stack unwinding problems (see note in MenuRunner for more details).

Done.

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/8201006/diff/11001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:573:
Source<Profile>(browser_->profile()));
On 2011/10/10 17:28:53, sky wrote:
> Do we need original profile here, or is profile() ok?

That's a good question. For the errors that I'm adding, I wouldn't want them to
appear on the incognito browser so I think using profile() is ok.

Also, this is what I've been doing for GTK and Cocoa as well. I think once we've
used this more we can revisit this.

[sky, 2011-10-10 21:13:47]

LGTM

http://codereview.chromium.org/8201006/diff/14001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/8201006/diff/14001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:781: root_->Cancel();
Add a comment as to why we need this.

[sail, 2011-10-10 21:18:24]

http://codereview.chromium.org/8201006/diff/14001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/8201006/diff/14001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:781: root_->Cancel();
On 2011/10/10 21:13:47, sky wrote:
> Add a comment as to why we need this.

Done.

[commit-bot: I haz the power, 2011-10-10 21:18:41]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8201006/14003

[commit-bot: I haz the power, 2011-10-10 22:38:08]

Change committed as 104808