Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465303002:
[cronet] make startNetLogToFile write to correct file (Closed)
|
Created:
4 years, 1 month ago by lilyhoughton1 Modified:
3 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmake startNetLogToFile generate full path before passing to C++ function
additionally, this adds a couple lines to the C++ backend so that it branches on
whether the input file is relative or absolute, and leaves absolute paths unchanged
BUG=616918
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2465303002
Cr-Commit-Position: refs/heads/master@{#442314}
Committed: https://chromium.googlesource.com/chromium/src/+/214b3a27cc09cc76ba9b24a2eac7e41478cda770
Patch Set 1 #
Total comments: 25
Patch Set 2 : per mef's comments #Patch Set 3 : fix kapishnikov nits #
Total comments: 12
Patch Set 4 : cl format #Patch Set 5 : test to see that netlog successfully creates the file its trying to #Patch Set 6 : Sync #
Total comments: 16
Patch Set 7 : per #15 #
Total comments: 9
Patch Set 8 : remove unused includes #Patch Set 9 : sync #
Messages
Total messages: 35 (15 generated)
Description was changed from ========== remove banned function cl format Merge branch 'master' into netlogpath add more netlog tests; make startnetlog not crassh when given an empty filename add more netlog tests add (very simple) test for netlog make ios api for Cronet's netlog return error status make the C++ StartNetLog API return false if it fails to open the file also move the file-open code to StartNetLog, before StartNetLogOnNetworkThread is posted to the network thread. this changes the signature for StartNetLogOnNetworkThread to take a (Scoped)FILE object instead of a filename, and means the logging format is changed slightly as well, since StartNetLogOnNetworkThread no longer know what the path being writ to is. make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 ========== to ========== remove banned function cl format Merge branch 'master' into netlogpath add more netlog tests; make startnetlog not crassh when given an empty filename add more netlog tests add (very simple) test for netlog make ios api for Cronet's netlog return error status make the C++ StartNetLog API return false if it fails to open the file also move the file-open code to StartNetLog, before StartNetLogOnNetworkThread is posted to the network thread. this changes the signature for StartNetLogOnNetworkThread to take a (Scoped)FILE object instead of a filename, and means the logging format is changed slightly as well, since StartNetLogOnNetworkThread no longer know what the path being writ to is. make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 ==========
lilyhoughton@chromium.org changed reviewers: + kapishnikov@chromium.org, mef@chromium.org
lilyhoughton@chromium.org changed reviewers: + lilyhoughton@chromium.org
Looks good, couple of quick comments. Also, please update description. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Cronet.h File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:81: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes; nit: ObjC uses capital BOOL. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:236: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes { BOOL https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_netlog_test.mm:18: nit: no need for empty line. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_netlog_test.mm:26: StartCronetIfNecessary(); Not having to repeat boiler-plate code (StartCronetIfNecessary) seems like a good reason to have a Test class.
Here are some suggestions. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Cronet.h File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:77: // temporary directory. |fileName| must not be empty. Log level is determined The current implementation is writing the netlog to the app document directory. The document directory is not temporary. In fact, it will be backed up by iTunes/Cloud. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:81: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes; If the method can fail, should we add NSError** as an out argument to the method? See https://developer.apple.com/library/prerelease/content/documentation/Cocoa/Co... https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:237: if (gChromeNet.Get().get() && [fileName length]) { I think it would be better if we did the validation first and return 'NO' if the fileName is invalid. Thus, we would have less code inside the 'if' statement. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:245: return false; Use 'NO' instead of 'false' since it is Objective C. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:157: if (!PathService::Get(base::DIR_HOME, &full_path)) What is the value of base::DIR_HOME on iOS? Can application write into it? Do we expect that some other code rather than Cronet.cc will be calling this method? https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:173: base::Unretained(this), base::Passed(&file), log_bytes)); Why do we need base::Passes here? https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:194: LOG(WARNING) << "Started NetLog"; We already logging a message in StartNetLog(). I think we should remove this message or reduce the level to debug/verbose.
Description was changed from ========== remove banned function cl format Merge branch 'master' into netlogpath add more netlog tests; make startnetlog not crassh when given an empty filename add more netlog tests add (very simple) test for netlog make ios api for Cronet's netlog return error status make the C++ StartNetLog API return false if it fails to open the file also move the file-open code to StartNetLog, before StartNetLogOnNetworkThread is posted to the network thread. this changes the signature for StartNetLogOnNetworkThread to take a (Scoped)FILE object instead of a filename, and means the logging format is changed slightly as well, since StartNetLogOnNetworkThread no longer know what the path being writ to is. make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 ========== to ========== make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 ==========
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Cronet.h File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:77: // temporary directory. |fileName| must not be empty. Log level is determined On 2016/11/10 20:52:17, kapishnikov wrote: > The current implementation is writing the netlog to the app document directory. > The document directory is not temporary. In fact, it will be backed up by > iTunes/Cloud. Done. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:81: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes; On 2016/11/10 17:09:48, mef wrote: > nit: ObjC uses capital BOOL. Done. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:81: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes; On 2016/11/10 20:52:17, kapishnikov wrote: > If the method can fail, should we add NSError** as an out argument to the > method? See > https://developer.apple.com/library/prerelease/content/documentation/Cocoa/Co... > it can only fail in one way (unable to open file), so having an out-argument seemed excessive, but I can add an optional one. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:236: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes { On 2016/11/10 17:09:48, mef wrote: > BOOL Done. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:237: if (gChromeNet.Get().get() && [fileName length]) { On 2016/11/10 20:52:17, kapishnikov wrote: > I think it would be better if we did the validation first and return 'NO' if the > fileName is invalid. Thus, we would have less code inside the 'if' statement. I wanted to keep the obj-c wrapper as thin as possible, and keep as much code as possible in the cpp api. (Right now though it does check filelength twice, and I think that should change) https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:245: return false; On 2016/11/10 20:52:17, kapishnikov wrote: > Use 'NO' instead of 'false' since it is Objective C. Done. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:157: if (!PathService::Get(base::DIR_HOME, &full_path)) On 2016/11/10 20:52:17, kapishnikov wrote: > What is the value of base::DIR_HOME on iOS? Can application write into it? Do we > expect that some other code rather than Cronet.cc will be calling this method? iOS apps definitely can't write to it, per bug 616918; I left the if-clause here so as to not break other functions that might want the old behavior, but if that isn't used anywhere, then this should definitely be removed. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:173: base::Unretained(this), base::Passed(&file), log_bytes)); On 2016/11/10 20:52:17, kapishnikov wrote: > Why do we need base::Passes here? Because `base::Bind` will not accept an undecorated `std::unique_ptr` (which is what a ScopedFILE is) https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:194: LOG(WARNING) << "Started NetLog"; On 2016/11/10 20:52:17, kapishnikov wrote: > We already logging a message in StartNetLog(). I think we should remove this > message or reduce the level to debug/verbose. I wanted to include both whether or not `startNetLog()` succeeded and what file it is writing too. I think ideally the format is Starting NetLog to <file>...OK or Starting NetLog to <file>...FAIL but I don't know how to make LOG() continue on the same line. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_netlog_test.mm:18: On 2016/11/10 17:09:48, mef wrote: > nit: no need for empty line. Done. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_netlog_test.mm:26: StartCronetIfNecessary(); On 2016/11/10 17:09:49, mef wrote: > Not having to repeat boiler-plate code (StartCronetIfNecessary) seems like a > good reason to have a Test class. Done.
https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.h (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:114: void StartNetLogOnNetworkThread(base::ScopedFILE file, bool log_bytes); Add #include "base/files/scoped_file.h". https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... components/cronet/ios/test/cronet_netlog_test.mm:8: #include "base/logging.h" IWYU - remove unused includes.
Here are some extra comments. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Cronet.h File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.h:81: + (bool)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes; On 2016/11/16 16:04:09, lilyhoughton wrote: > On 2016/11/10 20:52:17, kapishnikov wrote: > > If the method can fail, should we add NSError** as an out argument to the > > method? See > > > https://developer.apple.com/library/prerelease/content/documentation/Cocoa/Co... > > > > it can only fail in one way (unable to open file), so having an out-argument > seemed excessive, but I can add an optional one. It can actually fail for multiple reasons, e.g. the user passed NULL, an invalid value, the directory doesn't exist, there is no permission to write to the file, i.e. everywhere the method return NO/false. Since it is a debug API, I am okay to keep the method signature as is but I think there could be some benefit to return an error message that describes what went wrong. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/Crone... components/cronet/ios/Cronet.mm:237: if (gChromeNet.Get().get() && [fileName length]) { On 2016/11/16 16:04:10, lilyhoughton wrote: > On 2016/11/10 20:52:17, kapishnikov wrote: > > I think it would be better if we did the validation first and return 'NO' if > the > > fileName is invalid. Thus, we would have less code inside the 'if' statement. > > I wanted to keep the obj-c wrapper as thin as possible, and keep as much code as > possible in the cpp api. > > (Right now though it does check filelength twice, and I think that should > change) Acknowledged. https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2465303002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:194: LOG(WARNING) << "Started NetLog"; On 2016/11/16 16:04:10, lilyhoughton wrote: > On 2016/11/10 20:52:17, kapishnikov wrote: > > We already logging a message in StartNetLog(). I think we should remove this > > message or reduce the level to debug/verbose. > > I wanted to include both whether or not `startNetLog()` succeeded and what file > it is writing too. I think ideally the format is > > Starting NetLog to <file>...OK > > or > > Starting NetLog to <file>...FAIL > > but I don't know how to make LOG() continue on the same line. If StartNetLogOnNetworkThread is called, this line is printed unconditionally (unless net_log_observer_ is NULL). I don't see value in this LOG since we print the result in StartNetLog. I agree that we need to log if there is an unexpected failure. https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.h:77: // relative, it puts it in the application home directory. |fileName| must not Usually iOS documentation refers to 'documents' and 'temp' directory. These are the directories where app has permissions to write to. I would assume that the 'home' directory is the root directory of the app where the app has no permission. I think we should reference the documents directory here. https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.h:79: // otherwise LOG_ALL_BUT_BYTES. If the file exists it is truncated before It is not clear what LOG_ALL_BUT_BYTES mean here. Is it a constant defined somewhere else? https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.mm:241: lastObject] URLByAppendingPathComponent:fileName]; The documentation of the method implies that absolute path is supported. What will be the value of |file| if the absolute path is passed? https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... components/cronet/ios/test/cronet_netlog_test.mm:37: EXPECT_TRUE(netlog_started); Can we check that the file was created? The file name should be unique in that case.
fixes the smaller issues, still working on the more complex ones https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.h:77: // relative, it puts it in the application home directory. |fileName| must not On 2016/11/18 22:17:27, kapishnikov wrote: > Usually iOS documentation refers to 'documents' and 'temp' directory. These are > the directories where app has permissions to write to. I would assume that the > 'home' directory is the root directory of the app where the app has no > permission. I think we should reference the documents directory here. Done. https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.h:79: // otherwise LOG_ALL_BUT_BYTES. If the file exists it is truncated before On 2016/11/18 22:17:27, kapishnikov wrote: > It is not clear what LOG_ALL_BUT_BYTES mean here. Is it a constant defined > somewhere else? It doesn't appear to refer to a constant used by the NetLog functions, and I'm not familiar enough with the code that logBytes affects yet to know what it's referring to. https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/C... components/cronet/ios/Cronet.mm:241: lastObject] URLByAppendingPathComponent:fileName]; On 2016/11/18 22:17:27, kapishnikov wrote: > The documentation of the method implies that absolute path is supported. What > will be the value of |file| if the absolute path is passed? This is a documentation error. (Absolute path is supported for the internal cpp function, but not the Obj-C wrapper). https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.h (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:114: void StartNetLogOnNetworkThread(base::ScopedFILE file, bool log_bytes); On 2016/11/17 15:58:48, mef wrote: > Add #include "base/files/scoped_file.h". Done. https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... components/cronet/ios/test/cronet_netlog_test.mm:8: #include "base/logging.h" On 2016/11/17 15:58:48, mef wrote: > IWYU - remove unused includes. Done.
https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/60001/components/cronet/ios/t... components/cronet/ios/test/cronet_netlog_test.mm:37: EXPECT_TRUE(netlog_started); On 2016/11/18 22:17:27, kapishnikov wrote: > Can we check that the file was created? The file name should be unique in that > case. Done.
Description was changed from ========== make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 ========== to ========== make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/Cronet.h:80: + (NSString*)netLogPath:(NSString*)fileName; maybe call it 'getNetLogPathForFile:' https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/Cronet.mm:233: + (NSString*)netLogPath:(NSString*)fileName { So, fileName can never be an absolute path, right? https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/cronet_environment.mm:154: if (!full_path.IsAbsolute()) { This seems redundant now that we get absolute path in Cronet.mm. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/cronet_environment.mm:192: nit: spurious nl https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:33: [Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES]; this may behave differently during first and consecutive runs. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:49: EXPECT_TRUE(file_created); Delete that [Cronet netLogPath:filename] at the end? https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:52: TEST(NetLogTest, NonExistantDir) { Add test for creating netlog in existing directory? https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:60: TEST(NetLogTest, EmptyFilename) { Add test for passing absolute path name.
https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/Cronet.h (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/Cronet.h:80: + (NSString*)netLogPath:(NSString*)fileName; On 2016/12/09 18:30:40, mef wrote: > maybe call it 'getNetLogPathForFile:' Done. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/Cronet.mm:233: + (NSString*)netLogPath:(NSString*)fileName { On 2016/12/09 18:30:40, mef wrote: > So, fileName can never be an absolute path, right? Yeah https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/cronet_environment.mm:154: if (!full_path.IsAbsolute()) { On 2016/12/09 18:30:41, mef wrote: > This seems redundant now that we get absolute path in Cronet.mm. The only reason I left it was because I wasn't sure where else StartNetLog() was used. If it's reasonable to expect that it will only be passed an absolute path, then this whole block can go. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/cronet_environment.mm:192: On 2016/12/09 18:30:40, mef wrote: > nit: spurious nl Done. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:33: [Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES]; On 2016/12/09 18:30:41, mef wrote: > this may behave differently during first and consecutive runs. Not sure how to account for that besides just having a second identical test? https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:49: EXPECT_TRUE(file_created); On 2016/12/09 18:30:41, mef wrote: > Delete that [Cronet netLogPath:filename] at the end? Done. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:52: TEST(NetLogTest, NonExistantDir) { On 2016/12/09 18:30:41, mef wrote: > Add test for creating netlog in existing directory? Done. https://codereview.chromium.org/2465303002/diff/120001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:60: TEST(NetLogTest, EmptyFilename) { On 2016/12/09 18:30:41, mef wrote: > Add test for passing absolute path name. Added a check to startNetLogToFile just to make sure, too.
lgtm mod nits. https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... File components/cronet/ios/test/cronet_netlog_test.mm (right): https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:8: #include "base/logging.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:9: #include "base/mac/scoped_nsobject.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:10: #include "base/strings/sys_string_conversions.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:11: #include "components/cronet/ios/test/test_server.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:12: #include "net/base/mac/url_conversions.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:13: #include "net/base/net_errors.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:14: #include "net/cert/mock_cert_verifier.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:16: #include "testing/gtest_mac.h" nit: unused https://codereview.chromium.org/2465303002/diff/140001/components/cronet/ios/... components/cronet/ios/test/cronet_netlog_test.mm:17: #include "url/gurl.h" nit: unused
The CQ bit was checked by lilyhoughton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org Link to the patchset: https://codereview.chromium.org/2465303002/#ps160001 (title: "remove unused includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/cronet/ios/cronet_environment.h:
While running git apply --index -p1;
error: patch failed: components/cronet/ios/cronet_environment.h:10
error: components/cronet/ios/cronet_environment.h: patch does not apply
Patch: components/cronet/ios/cronet_environment.h
Index: components/cronet/ios/cronet_environment.h
diff --git a/components/cronet/ios/cronet_environment.h
b/components/cronet/ios/cronet_environment.h
index
f6446c7ae3ec965c136c5ae6f8d0e442b2e44e82..8f42c0a933bbc41915f72da9ad5f2a2b49e9a2c8
100644
--- a/components/cronet/ios/cronet_environment.h
+++ b/components/cronet/ios/cronet_environment.h
@@ -10,6 +10,7 @@
#include <vector>
#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
#include "base/macros.h"
#include "base/message_loop/message_loop.h"
#include "base/synchronization/waitable_event.h"
@@ -55,7 +56,7 @@ class CronetEnvironment {
// Creates a new net log (overwrites existing file with this name). If
// actively logging, this call is ignored.
- void StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
+ bool StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
// Stops logging and flushes file. If not currently logging this call is
// ignored.
void StopNetLog();
@@ -105,8 +106,7 @@ class CronetEnvironment {
const base::Closure& task);
// Helper methods that start/stop net logging on the network thread.
- void StartNetLogOnNetworkThread(const base::FilePath::StringType& file_name,
- bool log_bytes);
+ void StartNetLogOnNetworkThread(base::ScopedFILE file, bool log_bytes);
void StopNetLogOnNetworkThread(base::WaitableEvent* log_stopped_event);
// Returns the HttpNetworkSession object from the passed in
On 2016/12/20 23:31:02, commit-bot: I haz the power wrote:
> Failed to apply patch for components/cronet/ios/cronet_environment.h:
> While running git apply --index -p1;
> error: patch failed: components/cronet/ios/cronet_environment.h:10
> error: components/cronet/ios/cronet_environment.h: patch does not apply
>
> Patch: components/cronet/ios/cronet_environment.h
> Index: components/cronet/ios/cronet_environment.h
> diff --git a/components/cronet/ios/cronet_environment.h
> b/components/cronet/ios/cronet_environment.h
> index
>
f6446c7ae3ec965c136c5ae6f8d0e442b2e44e82..8f42c0a933bbc41915f72da9ad5f2a2b49e9a2c8
> 100644
> --- a/components/cronet/ios/cronet_environment.h
> +++ b/components/cronet/ios/cronet_environment.h
> @@ -10,6 +10,7 @@
> #include <vector>
>
> #include "base/files/file_path.h"
> +#include "base/files/scoped_file.h"
> #include "base/macros.h"
> #include "base/message_loop/message_loop.h"
> #include "base/synchronization/waitable_event.h"
> @@ -55,7 +56,7 @@ class CronetEnvironment {
>
> // Creates a new net log (overwrites existing file with this name). If
> // actively logging, this call is ignored.
> - void StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
> + bool StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
> // Stops logging and flushes file. If not currently logging this call is
> // ignored.
> void StopNetLog();
> @@ -105,8 +106,7 @@ class CronetEnvironment {
> const base::Closure& task);
>
> // Helper methods that start/stop net logging on the network thread.
> - void StartNetLogOnNetworkThread(const base::FilePath::StringType&
file_name,
> - bool log_bytes);
> + void StartNetLogOnNetworkThread(base::ScopedFILE file, bool log_bytes);
> void StopNetLogOnNetworkThread(base::WaitableEvent* log_stopped_event);
>
> // Returns the HttpNetworkSession object from the passed in
I think you need to re-sync this CL and submit again.
lgtm
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/cronet/ios/cronet_environment.h:
While running git apply --index -p1;
error: patch failed: components/cronet/ios/cronet_environment.h:10
error: components/cronet/ios/cronet_environment.h: patch does not apply
Patch: components/cronet/ios/cronet_environment.h
Index: components/cronet/ios/cronet_environment.h
diff --git a/components/cronet/ios/cronet_environment.h
b/components/cronet/ios/cronet_environment.h
index
f6446c7ae3ec965c136c5ae6f8d0e442b2e44e82..8f42c0a933bbc41915f72da9ad5f2a2b49e9a2c8
100644
--- a/components/cronet/ios/cronet_environment.h
+++ b/components/cronet/ios/cronet_environment.h
@@ -10,6 +10,7 @@
#include <vector>
#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
#include "base/macros.h"
#include "base/message_loop/message_loop.h"
#include "base/synchronization/waitable_event.h"
@@ -55,7 +56,7 @@ class CronetEnvironment {
// Creates a new net log (overwrites existing file with this name). If
// actively logging, this call is ignored.
- void StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
+ bool StartNetLog(base::FilePath::StringType file_name, bool log_bytes);
// Stops logging and flushes file. If not currently logging this call is
// ignored.
void StopNetLog();
@@ -105,8 +106,7 @@ class CronetEnvironment {
const base::Closure& task);
// Helper methods that start/stop net logging on the network thread.
- void StartNetLogOnNetworkThread(const base::FilePath::StringType& file_name,
- bool log_bytes);
+ void StartNetLogOnNetworkThread(base::ScopedFILE file, bool log_bytes);
void StopNetLogOnNetworkThread(base::WaitableEvent* log_stopped_event);
// Returns the HttpNetworkSession object from the passed in
The CQ bit was checked by lilyhoughton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org Link to the patchset: https://codereview.chromium.org/2465303002/#ps180001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483985732654430,
"parent_rev": "57973ce2707ee1cbfb3354ab0f3764e55f34258d", "commit_rev":
"214b3a27cc09cc76ba9b24a2eac7e41478cda770"}
Message was sent while issue was closed.
Description was changed from ========== make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== make startNetLogToFile generate full path before passing to C++ function additionally, this adds a couple lines to the C++ backend so that it branches on whether the input file is relative or absolute, and leaves absolute paths unchanged BUG=616918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2465303002 Cr-Commit-Position: refs/heads/master@{#442314} Committed: https://chromium.googlesource.com/chromium/src/+/214b3a27cc09cc76ba9b24a2eac7... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/214b3a27cc09cc76ba9b24a2eac7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
