Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015123003:
Extension window.create API accepts state. (Closed)
|
Created:
5 years, 9 months ago by limasdf Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtension window.create API accepts state.
Adding new 'state' parameter. the 'state' has
["normal", "minimized", "maximized", "fullscreen"] which is same with 'state' parameter from window.update API.
BUG=459841
TEST=browser_tests
Committed: https://crrev.com/a7c5d3d68ff8d035e59d0a24fc9ffb78a3e6fdb8
Cr-Commit-Position: refs/heads/master@{#325782}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Seperate patch, first add 'state' param #Patch Set 3 : switch default #
Total comments: 7
Patch Set 4 : update test #Patch Set 5 : Minimize(), Maximize() after Show() #
Total comments: 1
Patch Set 6 : Make manually minimized if initial_state == minizized #
Total comments: 4
Patch Set 7 : typo #Patch Set 8 : minimized test #
Total comments: 10
Patch Set 9 : rebase and review comment #
Total comments: 1
Patch Set 10 : consider chromeos #
Total comments: 4
Patch Set 11 : mention crbug #
Total comments: 6
Patch Set 12 : grammer #
Messages
Total messages: 46 (9 generated)
limasdf@gmail.com changed reviewers: + kalman@chromium.org
PTAL! when you have time
Awesome. There are 2 changes here, adding the new 'state' parameter to create, and adding validation. Could you submit these as 2 separate changes? I'd suggest firstly adding the 'state' parameter then after that the validation logic. I would find that easier to spot which changes are for what. https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:196: default: No default case; have the NOTREACHED() outside the switch statement. Better compiler errors.
On 2015/03/19 15:43:45, kalman wrote: > Awesome. > > There are 2 changes here, adding the new 'state' parameter to create, and adding > validation. Could you submit these as 2 separate changes? I'd suggest firstly > adding the 'state' parameter then after that the validation logic. I would find > that easier to spot which changes are for what. > > https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... > File chrome/browser/extensions/api/tabs/tabs_api.cc (right): > > https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/tabs/tabs_api.cc:196: default: > No default case; have the NOTREACHED() outside the switch statement. Better > compiler errors. => Could you submit these as 2 separate changes? Done. updated as "adding 'state' param" only. Thank you Ben. ptal when you have time-
https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:196: default: On 2015/03/19 15:43:45, kalman wrote: > No default case; have the NOTREACHED() outside the switch statement. Better > compiler errors. Done.
https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:621: controller->SetFullscreenMode(true, extension()->url()); Can you double-check this works? I'm just wondering if setting fullscreen mode before Show() is called means that it shows fullscreen initially. https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:623: if (create_params.initial_show_state == ui::SHOW_STATE_MINIMIZED) Also, also double checking: do you need to manually Minimize()/Maximize() this, or does setting the initial show state automatically do that? If so, do you actually need to manually set focus yourself? https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:587: EXPECT_TRUE(RunCreateWindow("[{\"state\": \"fullscreen\"}]")); What is this testing? If anything I would have thought you need to look up the update() tests and see how it tests fullscreen/minimized/etc, and possibly adapt those for create().
please! https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:621: controller->SetFullscreenMode(true, extension()->url()); On 2015/03/19 17:56:31, kalman wrote: > Can you double-check this works? I'm just wondering if setting fullscreen mode > before Show() is called means that it shows fullscreen initially. It works well. and even more natural. If I move this after Show(), it looks like two steps(Show window first and then changed to full screen). https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:623: if (create_params.initial_show_state == ui::SHOW_STATE_MINIMIZED) On 2015/03/19 17:56:31, kalman wrote: > Also, also double checking: do you need to manually Minimize()/Maximize() this, > or does setting the initial show state automatically do that? If so, do you > actually need to manually set focus yourself? => do you need to manually Minimize()/Maximize() this No. Because |create_params.initial_show_state| is used. and newly created native window uses that value. From Ubuntu, I can see http://goo.gl/vibu8g is called when |create_params.initial_show_state| is SHOW_STATE_MINIMIZED. => does setting the initial show state automatically do that? Yes. => do you actually need to manually set focus yourself? No. for minimized state, the only action required is 'calling ShowInactive() instead of Show()'. I don't think minimized window need to get focused. Show() gets focus itself. http://goo.gl/Ehy8aM https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:587: EXPECT_TRUE(RunCreateWindow("[{\"state\": \"fullscreen\"}]")); On 2015/03/19 17:56:31, kalman wrote: > What is this testing? > > If anything I would have thought you need to look up the update() tests and see > how it tests fullscreen/minimized/etc, and possibly adapt those for create(). Done. But fullscreen test is added only. It looks like BaseWindow::Maximized, BaseWindow::IsMaxmized doesn't work properly. this is same in window.update funciton as well. plus, I'll add more test from next CL(Validate param).
Sorry. Hold on please. This doesn't seem to work on Mac. sorry. I'll check again.
kalman@chromium.org changed reviewers: + tapted@chromium.org
Ok. I am also away next week - perhaps Trent can take over as reviewer if we don't manage to get this done today (I am also super busy doing some admin stuff today)
Patchset #5 (id:80001) has been deleted
Hi taped! :) Could you take a look when you have time? https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:623: if (create_params.initial_show_state == ui::SHOW_STATE_MINIMIZED) On 2015/03/20 17:03:46, limasdf wrote: > On 2015/03/19 17:56:31, kalman wrote: > > Also, also double checking: do you need to manually Minimize()/Maximize() > this, > > or does setting the initial show state automatically do that? If so, do you > > actually need to manually set focus yourself? > > => do you need to manually Minimize()/Maximize() this > No. Because |create_params.initial_show_state| is used. and newly created native > window uses that value. From Ubuntu, I can see http://goo.gl/vibu8g is called > when |create_params.initial_show_state| is SHOW_STATE_MINIMIZED. > > => does setting the initial show state automatically do that? > Yes. > => do you actually need to manually set focus yourself? > No. for minimized state, the only action required is 'calling ShowInactive() > instead of Show()'. I don't think minimized window need to get focused. > > Show() gets focus itself. http://goo.gl/Ehy8aM The initial show state doesn't automatically Minimize()/Maximize(). especially on Mac. So I've added Minimize()/ Maximize() after Show().
It makes sense that maximise doesn't work on Mac since there is no such thing. But minimize should work - did you figure out why? (Is it a bug?) I've just had a skim - looks good. I'm travelling this week for work, but should be able to have a closer look at this at some point. https://codereview.chromium.org/1015123003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:45: class ExtensionWindowCreateTest : public InProcessBrowserTest {}; I think the preferred way to do this now is using ExtensionWindowCreateTest = InProcessBrowserTest; might as well change the one above so things are consistent.
patch set 4 - doesn't work. just pass initial_show_state when create |Browser|. patch set 5 - Works. pass initial_show_state when create |Browser|. and call Minimize() as well. I'm wondering why initial_show_state doesn't work except from 'fullscreen'. I'll debug it. Have a good travel :)
Patchset #6 (id:120001) has been deleted
PTAL. the window cannot initially become minimized. so manually make it minimized. It's same as 'fullscreen'. kalman, I think you're busy when you're back. please take a look when you have time after relaxation.
Thanks :-) https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:630: // Make window minizmied. Because Window cann't be initially minimized. s/minizmied/minimized/ ; s/cann't/can't/ https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:576: EXPECT_TRUE(new_window->window()->IsFullscreen()); also test minimised
~_~ https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:630: // Make window minizmied. Because Window cann't be initially minimized. On 2015/03/30 18:00:09, kalman wrote: > s/minizmied/minimized/ ; s/cann't/can't/ Done. Thanks. https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:576: EXPECT_TRUE(new_window->window()->IsFullscreen()); On 2015/03/30 18:00:09, kalman wrote: > also test minimised I cannot write minimized test for now. Because, Interestingly, IsMinimized test fail only on Linux. I've just simply debugged below code. ... window()->Minimized(); LOG(INFO) << window()->IsMiniized(); ... result is 0. but window is actually minimized. Maybe it's a bug? Here's try bot result, https://codereview.chromium.org/1044093004/
On 2015/03/31 12:36:11, limasdf wrote: > ~_~ > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_api.cc (right): > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_api.cc:630: // Make window minizmied. > Because Window cann't be initially minimized. > On 2015/03/30 18:00:09, kalman wrote: > > s/minizmied/minimized/ ; s/cann't/can't/ > > Done. Thanks. > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_test.cc:576: > EXPECT_TRUE(new_window->window()->IsFullscreen()); > On 2015/03/30 18:00:09, kalman wrote: > > also test minimised > > I cannot write minimized test for now. > Because, Interestingly, IsMinimized test fail only on Linux. > I've just simply debugged below code. > > ... > window()->Minimized(); > LOG(INFO) << window()->IsMiniized(); > ... > > result is 0. but window is actually minimized. > > Maybe it's a bug? > Here's try bot result, https://codereview.chromium.org/1044093004/ Interesting - are you testing on linux, and can verify that it's working?
by testing I mean by hand, not unit testing.
On 2015/03/31 14:40:38, kalman wrote:
> by testing I mean by hand, not unit testing.
Sorry that I didn't mentioned previously.
Sure. I tested from both Linux and Mac.
Here's result. Test cases file is attached in crbug.com/459841#c11
fullscreen| maximized| minimized| normal| wrong_state
---------------------------------------------------------------
Linux O O O O O[1]
---------------------------------------------------------------
Mac O X[2] O O O[1]
[1] As expected, "Invalid value for argument".
[2] Mac originally doesn't support maximized window. Created window is visually
same with "normal" state.
On 2015/03/31 12:36:11, limasdf wrote: > ~_~ > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_api.cc (right): > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_api.cc:630: // Make window minizmied. > Because Window cann't be initially minimized. > On 2015/03/30 18:00:09, kalman wrote: > > s/minizmied/minimized/ ; s/cann't/can't/ > > Done. Thanks. > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_test.cc:576: > EXPECT_TRUE(new_window->window()->IsFullscreen()); > On 2015/03/30 18:00:09, kalman wrote: > > also test minimised > > I cannot write minimized test for now. > Because, Interestingly, IsMinimized test fail only on Linux. > I've just simply debugged below code. > > ... > window()->Minimized(); > LOG(INFO) << window()->IsMiniized(); > ... > > result is 0. but window is actually minimized. > > Maybe it's a bug? > Here's try bot result, https://codereview.chromium.org/1044093004/ On (desktop) linux, window operations are often asynchronous. See e.g. `WMStateWaiter` from desktop_window_tree_host_x11_unittest -- I think something like `WMStateWaiter waiter(xid, "_NET_WM_STATE_HIDDEN", true);` would ensure a minimize had propagated to the window server. That's platform-specific (to Linux), but you could achieve the same with a views::WidgetObserver. That's also platform-specific (to toolkit-views platforms, i.e. not-Mac), so also not ideal. I'd suggest skipping the EXPECTs for IsMinimized on Desktop Linux ( #if defined(OS_LINUX) && !defined(OS_CHROMEOS) ), with a comment like ~ `DesktopWindowTreeHostX11::IsMinimized() relies on an asynchronous update from the window server.`
Patchset #8 (id:180001) has been deleted
On 2015/04/01 00:48:26, tapted wrote: > On 2015/03/31 12:36:11, limasdf wrote: > > ~_~ > > > > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > > File chrome/browser/extensions/api/tabs/tabs_api.cc (right): > > > > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/api/tabs/tabs_api.cc:630: // Make window minizmied. > > Because Window cann't be initially minimized. > > On 2015/03/30 18:00:09, kalman wrote: > > > s/minizmied/minimized/ ; s/cann't/can't/ > > > > Done. Thanks. > > > > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > https://codereview.chromium.org/1015123003/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/api/tabs/tabs_test.cc:576: > > EXPECT_TRUE(new_window->window()->IsFullscreen()); > > On 2015/03/30 18:00:09, kalman wrote: > > > also test minimised > > > > I cannot write minimized test for now. > > Because, Interestingly, IsMinimized test fail only on Linux. > > I've just simply debugged below code. > > > > ... > > window()->Minimized(); > > LOG(INFO) << window()->IsMiniized(); > > ... > > > > result is 0. but window is actually minimized. > > > > Maybe it's a bug? > > Here's try bot result, https://codereview.chromium.org/1044093004/ > > On (desktop) linux, window operations are often asynchronous. See e.g. > `WMStateWaiter` from desktop_window_tree_host_x11_unittest -- I think something > like `WMStateWaiter waiter(xid, "_NET_WM_STATE_HIDDEN", true);` would ensure a > minimize had propagated to the window server. That's platform-specific (to > Linux), but you could achieve the same with a views::WidgetObserver. That's also > platform-specific (to toolkit-views platforms, i.e. not-Mac), so also not ideal. > > I'd suggest skipping the EXPECTs for IsMinimized on Desktop Linux ( #if > defined(OS_LINUX) && !defined(OS_CHROMEOS) ), with a comment like ~ > `DesktopWindowTreeHostX11::IsMinimized() relies on an asynchronous update from > the window server.` Thank you for the infomation. Interesting. There could be diffrent behavior for Desktop Linux window. That might be unavoidable issue to support multi platform? Anyway, I've added test for minimized state. Thanks again!
https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); These additions still don't look right to me. On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call just above. On non-Mac, views::Widget::Show() checks for saved_show_state_ being maximized, and for IsFullscreen(). saved_show_state_ is set in Widget::SetInitialBounds() using GetSavedWindowPlacement() which uses widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView implements WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() calls chrome::GetSavedWindowBoundsAndShowState() and other things, ending up in WindowSizer::DetermineWindowBoundsAndShowState() here, there's a `#if defined(USE_ASH)` which is probably out of date, since that's now true on everything but Mac -- the code in this function below that #ifdef is probably dead code. Ignoring that, it still does `*show_state = GetWindowDefaultShowState();` GetWindowDefaultShowState() returns browser_->initial_show_state(), and this should be the value that's passed in on line 585 above. So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already handling this? Having separate calls after the call to Show() will result in in-between states -- really it needs to be dealt with properly on the initial Show(), using either NativeWidget::ShowMaximizedWithBounds() or NativeWidget::ShowWithWindowState() https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:576: EXPECT_TRUE(new_window->window()->IsFullscreen()); EXPECT_TRUE(error.empty()) ? https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:585: window_id, &error); same here, immediately after this line. Perhaps also `EXPECT_TRUE(new_window);` Just so desktop-linux has something to gnaw on.
https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); On 2015/04/01 03:41:55, tapted wrote: > These additions still don't look right to me. > > On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being > MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call just > above. > > On non-Mac, views::Widget::Show() checks for saved_show_state_ being maximized, > and for IsFullscreen(). saved_show_state_ is set in Widget::SetInitialBounds() > using GetSavedWindowPlacement() which uses > widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView implements > WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() calls > chrome::GetSavedWindowBoundsAndShowState() and other things, ending up in > > WindowSizer::DetermineWindowBoundsAndShowState() > > here, there's a `#if defined(USE_ASH)` which is probably out of date, since > that's now true on everything but Mac -- the code in this function below that > #ifdef is probably dead code. Ignoring that, it still does > > `*show_state = GetWindowDefaultShowState();` > > GetWindowDefaultShowState() returns browser_->initial_show_state(), and this > should be the value that's passed in on line 585 above. > > So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already > handling this? > > Having separate calls after the call to Show() will result in in-between states > -- really it needs to be dealt with properly on the initial Show(), using either > NativeWidget::ShowMaximizedWithBounds() or NativeWidget::ShowWithWindowState() Yeah. views::Widget::Show() make newly made window visible.(minimzie->normal). but BrowserWindowCocoa::Show() doesn't. I'll create new issue. and try to fix it first for ideal implementation! Thank you for detail information.
https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); On 2015/04/01 23:59:45, limasdf wrote: > On 2015/04/01 03:41:55, tapted wrote: > > These additions still don't look right to me. > > > > On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being > > MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call just > > above. > > > > On non-Mac, views::Widget::Show() checks for saved_show_state_ being > maximized, > > and for IsFullscreen(). saved_show_state_ is set in Widget::SetInitialBounds() > > using GetSavedWindowPlacement() which uses > > widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView > implements > > WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() calls > > chrome::GetSavedWindowBoundsAndShowState() and other things, ending up in > > > > WindowSizer::DetermineWindowBoundsAndShowState() > > > > here, there's a `#if defined(USE_ASH)` which is probably out of date, since > > that's now true on everything but Mac -- the code in this function below that > > #ifdef is probably dead code. Ignoring that, it still does > > > > `*show_state = GetWindowDefaultShowState();` > > > > GetWindowDefaultShowState() returns browser_->initial_show_state(), and this > > should be the value that's passed in on line 585 above. > > > > So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already > > handling this? > > > > Having separate calls after the call to Show() will result in in-between > states > > -- really it needs to be dealt with properly on the initial Show(), using > either > > NativeWidget::ShowMaximizedWithBounds() or NativeWidget::ShowWithWindowState() > > Yeah. views::Widget::Show() make newly made window visible.(minimzie->normal). > but BrowserWindowCocoa::Show() doesn't. > > I'll create new issue. and try to fix it first for ideal implementation! > > Thank you for detail information. I found that minimized window creation from X11 is not implemented yet. so it act like "normal" state. I filed an issue, crbug.com/473228
https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); On 2015/04/02 16:57:48, limasdf wrote: > On 2015/04/01 23:59:45, limasdf wrote: > > On 2015/04/01 03:41:55, tapted wrote: > > > These additions still don't look right to me. > > > > > > On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being > > > MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call > just > > > above. > > > > > > On non-Mac, views::Widget::Show() checks for saved_show_state_ being > > maximized, > > > and for IsFullscreen(). saved_show_state_ is set in > Widget::SetInitialBounds() > > > using GetSavedWindowPlacement() which uses > > > widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView > > implements > > > WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() > calls > > > chrome::GetSavedWindowBoundsAndShowState() and other things, ending up in > > > > > > WindowSizer::DetermineWindowBoundsAndShowState() > > > > > > here, there's a `#if defined(USE_ASH)` which is probably out of date, since > > > that's now true on everything but Mac -- the code in this function below > that > > > #ifdef is probably dead code. Ignoring that, it still does > > > > > > `*show_state = GetWindowDefaultShowState();` > > > > > > GetWindowDefaultShowState() returns browser_->initial_show_state(), and this > > > should be the value that's passed in on line 585 above. > > > > > > So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already > > > handling this? > > > > > > Having separate calls after the call to Show() will result in in-between > > states > > > -- really it needs to be dealt with properly on the initial Show(), using > > either > > > NativeWidget::ShowMaximizedWithBounds() or > NativeWidget::ShowWithWindowState() > > > > Yeah. views::Widget::Show() make newly made window visible.(minimzie->normal). > > > but BrowserWindowCocoa::Show() doesn't. > > > > I'll create new issue. and try to fix it first for ideal implementation! > > > > Thank you for detail information. > > I found that minimized window creation from X11 is not implemented yet. > so it act like "normal" state. > I filed an issue, crbug.com/473228 So is it just X11 that's affected? reading DesktopWindowTreeHostX11::MapWindow() it sounds like these lines would never have worked reliably on X11, since minimize/maximize hints may be ignored until the window is mapped, which happens asynchronously. So, if you take out the calls here to Minimize() and SetFullscreenMode(), does the CL work everywhere except desktop linux? IMO it's OK to leave desktop linux as a TODO - just disable the test there and reference http://crbug.com/473228 . Then a follow-up can make necessary changes to DesktopWindowTreeHostX11 to give minimize/fullscreen the same treatment as maximize (e.g. similar to DesktopWindowTreeHostX11::should_maximize_after_map_).
Thank you for the review! https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); On 2015/04/15 00:36:12, tapted wrote: > On 2015/04/02 16:57:48, limasdf wrote: > > On 2015/04/01 23:59:45, limasdf wrote: > > > On 2015/04/01 03:41:55, tapted wrote: > > > > These additions still don't look right to me. > > > > > > > > On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being > > > > MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call > > just > > > > above. > > > > > > > > On non-Mac, views::Widget::Show() checks for saved_show_state_ being > > > maximized, > > > > and for IsFullscreen(). saved_show_state_ is set in > > Widget::SetInitialBounds() > > > > using GetSavedWindowPlacement() which uses > > > > widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView > > > implements > > > > WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() > > calls > > > > chrome::GetSavedWindowBoundsAndShowState() and other things, ending up in > > > > > > > > WindowSizer::DetermineWindowBoundsAndShowState() > > > > > > > > here, there's a `#if defined(USE_ASH)` which is probably out of date, > since > > > > that's now true on everything but Mac -- the code in this function below > > that > > > > #ifdef is probably dead code. Ignoring that, it still does > > > > > > > > `*show_state = GetWindowDefaultShowState();` > > > > > > > > GetWindowDefaultShowState() returns browser_->initial_show_state(), and > this > > > > should be the value that's passed in on line 585 above. > > > > > > > > So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already > > > > handling this? > > > > > > > > Having separate calls after the call to Show() will result in in-between > > > states > > > > -- really it needs to be dealt with properly on the initial Show(), using > > > either > > > > NativeWidget::ShowMaximizedWithBounds() or > > NativeWidget::ShowWithWindowState() > > > > > > Yeah. views::Widget::Show() make newly made window > visible.(minimzie->normal). > > > > > but BrowserWindowCocoa::Show() doesn't. > > > > > > I'll create new issue. and try to fix it first for ideal implementation! > > > > > > Thank you for detail information. > > > > I found that minimized window creation from X11 is not implemented yet. > > so it act like "normal" state. > > I filed an issue, crbug.com/473228 > > So is it just X11 that's affected? > > reading DesktopWindowTreeHostX11::MapWindow() it sounds like these lines would > never have worked reliably on X11, since minimize/maximize hints may be ignored > until the window is mapped, which happens asynchronously. > > So, if you take out the calls here to Minimize() and SetFullscreenMode(), does > the CL work everywhere except desktop linux? > > IMO it's OK to leave desktop linux as a TODO - just disable the test there and > reference http://crbug.com/473228 . Then a follow-up can make necessary changes > to DesktopWindowTreeHostX11 to give minimize/fullscreen the same treatment as > maximize (e.g. similar to DesktopWindowTreeHostX11::should_maximize_after_map_). > About creation of minimized window, Only X11 doesn't work. About creation of fullscreen window, Both X11 and Windows doesn't work. (I can see HWNDMessageHandler::ShowWindowWithState doesn't handle fullscreen state http://goo.gl/Kxh31O ). I think creation of fullscreen window also should be implemented from both X11 and Windows. Let me file an issue about it. https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:576: EXPECT_TRUE(new_window->window()->IsFullscreen()); On 2015/04/01 03:41:55, tapted wrote: > EXPECT_TRUE(error.empty()) ? Done. https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:585: window_id, &error); On 2015/04/01 03:41:55, tapted wrote: > same here, immediately after this line. > > Perhaps also `EXPECT_TRUE(new_window);` Just so desktop-linux has something to > gnaw on. Done for |error|. `EXPECT_TRUE(new_window);` is not needed. because ExtensionTabUtil::GetBrowserFromWindowID checks null window and set |error|.
https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/190005/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: new_window->window()->Minimize(); On 2015/04/15 23:34:49, limasdf wrote: > On 2015/04/15 00:36:12, tapted wrote: > > On 2015/04/02 16:57:48, limasdf wrote: > > > On 2015/04/01 23:59:45, limasdf wrote: > > > > On 2015/04/01 03:41:55, tapted wrote: > > > > > These additions still don't look right to me. > > > > > > > > > > On Mac, BrowserWindowCocoa::Show() checks for initial_show_state_ being > > > > > MINIMIZED or FULLSCREEN, so it should be dealt with on the ->Show() call > > > just > > > > > above. > > > > > > > > > > On non-Mac, views::Widget::Show() checks for saved_show_state_ being > > > > maximized, > > > > > and for IsFullscreen(). saved_show_state_ is set in > > > Widget::SetInitialBounds() > > > > > using GetSavedWindowPlacement() which uses > > > > > widget_delegate_->GetSavedWindowPlacement(). On non-Mac, BrowserView > > > > implements > > > > > WidgetDelegate in this case, and BrowserView::GetSavedWindowPlacement() > > > calls > > > > > chrome::GetSavedWindowBoundsAndShowState() and other things, ending up > in > > > > > > > > > > WindowSizer::DetermineWindowBoundsAndShowState() > > > > > > > > > > here, there's a `#if defined(USE_ASH)` which is probably out of date, > > since > > > > > that's now true on everything but Mac -- the code in this function below > > > that > > > > > #ifdef is probably dead code. Ignoring that, it still does > > > > > > > > > > `*show_state = GetWindowDefaultShowState();` > > > > > > > > > > GetWindowDefaultShowState() returns browser_->initial_show_state(), and > > this > > > > > should be the value that's passed in on line 585 above. > > > > > > > > > > So why isn't BrowserWindowCocoa::Show() or views::Widget::Show() already > > > > > handling this? > > > > > > > > > > Having separate calls after the call to Show() will result in in-between > > > > states > > > > > -- really it needs to be dealt with properly on the initial Show(), > using > > > > either > > > > > NativeWidget::ShowMaximizedWithBounds() or > > > NativeWidget::ShowWithWindowState() > > > > > > > > Yeah. views::Widget::Show() make newly made window > > visible.(minimzie->normal). > > > > > > > but BrowserWindowCocoa::Show() doesn't. > > > > > > > > I'll create new issue. and try to fix it first for ideal implementation! > > > > > > > > Thank you for detail information. > > > > > > I found that minimized window creation from X11 is not implemented yet. > > > so it act like "normal" state. > > > I filed an issue, crbug.com/473228 > > > > So is it just X11 that's affected? > > > > reading DesktopWindowTreeHostX11::MapWindow() it sounds like these lines would > > never have worked reliably on X11, since minimize/maximize hints may be > ignored > > until the window is mapped, which happens asynchronously. > > > > So, if you take out the calls here to Minimize() and SetFullscreenMode(), does > > the CL work everywhere except desktop linux? > > > > IMO it's OK to leave desktop linux as a TODO - just disable the test there and > > reference http://crbug.com/473228 . Then a follow-up can make necessary > changes > > to DesktopWindowTreeHostX11 to give minimize/fullscreen the same treatment as > > maximize (e.g. similar to > DesktopWindowTreeHostX11::should_maximize_after_map_). > > > > About creation of minimized window, Only X11 doesn't work. > About creation of fullscreen window, Both X11 and Windows doesn't work. > (I can see HWNDMessageHandler::ShowWindowWithState doesn't handle fullscreen > state http://goo.gl/Kxh31O ). > I think creation of fullscreen window also should be implemented from both X11 > and Windows. Let me file an issue about it. For Windows and fullscreen, I've seen this too. See https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi... for my solution. The bug is already filed -- http://crbug.com/475813 i.e. the window is full-screened, but doesn't update the show state. If you can verify that this CL experiences the same on Windows once the extra SetFullscreenMode call is removed, then I would just put an #if defined(OS_WIN) in the test case, with a check for whatever Windows currently reports. https://codereview.chromium.org/1015123003/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/210001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:625: // TODO(limasdf): Minimize() should be removed after DesktopWindowTreeHostX11 put this in #if defined(OS_LINUX) && !defined(OS_CHROMEOS) Then I think the comment should say "On Desktop Linux, window managers may ignore hints until the X11 window is mapped, which happens in the blocking call to Show() above. DesktopWindowTreeHostX11 currently only checks for an attempt to maximize once mapped, but not minimize or fullscreen."
Sorry about late response. I have run from linux(typical), linux(chromeos build), mac, and Windows. Now it's working for every platfrom. with #ifdef guard. https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:632: if (create_params.initial_show_state == ui::SHOW_STATE_MINIMIZED) I also catched that chromeos also doens't handle ui::SHOW_STATE_MINIMIZED , http://goo.gl/Z0DtT4 https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:639: controller->SetFullscreenMode(true, extension()->url()); without his, it's not fullscreen-ed from OS_WIN.
lgtm, but wait for kalman too I've updated http://crbug.com/473228. Ideally we fix that first, but there might be Reasons why things are being done this way :/ https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:631: // considered to create new window. Add "See http://crbug.com/473228."
On 2015/04/17 06:46:43, tapted wrote: > lgtm, but wait for kalman too > > I've updated http://crbug.com/473228. Ideally we fix that first, but there might > be Reasons why things are being done this way :/ > > https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_api.cc (right): > > https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_api.cc:631: // considered to create new > window. > Add "See http://crbug.com/473228.%22 Yeah. When I start this issue, I was thinking this is normal to call explicitly Minmize(), Fullscreen(). as AppWindow::Init (http://goo.gl/PhcKWO) already did. But we found out this is abnormal and should be fixed first ideally. I'll try to fix them when I have time. (Now My home devices is able to build all platform. :) ). Thank you for the hugh help and I admire your insight.
Benjamin, Could you take a look? https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:631: // considered to create new window. On 2015/04/17 06:46:43, tapted wrote: > Add "See http://crbug.com/473228.%22 Done.
most of my comments are only really true assuming that OS_CHROMEOS doesn't already imply OS_LINUX, but that would surprise me if it were. https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:635: #if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) it seems like the expression defined(OS_LINUX) && !defined(OS_CHROMEOS) is equivalent to defined(OS_LINUX) because defined(OS_LINUX) is only ever true for OS_LINUX, and !defined(OS_CHROMEOS) is true for OS_LINUX? Therefore this entire expression can be if defined(OS_LINUX) || defined(OS_WIN) https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:637: // create window for now. is this something to do with aura? (btw, you might want to put a comma after Windows, because this might imply "windows manager" not "a manager on the windows platform; working is a little confusing) https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:590: #if !defined(OS_LINUX) || defined(OS_CHROMEOS) does defined(OS_CHROMEOS) not already imply !defined(OS_LINUX); my point being, is !defined(OS_LINUX) not enough?
https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:635: #if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) On 2015/04/17 16:49:47, kalman wrote: > it seems like the expression > > defined(OS_LINUX) && !defined(OS_CHROMEOS) > > is equivalent to > > defined(OS_LINUX) > > because defined(OS_LINUX) is only ever true for OS_LINUX, and > !defined(OS_CHROMEOS) is true for OS_LINUX? > > Therefore this entire expression can be > > if defined(OS_LINUX) || defined(OS_WIN) This #if guard should be enabled for Linux desktop and Windows. And defined(OS_LINUX) && !defined(OS_CHROMEOS) is not equivalent to defined(OS_LINUX). isn't it? I've asked to chromium-dev , http://goo.gl/SU1isU If I change to OS_LINUX || OS_WIN, then it will enable for chrome os. too. that's not what I intend. If I wrongly understand your question, please correct me. https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:637: // create window for now. On 2015/04/17 16:49:47, kalman wrote: > is this something to do with aura? > > (btw, you might want to put a comma after Windows, because this might imply > "windows manager" not "a manager on the windows platform; working is a little > confusing) Yes, It's todo with aura, exactly DesktopWindowTreeHost::ShowWindowWithState override for X11 and Windows (DesktopWindowTreeHostX11::ShowWindowWithState(), DesktopWindowTreeHostWin::ShowWindowWithState()). You can see they don't handle fullscreen state. And thanks much for grammer correction. https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1015123003/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:590: #if !defined(OS_LINUX) || defined(OS_CHROMEOS) On 2015/04/17 16:49:47, kalman wrote: > does defined(OS_CHROMEOS) not already imply !defined(OS_LINUX); my point being, > is !defined(OS_LINUX) not enough? This #if guard should NOT enabled for only 'Linux desktop'. As you might know, For only Linux desktop, We use "OS_LINUX && !OS_CHROMEOS" which is same with !OS_LINUX || OS_CHROME_OS by De Morgan's law. If I take !defined(OS_LINUX), then this #if guard will not be enabled for Linux Desktop and *Chrome OS. that's not what I intended.
Thanks for explaining, lgtm.
The CQ bit was checked by limasdf@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1015123003/#ps270001 (title: "grammer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015123003/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015123003/270001
Message was sent while issue was closed.
Committed patchset #12 (id:270001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a7c5d3d68ff8d035e59d0a24fc9ffb78a3e6fdb8 Cr-Commit-Position: refs/heads/master@{#325782} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
