New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 710168 link

Starred by 1 user
Status: Fixed
Owner:
michae...@chromium.org
Closed: Aug 2017
Cc:
█ windmueller@chromium.org
abodenha@chromium.org
█ jmcgill@chromium.org
tbarzic@chromium.org
afakhry@chromium.org
michae...@chromium.org
sque@chromium.org
█ zelidrag@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 714463



Sign in to add a comment

Move feedbackPrivate.getSystemInformation out of Chrome.

Project Member Reported by r...@chromium.org, Apr 10 2017 
This API was originally written for gathering system logs for the feedback component app in Chrome. Since then, it is now being used by other clients to gather system logs - including GVC/CfM.

With GVC moving over to AppShell, we need to ensure that this API works outside Chrome.

This involves refactoring a few major pieces of code.

//c/b/e/feedback_private/
//c/b/chromeos/system_logs/
//c/b/feedback/system_logs/

All the code from the last two and related code from the the API implementation need to be moved out of Chrome at a minimum.

Doing this right away isn't super important, but it is something we want relatively soon given our plans with fishfood/dogfood of AppShell.

Comment 1 by r...@chromium.org, Apr 10 2017

Cc: tbarzic@chromium.org

Comment 2 by r...@chromium.org, May 4 2017

Labels: -M-61 M-62

Comment 3 by r...@chromium.org, May 17 2017

Owner: michae...@chromium.org

Comment 4 by michae...@chromium.org, May 17 2017

Blocking: 714463

Comment 5 by michae...@chromium.org, May 17 2017 

A fair amount of the logs pulled by //c/b/*/system_logs looks Chrome-specific. I wouldn't say all of it needs to be pulled out. This is probably a case where a delegate makes sense to allow clients to add extra log sources.

Comment 6 by michae...@chromium.org, Jun 2 2017

Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 9 2017 

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

commit caeb1d2b5481130de1b68bc1a91857637a8a92aa
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Fri Jun 09 01:27:46 2017

Collapse SystemLogsFetchers into one concrete class

Simplify the definition of SystemLogsFetcher into a single class.
Provide functions that build and return a SystemLogsFetcher with the
relevant log sources.

Currently, SystemLogsFetcherBase is implemented by
ScrubbedSystemLogsFetcher and AboutSystemLogsFetcher. The "scrubbed"
version does anonymize data, but also uses slightly different sources.
The About version is intended for use by chrome://about. So the
distinction between these two is more than just whether the logs are
re-written to be anonymous.

The virtual Rewrite() method was only used to scrub data, so I've made
scrubbiness a parameter instead.

The next step will be to create an extensions API delegate function to
call the appropriate factory function so that system logs can be fetched
from outside of //chrome. This is blocking feedbackPrivate from being
used by app_shell.

Bug:  710168 
Change-Id: I512df988a4745479e41b5f7722217cd884c31cc5
Reviewed-on: https://chromium-review.googlesource.com/522844
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478155}
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/BUILD.gn
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/command_line_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/dbus_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/debug_daemon_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/device_event_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/lsb_release_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/single_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/chromeos/system_logs/touch_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/extensions/api/feedback_private/feedback_private_api.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/extensions/api/feedback_private/feedback_service.cc
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/extensions/api/feedback_private/feedback_service.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/extensions/api/log_private/log_private_api.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/extensions/api/log_private/log_private_api_chromeos.cc
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/about_system_logs_fetcher.cc
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/about_system_logs_fetcher.h
[add] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.cc
[add] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h
[delete] https://crrev.com/7fbcb3b02e0e1e1b3e42c46a4c0c0c1113357fdc/chrome/browser/feedback/system_logs/scrubbed_system_logs_fetcher.cc
[delete] https://crrev.com/7fbcb3b02e0e1e1b3e42c46a4c0c0c1113357fdc/chrome/browser/feedback/system_logs/scrubbed_system_logs_fetcher.h
[add] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/system_logs_fetcher.cc
[add] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/feedback/system_logs/system_logs_fetcher.h
[delete] https://crrev.com/7fbcb3b02e0e1e1b3e42c46a4c0c0c1113357fdc/chrome/browser/feedback/system_logs/system_logs_fetcher_base.cc
[delete] https://crrev.com/7fbcb3b02e0e1e1b3e42c46a4c0c0c1113357fdc/chrome/browser/feedback/system_logs/system_logs_fetcher_base.h
[modify] https://crrev.com/caeb1d2b5481130de1b68bc1a91857637a8a92aa/chrome/browser/ui/webui/system_info_ui.cc

