[DevTools] Handle frontend zoom similarly to tab zoom.

[DevTools] Handle frontend zoom similarly to tab zoom.

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

[dgozman, 2014-02-12 13:09:05]

Could you please take a look?

[vsevik, 2014-02-12 15:13:42]

lgtm

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1769: SupportsWindowFeature(FEATURE_LOCATIONBAR));
This will disable zoom bubble for Chrome Apps.
I don't think you should do this, we should fix how it looks when there is no
location bar instead.

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:896:
browser->tab_strip_model()->GetActiveWebContents());
nit: You could extract browser->tab_strip_model()->GetActiveWebContents()

[jochen (gone - plz use gerrit), 2014-02-12 15:17:42]

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1769: SupportsWindowFeature(FEATURE_LOCATIONBAR));
On 2014/02/12 15:13:43, vsevik wrote:
> This will disable zoom bubble for Chrome Apps.
> I don't think you should do this, we should fix how it looks when there is no
> location bar instead.

I agree, this should be addressed before we land this

[dgozman, 2014-02-12 16:27:15]

Take another look please.

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1769: SupportsWindowFeature(FEATURE_LOCATIONBAR));
On 2014/02/12 15:13:43, vsevik wrote:
> This will disable zoom bubble for Chrome Apps.
> I don't think you should do this, we should fix how it looks when there is no
> location bar instead.

Apps don't have zoom, and they have not decided yet on the UI. See
crbug.com/227175.

Undocked DevTools is the only case here right now, and it's ok without UI
indication. I've added a comment explaining.

Fixing this for Apps (which seems like a thing apps team will definitely do)
will also improve DevTools case.

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:896:
browser->tab_strip_model()->GetActiveWebContents());
On 2014/02/12 15:13:43, vsevik wrote:
> nit: You could extract browser->tab_strip_model()->GetActiveWebContents()

Done.

[vsevik, 2014-02-13 08:02:26]

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1769: SupportsWindowFeature(FEATURE_LOCATIONBAR));
I agree that DevTools do not need UI for zoom (we didn't have it before and can
add one in the frontend if needed).
Let's disable it for DevTools explicitly then. (&& !is_devtools())

[jochen (gone - plz use gerrit), 2014-02-13 08:57:14]

lgtm with !is_devtools()

[dgozman, 2014-02-13 10:08:25]

Thank you for review.

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/139983006/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1769: SupportsWindowFeature(FEATURE_LOCATIONBAR));
On 2014/02/13 08:02:26, vsevik wrote:
> I agree that DevTools do not need UI for zoom (we didn't have it before and
can
> add one in the frontend if needed).
> Let's disable it for DevTools explicitly then. (&& !is_devtools())

Done.

[dgozman, 2014-02-13 12:54:52]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-02-13 12:55:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/139983006/130001

[commit-bot: I haz the power, 2014-02-13 14:41:11]

Change committed as 250993