Issue 2764002: Adding possibility for client to upload the file (Closed)
|
Created:
10 years, 3 months ago by olivierrobin Modified:
10 years, 2 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdding possibility for client to upload the file This CL adds three features that will allow the client to upload the report file. Three main modifications are made : - Allow upload url to have a file:// scheme, and write the HTTP request to file in that case - Split the request in two parts in case of a file:// scheme, the request time and the response time. A new API [handleNetworkResponse] is added. - Give the opportunity to the client to get the configuration NSDictionary to be able to recreate the breakpad context at response time. Patch Set 1 #
Total comments: 17
Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #
MessagesTotal messages: 37
Hi, This is the Breakpad part of the "background fetch" report uploading. The goal is to let all the behaviour unchanged, except if the upload URL scheme is file://. link to the discussion document : https://docs.google.com/a/google.com/document/d/1SY-wZ4BSXHPBVXd3w8KSGIWGDxpR... Olivier Robin
Sign in to reply to this message.
A link to the discussion page : https://sites.google.com/a/chromium.org/dev/developers/crash-reports/handle-f...
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.h File src/client/ios/Breakpad.h (right): https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.h#newcode203 src/client/ios/Breakpad.h:203: NSDictionary* BreakpadGetNextReportConfiguration(BreakpadRef ref); Style, for consistency the * should be after the space, (here and in the other files) https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.mm#newcod... src/client/ios/Breakpad.mm:481: if (uploader) This test can be before the for loop. https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.h#... src/client/mac/sender/uploader.h:48: NSDictionary* readConfigurationData(const char *configFile); Is there any reason for this to be a C++ namespaced method. This is not really the style of this files. Why not a class method on Uploader? https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm... src/client/mac/sender/uploader.mm:559: [self handleResponse:data withError:error]; Is this handleNetworkResponse? Or am I missing something? https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm... src/client/mac/sender/uploader.mm:569: if (logFileData_) { Do you care that the case without a minidump doesn't follow the same pattern? https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUpload.m File src/common/mac/HTTPMultipartUpload.m (right): https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:148: cachePolicy:NSURLRequestUseProtocolCachePolicy Why is there a change here? https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:154: [NSString stringWithFormat:@"multipart/form-data; boundary=%@", boundary_] And here? https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:191: if (response_) { Why do you need this if?
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.h File src/client/ios/Breakpad.h (right): https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.h#newcode203 src/client/ios/Breakpad.h:203: NSDictionary* BreakpadGetNextReportConfiguration(BreakpadRef ref); On 2014/08/25 16:07:31, qsr wrote: > Style, for consistency the * should be after the space, (here and in the other > files) Done. https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/2764002/diff/1/src/client/ios/Breakpad.mm#newcod... src/client/ios/Breakpad.mm:481: if (uploader) On 2014/08/25 16:07:31, qsr wrote: > This test can be before the for loop. Done. https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.h#... src/client/mac/sender/uploader.h:48: NSDictionary* readConfigurationData(const char *configFile); On 2014/08/25 16:07:31, qsr wrote: > Is there any reason for this to be a C++ namespaced method. This is not really > the style of this files. Why not a class method on Uploader? Done. https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm... src/client/mac/sender/uploader.mm:559: [self handleResponse:data withError:error]; On 2014/08/25 16:07:31, qsr wrote: > Is this handleNetworkResponse? Or am I missing something? Done. https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm... src/client/mac/sender/uploader.mm:569: if (logFileData_) { On 2014/08/25 16:07:31, qsr wrote: > Do you care that the case without a minidump doesn't follow the same pattern? This case sends logFileData using HTTPMultipartUpload. So it will write the content to file, and ignore the result, which was the previous behavior. At response time, it is a bit trickier. If the client calls handleNetworkResponse, this was not done before. The difference is that: - If there was a minidump, but the read was unsuccessful, it will be renamed, which I think is fine - I there was no minidump, we will get an extra log line that was not here on the previous behavior. If you think I should add a guard for that, it would mean that the information must be stored that the minidump content was not uploaded. https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUpload.m File src/common/mac/HTTPMultipartUpload.m (right): https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:148: cachePolicy:NSURLRequestUseProtocolCachePolicy On 2014/08/25 16:07:31, qsr wrote: > Why is there a change here? Because I changed the function and there were 2 arguments on the same line, so I thought it was against guideline. Restored. https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:154: [NSString stringWithFormat:@"multipart/form-data; boundary=%@", boundary_] On 2014/08/25 16:07:31, qsr wrote: > And here? Done. https://breakpad.appspot.com/2764002/diff/1/src/common/mac/HTTPMultipartUploa... src/common/mac/HTTPMultipartUpload.m:191: if (response_) { On 2014/08/25 16:07:31, qsr wrote: > Why do you need this if? response_ can be nil in some scenario, but this is an extra guard. Done.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.h:127: - (void)threadUnsafeSendReportWithConfiguration:(NSDictionary*)configuration Missed this one... Correcting
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/1/src/client/mac/sender/uploader.mm... src/client/mac/sender/uploader.mm:569: if (logFileData_) { On 2014/08/26 07:58:51, olivierrobin wrote: > On 2014/08/25 16:07:31, qsr wrote: > > Do you care that the case without a minidump doesn't follow the same pattern? > > This case sends logFileData using HTTPMultipartUpload. So it will write the > content to file, and ignore the result, which was the previous behavior. > > At response time, it is a bit trickier. If the client calls > handleNetworkResponse, this was not done before. > The difference is that: > - If there was a minidump, but the read was unsuccessful, it will be renamed, > which I think is fine > - I there was no minidump, we will get an extra log line that was not here on > the previous behavior. > > If you think I should add a guard for that, it would mean that the information > must be stored that the minidump content was not uploaded. No it is fine, given that this is the same behavior as before. https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/Breakpad.mm#ne... src/client/ios/Breakpad.mm:826: Breakpad *breakpad = (Breakpad *)ref; Any reason not to check for breakpad not being null here? It is done everywhere else. https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:72: + (NSDictionary *)readConfigurationDataFromFile:(NSString*)configFile; space before * here and below, and in the implementation file. https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:520: const char *src = 0;//[srcString fileSystemRepresentation]; ??? https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:533: Kill this blank line.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/ios/Breakpad.mm#ne... src/client/ios/Breakpad.mm:826: Breakpad *breakpad = (Breakpad *)ref; On 2014/08/26 08:09:37, qsr wrote: > Any reason not to check for breakpad not being null here? It is done everywhere > else. Done. https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:72: + (NSDictionary *)readConfigurationDataFromFile:(NSString*)configFile; On 2014/08/26 08:09:37, qsr wrote: > space before * here and below, and in the implementation file. Done. https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:520: const char *src = 0;//[srcString fileSystemRepresentation]; On 2014/08/26 08:09:37, qsr wrote: > ??? Done. https://breakpad.appspot.com/2764002/diff/40003/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:533: On 2014/08/26 08:09:37, qsr wrote: > Kill this blank line. Done.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.h:125: // Sends synchromously the report specified by |configuration|. This method is s/synchromously/synchronously/ https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:71: // |configFile| is deleted. s/is/will be/ Do you mean s/deleted/deleted after reading/ ? https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:85: // This method process the HTTP response and s/process/processes/ "and" something else? https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:86: - (void)handleNetworkResponse:(NSData *)data withError:(NSError *)error; delete extra space before "withError:" Same for implementation of this method.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.h:125: // Sends synchromously the report specified by |configuration|. This method is synchronously https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.mm:158: // This method must be called from the breakpad queue. Is there no way to actually check this in the code? https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:85: // This method process the HTTP response and ? https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:565: fprintf(stderr, "Breakpad Uploader: Error writing request file: %s\n", hmm, does this information get propagated back to the client? If it doesn't, should it?
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.h:125: // Sends synchromously the report specified by |configuration|. This method is On 2014/08/26 09:25:54, pkl wrote: > s/synchromously/synchronously/ Done. https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.h:125: // Sends synchromously the report specified by |configuration|. This method is On 2014/08/26 09:30:03, blundell wrote: > synchronously Done. https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... File src/client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/ios/BreakpadContro... src/client/ios/BreakpadController.mm:158: // This method must be called from the breakpad queue. On 2014/08/26 09:30:03, blundell wrote: > Is there no way to actually check this in the code? I discussed that issue with qsr. dispatch_get_current_queue is deprecated, so it cannot be used. qsr suggested to have the BreakpadRef in the arguments to force the function to be called from [BreakpadController withBreakpadRef:], the BreakpadRef being only accessible from there. https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploader.h File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:71: // |configFile| is deleted. On 2014/08/26 09:25:54, pkl wrote: > s/is/will be/ > Do you mean s/deleted/deleted after reading/ ? Done. https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:85: // This method process the HTTP response and On 2014/08/26 09:25:54, pkl wrote: > s/process/processes/ > "and" something else? Done. https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.h:86: - (void)handleNetworkResponse:(NSData *)data withError:(NSError *)error; On 2014/08/26 09:25:54, pkl wrote: > delete extra space before "withError:" > Same for implementation of this method. Done. https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:565: fprintf(stderr, "Breakpad Uploader: Error writing request file: %s\n", On 2014/08/26 09:30:03, blundell wrote: > hmm, does this information get propagated back to the client? If it doesn't, > should it? At the moment, the client has to check if the file exists before uploading it. Propagating the result would require to change the API (at least return type).
Sign in to reply to this message.
lgtm https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/90002/src/client/mac/sender/uploade... src/client/mac/sender/uploader.mm:565: fprintf(stderr, "Breakpad Uploader: Error writing request file: %s\n", On 2014/08/26 09:39:39, olivierrobin wrote: > On 2014/08/26 09:30:03, blundell wrote: > > hmm, does this information get propagated back to the client? If it doesn't, > > should it? > > At the moment, the client has to check if the file exists before uploading it. > Propagating the result would require to change the API (at least return type). > Since the error information was not previously getting reported back to the client, I guess this is OK.
Sign in to reply to this message.
lgtm with comment. https://breakpad.appspot.com/2764002/diff/230001/src/client/mac/sender/upload... File src/client/mac/sender/uploader.h (right): https://breakpad.appspot.com/2764002/diff/230001/src/client/mac/sender/upload... src/client/mac/sender/uploader.h:85: // This method process the HTTP response and renames the minidump file with the s/process/processes/
Sign in to reply to this message.
LGTM, but please wait for a high level review of mark before pushing.
Sign in to reply to this message.
On 2014/08/26 13:14:48, qsr wrote: > LGTM, but please wait for a high level review of mark before pushing. Sure, I will. Thank you
Sign in to reply to this message.
Had to expose the send delay method to limit the upload rate.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... src/client/ios/BreakpadController.h:134: - (int)threadUnsafeSendDelayWithBreakpadRef:(BreakpadRef)ref; I really do not link adding more unsafe methods :( Can we find a way to combine both methods.... Maybe return an error if you are not supposed to send a report.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... File src/client/ios/BreakpadController.h (right): https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... src/client/ios/BreakpadController.h:134: - (int)threadUnsafeSendDelayWithBreakpadRef:(BreakpadRef)ref; On 2014/08/28 08:58:08, qsr wrote: > I really do not link adding more unsafe methods :( > > Can we find a way to combine both methods.... Maybe return an error if you are > not supposed to send a report. Not easy to do as the other API needs the configuration dictionary and getting the dictionary deletes the config file. So we need to test this before. Would you prefer a solution like the getCrashReportCount API, with a callback ?
Sign in to reply to this message.
On 2014/08/28 09:05:44, olivierrobin wrote: > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > File src/client/ios/BreakpadController.h (right): > > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > src/client/ios/BreakpadController.h:134: - > (int)threadUnsafeSendDelayWithBreakpadRef:(BreakpadRef)ref; > On 2014/08/28 08:58:08, qsr wrote: > > I really do not link adding more unsafe methods :( > > > > Can we find a way to combine both methods.... Maybe return an error if you are > > not supposed to send a report. > > Not easy to do as the other API needs the configuration dictionary and getting > the dictionary deletes the config file. So we need to test this before. > > Would you prefer a solution like the getCrashReportCount API, with a callback ? Yes, I would prefer this. But would that work with your use case?
Sign in to reply to this message.
On 2014/08/28 09:08:26, qsr wrote: > On 2014/08/28 09:05:44, olivierrobin wrote: > > > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > > File src/client/ios/BreakpadController.h (right): > > > > > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > > src/client/ios/BreakpadController.h:134: - > > (int)threadUnsafeSendDelayWithBreakpadRef:(BreakpadRef)ref; > > On 2014/08/28 08:58:08, qsr wrote: > > > I really do not link adding more unsafe methods :( > > > > > > Can we find a way to combine both methods.... Maybe return an error if you > are > > > not supposed to send a report. > > > > Not easy to do as the other API needs the configuration dictionary and getting > > the dictionary deletes the config file. So we need to test this before. > > > > Would you prefer a solution like the getCrashReportCount API, with a callback > ? > > Yes, I would prefer this. But would that work with your use case? As there will be no assurance that another thread did not dispatch another task between my two dispatch async, it will not work (except if there is a way to keep a queue lock). Other options can be : - Consider that for breakpad, getting the dictionary is the actual moment of the sending and add a getNextReport with a callback. This method will -call [BreakpadController sendDelay] and give a null report if delay != 0 - call the [BreakpadController reportWillBeSent] method if there is a report. - return a report if any available This will not ensure that the reports are sent in order, but this will ensure we do not overload the server. - Use a NSInvocation to call synchronously [sendDelay]
Sign in to reply to this message.
On 2014/08/28 09:50:54, olivierrobin wrote: > On 2014/08/28 09:08:26, qsr wrote: > > On 2014/08/28 09:05:44, olivierrobin wrote: > > > > > > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > > > File src/client/ios/BreakpadController.h (right): > > > > > > > > > https://breakpad.appspot.com/2764002/diff/280001/src/client/ios/BreakpadContr... > > > src/client/ios/BreakpadController.h:134: - > > > (int)threadUnsafeSendDelayWithBreakpadRef:(BreakpadRef)ref; > > > On 2014/08/28 08:58:08, qsr wrote: > > > > I really do not link adding more unsafe methods :( > > > > > > > > Can we find a way to combine both methods.... Maybe return an error if you > > are > > > > not supposed to send a report. > > > > > > Not easy to do as the other API needs the configuration dictionary and > getting > > > the dictionary deletes the config file. So we need to test this before. > > > > > > Would you prefer a solution like the getCrashReportCount API, with a > callback > > ? > > > > Yes, I would prefer this. But would that work with your use case? > > As there will be no assurance that another thread did not dispatch another task > between my two dispatch async, it will not work (except if there is a way to > keep a queue lock). > > Other options can be : > > - Consider that for breakpad, getting the dictionary is the actual moment of the > sending and add a getNextReport with a callback. This method will > -call [BreakpadController sendDelay] and give a null report if delay != 0 > - call the [BreakpadController reportWillBeSent] method if there is a report. > - return a report if any available > This will not ensure that the reports are sent in order, but this will ensure we > do not overload the server. This seems fine to me. Why wouldn't the report be sent in order? > > - Use a NSInvocation to call synchronously [sendDelay]
Sign in to reply to this message.
[getReportConfiguration] and [sendReport] will be two tasks, and it is possible, is sendDelay is 0, to send another report in the time between the two tasks.
Sign in to reply to this message.
On 2014/08/28 12:32:56, olivierrobin wrote: Not sure to understand, you mean if breakpad is configured to not have a delay between sends? I do not really care about this case, as this is a test setup.
Sign in to reply to this message.
On 2014/08/28 14:27:00, qsr wrote: > On 2014/08/28 12:32:56, olivierrobin wrote: > > Not sure to understand, you mean if breakpad is configured to not have a delay > between sends? I do not really care about this case, as this is a test setup. yes, no delay, or extremly short one (1 ms ?)
Sign in to reply to this message.
Hi Mark, Could you take a look and validate the new API and behavior introduced by this CL ? Thank you Olviier
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:1: // Copyright (c) 2011, Google Inc. The changes in this file are the only ones that the CL description seems to apply to. The overall change doesn’t seem to be about file: URLs, it seems to be about changing where the upload happens, and only part of that is making this code deal with file: URLs. https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:178: [NSString stringWithUTF8String:configFile]]; What’s the point of turning this into an NSString when the method really just wants a const char*? https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:210: return ::readConfigurationData([configFile UTF8String]); We don’t use the :: qualification unless it’s needed to resolve an ambiguity.
Sign in to reply to this message.
https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:1: // Copyright (c) 2011, Google Inc. On 2014/08/29 13:57:47, Mark Mentovai wrote: > The changes in this file are the only ones that the CL description seems to > apply to. The overall change doesn’t seem to be about file: URLs, it seems to be > about changing where the upload happens, and only part of that is making this > code deal with file: URLs. Done. https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:178: [NSString stringWithUTF8String:configFile]]; On 2014/08/29 13:57:47, Mark Mentovai wrote: > What’s the point of turning this into an NSString when the method really just > wants a const char*? Done. https://breakpad.appspot.com/2764002/diff/410001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:210: return ::readConfigurationData([configFile UTF8String]); On 2014/08/29 13:57:47, Mark Mentovai wrote: > We don’t use the :: qualification unless it’s needed to resolve an ambiguity. Done.
Sign in to reply to this message.
LG otherwise https://breakpad.appspot.com/2764002/diff/470001/src/client/mac/sender/upload... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/470001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:209: return readConfigurationData([configFile UTF8String]); Don’t use -UTF8String for coming up with const char* paths from NSStrings, use -fileSystemEncoding.
Sign in to reply to this message.
Done Thank you https://breakpad.appspot.com/2764002/diff/470001/src/client/mac/sender/upload... File src/client/mac/sender/uploader.mm (right): https://breakpad.appspot.com/2764002/diff/470001/src/client/mac/sender/upload... src/client/mac/sender/uploader.mm:209: return readConfigurationData([configFile UTF8String]); On 2014/08/29 14:45:13, Mark Mentovai wrote: > Don’t use -UTF8String for coming up with const char* paths from NSStrings, use > -fileSystemEncoding. I put fileSystemRepresentation. Done Done.
Sign in to reply to this message.
Thank you. Can someone push it for me ? Thank you
Sign in to reply to this message.
Landed as r1368. You'll have to close this issue manually.
Sign in to reply to this message.
On 2014/09/01 11:03:42, blundell wrote: > Landed as r1368. You'll have to close this issue manually. Thank you
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/09/01 11:06:30, olivierrobin wrote: > On 2014/09/01 11:03:42, blundell wrote: > > Landed as r1368. You'll have to close this issue manually. > > Thank you This change seems to be causing the following error in our clients tests: third_party/breakpad/src/common/mac/HTTPMultipartUpload.m:195:7: error: variable 'data' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] Would you, please, be able to address and fix it asap? Thanks, Maria
Sign in to reply to this message.
Message was sent while issue was closed.
Fix submitted in issue 2794002.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
