Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441623002:
Split MinidumpUploadService into core- and Chrome-implementation. (Closed)
DescriptionSplit MinidumpUploadService into core- and Chrome-implementation.
To componentize MinidumpUploadService we split it into its core
implementation and an implementation dependent on Chrome. The
Chrome-dependent parts now live in ChromeMinidumpUploadDelegate.
To inject this delegate into the MinidumpUploadService we have to create
a Chrome-specific version of the MinidumpUploadService (inheriting from
it) named ChromeMinidumpUploadService.
BUG=652719
Committed: https://crrev.com/271ef66ac8bab41f55f18586a1cd2fd2970d4d79
Cr-Commit-Position: refs/heads/master@{#427989}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fix Ilya's nits and add some Chrome-specific tests. #
Total comments: 7
Patch Set 3 : Added static setter/getter for MinidumpUploadDelegate + removed ChromeMinidumpUploadService. #
Total comments: 23
Patch Set 4 : Check crashType in MinidmpUploadServiceTests + ensure minidump delegate is set in minidump service … #
Total comments: 3
Patch Set 5 : Rebase (preference/permission changes). #Messages
Total messages: 27 (6 generated)
gsennton@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, here's another componentization CL, after this one we should be able to move everything to the new component :) There are still a couple of things I need to check before committing this CL (I haven't run the code yet), but it should be reviewable right now, so PTAL :)
Thanks! LGTM % some nits: https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:771: <!-- TODO Could add a test to ensure you can start ChromeMinidumpUploadService through startService... --> nit: Is this a TODO that you're intending to commit, or one that you intended to address as part of this CL? If it's the prior, please write the TODO in the format "TODO(username):" https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:31: // Increment the count of success by 1 and distinguish between different types of nit: Please remove the extra space before "Increment" https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadService.java:16: return new ChromeMinidumpUploadDelegate(); Would it make sense to cache the returned pointer, rather than creating a new object each time? https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:10: * Interface giving clients of the Minidump uploader component the ability to control and log Optional nit: It's not clear to me that capitalizing "Minidump" is helpful -- I'd lowercase it throughout. https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:16: * @param fileName is the name of the Minidump file that was uploaded. nit: Please document the |context| param as well. https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:18: void onSuccessfulUpload(Context context, String fileName); nit: Would it make sense to pass a crash type rather than a filename? (Ditto below.)
https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:771: <!-- TODO Could add a test to ensure you can start ChromeMinidumpUploadService through startService... --> On 2016/10/21 00:27:27, Ilya Sherman wrote: > nit: Is this a TODO that you're intending to commit, or one that you intended to > address as part of this CL? If it's the prior, please write the TODO in the > format "TODO(username):" I've added ChromeMinidumpUploadServiceTest now :). https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:31: // Increment the count of success by 1 and distinguish between different types of On 2016/10/21 00:27:27, Ilya Sherman wrote: > nit: Please remove the extra space before "Increment" Done. https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadService.java:16: return new ChromeMinidumpUploadDelegate(); On 2016/10/21 00:27:27, Ilya Sherman wrote: > Would it make sense to cache the returned pointer, rather than creating a new > object each time? Sure, done :) https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:116: Intent intent = new Intent(context, MinidumpUploadService.class); Hmmm, this shouldn't work - MinidumpUploadService isn't declared in the AndroidManifest. I guess we never try to call startService(createFindAndUploadLastCrashIntent)) :/ The tests that call createFindAndUploadLastCrashIntent just pass the returned Intent to service.onHandleIntent. Anyway, this is a bit ugly - all of the create****Intent methods are static and need to pass a child-class of MinidumpUploadService rather than MinidumpUploadService itself into the Intent ctor. We could either 1. make these method non-static (so that we can call new ChromeMinidumpUploadService.createFindAndUploadLastCrashIntent()) and pass this.getClass to the Intent ctor, or we can 2. move these static methods to ChromeMinidumpUploadService. (There's also the third option of passing a class to createFindAndUploadLastCrashIntent but that seems ugly..?). It's kind of ugly to move the static methods to ChromeMinidumpUploadService since if we would ever want another Service to inherit MinidumpUploadService it would have to redefine those methods. Any thoughts? https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:10: * Interface giving clients of the Minidump uploader component the ability to control and log On 2016/10/21 00:27:27, Ilya Sherman wrote: > Optional nit: It's not clear to me that capitalizing "Minidump" is helpful -- > I'd lowercase it throughout. Done. https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:16: * @param fileName is the name of the Minidump file that was uploaded. On 2016/10/21 00:27:27, Ilya Sherman wrote: > nit: Please document the |context| param as well. Hah, it turns out the Context isn't even needed :) (See ChromePreferenceManager - it was probably used to fetch shared preferences before the method ContextUtils.getAppSharedPreferences() came along). It seems ChromePreferenceManager is used in a lot of places though - maybe I should remove that context in another CL? Generally people are not very good at documenting what a Context is for - an Android Context is usually something you just pass around everywhere because it is used in ctors of many Android objects and it holds pointers to useful stuff. https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:18: void onSuccessfulUpload(Context context, String fileName); On 2016/10/21 00:27:27, Ilya Sherman wrote: > nit: Would it make sense to pass a crash type rather than a filename? (Ditto > below.) Definitely :), done.
https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:116: Intent intent = new Intent(context, MinidumpUploadService.class); On 2016/10/21 13:43:32, gsennton wrote: > Hmmm, this shouldn't work - MinidumpUploadService isn't declared in the > AndroidManifest. > I guess we never try to call startService(createFindAndUploadLastCrashIntent)) > :/ > The tests that call createFindAndUploadLastCrashIntent just pass the returned > Intent to service.onHandleIntent. I'm not sure what you mean. I think the code is called here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > Anyway, this is a bit ugly - all of the create****Intent methods are static and > need to pass a child-class of MinidumpUploadService rather than > MinidumpUploadService itself into the Intent ctor. > We could either 1. make these method non-static (so that we can call new > ChromeMinidumpUploadService.createFindAndUploadLastCrashIntent()) and pass > this.getClass to the Intent ctor, or we can 2. move these static methods to > ChromeMinidumpUploadService. (There's also the third option of passing a class > to createFindAndUploadLastCrashIntent but that seems ugly..?). > It's kind of ugly to move the static methods to ChromeMinidumpUploadService > since if we would ever want another Service to inherit MinidumpUploadService it > would have to redefine those methods. > Any thoughts? Hmm, interesting question! I think this is beyond my Java/Android expertise -- I'm not sure that I understand Services and Intents well enough to provide great guidance here. I think that maybe passing a class into these methods isn't such a bad idea, since that's what's eventually passed to the "new Intent()" call... but my intuitions for this sort of question are weak. FWIW, I don't think it quite works to make these methods non-static, since the services are meant to run in a separate process from the calling process. Again, I'm not quite sure that my understanding is correct... https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:16: * @param fileName is the name of the Minidump file that was uploaded. On 2016/10/21 13:43:32, gsennton wrote: > On 2016/10/21 00:27:27, Ilya Sherman wrote: > > nit: Please document the |context| param as well. > > Hah, it turns out the Context isn't even needed :) (See ChromePreferenceManager > - it was probably used to fetch shared preferences before the method > ContextUtils.getAppSharedPreferences() came along). It seems > ChromePreferenceManager is used in a lot of places though - maybe I should > remove that context in another CL? > Generally people are not very good at documenting what a Context is for - an > Android Context is usually something you just pass around everywhere because it > is used in ctors of many Android objects and it holds pointers to useful stuff. Yep, a separate CL to remove it would be great -- thanks! https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:38: public void onMaxedOutUploadFailures(Context context, String fileName) { Please update this function and the one below to use the passed crash type. (It's too bad that's not a compile error -- or is it?) https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java:29: mMinidumpFile = new File(mCrashDir, "chromium_renderer-111.dmp1"); Please document where this file will be cleaned up. https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java:63: assertTrue(mTargetContext.stopService(uploadIntent)); Hmm, these seem like very minimal tests. Could you please either document why minimal tests make sense for this code, or expand the tests to test more specific details of the created intents?
gsennton@chromium.org changed reviewers: + mariakhomenko@google.com
I'm adding Maria here in case she has any thoughts on the way I'm splitting MinidumpUploadService in two here. Hi Maria, I'm not sure whether we should componentize the MinidumpUploadService - see the comments in https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... do you have any thoughts here? IMO putting the core implementation in MinidumpUploadService and then adding ChromeMinidumpUploadService might introduce too much complexity (because then you can never start MinidumpUploadService directly - you will always have to call into ChromeMinidumpUploadService). https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:116: Intent intent = new Intent(context, MinidumpUploadService.class); On 2016/10/21 21:47:51, Ilya Sherman wrote: > On 2016/10/21 13:43:32, gsennton wrote: > > Hmmm, this shouldn't work - MinidumpUploadService isn't declared in the > > AndroidManifest. > > I guess we never try to call startService(createFindAndUploadLastCrashIntent)) > > :/ > > The tests that call createFindAndUploadLastCrashIntent just pass the returned > > Intent to service.onHandleIntent. > > I'm not sure what you mean. I think the code is called here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Right, it's called in reality - I meant it's not called in any tests :) (which is why I added ChromeMinidumpUploadServiceTest). > > Anyway, this is a bit ugly - all of the create****Intent methods are static > and > > need to pass a child-class of MinidumpUploadService rather than > > MinidumpUploadService itself into the Intent ctor. > > We could either 1. make these method non-static (so that we can call new > > ChromeMinidumpUploadService.createFindAndUploadLastCrashIntent()) and pass > > this.getClass to the Intent ctor, or we can 2. move these static methods to > > ChromeMinidumpUploadService. (There's also the third option of passing a class > > to createFindAndUploadLastCrashIntent but that seems ugly..?). > > It's kind of ugly to move the static methods to ChromeMinidumpUploadService > > since if we would ever want another Service to inherit MinidumpUploadService > it > > would have to redefine those methods. > > Any thoughts? > > Hmm, interesting question! I think this is beyond my Java/Android expertise -- > I'm not sure that I understand Services and Intents well enough to provide great > guidance here. I think that maybe passing a class into these methods isn't such > a bad idea, since that's what's eventually passed to the "new Intent()" call... > but my intuitions for this sort of question are weak. FWIW, I don't think it > quite works to make these methods non-static, since the services are meant to > run in a separate process from the calling process. Again, I'm not quite sure > that my understanding is correct... I'm starting to consider whether we should just leave MinidumpUploadService (and everything else except the classes WebView needs - CrashFileManager and MinidumpUploadCallable) in chrome/ :P. Mainly because it seems componentizing the upload service in this way would make the code more complex and error-prone (e.g. somebody might create a new child of MinidumpUploadService and then accidentally mix the use of that Service and the original ChromeMinidumpUploadService). WDYT? Regarding the comment about static/non-static methods + a separate process: AFAICT for Chrome these Service are being run in the same process as they are called, for WebView (where Service(s) will be running in a different process) we will create our own Service(s) and they should be unrelated to MinidumpUploadService. I don't see how it would matter whether the Services would be running in a different process either, we still need to reference them from the calling process to initiate them. What I mean when I say we would use non-static methods is that we would just create a new ChromeMinidumpUploadService and call some method on that instance - it wouldn't matter whether we saved that Service-object or not. I guess that is also a pretty ugly way of solving this, and maybe, as you say, we should just pass the class to these methods instead :) https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:38: public void onMaxedOutUploadFailures(Context context, String fileName) { On 2016/10/21 21:47:51, Ilya Sherman wrote: > Please update this function and the one below to use the passed crash type. > (It's too bad that's not a compile error -- or is it?) This compiles fine. What part of this would be a compilation error though? :/. The only potential compilation issue I could think of here is that we pass the @ProcessType String named fileName to MinidumpUploadService.getCrashType(String) - but that in itself shouldn't be a problem since fileName is indeed a String (though it should be a restricted one with one of only a certain set of values). So we are never actually using a non-ProcessType String as a ProcessType, we are just passing a ProcessType String to a method that takes a generic String (which should be OK, though in our case it's a bug). https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java:63: assertTrue(mTargetContext.stopService(uploadIntent)); On 2016/10/21 21:47:51, Ilya Sherman wrote: > Hmm, these seem like very minimal tests. Could you please either document why > minimal tests make sense for this code, or expand the tests to test more > specific details of the created intents? Yeah, they are just meant to ensure that we can actually start the upload service - but given that this should break startup after a crash, maybe we don't need these tests
gsennton@chromium.org changed reviewers: + mariakhomenko@chromium.org - mariakhomenko@google.com
Actually adding Maria.. please see my last comment.
In terms of not componentizing the service -- yes, maybe you're right. I hadn't realized that the static methods would make the service harder and more error-prone to use in a componentized setup. But, I wonder if there's a nice solution that hides most of this awkwardness. Typically, componentization works well when a Chrome-specific version only needs to be created in a single place, and everything else works fine without it. For example, could the componentized service have a static member that points to the concrete class* that should be used, and (a) error out when that member isn't set, and (b) only require Chrome code to set it once? If so, that gives clients the desired simplicity -- even for calling static methods -- without sacrificing the opportunity to componentize the code. * The concrete class could either be the Chrome-specific subclass of the service; or even better, the Chrome-specific implementation of the delegate. https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:38: public void onMaxedOutUploadFailures(Context context, String fileName) { On 2016/10/24 13:37:19, gsennton wrote: > On 2016/10/21 21:47:51, Ilya Sherman wrote: > > Please update this function and the one below to use the passed crash type. > > (It's too bad that's not a compile error -- or is it?) > > This compiles fine. What part of this would be a compilation error though? :/. > The only potential compilation issue I could think of here is > that we pass the @ProcessType String named fileName to > MinidumpUploadService.getCrashType(String) - but that in itself shouldn't be a > problem since fileName is indeed a String (though it should be a restricted one > with one of only a certain set of values). So we are never actually using a > non-ProcessType String as a ProcessType, we are just passing a ProcessType > String to a method that takes a generic String (which should be OK, though in > our case it's a bug). I guess I'd just hoped it would be treated as its own type, rather than a string.
On 2016/10/25 04:52:51, Ilya Sherman wrote: > In terms of not componentizing the service -- yes, maybe you're right. I hadn't > realized that the static methods would make the service harder and more > error-prone to use in a componentized setup. But, I wonder if there's a nice > solution that hides most of this awkwardness. Typically, componentization works > well when a Chrome-specific version only needs to be created in a single place, > and everything else works fine without it. For example, could the componentized > service have a static member that points to the concrete class* that should be > used, and (a) error out when that member isn't set, and (b) only require Chrome > code to set it once? If so, that gives clients the desired simplicity -- even > for calling static methods -- without sacrificing the opportunity to > componentize the code. > > * The concrete class could either be the Chrome-specific subclass of the > service; or even better, the Chrome-specific implementation of the delegate. Right! Having one single reference to the class that should be used as Update Service makes more sense (I'd completely forgotten that was a common pattern when componentizing), I'll give that a go and see if it seems fine :). Thanks for pointing that out! > https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java > (right): > > https://codereview.chromium.org/2441623002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java:38: > public void onMaxedOutUploadFailures(Context context, String fileName) { > On 2016/10/24 13:37:19, gsennton wrote: > > On 2016/10/21 21:47:51, Ilya Sherman wrote: > > > Please update this function and the one below to use the passed crash type. > > > (It's too bad that's not a compile error -- or is it?) > > > > This compiles fine. What part of this would be a compilation error though? :/. > > The only potential compilation issue I could think of here is > > that we pass the @ProcessType String named fileName to > > MinidumpUploadService.getCrashType(String) - but that in itself shouldn't be a > > problem since fileName is indeed a String (though it should be a restricted > one > > with one of only a certain set of values). So we are never actually using a > > non-ProcessType String as a ProcessType, we are just passing a ProcessType > > String to a method that takes a generic String (which should be OK, though in > > our case it's a bug). > > I guess I'd just hoped it would be treated as its own type, rather than a > string. Yeah, annotations are not as cool as we would want I guess :/
Ok, now I feel stupid :). As you pointed out: we can set the MinidumpUploadDelegate statically -- and then there is no need at all anymore for the ChromeMinidumpUploadService ;) (since the only point of that service was to provide a reference to the delegate).
https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java (right): https://codereview.chromium.org/2441623002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java:16: * @param fileName is the name of the Minidump file that was uploaded. On 2016/10/21 21:47:51, Ilya Sherman wrote: > On 2016/10/21 13:43:32, gsennton wrote: > > On 2016/10/21 00:27:27, Ilya Sherman wrote: > > > nit: Please document the |context| param as well. > > > > Hah, it turns out the Context isn't even needed :) (See > ChromePreferenceManager > > - it was probably used to fetch shared preferences before the method > > ContextUtils.getAppSharedPreferences() came along). It seems > > ChromePreferenceManager is used in a lot of places though - maybe I should > > remove that context in another CL? > > Generally people are not very good at documenting what a Context is for - an > > Android Context is usually something you just pass around everywhere because > it > > is used in ctors of many Android objects and it holds pointers to useful > stuff. > > Yep, a separate CL to remove it would be great -- thanks! Filed crbug.com/659038 so I don't forget this. https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2441623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadServiceTest.java:63: assertTrue(mTargetContext.stopService(uploadIntent)); On 2016/10/24 13:37:19, gsennton wrote: > On 2016/10/21 21:47:51, Ilya Sherman wrote: > > Hmm, these seem like very minimal tests. Could you please either document why > > minimal tests make sense for this code, or expand the tests to test more > > specific details of the created intents? > > Yeah, they are just meant to ensure that we can actually start the upload > service - but given that this should break startup after a crash, maybe we don't > need these tests I removed these tests now that I've removed ChromeMinidumpUploadService :). https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and it didn't show up except in the logs (if getCrashType fails with an IOException it just prints to the logs). Even some of our tests in MinidumpUploadServiceTest trigger the IOException inside getCrashType because we don't rename the minidump file after uploading it in the tests. Not sure if we should make this more robust / actually throw a run-time exception when catching an IOException..? (throwing a new exception seems scary though ;)). https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:309: getUploadDelegate().getCrashReportingPermissionManager()); It is slightly annoying that getUploadDelegate() is not called until we actually try to upload a Minidump - so if we ever forget to set the upload delegate at startup we won't realize this until we crash and try to upload a minidump :/ Would it be weird to throw a run-time exception somewhere early on if the delegate hasn't been set? (MinidumpUploadService ctor / handleIntent / each createFooIntent method).
Current split to have a Chrome specific delegate looks good. Just a few small comments. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:56: private static Object sDelegateLock = new Object(); this should be final https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:72: "The upload delegate has already been set."); you can only omit {} when the expression fits on the same line as if() statement. Please move throw statement to a new line and add {} around it. Also, we never seem to use AndroidRuntimeException in Chrome. How about switching this to RuntimeException instead? https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:80: "The upload delegate must be set before use."); same comments as above https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/25 13:13:53, gsennton wrote: > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and it didn't > show up except in the logs (if getCrashType fails with an IOException it just > prints to the logs). > Even some of our tests in MinidumpUploadServiceTest trigger the IOException > inside getCrashType because we don't rename the minidump file after uploading it > in the tests. > Not sure if we should make this more robust / actually throw a run-time > exception when catching an IOException..? (throwing a new exception seems scary > though ;)). A couple of things here: 1) The method is currently badly named -- get usually implies a no-side-effects getter rather than a mutator method. Can you rename this method to "updateNameAfterSuccessfulUpload"? 2) The tests should fail if IOException is not expected -- rather than throwing runtime exception, how about asserting in the test that the crash type returned is not OTHER? https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:309: getUploadDelegate().getCrashReportingPermissionManager()); On 2016/10/25 13:13:53, gsennton wrote: > It is slightly annoying that getUploadDelegate() is not called until we actually > try to upload a Minidump - so if we ever forget to set the upload delegate at > startup we won't realize this until we crash and try to upload a minidump :/ > > Would it be weird to throw a run-time exception somewhere early on if the > delegate hasn't been set? (MinidumpUploadService ctor / handleIntent / each > createFooIntent method). I would be okay with a no-op getUploadDelegate() call in constructor with a comment that this is done to ensure the delegate is set.
Thanks! https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:55: // Lock for the MinidumpUploadDelegate nit: What's the difference between "//" comments (as on this line) and "/** ... */" comments as for the previous member, when documenting variables? (Mostly for my edification.) https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:70: synchronized (sDelegateLock) { I don't suppose we could instead assert that the work is done on the "expected" thread? That's typically what we'd do in similar C++ code. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/25 17:34:44, Maria wrote: > On 2016/10/25 13:13:53, gsennton wrote: > > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and it > didn't > > show up except in the logs (if getCrashType fails with an IOException it just > > prints to the logs). > > Even some of our tests in MinidumpUploadServiceTest trigger the IOException > > inside getCrashType because we don't rename the minidump file after uploading > it > > in the tests. > > Not sure if we should make this more robust / actually throw a run-time > > exception when catching an IOException..? (throwing a new exception seems > scary > > though ;)). > > A couple of things here: > 1) The method is currently badly named -- get usually implies a no-side-effects > getter rather than a mutator method. Can you rename this method to > "updateNameAfterSuccessfulUpload"? Hmm, what do you mean? That is, what is the side effect that's inappropriate? I *think* the method just takes a string input and returns a string output. > 2) The tests should fail if IOException is not expected -- rather than throwing > runtime exception, how about asserting in the test that the crash type returned > is not OTHER? Agreed =) I don't think that it's appropriate to raise an exception if the file can't be read, because presumably sometimes this will happen for some crash files due to corruption, and we don't want the crash reporter to throw an exception and bail on whatever other files are present. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:309: getUploadDelegate().getCrashReportingPermissionManager()); On 2016/10/25 17:34:44, Maria wrote: > On 2016/10/25 13:13:53, gsennton wrote: > > It is slightly annoying that getUploadDelegate() is not called until we > actually > > try to upload a Minidump - so if we ever forget to set the upload delegate at > > startup we won't realize this until we crash and try to upload a minidump :/ > > > > Would it be weird to throw a run-time exception somewhere early on if the > > delegate hasn't been set? (MinidumpUploadService ctor / handleIntent / each > > createFooIntent method). > > I would be okay with a no-op getUploadDelegate() call in constructor with a > comment that this is done to ensure the delegate is set. +1
https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:55: // Lock for the MinidumpUploadDelegate On 2016/10/25 21:16:54, Ilya Sherman wrote: > nit: What's the difference between "//" comments (as on this line) and "/** ... > */" comments as for the previous member, when documenting variables? (Mostly > for my edification.) /** are comments that would show up in javadoc, whereas // wouldn't. Generally the split is between documenting interfaces vs implementation comments, but it's not strictly followed. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:70: synchronized (sDelegateLock) { On 2016/10/25 21:16:54, Ilya Sherman wrote: > I don't suppose we could instead assert that the work is done on the "expected" > thread? That's typically what we'd do in similar C++ code. This is called from a background task that runs on a thread pool https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/25 21:16:54, Ilya Sherman wrote: > On 2016/10/25 17:34:44, Maria wrote: > > On 2016/10/25 13:13:53, gsennton wrote: > > > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and it > > didn't > > > show up except in the logs (if getCrashType fails with an IOException it > just > > > prints to the logs). > > > Even some of our tests in MinidumpUploadServiceTest trigger the IOException > > > inside getCrashType because we don't rename the minidump file after > uploading > > it > > > in the tests. > > > Not sure if we should make this more robust / actually throw a run-time > > > exception when catching an IOException..? (throwing a new exception seems > > scary > > > though ;)). > > > > A couple of things here: > > 1) The method is currently badly named -- get usually implies a > no-side-effects > > getter rather than a mutator method. Can you rename this method to > > "updateNameAfterSuccessfulUpload"? > > Hmm, what do you mean? That is, what is the side effect that's inappropriate? > I *think* the method just takes a string input and returns a string output. Hmm, I guess you are right - for some reason I thought file name field was being changed, but I guess it's just the param. Never mind. > > > 2) The tests should fail if IOException is not expected -- rather than > throwing > > runtime exception, how about asserting in the test that the crash type > returned > > is not OTHER? > > Agreed =) > > I don't think that it's appropriate to raise an exception if the file can't be > read, because presumably sometimes this will happen for some crash files due to > corruption, and we don't want the crash reporter to throw an exception and bail > on whatever other files are present.
Here we go! https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:55: // Lock for the MinidumpUploadDelegate On 2016/10/25 21:50:20, Maria wrote: > On 2016/10/25 21:16:54, Ilya Sherman wrote: > > nit: What's the difference between "//" comments (as on this line) and "/** > ... > > */" comments as for the previous member, when documenting variables? (Mostly > > for my edification.) > > /** are comments that would show up in javadoc, whereas // wouldn't. Generally > the split is between documenting interfaces vs implementation comments, but it's > not strictly followed. Right :P, I turned the javadoc comment here into a normal comment since it's an implementation detail. Also updated the actual comment. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:56: private static Object sDelegateLock = new Object(); On 2016/10/25 17:34:44, Maria wrote: > this should be final Done. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:72: "The upload delegate has already been set."); On 2016/10/25 17:34:44, Maria wrote: > you can only omit {} when the expression fits on the same line as if() > statement. Please move throw statement to a new line and add {} around it. > > Also, we never seem to use AndroidRuntimeException in Chrome. How about > switching this to RuntimeException instead? Done. Out of curiosity, do you know if there's any specific reason for using RuntimeException instead of AndroidRuntimeException? https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:80: "The upload delegate must be set before use."); On 2016/10/25 17:34:44, Maria wrote: > same comments as above Done. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/25 21:50:20, Maria wrote: > On 2016/10/25 21:16:54, Ilya Sherman wrote: > > On 2016/10/25 17:34:44, Maria wrote: > > > On 2016/10/25 13:13:53, gsennton wrote: > > > > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and it > > > didn't > > > > show up except in the logs (if getCrashType fails with an IOException it > > just > > > > prints to the logs). > > > > Even some of our tests in MinidumpUploadServiceTest trigger the > IOException > > > > inside getCrashType because we don't rename the minidump file after > > uploading > > > it > > > > in the tests. > > > > Not sure if we should make this more robust / actually throw a run-time > > > > exception when catching an IOException..? (throwing a new exception seems > > > scary > > > > though ;)). > > > > > > A couple of things here: > > > 1) The method is currently badly named -- get usually implies a > > no-side-effects > > > getter rather than a mutator method. Can you rename this method to > > > "updateNameAfterSuccessfulUpload"? > > > > Hmm, what do you mean? That is, what is the side effect that's inappropriate? > > > I *think* the method just takes a string input and returns a string output. > > Hmm, I guess you are right - for some reason I thought file name field was being > changed, but I guess it's just the param. Never mind. > > > > > 2) The tests should fail if IOException is not expected -- rather than > > throwing > > > runtime exception, how about asserting in the test that the crash type > > returned > > > is not OTHER? > > > > Agreed =) > > > > I don't think that it's appropriate to raise an exception if the file can't be > > read, because presumably sometimes this will happen for some crash files due > to > > corruption, and we don't want the crash reporter to throw an exception and > bail > > on whatever other files are present. > Hmm, so I want to check the return-value of getCrashType but currently the file-renaming after a successful upload happens in MinidumpUploadCallable.handleExecutionResponse - there is even a TODO there by acleung@ to move the renaming into MinidumpUploadService. Since some of the tests in MinidumpUploadServiceTest uses a custom MinidumpUploadCallable.call() method the renaming will never happen during those tests - so getCrashType will then fail to read the new file (and return MinidumpUploadDELEGATE.OTHER). I would like to move the file-renaming out to MinidumpUploadService but it seems wrong to include a change like that in the current CL, do you mind if I add a bug and do this in a follow-up instead? And thus wait to check the crash-type - so that it's not OTHER - inside onSuccessfulUpload until I upload that change? https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:309: getUploadDelegate().getCrashReportingPermissionManager()); On 2016/10/25 21:16:54, Ilya Sherman wrote: > On 2016/10/25 17:34:44, Maria wrote: > > On 2016/10/25 13:13:53, gsennton wrote: > > > It is slightly annoying that getUploadDelegate() is not called until we > > actually > > > try to upload a Minidump - so if we ever forget to set the upload delegate > at > > > startup we won't realize this until we crash and try to upload a minidump :/ > > > > > > Would it be weird to throw a run-time exception somewhere early on if the > > > delegate hasn't been set? (MinidumpUploadService ctor / handleIntent / each > > > createFooIntent method). > > > > I would be okay with a no-op getUploadDelegate() call in constructor with a > > comment that this is done to ensure the delegate is set. > > +1 Done.
lgtm https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:72: "The upload delegate has already been set."); On 2016/10/26 12:34:50, gsennton wrote: > On 2016/10/25 17:34:44, Maria wrote: > > you can only omit {} when the expression fits on the same line as if() > > statement. Please move throw statement to a new line and add {} around it. > > > > Also, we never seem to use AndroidRuntimeException in Chrome. How about > > switching this to RuntimeException instead? > > Done. Out of curiosity, do you know if there's any specific reason for using > RuntimeException instead of AndroidRuntimeException? I think AndroidRuntimeException is generally used by the Android framework, rather than applications. But that's my best guess -- not 100% sure. https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/26 12:34:50, gsennton wrote: > On 2016/10/25 21:50:20, Maria wrote: > > On 2016/10/25 21:16:54, Ilya Sherman wrote: > > > On 2016/10/25 17:34:44, Maria wrote: > > > > On 2016/10/25 13:13:53, gsennton wrote: > > > > > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and > it > > > > didn't > > > > > show up except in the logs (if getCrashType fails with an IOException it > > > just > > > > > prints to the logs). > > > > > Even some of our tests in MinidumpUploadServiceTest trigger the > > IOException > > > > > inside getCrashType because we don't rename the minidump file after > > > uploading > > > > it > > > > > in the tests. > > > > > Not sure if we should make this more robust / actually throw a run-time > > > > > exception when catching an IOException..? (throwing a new exception > seems > > > > scary > > > > > though ;)). > > > > > > > > A couple of things here: > > > > 1) The method is currently badly named -- get usually implies a > > > no-side-effects > > > > getter rather than a mutator method. Can you rename this method to > > > > "updateNameAfterSuccessfulUpload"? > > > > > > Hmm, what do you mean? That is, what is the side effect that's > inappropriate? > > > > > I *think* the method just takes a string input and returns a string output. > > > > Hmm, I guess you are right - for some reason I thought file name field was > being > > changed, but I guess it's just the param. Never mind. > > > > > > > 2) The tests should fail if IOException is not expected -- rather than > > > throwing > > > > runtime exception, how about asserting in the test that the crash type > > > returned > > > > is not OTHER? > > > > > > Agreed =) > > > > > > I don't think that it's appropriate to raise an exception if the file can't > be > > > read, because presumably sometimes this will happen for some crash files due > > to > > > corruption, and we don't want the crash reporter to throw an exception and > > bail > > > on whatever other files are present. > > > > Hmm, so I want to check the return-value of getCrashType but currently the > file-renaming after a successful upload happens in > MinidumpUploadCallable.handleExecutionResponse - there is even a TODO there by > acleung@ to move the renaming into MinidumpUploadService. > > Since some of the tests in MinidumpUploadServiceTest uses a custom > MinidumpUploadCallable.call() method the renaming will never happen during those > tests - so getCrashType will then fail to read the new file (and return > MinidumpUploadDELEGATE.OTHER). > > I would like to move the file-renaming out to MinidumpUploadService but it seems > wrong to include a change like that in the current CL, do you mind if I add a > bug and do this in a follow-up instead? And thus wait to check the crash-type - > so that it's not OTHER - inside onSuccessfulUpload until I upload that change? SGTM
LGTM, thanks! https://codereview.chromium.org/2441623002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:66: // Ensure the upload delegate has been set before the Service starts nit: Please end the sentence with a period.
Alright, thanks for the quick reviews! Committing... :D https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:229: getCrashType(getNewNameAfterSuccessfulUpload(minidumpFileName))); On 2016/10/26 21:28:12, Maria wrote: > On 2016/10/26 12:34:50, gsennton wrote: > > On 2016/10/25 21:50:20, Maria wrote: > > > On 2016/10/25 21:16:54, Ilya Sherman wrote: > > > > On 2016/10/25 17:34:44, Maria wrote: > > > > > On 2016/10/25 13:13:53, gsennton wrote: > > > > > > Hah, in PS2 I forgot to call getNewNameAfterSuccessfulUpload here, and > > it > > > > > didn't > > > > > > show up except in the logs (if getCrashType fails with an IOException > it > > > > just > > > > > > prints to the logs). > > > > > > Even some of our tests in MinidumpUploadServiceTest trigger the > > > IOException > > > > > > inside getCrashType because we don't rename the minidump file after > > > > uploading > > > > > it > > > > > > in the tests. > > > > > > Not sure if we should make this more robust / actually throw a > run-time > > > > > > exception when catching an IOException..? (throwing a new exception > > seems > > > > > scary > > > > > > though ;)). > > > > > > > > > > A couple of things here: > > > > > 1) The method is currently badly named -- get usually implies a > > > > no-side-effects > > > > > getter rather than a mutator method. Can you rename this method to > > > > > "updateNameAfterSuccessfulUpload"? > > > > > > > > Hmm, what do you mean? That is, what is the side effect that's > > inappropriate? > > > > > > > I *think* the method just takes a string input and returns a string > output. > > > > > > Hmm, I guess you are right - for some reason I thought file name field was > > being > > > changed, but I guess it's just the param. Never mind. > > > > > > > > > 2) The tests should fail if IOException is not expected -- rather than > > > > throwing > > > > > runtime exception, how about asserting in the test that the crash type > > > > returned > > > > > is not OTHER? > > > > > > > > Agreed =) > > > > > > > > I don't think that it's appropriate to raise an exception if the file > can't > > be > > > > read, because presumably sometimes this will happen for some crash files > due > > > to > > > > corruption, and we don't want the crash reporter to throw an exception and > > > bail > > > > on whatever other files are present. > > > > > > > Hmm, so I want to check the return-value of getCrashType but currently the > > file-renaming after a successful upload happens in > > MinidumpUploadCallable.handleExecutionResponse - there is even a TODO there by > > acleung@ to move the renaming into MinidumpUploadService. > > > > Since some of the tests in MinidumpUploadServiceTest uses a custom > > MinidumpUploadCallable.call() method the renaming will never happen during > those > > tests - so getCrashType will then fail to read the new file (and return > > MinidumpUploadDELEGATE.OTHER). > > > > I would like to move the file-renaming out to MinidumpUploadService but it > seems > > wrong to include a change like that in the current CL, do you mind if I add a > > bug and do this in a follow-up instead? And thus wait to check the crash-type > - > > so that it's not OTHER - inside onSuccessfulUpload until I upload that change? > > SGTM Filed crbug.com/659938 https://codereview.chromium.org/2441623002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2441623002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:66: // Ensure the upload delegate has been set before the Service starts On 2016/10/26 22:32:54, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/2441623002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2441623002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:105: mIsLimited = false; Had to rebase because mIsLimited was replaced by mIsNetworkAvailable.
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2441623002/#ps80001 (title: "Rebase (preference/permission changes).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Split MinidumpUploadService into core- and Chrome-implementation. To componentize MinidumpUploadService we split it into its core implementation and an implementation dependent on Chrome. The Chrome-dependent parts now live in ChromeMinidumpUploadDelegate. To inject this delegate into the MinidumpUploadService we have to create a Chrome-specific version of the MinidumpUploadService (inheriting from it) named ChromeMinidumpUploadService. BUG=652719 ========== to ========== Split MinidumpUploadService into core- and Chrome-implementation. To componentize MinidumpUploadService we split it into its core implementation and an implementation dependent on Chrome. The Chrome-dependent parts now live in ChromeMinidumpUploadDelegate. To inject this delegate into the MinidumpUploadService we have to create a Chrome-specific version of the MinidumpUploadService (inheriting from it) named ChromeMinidumpUploadService. BUG=652719 Committed: https://crrev.com/271ef66ac8bab41f55f18586a1cd2fd2970d4d79 Cr-Commit-Position: refs/heads/master@{#427989} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/271ef66ac8bab41f55f18586a1cd2fd2970d4d79 Cr-Commit-Position: refs/heads/master@{#427989}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2466443002/ by gsennton@chromium.org. The reason for reverting is: Componentizing the MinidumpUploadService is a pain - and not strictly necessary - we have to set the MinidumpUploadDelegate before the service uses this delegate, but the service might be started by the system rather than by Chrome itself. See crbug.com/660075. |
