Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
944733003:
Add flag to communicate last casted app to renderer process (Closed)
|
Created:
5 years, 10 months ago by kjoswiak Modified:
5 years, 10 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, sadrul, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd flag to communicate last casted app to renderer process
Add last launched app flag, and read flag value into crash key on renderer start
BUG=internal b/18763538
Committed: https://crrev.com/ef0c608055067fb0cfceb90d4693479878307914
Cr-Commit-Position: refs/heads/master@{#317874}
Patch Set 1 #Patch Set 2 #
Total comments: 2
Patch Set 3 : Moved read to renderer start up #Patch Set 4 : add previous app #
Total comments: 4
Patch Set 5 : comment + rebase #
Total comments: 2
Patch Set 6 : Additional comments #
Messages
Total messages: 23 (3 generated)
kjoswiak@chromium.org changed reviewers: + byungchul@chromium.org, gunsch@chromium.org
The other option would be to read off during renderer startup, but I'm not sure best place to do that. I have a CL that adds to CastContentRendererClient::RenderThreadStarted, and it works, but chromium cl upload doesn't like that it uses an include from chromecast/crash/. Is there better place to put it in start up sequence? Or should I just use this CL?
On 2015/02/23 20:38:59, kjoswiak wrote: > The other option would be to read off during renderer startup, but I'm not sure > best place to do that. I have a CL that adds to > CastContentRendererClient::RenderThreadStarted, and it works, but chromium cl > upload doesn't like that it uses an include from chromecast/crash/. > > Is there better place to put it in start up sequence? > Or should I just use this CL? CCRC::RenderThreadStarted sounds like a good place for this. You can add a DEPS entry from chromecast/renderer to chromecast/crash.
https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... File chromecast/common/chromecast_switches.h (right): https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... chromecast/common/chromecast_switches.h:22: extern const char kLastLaunchedAppId[]; it's a string that's more than just the app id, right? probably "kLastLaunchedApp" is okay
On 2015/02/23 21:29:27, gunsch wrote: > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > File chromecast/common/chromecast_switches.h (right): > > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > chromecast/common/chromecast_switches.h:22: extern const char > kLastLaunchedAppId[]; > it's a string that's more than just the app id, right? probably > "kLastLaunchedApp" is okay Is it to report what app is it for this renderer process? Why not for all app ids', last, previous and current? Other platforms also have same problem.
On 2015/02/23 22:01:13, byungchul wrote: > On 2015/02/23 21:29:27, gunsch wrote: > > > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > > File chromecast/common/chromecast_switches.h (right): > > > > > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > > chromecast/common/chromecast_switches.h:22: extern const char > > kLastLaunchedAppId[]; > > it's a string that's more than just the app id, right? probably > > "kLastLaunchedApp" is okay > > Is it to report what app is it for this renderer process? Why not for all app > ids', last, previous and current? > Other platforms also have same problem. Right now, current_app and previous_app do not seem to be set to relevant values when renderer starts, so passing as a flag wouldn't work, would need ipc to communicate when/if they get updated (should I do this?) Also, it turns out this does not seem to be an issue on chromecast, since browser proc seems to do the writing to our crash lock file, it writes the metadata (and has access to current app state interface) and then just associates it to the dump file renderer wrote. Crash keys write directly to dump file, so renderer needs to know. Patch set 2 makes this flag available to all platforms, anyway
On 2015/02/23 22:12:12, kjoswiak wrote: > On 2015/02/23 22:01:13, byungchul wrote: > > On 2015/02/23 21:29:27, gunsch wrote: > > > > > > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > > > File chromecast/common/chromecast_switches.h (right): > > > > > > > > > https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... > > > chromecast/common/chromecast_switches.h:22: extern const char > > > kLastLaunchedAppId[]; > > > it's a string that's more than just the app id, right? probably > > > "kLastLaunchedApp" is okay > > > > Is it to report what app is it for this renderer process? Why not for all app > > ids', last, previous and current? > > Other platforms also have same problem. > > Right now, current_app and previous_app do not seem to be set to relevant values > when renderer starts, so passing as a flag wouldn't work, would need ipc to > communicate when/if they get updated (should I do this?) > > Also, it turns out this does not seem to be an issue on chromecast, since > browser proc seems to do the writing to our crash lock file, it writes the > metadata (and has access to current app state interface) and then just > associates it to the dump file renderer wrote. Crash keys write directly to dump > file, so renderer needs to know. Patch set 2 makes this flag available to all > platforms, anyway Patch set 3*
https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... File chromecast/common/chromecast_switches.h (right): https://codereview.chromium.org/944733003/diff/20001/chromecast/common/chrome... chromecast/common/chromecast_switches.h:22: extern const char kLastLaunchedAppId[]; On 2015/02/23 21:29:26, gunsch wrote: > it's a string that's more than just the app id, right? probably > "kLastLaunchedApp" is okay Technically last_app is just the app id, its current_app/previous_app that aren't, and right now aren't only on chromecast platform. But, I think kLastLaunchedApp is more consistent with other naming so Done.
lgtm
https://codereview.chromium.org/944733003/diff/60001/chromecast/common/chrome... File chromecast/common/chromecast_switches.h (right): https://codereview.chromium.org/944733003/diff/60001/chromecast/common/chrome... chromecast/common/chromecast_switches.h:22: extern const char kLastLaunchedApp[]; Add a comment above describing these https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... chromecast/renderer/cast_content_renderer_client.cc:119: base::debug::SetCrashKeyValue(crash_keys::kLastApp, last_launched_app); do we also want to set the kCurrentApp crash key here, or just leave it empty on the renderer?
https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... chromecast/renderer/cast_content_renderer_client.cc:119: base::debug::SetCrashKeyValue(crash_keys::kLastApp, last_launched_app); On 2015/02/24 00:54:43, gunsch wrote: > do we also want to set the kCurrentApp crash key here, or just leave it empty on > the renderer? Either way fine to me.
On 2015/02/24 01:04:13, byungchul wrote: > https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... > File chromecast/renderer/cast_content_renderer_client.cc (right): > > https://codereview.chromium.org/944733003/diff/60001/chromecast/renderer/cast... > chromecast/renderer/cast_content_renderer_client.cc:119: > base::debug::SetCrashKeyValue(crash_keys::kLastApp, last_launched_app); > On 2015/02/24 00:54:43, gunsch wrote: > > do we also want to set the kCurrentApp crash key here, or just leave it empty > on > > the renderer? > > Either way fine to me. I think I'll leave empty then. Everything else all good?
I had one other comment in chromecast_switches.h, and you probably need to sync/rebase before submitting
Whoops, missed that https://codereview.chromium.org/944733003/diff/60001/chromecast/common/chrome... File chromecast/common/chromecast_switches.h (right): https://codereview.chromium.org/944733003/diff/60001/chromecast/common/chrome... chromecast/common/chromecast_switches.h:22: extern const char kLastLaunchedApp[]; On 2015/02/24 00:54:43, gunsch wrote: > Add a comment above describing these Done.
https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... File chromecast/common/chromecast_switches.cc (right): https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... chromecast/common/chromecast_switches.cc:23: const char kLastLaunchedApp[] = "last-launched-app"; Please include a more descriptive comment here. the .cc file should be more explicit in the description than the .h file (see content_switches.* for example)
بتاريخ Feb 23, 2015 11:39 PM، كتبها <kjoswiak@chromium.org>: > Reviewers: gunsch, byungchul, > > Message: > The other option would be to read off during renderer startup, but I'm not > sure > best place to do that. I have a CL that adds to > CastContentRendererClient::RenderThreadStarted, and it works, but > chromium cl > upload doesn't like that it uses an include from chromecast/crash/. > > Is there better place to put it in start up sequence? > Or should I just use this CL? > > Description: > Add flag to communicate last casted app to renderer process > > Add last app flag, and read to crash key on process crash > > BUG=internal b/18763538 > > Please review this at https://codereview.chromium.org/944733003/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+27, -0 lines): > M chromecast/common/chromecast_switches.h > M chromecast/common/chromecast_switches.cc > M chromecast/crash/android/cast_crash_reporter_client_android.h > M chromecast/crash/android/cast_crash_reporter_client_android.cc > > > Index: chromecast/common/chromecast_switches.cc > diff --git a/chromecast/common/chromecast_switches.cc b/chromecast/common/ > chromecast_switches.cc > index d66d2ffca8bfecd39f1d0cc169fc3336d5e30f64.. > 4bda0edeb4e86e02d6fbcf205a22d5f2e3c54636 100644 > --- a/chromecast/common/chromecast_switches.cc > +++ b/chromecast/common/chromecast_switches.cc > @@ -17,4 +17,6 @@ const char kEnableLocalFileAccesses[] = > "enable-local-file-accesses"; > // Override the URL to which metrics logs are sent for debugging. > const char kOverrideMetricsUploadUrl[] = "override-metrics-upload-url"; > > +const char kLastLaunchedAppId[] = "last-app-id"; > + > } // namespace switches > Index: chromecast/common/chromecast_switches.h > diff --git a/chromecast/common/chromecast_switches.h b/chromecast/common/ > chromecast_switches.h > index 34b0f762baf29c001b016e7ed5ddebafcb2c37a5.. > 20cc95680bd52a91f9044e269b0cfdd74ec8e72c 100644 > --- a/chromecast/common/chromecast_switches.h > +++ b/chromecast/common/chromecast_switches.h > @@ -19,6 +19,8 @@ extern const char kEnableLocalFileAccesses[]; > // Metrics switches > extern const char kOverrideMetricsUploadUrl[]; > > +extern const char kLastLaunchedAppId[]; > + > } // namespace switches > > #endif // CHROMECAST_COMMON_CHROMECAST_SWITCHES_H_ > Index: chromecast/crash/android/cast_crash_reporter_client_android.cc > diff --git a/chromecast/crash/android/cast_crash_reporter_client_android.cc > b/chromecast/crash/android/cast_crash_reporter_client_android.cc > index 41717f892f8addde369c70fc0e0f8cb04b330ebf.. > 05b1dc54235f8bbd389e953515c3d4c4e4fe52eb 100644 > --- a/chromecast/crash/android/cast_crash_reporter_client_android.cc > +++ b/chromecast/crash/android/cast_crash_reporter_client_android.cc > @@ -5,10 +5,12 @@ > #include "chromecast/crash/android/cast_crash_reporter_client_android.h" > > #include "base/base_paths.h" > +#include "base/command_line.h" > #include "base/files/file_path.h" > #include "base/files/file_util.h" > #include "base/path_service.h" > #include "chromecast/android/chromecast_config_android.h" > +#include "chromecast/common/chromecast_switches.h" > #include "chromecast/common/global_descriptors.h" > #include "chromecast/common/version.h" > #include "chromecast/crash/cast_crash_keys.h" > @@ -74,4 +76,20 @@ bool CastCrashReporterClientAndroid > ::EnableBreakpadForProcess( > process_type == switches::kGpuProcess; > } > > +bool CastCrashReporterClientAndroid::HandleCrashDump( > + const char* crashdump_filename) { > + base::CommandLine* command_line = base::CommandLine:: > ForCurrentProcess(); > + if (command_line->GetSwitchValueNative(switches::kProcessType) == > + switches::kRendererProcess) { > + std::string last_launched_app = > + command_line->GetSwitchValueASCII(switches::kLastLaunchedAppId); > + if (!last_launched_app.empty()) > + base::debug::SetCrashKeyValue(crash_keys::kLastApp, > last_launched_app); > + } > + > + // Now that we've registered crash metadata, return false to use default > + // breakpad dump writer > + return false; > +} > + > } // namespace chromecast > Index: chromecast/crash/android/cast_crash_reporter_client_android.h > diff --git a/chromecast/crash/android/cast_crash_reporter_client_android.h > b/chromecast/crash/android/cast_crash_reporter_client_android.h > index e98eef72605b0ac161193879382c74063da9c009.. > 1b45fe7adc8b064eee242a006fe0795dd5c48a2e 100644 > --- a/chromecast/crash/android/cast_crash_reporter_client_android.h > +++ b/chromecast/crash/android/cast_crash_reporter_client_android.h > @@ -27,6 +27,11 @@ class CastCrashReporterClientAndroid > const std::string& process_type) override; > size_t RegisterCrashKeys() override; > > + // Used as hook to register meta data when we receive crash. > + // Does not actually handle crash file, and returns false with > expectation > + // default breakpad code will do aditional processing. > + bool HandleCrashDump(const char* crashdump_filename) override; > + > private: > DISALLOW_COPY_AND_ASSIGN(CastCrashReporterClientAndroid); > }; > > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Who/what is this wfa511com? https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... File chromecast/common/chromecast_switches.cc (right): https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... chromecast/common/chromecast_switches.cc:23: const char kLastLaunchedApp[] = "last-launched-app"; On 2015/02/24 01:51:22, gunsch wrote: > Please include a more descriptive comment here. the .cc file should be more > explicit in the description than the .h file (see content_switches.* for > example) Done.
On 2015/02/24 02:05:09, kjoswiak wrote: > Who/what is this wfa511com? > > https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... > File chromecast/common/chromecast_switches.cc (right): > > https://codereview.chromium.org/944733003/diff/80001/chromecast/common/chrome... > chromecast/common/chromecast_switches.cc:23: const char kLastLaunchedApp[] = > "last-launched-app"; > On 2015/02/24 01:51:22, gunsch wrote: > > Please include a more descriptive comment here. the .cc file should be more > > explicit in the description than the .h file (see content_switches.* for > > example) > > Done. lgtm looking at other CLs wfa511com has jumped on, looks like a random spambot (?).
The CQ bit was checked by kjoswiak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from byungchul@chromium.org Link to the patchset: https://codereview.chromium.org/944733003/#ps100001 (title: "Additional comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944733003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ef0c608055067fb0cfceb90d4693479878307914 Cr-Commit-Position: refs/heads/master@{#317874} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
