WIP: Refactor relaunch process on mac #15257
Conversation
github-actions
bot
added
the
potential-layer-violation-fixes
|
|
||
| namespace chrome { | ||
|
|
||
| void AttemptRestart() { |
There was a problem hiding this comment.
What about RelaunchIgnoreUnloadHandlers?
There was a problem hiding this comment.
I wonder why not add the code only once in AttemptRestartInternal. I also wonder whether the code in AttemptRestartInternal is necessary before the Sparkle relaunch. For example, this line:
// Set the flag to restore state after the restart.
pref_service->SetBoolean(prefs::kRestartLastSessionOnShutdown, true);
simonhong
force-pushed
the
refactor_relaunch_on_mac
branch
from
ebfddbf
to
86505d6
Compare
| SparkleGlue* sparkle_glue = [SparkleGlue sharedSparkleGlue]; | ||
| // Only relaunch via sparkle when update is available. | ||
| if ([sparkle_glue recentStatus] == kAutoupdateInstalled) { | ||
| [sparkle_glue relaunch]; |
There was a problem hiding this comment.
is there a reason to do this via sparkle? why can't we always restart via chromium's AttemptRestart() call?
It looks like this restart is pretty crazy for chromium (it kills the process in a non-chromium friendly way). I think the right thing here should be is to do this sparkle call as the last step in AttemptRestartInternal (put this condition there).
Because we don't call AttemptRestartInternal() currently on MacOS updates (via Settings/Hamburger), we may lose sessions, break local state/prefs and all other crazy things.
For example, right now this task is failing on MacOS most likely because prefs::kWasRestarted is not set during the restart triggered via a manual "Relaunch" click on brave://settings/help.
@simonhong we need to resurrect this PR and make it work as intended. I'll be happy to review it.
There was a problem hiding this comment.
Sparkle relaunch was introduced for fixing brave/brave-browser#1787.
When asking relaunch to sparkle, not sure but I think sparkle does its things and sends normal quit signals as you investigated.
Yeah, doing it as a last step of AttemptRestartInternal() could be more better 👍🏼
There was a problem hiding this comment.
Sparkle relaunch was introduced for fixing brave/brave-browser#1787. When asking relaunch to sparkle, not sure but I think sparkle does its things and sends normal quit signals as you investigated. Yeah, doing it as a last step of
AttemptRestartInternal()could be more better 👍🏼
well, we might have fixed one issue (or maybe just hid it under the rug), but created another one.
|
Closing as stale. @simonhong please re-open if we'd like to attempt this fix again. Thanks! 😄 |
fix brave/brave-browser#25576
Resolves
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: