Update Panel UI looks on Windows.

Update Panel UI looks on Windows.
1) Add back wrench menu.
2) Remove about panel bubble.
3) Differentiate active and inactive panel by using different chrome theme.

BUG=none
TEST=panel_browser_view_browsertest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87703

[Dmitry Titov, 2011-06-01 18:37:27]

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:662:
wrench_menu_contents_->AddItem(COMMAND_NAME, UTF8ToUTF16(extension->name()));
It seems that with this, we don't have any clue whatsoever that comes from
Chrome, right? Both title of Panel and menu top item show the name that comes
from extension itself.. Are we ok with that?

[Dmitry Titov, 2011-06-01 20:06:20]

Answering my own question above and few new questions:

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:461: case COMMAND_DISABLE:
Should we also Close the Panel here? Same for Uninstall. 

Also, If we show a dialog for uninstall, why not show similar dialog on Disable?
They feel similar.

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:466:
extension_uninstall_dialog_.reset(new ExtensionUninstallDialog(
Do we need to ensure the panel is not closed (possibly making dislog disappear)
while user is still thinking on answer? We had to do this for Notifications.

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:662:
wrench_menu_contents_->AddItem(COMMAND_NAME, UTF8ToUTF16(extension->name()));
Offline discussion shows we are ok with that, on a premise that popups already
do that and there are links leading to Chrome tabs in that menu anyways. Will
monitor feedback.

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:87: virtual void
ExtensionDialogAccepted();
OVERRIDE?

[jianli, 2011-06-01 20:39:50]

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:461: case COMMAND_DISABLE:
On 2011/06/01 20:06:20, Dmitry Titov wrote:
> Should we also Close the Panel here? Same for Uninstall. 
> 
It is not needed for both Close and Uninstall. Since when an extension is
unloaded, as in close and uninstall case, Browser object will receive
NotificationType::EXTENSION_UNLOADED event and  close all the tab contents,
which finally causes Browser::CloseFrame being called and it calls Panel::Close.

> Also, If we show a dialog for uninstall, why not show similar dialog on
Disable?
> They feel similar.

I am doing the same thing as ExtensionContextMenuModel::ExecuteCommand.

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:466:
extension_uninstall_dialog_.reset(new ExtensionUninstallDialog(
On 2011/06/01 20:06:20, Dmitry Titov wrote:
> Do we need to ensure the panel is not closed (possibly making dislog
disappear)
> while user is still thinking on answer? We had to do this for Notifications.

The panel can be closed since the uninstall dialog is a model dialog.

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:87: virtual void
ExtensionDialogAccepted();
On 2011/06/01 20:06:20, Dmitry Titov wrote:
> OVERRIDE?

Done.

[Dmitry Titov, 2011-06-01 20:49:51]

On 2011/06/01 20:39:50, jianli wrote:
> 
> > Also, If we show a dialog for uninstall, why not show similar dialog on
> Disable?
> > They feel similar.
> 
> I am doing the same thing as ExtensionContextMenuModel::ExecuteCommand.

I understand that it's a copy of functionality. However, since we are copying
it, I am asking if this is the right thing to do. 

> 
>
http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
> chrome/browser/ui/panels/panel_browser_frame_view.cc:466:
> extension_uninstall_dialog_.reset(new ExtensionUninstallDialog(
> On 2011/06/01 20:06:20, Dmitry Titov wrote:
> > Do we need to ensure the panel is not closed (possibly making dislog
> disappear)
> > while user is still thinking on answer? We had to do this for Notifications.
> 
> The panel can be closed since the uninstall dialog is a model dialog.

Extension can close it via API. If that causes both Panel and dialog to abruptly
close on the user, it's not very good.

[jennb, 2011-06-01 22:24:14]

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:98: ButtonResources
wrench_button_resources;
s/wrench/settings/g

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:425: if (command_id ==
COMMAND_NAME) {
Would a switch statement be easier to read?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:435: return true;
Should return false for any unrecognized command_id?  With a NOTREACHED?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:458:
browser->GetProfile()->GetExtensionProcessManager()->OpenOptionsPage(
Does this open in a new foreground tab?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:473: SINGLETON_TAB,
If there is no chrome://extensions page already open, does this result in a new
foreground tab?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:645: const Extension*
PanelBrowserFrameView::GetExtension() const {
Is this a utility all Panel instances, regardless of platform, will need?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:93:
FRIEND_TEST_ALL_PREFIXES(PanelBrowserViewTest, ShowOrHideWrenchButton);
Can we avoid using Wrench in the name of things? That way, if the icon changes,
we don't have to rename all these functions.  SettingsButton?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:101: enum {
Can you give the enum a name like MenuCommand ?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:158: void
EnsureCreateWrenchMenu();
EnsureSettingsMenuCreated?

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:171: void
ValidateWrenchMenuItems(ui::SimpleMenuModel* wrench_menu_contents,
which parameter is the expected menu items?

[jianli, 2011-06-01 23:23:38]

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:466:
extension_uninstall_dialog_.reset(new ExtensionUninstallDialog(
On 2011/06/01 20:06:20, Dmitry Titov wrote:
> Do we need to ensure the panel is not closed (possibly making dislog
disappear)
> while user is still thinking on answer? We had to do this for Notifications.

Added a TODO item as discussed.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:98: ButtonResources
wrench_button_resources;
On 2011/06/01 22:24:14, jennb wrote:
> s/wrench/settings/g

Done.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:425: if (command_id ==
COMMAND_NAME) {
On 2011/06/01 22:24:14, jennb wrote:
> Would a switch statement be easier to read?

Done.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:435: return true;
On 2011/06/01 22:24:14, jennb wrote:
> Should return false for any unrecognized command_id?  With a NOTREACHED?

Done.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:458:
browser->GetProfile()->GetExtensionProcessManager()->OpenOptionsPage(
On 2011/06/01 22:24:14, jennb wrote:
> Does this open in a new foreground tab?

Yes.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:473: SINGLETON_TAB,
On 2011/06/01 22:24:14, jennb wrote:
> If there is no chrome://extensions page already open, does this result in a
new
> foreground tab?

Yes, we will open a extension tab when it is not present. If it is there, it
will get selected. I think this is SINGLETON_TAB means.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.cc:645: Browser* browser =
browser_view_->browser();
On 2011/06/01 22:24:14, jennb wrote:
> Is this a utility all Panel instances, regardless of platform, will need?

Moved it as Panel::GetExtension.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:93:
FRIEND_TEST_ALL_PREFIXES(PanelBrowserViewTest, ShowOrHideWrenchButton);
On 2011/06/01 22:24:14, jennb wrote:
> Can we avoid using Wrench in the name of things? That way, if the icon
changes,
> we don't have to rename all these functions.  SettingsButton?

Done.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:101: enum {
On 2011/06/01 22:24:14, jennb wrote:
> Can you give the enum a name like MenuCommand ?

The reason for using anonymous enum is to avoid referring to enum type by
mistake. The type of menu command id is int and we do not want to refer to enum
type directly in our code.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_frame_view.h:158: void
EnsureCreateWrenchMenu();
On 2011/06/01 22:24:14, jennb wrote:
> EnsureSettingsMenuCreated?

Done.

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/6975062/diff/12/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:171: void
ValidateWrenchMenuItems(ui::SimpleMenuModel* wrench_menu_contents,
On 2011/06/01 22:24:14, jennb wrote:
> which parameter is the expected menu items?

Renamed items to prefix with expected.

[jennb, 2011-06-01 23:45:53]

LGTM  - please wait for dimich

http://codereview.chromium.org/6975062/diff/10002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/6975062/diff/10002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:409:
EnsureCreateSettingsMenu();
usual wording is EnsureSettingsMenuCreated()

[Dmitry Titov, 2011-06-01 23:55:03]

LGTM

thanks for clarifying all the questions! (and asking Ken on UX ones)