Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
783533002:
Remove the WindowsLogoffRace experiment. (Closed)
|
Created:
6 years ago by Sigurður Ásgeirsson Modified:
5 years ago CC:
chromium-reviews, rginda+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Project:
chromium Visibility:
Public. |
DescriptionRemove the WindowsLogoffRace experiment.
Unfortunately these are not the droids we're looking for.
BUG=388741
Committed: https://crrev.com/5ed6480ab0d5c1cca5fd75b0682f15f33f32977c
Cr-Commit-Position: refs/heads/master@{#307026}
Patch Set 1 #Patch Set 2 : Remove unused includes. #
Total comments: 1
Messages
Total messages: 20 (5 generated)
siggi@chromium.org changed reviewers: + gab@chromium.org
PTAL
lgtm
siggi@chromium.org changed reviewers: + sky@chromium.org
Sky, please for owners approval?
LGTM
Make sure you nuke any includes that are no longer needed too.
The CQ bit was checked by siggi@chromium.org
The CQ bit was unchecked by siggi@chromium.org
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/783533002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5ed6480ab0d5c1cca5fd75b0682f15f33f32977c Cr-Commit-Position: refs/heads/master@{#307026}
Message was sent while issue was closed.
Oops...! Was just looking for this code and searching for it in an ifdef(OS_WIN)... and it wasn't...! https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:290: base::KillProcess(base::GetCurrentProcessHandle(), 0, false); I just realized that this made this the behavior on all OSes! I don't think this was intentional? At least when the experiment was introduced (https://codereview.chromium.org/385123004) this code was under an ifdef(OS_WIN). The Windows specific side was then assumed by the experiment. And here after removing the experiment, this become true for all platforms..! But hey... maybe that's a good thing after all? Shutdown determinism across platforms? Means we should probably delete all code after this point as it hasn't ran in a year?!
Message was sent while issue was closed.
Last I knew, this was only called on Windows. Has that changed? On Thu, Dec 3, 2015 at 2:12 PM <gab@chromium.org> wrote: > Oops...! Was just looking for this code and searching for it in an > ifdef(OS_WIN)... and it wasn't...! > > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > File chrome/browser/lifetime/application_lifetime.cc (right): > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > chrome/browser/lifetime/application_lifetime.cc:290 > <https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/...> > : > base::KillProcess(base::GetCurrentProcessHandle(), 0, false); > I just realized that this made this the behavior on all OSes! > > I don't think this was intentional? At least when the experiment was > introduced (https://codereview.chromium.org/385123004) this code was > under an ifdef(OS_WIN). The Windows specific side was then assumed by > the experiment. And here after removing the experiment, this become true > for all platforms..! > > But hey... maybe that's a good thing after all? Shutdown determinism > across platforms? Means we should probably delete all code after this > point as it hasn't ran in a year?! > > https://codereview.chromium.org/783533002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/12/03 19:20:01, Sigurður Ásgeirsson wrote: > Last I knew, this was only called on Windows. Has that changed? That's what I'm saying (see my inline comment). This CL took it out of the Windows-only section..! > > On Thu, Dec 3, 2015 at 2:12 PM <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > Oops...! Was just looking for this code and searching for it in an > > ifdef(OS_WIN)... and it wasn't...! > > > > > > > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > > File chrome/browser/lifetime/application_lifetime.cc (right): > > > > > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > > chrome/browser/lifetime/application_lifetime.cc:290 > > > <https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/...> > > : > > base::KillProcess(base::GetCurrentProcessHandle(), 0, false); > > I just realized that this made this the behavior on all OSes! > > > > I don't think this was intentional? At least when the experiment was > > introduced (https://codereview.chromium.org/385123004) this code was > > under an ifdef(OS_WIN). The Windows specific side was then assumed by > > the experiment. And here after removing the experiment, this become true > > for all platforms..! > > > > But hey... maybe that's a good thing after all? Shutdown determinism > > across platforms? Means we should probably delete all code after this > > point as it hasn't ran in a year?! > > > > https://codereview.chromium.org/783533002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
Message was sent while issue was closed.
On Thu, Dec 3, 2015 at 2:37 PM <gab@chromium.org> wrote: > On 2015/12/03 19:20:01, Sigurður Ásgeirsson wrote: > > Last I knew, this was only called on Windows. Has that changed? > > That's what I'm saying (see my inline comment). This CL took it out of the > Windows-only section..! > > I don't understand the comment, are you proposing the process termination should be under an ifdef? > > > On Thu, Dec 3, 2015 at 2:12 PM > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> > wrote: > > > > Oops...! Was just looking for this code and searching for it in an > > > ifdef(OS_WIN)... and it wasn't...! > > > > > > > > > > > > > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > > > File chrome/browser/lifetime/application_lifetime.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > > > chrome/browser/lifetime/application_lifetime.cc:290 > > > > > < > https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... > > > > > : > > > base::KillProcess(base::GetCurrentProcessHandle(), 0, false); > > > I just realized that this made this the behavior on all OSes! > > > > > > I don't think this was intentional? At least when the experiment was > > > introduced (https://codereview.chromium.org/385123004) this code was > > > under an ifdef(OS_WIN). The Windows specific side was then assumed by > > > the experiment. And here after removing the experiment, this become > true > > > for all platforms..! > > > > > > But hey... maybe that's a good thing after all? Shutdown determinism > > > across platforms? Means we should probably delete all code after this > > > point as it hasn't ran in a year?! > > > > > > https://codereview.chromium.org/783533002/ > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri... > . > > > > https://codereview.chromium.org/783533002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Note < https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...> it looks like this is the correct behavior on X11 too, as far as I understand, as it's not clear that the program will terminate at all otherwise? On Fri, Dec 4, 2015 at 11:20 AM Sigurður Ásgeirsson <siggi@chromium.org> wrote: > On Thu, Dec 3, 2015 at 2:37 PM <gab@chromium.org> wrote: > >> On 2015/12/03 19:20:01, Sigurður Ásgeirsson wrote: >> > Last I knew, this was only called on Windows. Has that changed? >> >> That's what I'm saying (see my inline comment). This CL took it out of the >> Windows-only section..! >> >> I don't understand the comment, are you proposing the process termination > should be under an ifdef? > > >> >> > On Thu, Dec 3, 2015 at 2:12 PM >> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> >> wrote: >> >> > > Oops...! Was just looking for this code and searching for it in an >> > > ifdef(OS_WIN)... and it wasn't...! >> > > >> > > >> > > >> > > >> >> >> https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... >> > > File chrome/browser/lifetime/application_lifetime.cc (right): >> > > >> > > >> > > >> >> >> https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... >> > > chrome/browser/lifetime/application_lifetime.cc:290 >> > > >> >> < >> https://codereview.chromium.org/783533002/diff/20001/chrome/browser/lifetime/... >> > >> > > : >> > > base::KillProcess(base::GetCurrentProcessHandle(), 0, false); >> > > I just realized that this made this the behavior on all OSes! >> > > >> > > I don't think this was intentional? At least when the experiment was >> > > introduced (https://codereview.chromium.org/385123004) this code was >> > > under an ifdef(OS_WIN). The Windows specific side was then assumed by >> > > the experiment. And here after removing the experiment, this become >> true >> > > for all platforms..! >> > > >> > > But hey... maybe that's a good thing after all? Shutdown determinism >> > > across platforms? Means we should probably delete all code after this >> > > point as it hasn't ran in a year?! >> > > >> > > https://codereview.chromium.org/783533002/ >> > > >> >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to >> >> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri... >> . >> >> >> >> https://codereview.chromium.org/783533002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Here's what I'm saying: Before https://codereview.chromium.org/385123004 your code was behind an ifdef(OS_WIN). After it that CL it was in the else{} block on the experiment (i.e. ON on all OSes, no longer ifdef'ed -- I'd originally misread that CL and thought it was only enabled on all OSes in this CL but looks like it was in that one instead). And then in this CL the experiment was removed, leaving this code enabled on all platforms. I don't think it was intentional to do this on non-Windows or was it? The comment on SessionEnding() says "// Begins shutdown of the application when the desktop session is ending." while it's a lot more drastic then "beginning" shutdown. Not saying it's wrong, just saying it's unexpected to me that this is live on all platforms and we should probably change the method's name and comment accordingly (and make sure there is no code after such a call as it'd be pointless.
Message was sent while issue was closed.
K, I'd review that change. On Fri, Dec 4, 2015 at 1:54 PM <gab@chromium.org> wrote: > Here's what I'm saying: > > Before https://codereview.chromium.org/385123004 your code was behind an > ifdef(OS_WIN). > > After it that CL it was in the else{} block on the experiment (i.e. ON on > all > OSes, no longer ifdef'ed -- I'd originally misread that CL and thought it > was > only enabled on all OSes in this CL but looks like it was in that one > instead). > > And then in this CL the experiment was removed, leaving this code enabled > on all > platforms. > > I don't think it was intentional to do this on non-Windows or was it? > > The comment on SessionEnding() says "// Begins shutdown of the application > when > the desktop session is ending." while it's a lot more drastic > then "beginning" > shutdown. > > Not saying it's wrong, just saying it's unexpected to me that this is live > on > all platforms and we should probably change the method's name and comment > accordingly (and make sure there is no code after such a call as it'd be > pointless. > > https://codereview.chromium.org/783533002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
