Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901173002:
Move RedirectChromeLogging to src/chromeos. (Closed)
DescriptionMove RedirectChromeLogging to src/chromeos.
BUG=chromium:724273
Review-Url: https://codereview.chromium.org/2901173002
Cr-Original-Commit-Position: refs/heads/master@{#474725}
Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988ba3239e00f35a
Review-Url: https://codereview.chromium.org/2901173002
Cr-Commit-Position: refs/heads/master@{#476417}
Committed: https://chromium.googlesource.com/chromium/src/+/7ea0aaca6b630b50a600fc57473d24e27302b9cb
Patch Set 1 #
Total comments: 7
Patch Set 2 : Scott feedback. #
Total comments: 1
Patch Set 3 : Using blocking pool #
Total comments: 4
Patch Set 4 : xiyuan feedback. #Patch Set 5 : Rebase #
Messages
Total messages: 56 (28 generated)
Description was changed from ========== Move RedirectChromeLogging to src/chromeos. BUG= ========== to ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 ==========
achuith@chromium.org changed reviewers: + xiyuan@chromium.org
PTAL, Xiyuan.
On 2017/05/23 23:34:42, achuithb wrote: > PTAL, Xiyuan. Context: https://codereview.chromium.org/2893313003/diff/1/chrome/common/logging_chrom...
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrome.h File chrome/common/logging_chrome.h (right): https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrom... chrome/common/logging_chrome.h:37: #if defined(OS_CHROMEOS) Move everything in this section to the new chrome/logging.h|cc ?
https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrome.h File chrome/common/logging_chrome.h (right): https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrom... chrome/common/logging_chrome.h:37: #if defined(OS_CHROMEOS) On 2017/05/24 15:15:33, xiyuan wrote: > Move everything in this section to the new chrome/logging.h|cc ? These are all used by the platform-independent InitChromeLogging :/
lgtm https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrome.h File chrome/common/logging_chrome.h (right): https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrom... chrome/common/logging_chrome.h:37: #if defined(OS_CHROMEOS) On 2017/05/24 21:07:55, achuithb wrote: > On 2017/05/24 15:15:33, xiyuan wrote: > > Move everything in this section to the new chrome/logging.h|cc ? > > These are all used by the platform-independent InitChromeLogging :/ Okay. :(
achuith@chromium.org changed reviewers: + sky@chromium.org
Scott, PTAL
https://codereview.chromium.org/2901173002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/logging.cc (right): https://codereview.chromium.org/2901173002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/logging.cc:18: bool chrome_logging_redirected_ = false; How about making this local to the function that uses it (a static in the function)? https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrome.h File chrome/common/logging_chrome.h (right): https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrom... chrome/common/logging_chrome.h:35: LoggingDestination DetermineLogMode(const base::CommandLine& command_line); Given the return value, how about DetermineLoggingDestination?
PTAL Scott. Sorry about the rebase. The only changes are in logging.c and logging_chrome.[h|cc] https://codereview.chromium.org/2901173002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/logging.cc (right): https://codereview.chromium.org/2901173002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/logging.cc:18: bool chrome_logging_redirected_ = false; On 2017/05/25 02:23:56, sky wrote: > How about making this local to the function that uses it (a static in the > function)? Done. https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrome.h File chrome/common/logging_chrome.h (right): https://codereview.chromium.org/2901173002/diff/1/chrome/common/logging_chrom... chrome/common/logging_chrome.h:35: LoggingDestination DetermineLogMode(const base::CommandLine& command_line); On 2017/05/25 02:23:56, sky wrote: > Given the return value, how about DetermineLoggingDestination? Done. https://codereview.chromium.org/2901173002/diff/20001/chrome/common/logging_c... File chrome/common/logging_chrome.cc (right): https://codereview.chromium.org/2901173002/diff/20001/chrome/common/logging_c... chrome/common/logging_chrome.cc:131: // only use OutputDebugString in debug mode git cl format seems to prefer this
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by achuith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2901173002/#ps20001 (title: "Scott feedback.")
On 2017/05/25 18:11:49, sky wrote: > LGTM Ty!
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495737661756630,
"parent_rev": "e745346e71a014a64f007da11dd719427175d872", "commit_rev":
"cb9bde9860d070b022407e3d988ba3239e00f35a"}
Message was sent while issue was closed.
Description was changed from ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 ========== to ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 Review-Url: https://codereview.chromium.org/2901173002 Cr-Commit-Position: refs/heads/master@{#474725} Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2909483003/ by sky@chromium.org. The reason for reverting is: This is causing IO failures. Specifically the SetUpSymlinkIfNeeded causes an IO assertion to hit and login to crash..
Message was sent while issue was closed.
On 2017/05/25 20:03:11, sky wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2909483003/ by mailto:sky@chromium.org. > > The reason for reverting is: This is causing IO failures. Specifically the > SetUpSymlinkIfNeeded causes an IO assertion to hit and login to crash.. I tried this patch on a chromebook and on my workstation and didn't see this. How did you reproduce this?
Message was sent while issue was closed.
Make sure you have dchecks enabled, e.g. dcheck_always_on = true in your args.gn. On Thu, May 25, 2017 at 1:04 PM, <achuith@chromium.org> wrote: > On 2017/05/25 20:03:11, sky wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/2909483003/ by mailto:sky@chromium.org. > > > > The reason for reverting is: This is causing IO failures. Specifically > the > > SetUpSymlinkIfNeeded causes an IO assertion to hit and login to crash.. > > I tried this patch on a chromebook and on my workstation and didn't see > this. > How did you reproduce this? > > https://codereview.chromium.org/2901173002/ > -- 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.
If you look at the old code you'll see it uses an object to allow the IO. On Thu, May 25, 2017 at 1:06 PM, Scott Violet <sky@chromium.org> wrote: > Make sure you have dchecks enabled, e.g. dcheck_always_on = true in your > args.gn. > > On Thu, May 25, 2017 at 1:04 PM, <achuith@chromium.org> wrote: > >> On 2017/05/25 20:03:11, sky wrote: >> > A revert of this CL (patchset #2 id:20001) has been created in >> > https://codereview.chromium.org/2909483003/ by mailto:sky@chromium.org. >> > >> > The reason for reverting is: This is causing IO failures. Specifically >> the >> > SetUpSymlinkIfNeeded causes an IO assertion to hit and login to crash.. >> >> I tried this patch on a chromebook and on my workstation and didn't see >> this. >> How did you reproduce this? >> >> https://codereview.chromium.org/2901173002/ >> > > -- 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 2017/05/25 20:12:29, sky wrote: > If you look at the old code you'll see it uses an object to allow the IO. > Yup, the problem is that the presubmits would not allow me to check-in 'new' code with ScopedAllowIO (it would not recognize that I moved this object from one location to another). I couldn't find a way to suppress the presubmit, so I just removed ScopedAllowIO and tested it on the device and desktop, and saw no problems. I built debug on the desktop and release on the device. I assume you saw this on the device, since I don't think we make the symlinks on the desktop, and the bots didn't catch this either.
Message was sent while issue was closed.
Yes, this was on a device. On Thu, May 25, 2017 at 1:22 PM, <achuith@chromium.org> wrote: > On 2017/05/25 20:12:29, sky wrote: > > If you look at the old code you'll see it uses an object to allow the IO. > > > > Yup, the problem is that the presubmits would not allow me to check-in > 'new' > code with ScopedAllowIO (it would not recognize that I moved this object > from > one location to another). I couldn't find a way to suppress the presubmit, > so I > just removed ScopedAllowIO and tested it on the device and desktop, and > saw no > problems. I built debug on the desktop and release on the device. > > I assume you saw this on the device, since I don't think we make the > symlinks on > the desktop, and the bots didn't catch this either. > > https://codereview.chromium.org/2901173002/ > -- 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.
Description was changed from ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 Review-Url: https://codereview.chromium.org/2901173002 Cr-Commit-Position: refs/heads/master@{#474725} Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b... ========== to ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 Review-Url: https://codereview.chromium.org/2901173002 Cr-Commit-Position: refs/heads/master@{#474725} Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b... ==========
Xiyuan, could you PTAL? The only change is in user_session_manager.cc. Please excuse the rebase.
On 2017/05/30 23:05:35, achuithb wrote: > Xiyuan, could you PTAL? The only change is in user_session_manager.cc. Please > excuse the rebase. I've tested this change on the device with dcheck_always_on and no crash.
https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:673: logging::RedirectChromeLogging(parsed_command_line()); This call runs on crash-n-restart case. Would it trigger the file IO DCHECK ? https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/logging.cc (right): https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/logging.cc:39: if (!logging::InitLogging(settings)) { Would logging::InitLogging be safe to be called from a thread other than the main thread?
PTAL, Xiyuan! https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:673: logging::RedirectChromeLogging(parsed_command_line()); On 2017/05/31 15:32:22, xiyuan wrote: > This call runs on crash-n-restart case. Would it trigger the file IO DCHECK ? Done. https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/logging.cc (right): https://codereview.chromium.org/2901173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/logging.cc:39: if (!logging::InitLogging(settings)) { On 2017/05/31 15:32:22, xiyuan wrote: > Would logging::InitLogging be safe to be called from a thread other than the > main thread? Done.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm++ Cool.
On 2017/06/01 01:46:12, xiyuan wrote: > lgtm++ > > Cool. Thank you, Xiyuan! Scott, could you please take another look?
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM
On 2017/06/01 18:25:09, sky wrote: > LGTM Ty!
The CQ bit was checked by achuith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2901173002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496342954157700,
"parent_rev": "e449d3e6409078716b8ebeea8fee9c9cb865978f", "commit_rev":
"7ea0aaca6b630b50a600fc57473d24e27302b9cb"}
Message was sent while issue was closed.
Description was changed from ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 Review-Url: https://codereview.chromium.org/2901173002 Cr-Commit-Position: refs/heads/master@{#474725} Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b... ========== to ========== Move RedirectChromeLogging to src/chromeos. BUG=chromium:724273 Review-Url: https://codereview.chromium.org/2901173002 Cr-Original-Commit-Position: refs/heads/master@{#474725} Committed: https://chromium.googlesource.com/chromium/src/+/cb9bde9860d070b022407e3d988b... Review-Url: https://codereview.chromium.org/2901173002 Cr-Commit-Position: refs/heads/master@{#476417} Committed: https://chromium.googlesource.com/chromium/src/+/7ea0aaca6b630b50a600fc57473d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7ea0aaca6b630b50a600fc57473d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
