Allow panels to be created as detached panels.

Allow panels to be created as detached panels.

Default position is supplied if not specified in chrome.windows.create.
Cleaned up window/popup/panel bounds logic in CreateWindowFunction.

TBR=yoshiki,stevenjb

BUG=145615
TEST=New tests added.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155071
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=155080

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

[jennb, 2012-08-31 22:58:33]

jianli - for overall review
mihai - for extensions changes
stevenjb - make sure my bounds changes in CreateWindowFunction (tabs.cc) didn't
break ChromeOS. Should OS_CHROMEOS be changed to USE_ASH in the '#if defined'?

Thanks,
Jenn

[jianli, 2012-08-31 23:25:55]

Some initial reviews.

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:15: const int kPanelTilePixels
= 10;
Why choosing 10? Could different platforms have different metrics?

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:246:
default_panel_origin_.SetPoint(kPanelTilePixels + display_area.x(),
Why starting from left top corner? Would it be better if we could keep shifting
the default panel origin?

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:247: kPanelTilePixels +
display_area.y());
nit: space

[jennb, 2012-08-31 23:31:14]

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:15: const int kPanelTilePixels
= 10;
On 2012/08/31 23:25:55, jianli wrote:
> Why choosing 10? Could different platforms have different metrics?

kWindowTilePixels in WindowSizer is always 10. That one is allowed to be
different per platform, but all set it to 10 so I just use 10 here rather than
have 3 places that all set it to 10.

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:246:
default_panel_origin_.SetPoint(kPanelTilePixels + display_area.x(),
On 2012/08/31 23:25:55, jianli wrote:
> Why starting from left top corner? Would it be better if we could keep
shifting
> the default panel origin?

This is the first position only. After that, it starts stair-stepping downwards.

http://codereview.chromium.org/10919046/diff/1/chrome/browser/ui/panels/detac...
chrome/browser/ui/panels/detached_panel_strip.cc:247: kPanelTilePixels +
display_area.y());
On 2012/08/31 23:25:55, jianli wrote:
> nit: space

Done.

[jianli, 2012-09-04 21:58:47]

http://codereview.chromium.org/10919046/diff/20/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs.cc:546:
PanelManager::GetInstance()->GetDefaultDetachedPanelOrigin());
Why not just passing (0, 0) and letting PanelManager to figure out the default
position?

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
chrome/browser/ui/panels/detached_panel_strip.cc:79:
default_panel_origin_.Offset(kPanelTilePixels, kPanelTilePixels);
What if next default panel origin is out of screen when too many detached panels
are created?

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
chrome/browser/ui/panels/detached_panel_strip.h:70: const gfx::Point&
GetDefaultPanelOrigin();
Safer to return gfx::Point.

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_manager.cc:240: if (CREATE_DOCKED == mode)
Probably simpler to say as:
  PanelStrip* panel_strip;
  PanelStrip::PositioningMask positioning_mask;
  if (CREATE_DOCKED == mode) {
    panel_strip = docked_strip_;
    positioning_mask = PanelStrip::DELAY_LAYOUT_REFRESH;
  } else {
    ...
  }
  panel_strip->AddPanel(panel, positioning_mask);
  panel_strip->UpdatePanelOnStripChange(panel);

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_manager.h:32: CREATE_DOCKED,  // Creates a docked
panel. The default.
Maybe CREATE_AS_DOCKED?

[jennb, 2012-09-04 23:02:43]

http://codereview.chromium.org/10919046/diff/20/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/extensions/api...
chrome/browser/extensions/api/tabs/tabs.cc:546:
PanelManager::GetInstance()->GetDefaultDetachedPanelOrigin());
On 2012/09/04 21:58:47, jianli wrote:
> Why not just passing (0, 0) and letting PanelManager to figure out the default
> position?

(0, 0) is a valid position. We wanted a way to distinguish between the app
asking for (0, 0) vs the app asking for the default position.

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
chrome/browser/ui/panels/detached_panel_strip.cc:79:
default_panel_origin_.Offset(kPanelTilePixels, kPanelTilePixels);
On 2012/09/04 21:58:47, jianli wrote:
> What if next default panel origin is out of screen when too many detached
panels
> are created?

