Adjusts panel ifdefs for aura

Adjusts panel ifdefs for aura

I believe this gives us the behavior we're after.

BUG=289535
TEST=see bug
R=dimich@chromium.org, mpcomplete@chromium.org, stevenjb@chromium.org

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

[Dmitry Titov, 2013-11-05 05:53:07]

Thanks a lot Scott!

Couple of notes:

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs_api.cc:207: #if defined(OS_WIN) &&
!defined(USE_AURA)
IsSingleWindowMetroMode() returns false if USE_ASH. I think it was needed for
old mode when we had a single browser window (no Ash) in Metro. If so, is this
USE_AURA check needed?

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs_api.cc:535: chrome::GetActiveDesktop()
== chrome::HOST_DESKTOP_TYPE_ASH) {
There should be an else branch that creates a panel via PanelManager - in case
when GetActiveDesktop() == HOST_DESKTOP_TYPE_NATIVE. Now this code is under
"#else" below, so it won't get compiled in. 

I think we still want to create native panels when browser runs on Desktop.

https://codereview.chromium.org/58873002/diff/1/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://codereview.chromium.org/58873002/diff/1/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_manager.cc:127: #if defined(OS_WIN) &&
!defined(USE_AURA)
same as in other file, I don't see why we need test for USE_AURA here, unless I
miss something.

[sky, 2013-11-05 15:49:36]

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs_api.cc:207: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2013/11/05 05:53:07, Dmitry Titov wrote:
> IsSingleWindowMetroMode() returns false if USE_ASH. I think it was needed for
> old mode when we had a single browser window (no Ash) in Metro. If so, is this
> USE_AURA check needed?

You are right. I was trying to make it clear this isn't needed for aura. But
I'll remove the USE_AURA.

https://codereview.chromium.org/58873002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs_api.cc:535: chrome::GetActiveDesktop()
== chrome::HOST_DESKTOP_TYPE_ASH) {
On 2013/11/05 05:53:07, Dmitry Titov wrote:
> There should be an else branch that creates a panel via PanelManager - in case
> when GetActiveDesktop() == HOST_DESKTOP_TYPE_NATIVE. Now this code is under
> "#else" below, so it won't get compiled in. 
> 
> I think we still want to create native panels when browser runs on Desktop.

I think you're saying nuke the ifdef here. Done. GetActiveDesktop() is safe to
call on non-aura/ash.

[stevenjb, 2013-11-05 16:31:41]

lgtm

[Dmitry Titov, 2013-11-05 17:47:18]

lgtm with small nit:

https://codereview.chromium.org/58873002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/58873002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_api.cc:533: if
(PanelManager::ShouldUsePanels(extension_id) &&
This check for ShouldUsePanels can be removed since create_panel variable
checked above already means the same thing, it does this check earlier.

[sky, 2013-11-05 18:07:54]

https://codereview.chromium.org/58873002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/58873002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_api.cc:533: if
(PanelManager::ShouldUsePanels(extension_id) &&
On 2013/11/05 17:47:19, Dmitry Titov wrote:
> This check for ShouldUsePanels can be removed since create_panel variable
> checked above already means the same thing, it does this check earlier.

Done.

[commit-bot: I haz the power, 2013-11-05 18:10:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/58873002/160001

[commit-bot: I haz the power, 2013-11-05 18:50:01]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps,
chromedriver2_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[sky, 2013-11-05 20:17:03]

I had to add back an ifdef as AshPanelContents only makes sense on ash. Diff
between 5&6 to see differences. Take another look?

[Matt Perry, 2013-11-05 20:22:29]

I don't know enough about this to review it. The other 2 LGTMs should be good
enough.

[stevenjb, 2013-11-05 20:34:43]

lgtm

[Dmitry Titov, 2013-11-05 21:29:31]

Lgtm 2

[commit-bot: I haz the power, 2013-11-05 21:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/58873002/300002

[commit-bot: I haz the power, 2013-11-05 23:14:37]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 00:26:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/58873002/300002

[commit-bot: I haz the power, 2013-11-06 04:44:42]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 14:59:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/58873002/300002

[commit-bot: I haz the power, 2013-11-06 18:40:20]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[sky, 2013-11-06 22:43:57]

Committed patchset #6 manually as r233402 (presubmit successful).

[Dmitry Titov, 2013-11-06 22:51:30]

I will take care of merging it into m32 once we have this on Canary for a couple
days.

[sky, 2013-11-07 01:36:24]

Thanks! I passed the bug your way.

  -Scott

On Wed, Nov 6, 2013 at 2:51 PM,  <dimich@chromium.org> wrote:
> I will take care of merging it into m32 once we have this on Canary for a
> couple
> days.
>
> https://codereview.chromium.org/58873002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.