Comment 8 by michae...@chromium.org, Jun 28 2017 

c/b/feedback/system_logs looks like a good candidate for moving to components/feedback. I'll start on that.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 6 2017 

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

commit e28523ca804072b27d88cf58d597afd3890a20ec
Author: michaelpg <michaelpg@chromium.org>
Date: Thu Jul 06 18:17:04 2017

Move SystemLogsSource class into new file

Splits SystemLogsSource out of system_logs_fetcher.h.

BUG= 710168 

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

[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/BUILD.gn
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/command_line_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/dbus_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/debug_daemon_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/device_event_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/lsb_release_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/single_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/chromeos/system_logs/touch_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/extensions/api/feedback_private/feedback_private_api.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/extensions/api/feedback_private/log_source_access_manager.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/about_system_logs_fetcher.cc
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h
[modify] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/system_logs_fetcher.h
[add] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/system_logs_source.cc
[add] https://crrev.com/e28523ca804072b27d88cf58d597afd3890a20ec/chrome/browser/feedback/system_logs/system_logs_source.h
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 7 2017 

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

commit e56f0726e73f8b032df5d3393f7edf1b765366f9
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Fri Jul 07 19:58:28 2017

Fix SystemLogsSource ODR violation

The CL crrev.com/2969573002 moved the constructor and destructor for
SystemLogsSource to a different file - except it failed to delete the
old ones, leading to duplicate symbol warnings. I'm surprised that the
warnings weren't errors but hey, that's ODR for you.

Warnings were:
system_logs_fetcher.obj : warning LNK4006: "...
system_logs::SystemLogsSource::SystemLogsSource(...)" ...
already defined in system_logs_source.obj; second definition ignored
system_logs_fetcher.obj : warning LNK4006: "...
system_logs::SystemLogsSource::~SystemLogsSource(void)" ...
already defined in system_logs_source.obj; second definition ignored

Bug:  710168 
Change-Id: Ifc336149cd7f0f4e58b5bd0183b34d86164dc325
Reviewed-on: https://chromium-review.googlesource.com/563762
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485013}
[modify] https://crrev.com/e56f0726e73f8b032df5d3393f7edf1b765366f9/chrome/browser/feedback/system_logs/system_logs_fetcher.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 17 2017 

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

commit 5d600d42d1f75df892ac447293e9c40a8707adef
Author: michaelpg <michaelpg@chromium.org>
Date: Mon Jul 17 22:04:16 2017

Move some of c/b/feedback/system_logs to //components/feedback

BUG= 710168 

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

[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/BUILD.gn
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/command_line_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/dbus_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/debug_daemon_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/device_event_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/lsb_release_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/single_log_file_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/chromeos/system_logs/touch_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/extensions/api/feedback_private/feedback_private_api.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/extensions/api/feedback_private/feedback_service.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/extensions/api/feedback_private/log_source_access_manager.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/extensions/api/feedback_private/log_source_resource.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/extensions/api/feedback_private/single_log_source_factory.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/feedback/system_logs/about_system_logs_fetcher.cc
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.cc
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/chrome/browser/ui/webui/system_info_ui.cc
[modify] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/components/feedback/BUILD.gn
[rename] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/components/feedback/system_logs/system_logs_fetcher.cc
[rename] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/components/feedback/system_logs/system_logs_fetcher.h
[rename] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/components/feedback/system_logs/system_logs_source.cc
[rename] https://crrev.com/5d600d42d1f75df892ac447293e9c40a8707adef/components/feedback/system_logs/system_logs_source.h

Comment 12 by michae...@chromium.org, Jul 26 2017 

The feedback API uses BrowserProcess::GetApplicationLocale(). Replacing that is a bit tricky because of how it's set. Some notes for my future reference:

g_browser_process->GetApplicationLocale() will return the same as
g_browser_process->local_state()->GetString(prefs::kApplicationLocale)

and the local state PrefService is something we could think about exposing. However, the
pref itself is a Chrome preference. It's not set in AppShell, where we simply use the
system's locale.

Furthermore, the application locale does not change during execution, but the kApplicationLocale 
pref *can* change, so we can't rely on that pref if we want the actual (currently used) locale.

I think adding a GetApplicationLocale() method to ExtensionsBrowserClient is the simplest solution.

Comment 13 by michae...@chromium.org, Aug 28 2017

Status: Fixed (was: Started)
Locale implementation tracked in issue 759781. We have basic logs in AppShell now (version numbers and extensions list). Additional desired log sources should be filed as separate bugs.
►

Sign in to add a comment