Will modify to "wrap-around". Detached panel bounds are also adjusted to fit on
screen when a panel is created.

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/deta...
chrome/browser/ui/panels/detached_panel_strip.h:70: const gfx::Point&
GetDefaultPanelOrigin();
On 2012/09/04 21:58:47, jianli wrote:
> Safer to return gfx::Point.

Done.

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_manager.cc:240: if (CREATE_DOCKED == mode)
On 2012/09/04 21:58:47, jianli wrote:
> Probably simpler to say as:
>   PanelStrip* panel_strip;
>   PanelStrip::PositioningMask positioning_mask;
>   if (CREATE_DOCKED == mode) {
>     panel_strip = docked_strip_;
>     positioning_mask = PanelStrip::DELAY_LAYOUT_REFRESH;
>   } else {
>     ...
>   }
>   panel_strip->AddPanel(panel, positioning_mask);
>   panel_strip->UpdatePanelOnStripChange(panel);

Done.

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/10919046/diff/20/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_manager.h:32: CREATE_DOCKED,  // Creates a docked
panel. The default.
On 2012/09/04 21:58:47, jianli wrote:
> Maybe CREATE_AS_DOCKED?

Done.

[jianli, 2012-09-05 20:21:40]

lgtm

http://codereview.chromium.org/10919046/diff/11004/chrome/browser/ui/panels/d...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/11004/chrome/browser/ui/panels/d...
chrome/browser/ui/panels/detached_panel_strip.cc:252: void
DetachedPanelStrip::OffsetDefaultPanelOrigin() {
nit: Probably better name like ComputeNextDefaultPanelOrigin?

[Mihai Parparita -not on Chrome, 2012-09-05 20:36:59]

LGTM

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/window_open/panel_detached/test.js
(right):

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
chrome/test/data/extensions/api_test/window_open/panel_detached/test.js:6:
chrome.test.assertEq('panel', win.type);
Nit: indented one too many times.

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
chrome/test/data/extensions/api_test/window_open/panel_detached/test.js:21: {
'url': chrome.extension.getURL('about:blank'),
chrome.extension.getURL('about:blank') doesn't quite make sense. getURL is meant
for getting URLs of resources in the extension packages, but about:blank isn't a
resource. You should be able to use about:blank directly.

[jennb, 2012-09-05 21:11:07]

http://codereview.chromium.org/10919046/diff/11004/chrome/browser/ui/panels/d...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10919046/diff/11004/chrome/browser/ui/panels/d...
chrome/browser/ui/panels/detached_panel_strip.cc:252: void
DetachedPanelStrip::OffsetDefaultPanelOrigin() {
On 2012/09/05 20:21:40, jianli wrote:
> nit: Probably better name like ComputeNextDefaultPanelOrigin?

Done.

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/window_open/panel_detached/test.js
(right):

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
chrome/test/data/extensions/api_test/window_open/panel_detached/test.js:6:
chrome.test.assertEq('panel', win.type);
On 2012/09/05 20:37:00, Mihai Parparita wrote:
> Nit: indented one too many times.

Done.

http://codereview.chromium.org/10919046/diff/11004/chrome/test/data/extension...
chrome/test/data/extensions/api_test/window_open/panel_detached/test.js:21: {
'url': chrome.extension.getURL('about:blank'),
On 2012/09/05 20:37:00, Mihai Parparita wrote:
> chrome.extension.getURL('about:blank') doesn't quite make sense. getURL is
meant
> for getting URLs of resources in the extension packages, but about:blank isn't
a
> resource. You should be able to use about:blank directly.

Good catch, thanks! Copy/paste/edit fail fixed.

[commit-bot: I haz the power, 2012-09-05 21:21:50]

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

[commit-bot: I haz the power, 2012-09-06 00:30:40]

Change committed as 155071

[commit-bot: I haz the power, 2012-09-06 04:46:45]

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

[commit-bot: I haz the power, 2012-09-06 09:10:53]

Change committed as 155152