New issue
Advanced search Search tips

Issue 617830 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Refactor restart/relaunch codepaths in MD Settings.

Project Member Reported by dpa...@chromium.org, Jun 7 2016

Issue description

Problem:
Three different codepaths exist for triggering a browser restart, from different
parts of the UI.

 1 SystemHandler::HandleRestartBrowser delegates to chrome::AttemptRestart(),
   but also does some additional work for Windows only (see Windows-only big
   comment about not accidentally reporting crashes).
 2 LanguagesHandler::HandleRestart, delegates to chrome::AttemptRestart() on
   non-CrOS, and to chrome::AttemptUserExit() on CrOS.
 3 AboutHandler::HandleRelaunchNow, delegates to VersionUpdater::RelaunchBrowser
   which delegates to chrome::AttemptRestart() on all non-CrOS platforms and to
   DBusThreadManager::Get()->GetPowerManagerClient()->RequestRestart();
   on ChromeOS.

Refactoring plan.
 1 Create a dedicated C++ handler for "relaunch" and "factory reset", called
   BrowserLifetimeHandler.
 2 Create a corresponding JS LifetimeBrowserPorxy that talks to the new C++
   handler.
 3 Update all JS pages mentioned above to use the new browser proxy to trigger
   "relaunch" and "reset".
 4 Remove obsolete code from all C++ handlers and corresponding browser proxies
   mentioned in previous paragraph.

"Factory reset" functionality will also be rolled into the new C++ handler after
the above refactoring has been completed.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/615a9567426f289f43b9e3d614a646c59528ad93

commit 615a9567426f289f43b9e3d614a646c59528ad93
Author: dpapad <dpapad@chromium.org>
Date: Tue Jun 07 02:10:29 2016

Moving Windows Breakpad restart logic to lower layer.

Currently this logic needs to be copied and called every time
chrome::AttemptRestart() is called. Moving it within chrome::AttemptRestart()
allows it to be reused. Moreover, it seems that calling
chrome::AttemptRestart() without the said logic (there are such occurrences),
is not correct.

This is in preparation of reducing the number of "Restart" codepaths in MD
Settings.

BUG= 617830 

Review-Url: https://codereview.chromium.org/2033173002
Cr-Commit-Position: refs/heads/master@{#398195}

[modify] https://crrev.com/615a9567426f289f43b9e3d614a646c59528ad93/chrome/browser/lifetime/application_lifetime.cc
[modify] https://crrev.com/615a9567426f289f43b9e3d614a646c59528ad93/chrome/browser/metrics/chrome_metrics_service_accessor.h
[modify] https://crrev.com/615a9567426f289f43b9e3d614a646c59528ad93/chrome/browser/ui/tab_contents/tab_contents_iterator_unittest.cc
[modify] https://crrev.com/615a9567426f289f43b9e3d614a646c59528ad93/chrome/browser/ui/webui/options/browser_options_handler.cc
[modify] https://crrev.com/615a9567426f289f43b9e3d614a646c59528ad93/chrome/browser/ui/webui/settings/system_handler.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/374368fd0a260c0d13cab0447ba4b30c36d448b6

commit 374368fd0a260c0d13cab0447ba4b30c36d448b6
Author: dpapad <dpapad@chromium.org>
Date: Thu Jun 09 04:04:42 2016

Cleanup: Remove VersionUpdater::RelaunchBrowser interface method.

Specifically:
 - Introducing a chrome::AttemptRelaunch() method.
 - Removing VersionUpdater::RelaunchBrowser() method, since the extra
   level of indirection seems unnecessary.

This is in preparation of reducing the number of "Restart" codepaths in MD
Settings.

BUG= 617830 

Review-Url: https://codereview.chromium.org/2039273003
Cr-Commit-Position: refs/heads/master@{#398777}

[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/lifetime/application_lifetime.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/lifetime/application_lifetime.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/views/update_recommended_message_box.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/help_handler.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_basic.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_basic.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_chromeos.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_chromeos.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_mac.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_mac.mm
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_win.cc
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/help/version_updater_win.h
[modify] https://crrev.com/374368fd0a260c0d13cab0447ba4b30c36d448b6/chrome/browser/ui/webui/settings/about_handler.cc

Labels: Hotlist-MD-Settings-General
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e85dfff4e9e360d93be39fb73bb2197e44a67f17

commit e85dfff4e9e360d93be39fb73bb2197e44a67f17
Author: dpapad <dpapad@chromium.org>
Date: Tue Jun 14 00:30:46 2016

MD Settings: Add dedicated C++ handler for restart/relaunch/powerwash.

BUG= 617830 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2057593003
Cr-Commit-Position: refs/heads/master@{#399617}

[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/compiled_resources2.gyp
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/lifetime_browser_proxy.html
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/lifetime_browser_proxy.js
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/settings_resources.grd
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/browser_lifetime_handler.h
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/md_settings_ui.cc
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/chrome_browser_ui.gypi
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/test/data/webui/settings/test_lifetime_browser_proxy.js

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8

commit 8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8
Author: dpapad <dpapad@chromium.org>
Date: Tue Jun 14 18:18:05 2016

MD Settings: Hook up system_page to new LifetimeBrowserProxy.

BUG= 617830 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2055923002
Cr-Commit-Position: refs/heads/master@{#399749}

[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/compiled_resources2.gyp
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page.html
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/ui/webui/settings/system_handler.cc
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/ui/webui/settings/system_handler.h
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/test/data/webui/settings/system_page_tests.js

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e85dfff4e9e360d93be39fb73bb2197e44a67f17

commit e85dfff4e9e360d93be39fb73bb2197e44a67f17
Author: dpapad <dpapad@chromium.org>
Date: Tue Jun 14 00:30:46 2016

MD Settings: Add dedicated C++ handler for restart/relaunch/powerwash.

BUG= 617830 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2057593003
Cr-Commit-Position: refs/heads/master@{#399617}

[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/compiled_resources2.gyp
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/lifetime_browser_proxy.html
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/lifetime_browser_proxy.js
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/resources/settings/settings_resources.grd
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/browser_lifetime_handler.cc
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/browser_lifetime_handler.h
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/browser/ui/webui/settings/md_settings_ui.cc
[modify] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/chrome_browser_ui.gypi
[add] https://crrev.com/e85dfff4e9e360d93be39fb73bb2197e44a67f17/chrome/test/data/webui/settings/test_lifetime_browser_proxy.js

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8

commit 8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8
Author: dpapad <dpapad@chromium.org>
Date: Tue Jun 14 18:18:05 2016

MD Settings: Hook up system_page to new LifetimeBrowserProxy.

BUG= 617830 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2055923002
Cr-Commit-Position: refs/heads/master@{#399749}

[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/compiled_resources2.gyp
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page.html
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/ui/webui/settings/system_handler.cc
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/browser/ui/webui/settings/system_handler.h
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/8a84aa50f0c16bc9c98c4eaca37d5902b927ebf8/chrome/test/data/webui/settings/system_page_tests.js

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58f27774b5ae565859801b7039349f8621e786cb

commit 58f27774b5ae565859801b7039349f8621e786cb
Author: dpapad <dpapad@chromium.org>
Date: Wed Jun 15 22:50:27 2016

MD Settings: Hook up about_page to new LifetimeBrowserProxy.

BUG= 617830 , 546841 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2063873004
Cr-Commit-Position: refs/heads/master@{#400033}

[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/resources/settings/about_page/about_page.html
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/resources/settings/about_page/about_page.js
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/resources/settings/about_page/about_page_browser_proxy.js
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/resources/settings/about_page/compiled_resources2.gyp
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/ui/webui/settings/about_handler.cc
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/browser/ui/webui/settings/about_handler.h
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/test/data/webui/settings/about_page_tests.js
[modify] https://crrev.com/58f27774b5ae565859801b7039349f8621e786cb/chrome/test/data/webui/settings/cr_settings_browsertest.js

Status: Fixed (was: Started)

Sign in to add a comment