Fixing problem with browser invocations from a visiting files application

Fixing problem with browser invocations from a visiting files application

This is addressing the first of three problems. The others seem to be much harder to solve. But lets get this in first.

BUG=338606
TEST=visual

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

[Mr4D (OOO till 08-26), 2014-01-30 01:31:38]

Please have a look!

[mtomasz, 2014-01-30 01:47:17]

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/open_with_browser.cc:112: // Since the
ScopedTabbedBrowserDisplayer does not guarantee that the
If there is already a window with lots of tabs for user A on desktop A, and we
are using a teleported Files App to desktop B, then the tab will be added to the
last active window for user A, and it will teleport it to desktop B together
with all of the tabs in this window. Am I correct?

If so, then shouldn't we look for a browser window of profile B on desktop B
first? If it exists, then create a tab there. If not, create a new browser
window for profile A on desktop B and add a tab there to avoid teleporting all
of the tabs which were on desktop A?

The code would be more complicated, but I guess matching better user
expectations. WDYT?

[Mr4D (OOO till 08-26), 2014-01-30 16:49:58]

Please see my comment!

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/open_with_browser.cc:112: // Since the
ScopedTabbedBrowserDisplayer does not guarantee that the
I have spent yesterday already about 2 hours of implementing a new
ScopedTabbedBrowserDisplayer for this module to do exactly that. It enumerated
all windows from that user and picked a window which would be also on that
desktop - or create a new one.

However... I realized then that once a window is on the other desktop, that will
be chosen from then on as the LRU window. Somehow the effort to get that in
outweighs the benefit of doing it so I threw that code away. Furthermore we
informed the user that some odd things might happen when he teleports windows
and this solution seems to be much more acceptable then the dialog we discussed
yesterday.

If you disagree - let me know and I re-implement the displayer again. :)

[mtomasz, 2014-01-31 01:12:41]

LGTM with a nit for chrome/browser/chromeos/file_manager/open_with_browser.cc.

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/open_with_browser.cc:112: // Since the
ScopedTabbedBrowserDisplayer does not guarantee that the
On 2014/01/30 16:49:58, Mr4D wrote:
> I have spent yesterday already about 2 hours of implementing a new
> ScopedTabbedBrowserDisplayer for this module to do exactly that. It enumerated
> all windows from that user and picked a window which would be also on that
> desktop - or create a new one.
> 
> However... I realized then that once a window is on the other desktop, that
will
> be chosen from then on as the LRU window. Somehow the effort to get that in
> outweighs the benefit of doing it so I threw that code away. Furthermore we
> informed the user that some odd things might happen when he teleports windows
> and this solution seems to be much more acceptable then the dialog we
discussed
> yesterday.
> 
> If you disagree - let me know and I re-implement the displayer again. :)

SGTM. We can improve the logic in the future, if users don't like it.

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/open_with_browser.cc:113: // browser will
be show on the active desktop, we ensure the visibility.
nit: show -> shown

[Mr4D (OOO till 08-26), 2014-01-31 15:20:58]

Done. Thanks!

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/149703006/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/open_with_browser.cc:113: // browser will
be show on the active desktop, we ensure the visibility.
On 2014/01/31 01:12:42, mtomasz wrote:
> nit: show -> shown

Done.

[Mr4D (OOO till 08-26), 2014-01-31 15:21:43]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-01-31 15:21:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-01-31 16:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-01-31 16:00:39]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-31 16:00:46]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 16:00:48]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-01-31 17:30:19]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-01-31 17:30:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-01-31 19:46:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-01-31 19:46:03]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-31 19:46:03]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 19:46:05]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-01-31 20:31:49]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-01-31 20:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-01-31 21:20:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-01-31 23:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-01-31 23:33:49]

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

[commit-bot: I haz the power, 2014-01-31 23:33:51]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 23:33:52]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-01-31 23:37:29]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-01-31 23:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-02-01 02:16:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:16:48]

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

[commit-bot: I haz the power, 2014-02-01 02:16:53]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 02:16:56]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-02-01 02:54:52]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:55:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-02-01 05:03:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 05:03:58]

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

[commit-bot: I haz the power, 2014-02-01 05:04:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 05:04:05]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-02-01 05:06:53]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-02-01 05:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/30001

[commit-bot: I haz the power, 2014-02-01 07:11:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 07:11:06]

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

[commit-bot: I haz the power, 2014-02-01 07:11:13]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 07:11:16]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-02-01 18:16:08]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-02-01 18:16:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/700002

[commit-bot: I haz the power, 2014-02-01 19:00:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 19:00:21]

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

[commit-bot: I haz the power, 2014-02-01 19:00:28]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 19:00:30]

CQ bit was unchecked on CL. Ignoring.

[Mr4D (OOO till 08-26), 2014-02-01 19:04:06]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-02-01 19:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/149703006/700002

[commit-bot: I haz the power, 2014-02-02 06:09:42]

Change committed as 248389

[commit-bot: I haz the power, 2014-02-02 06:09:43]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-02 06:09:43]

CQ bit was unchecked on CL. Ignoring.