Enable keyboard shortcuts and some menu commands for browserless Panels.

Enable keyboard shortcuts and some menu commands for browserless Panels.

Prevent context menu in Panels.

BUG=127323
TEST=open panels and try various shortcuts and menu commands

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146580

[jennb, 2012-07-11 17:34:42]

Ben - could you review browser_commands change to make Zoom() usable without a
Browser?

Dimich - please review panel changes.

Thanks!

[Dmitry Titov, 2012-07-11 21:27:18]

LGTM for panels

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel.cc:167:
command_updater_.UpdateCommandEnabled(IDC_NEW_WINDOW, true);
It'd be nice to have a comment explaining why a panel need the ability to open a
new browser window...

[jennb, 2012-07-11 21:42:36]

Ping - Ben?


On Wed, Jul 11, 2012 at 2:27 PM, <dimich@chromium.org> wrote:

> LGTM for panels
>
>
> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/panels/panel.cc<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/panels/panel.cc>
> File chrome/browser/ui/panels/**panel.cc (right):
>
> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/panels/panel.cc#**newcode167<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/panels/panel.cc#newcode167>
> chrome/browser/ui/panels/**panel.cc:167:
> command_updater_.**UpdateCommandEnabled(IDC_NEW_**WINDOW, true);
> It'd be nice to have a comment explaining why a panel need the ability
> to open a new browser window...
>
>
Will do.


>
http://codereview.chromium.**org/10736037/<http://codereview.chromium.org/107...
>

[Ben Goodger (Google), 2012-07-11 22:28:26]

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_comm...
File chrome/browser/ui/browser_commands.cc (right):

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_comm...
chrome/browser/ui/browser_commands.cc:711: content::RenderViewHost* host =
web_contents->GetRenderViewHost();
I think you may need this in a different file since I don't think browserless
panels should include a file that mostly exists to support the browser.

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_comm...
File chrome/browser/ui/browser_commands.h (right):

http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_comm...
chrome/browser/ui/browser_commands.h:117: void Zoom(content::WebContents*
web_contents, Profile* profile,
newline

[jennb, 2012-07-11 23:31:55]

On Wed, Jul 11, 2012 at 3:28 PM, <ben@chromium.org> wrote:

>
> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/browser_commands.cc<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_commands.cc>
> File chrome/browser/ui/browser_**commands.cc (right):
>
> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/browser_commands.**cc#newcode711<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_commands.cc#newcode711>
> chrome/browser/ui/browser_**commands.cc:711: content::RenderViewHost* host
> = web_contents->**GetRenderViewHost();
> I think you may need this in a different file since I don't think
> browserless panels should include a file that mostly exists to support
> the browser.
>
>
Is chrome_page_zoom a logical choice? I'm hesitant to make that file pull
in WebContents, RenderViewHost, Profile, etc. Alternatively, I could just
copy this one function into the panel code.

Jenn


> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/browser_commands.h<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_commands.h>
> File chrome/browser/ui/browser_**commands.h (right):
>
> http://codereview.chromium.**org/10736037/diff/1/chrome/**
>
browser/ui/browser_commands.h#**newcode117<http://codereview.chromium.org/10736037/diff/1/chrome/browser/ui/browser_commands.h#newcode117>
> chrome/browser/ui/browser_**commands.h:117: void
> Zoom(content::WebContents* web_contents, Profile* profile,
> newline
>
>
http://codereview.chromium.**org/10736037/<http://codereview.chromium.org/107...
>

[Dmitry Titov, 2012-07-11 23:59:36]

Still looks good, although having to pull the set of zoom factors form browser
feels strange.

[Ben Goodger (Google), 2012-07-12 17:11:15]

Who else includes chrome_page_zoom.h?

It seems like you could add a Zoom() method to it that takes a content::PageZoom
and a content::WebContents*, and then call that from panel_host and
browser_commands.

[jennb, 2012-07-12 18:30:44]

On 2012/07/12 17:11:15, Ben Goodger (Google) wrote:
> Who else includes chrome_page_zoom.h?
> 

Only browser_commands and browser_option_handler.

> It seems like you could add a Zoom() method to it that takes a
content::PageZoom
> and a content::WebContents*, and then call that from panel_host and
> browser_commands.

Ah, nice. I didn't realize default_zoom_level could be retrieved from
WebContents directly. Done.

Ready for another look.

[Ben Goodger (Google), 2012-07-12 21:50:19]

lgtm

http://codereview.chromium.org/10736037/diff/12002/chrome/browser/chrome_page...
File chrome/browser/chrome_page_zoom.h (right):

http://codereview.chromium.org/10736037/diff/12002/chrome/browser/chrome_page...
chrome/browser/chrome_page_zoom.h:13: class WebContents;
outdent

[jennb, 2012-07-12 21:55:35]

On 2012/07/12 21:50:19, Ben Goodger (Google) wrote:
> lgtm
> 
>
http://codereview.chromium.org/10736037/diff/12002/chrome/browser/chrome_page...
> File chrome/browser/chrome_page_zoom.h (right):
> 
>
http://codereview.chromium.org/10736037/diff/12002/chrome/browser/chrome_page...
> chrome/browser/chrome_page_zoom.h:13: class WebContents;
> outdent

Done, thanks!

[commit-bot: I haz the power, 2012-07-12 21:57:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10736037/11004

[commit-bot: I haz the power, 2012-07-12 23:00:13]

Try job failure for 10736037-11004 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-12 23:48:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10736037/11004

[commit-bot: I haz the power, 2012-07-13 01:58:57]

Try job failure for 10736037-11004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...