Add "open as window" menu item to NTP app menu.

Add "open as window" menu item to NTP app menu.


BUG=59697
TEST=BrowserTest.OpenAppWindowLikeNtp,SessionRestoreUITest.RestoreAfterClosing*

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

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

[Sam Kerner (Chrome), 2010-11-17 05:07:35]

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_dom_ui.cc (left):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_dom_ui.cc:245: NOTREACHED() << "Chrome URL
override requested before extensions loaded";
Comment is obsolete now that extensions service loads synchronously.

[Aaron Boodman, 2010-11-17 06:12:47]

I think we may not have app windows on os x. Can you check on this? If not,
please do not expose this UI on that platform.

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_dom_ui.cc (left):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_dom_ui.cc:245: NOTREACHED() << "Chrome URL
override requested before extensions loaded";
On 2010/11/17 05:07:36, Sam Kerner (Chrome) wrote:
> Comment is obsolete now that extensions service loads synchronously.

In that case, you can remove this check completely. Also, maybe we should remove
is_ready() since ExtensionsService is always ready now?

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.html:250: <button
command="#apps-launch-type-window" launch-type="3"></button>
For some reason it feels to me like 'full screen' should be last.

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:499: if (!extensions_service->is_ready()) {
Again here, I think we can take this code out now.

[Aaron Boodman, 2010-11-17 06:13:29]

+andybons who can answer about mac.

[Bons, 2010-11-17 16:24:17]

On 2010/11/17 06:13:29, Aaron Boodman wrote:
> +andybons who can answer about mac.

Aaron is correct. Right now the Mac port does not support app windows.

[Sam Kerner (Chrome), 2010-11-18 03:53:36]

Ready for another look.

Removed the menu option on mac os.

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_dom_ui.cc (left):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_dom_ui.cc:245: NOTREACHED() << "Chrome URL
override requested before extensions loaded";
On 2010/11/17 06:12:47, Aaron Boodman wrote:
> On 2010/11/17 05:07:36, Sam Kerner (Chrome) wrote:
> > Comment is obsolete now that extensions service loads synchronously.
> 
> In that case, you can remove this check completely. Also, maybe we should
remove
> is_ready() since ExtensionsService is always ready now?

ExtensionsService::ready_ is set to false in the extensions service constructor,
and set to true by a call to ExtensionsService::Init().  Kept the internal bool
as a way to catch multiple Init() calls.  Got rid of is_ready().

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_new_tab.html:250: <button
command="#apps-launch-type-window" launch-type="3"></button>
On 2010/11/17 06:12:47, Aaron Boodman wrote:
> For some reason it feels to me like 'full screen' should be last.

Done.

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/5019005/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:499: if (!extensions_service->is_ready()) {
On 2010/11/17 06:12:47, Aaron Boodman wrote:
> Again here, I think we can take this code out now.

Done.

[Aaron Boodman, 2010-11-18 07:49:03]

Great. A few last minor things.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:243: if (!service)
I do not think it is ever possible to not have an ExtensionsService instance.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/dom_ui/app_l...
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/dom_ui/app_l...
chrome/browser/dom_ui/app_launcher_handler.cc:266: if (prefs_launch_type ==
ExtensionPrefs::LAUNCH_WINDOW) {
Nit: no need for braces here and omitting them would be more consistent with
this file.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:466: return LAUNCH_REGULAR;
Nice catch.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.cc:454: if (service)
service should always exist here.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:25:
$('apps-launch-type-window-menu-item').style.display =
I think it would be a little neater to just do this in JS, like:

if (navigator.platform.toLowerCase().indexOf('mac') == 0)

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (left):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:399: if (HasExtensionApps(*windows)
&& profile_->GetExtensionsService() &&
nice cleanup

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:608: if (extension_service) {
No need to check this here.

[Sam Kerner (Chrome), 2010-11-18 15:34:01]

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:243: if (!service)
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> I do not think it is ever possible to not have an ExtensionsService instance.

Done.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/dom_ui/app_l...
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/dom_ui/app_l...
chrome/browser/dom_ui/app_launcher_handler.cc:266: if (prefs_launch_type ==
ExtensionPrefs::LAUNCH_WINDOW) {
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> Nit: no need for braces here and omitting them would be more consistent with
> this file.

Done.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.cc:454: if (service)
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> service should always exist here.

Done.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:25:
$('apps-launch-type-window-menu-item').style.display =
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> I think it would be a little neater to just do this in JS, like:
> 
> if (navigator.platform.toLowerCase().indexOf('mac') == 0)

What if the user changes the user agent?  This is possible today with command
line flag --user-agent.

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:608: if (extension_service) {
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> No need to check this here.

Fixed here and several other places.

[Aaron Boodman, 2010-11-18 16:16:00]

lgtm

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:25:
$('apps-launch-type-window-menu-item').style.display =
On 2010/11/18 15:34:01, Sam Kerner (Chrome) wrote:
> On 2010/11/18 07:49:03, Aaron Boodman wrote:
> > I think it would be a little neater to just do this in JS, like:
> > 
> > if (navigator.platform.toLowerCase().indexOf('mac') == 0)
> 
> What if the user changes the user agent?  This is possible today with command
> line flag --user-agent.

Good point.

[arv (Not doing code reviews), 2010-11-18 19:02:49]

FYI

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:25:
$('apps-launch-type-window-menu-item').style.display =
On 2010/11/18 16:16:00, Aaron Boodman wrote:
> On 2010/11/18 15:34:01, Sam Kerner (Chrome) wrote:
> > On 2010/11/18 07:49:03, Aaron Boodman wrote:
> > > I think it would be a little neater to just do this in JS, like:
> > > 
> > > if (navigator.platform.toLowerCase().indexOf('mac') == 0)
> > 
> > What if the user changes the user agent?  This is possible today with
command
> > line flag --user-agent.
> 
> Good point.

If they change the user agent they are opening the flood gates...

http://codereview.chromium.org/5019005/diff/21001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:25:
$('apps-launch-type-window-menu-item').style.display =
On 2010/11/18 07:49:03, Aaron Boodman wrote:
> I think it would be a little neater to just do this in JS, like:
> 
> if (navigator.platform.toLowerCase().indexOf('mac') == 0)

cr.isMac

[Sam Kerner (Chrome), 2010-11-22 03:22:14]

Aaron

[Sam Kerner (Chrome), 2010-11-22 03:35:16]

Aaron,
  After committing, I found some interesting bugs, and backed out this change.

  Many observers of EXTENSION_LOADED used to test that the extensions service is
ready, and removing ExtensionsService::is_ready() caused them to have different
behavior.  There are cases where extensions fire EXTENSION_LOADED before
is_ready is true.  

  This could be fixed by rewriting the observers to ignore EXTENSION_LOADED
until EXTENSIONS_READY is fired.  It could also be fixed by never firing
EXTENSION_LOADED until EXTENSIONS_READY fires, but it is not obvious that this
would be correct.  Ensuring that each observer of EXTENSION_LOADED is updated
deserves its own change.

  To make this change correct, I re-added is_ready(), and undid changes to the
observers.  I filed a bug to look at each observer site and find a way to remove
is_ready().  I am not sure if this warrants a re-review, so I will let the
reviewer decide.

Sam

[Aaron Boodman, 2010-11-22 07:59:36]

Makes sense. Sorry for misdirecting you.

LGTM