Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529603004:
[Offline Pages] Expanding event logger to be used by harness. (Closed)
|
Created:
4 years ago by romax Modified:
4 years ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Expanding event logger to be used by harness.
Added some functionality in event logger to write log into files during
test harness run.
BUG=661411
Committed: https://crrev.com/d7ce8a5c3be5ebe48eac93746c617f0ea9d45cd0
Cr-Commit-Position: refs/heads/master@{#436712}
Patch Set 1 #
Total comments: 19
Patch Set 2 : comments. #
Total comments: 41
Patch Set 3 : comments. #
Total comments: 14
Patch Set 4 : Addressed comments by fgorski. #Patch Set 5 : fix build. #Patch Set 6 : Adding tests and fix build issue. #
Total comments: 6
Patch Set 7 : more comments. #Messages
Total messages: 46 (23 generated)
romax@chromium.org changed reviewers: + chili@chromium.org, dougarnett@chromium.org, petewil@google.com
PTAL, this might not be the final design since in this case all the classes in c/b/offlinepages would not have easy access to the logger. Please share your better ideas! Thanks.
https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { So... if I understand it correctly, CustomLog and the Client allows a way for classes to put the logged events elsewhere? If the only purpose is to write to a file, I don't see why we can't have an open file handle passed in?
petewil@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:184: new SimpleDateFormat("MM-dd HH:mm:ss.SSS", Locale.getDefault()); Did we think about whether it makes sense to include the year, too? We could be using this tool for several years. On the other hand, it makes the log string longer. I'm OK with either way, as long as you thought about it and picked one intentionally.
https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:184: new SimpleDateFormat("MM-dd HH:mm:ss.SSS", Locale.getDefault()); On 2016/11/28 18:27:48, Pete Williamson wrote: > Did we think about whether it makes sense to include the year, too? We could be > using this tool for several years. On the other hand, it makes the log string > longer. I'm OK with either way, as long as you thought about it and picked one > intentionally. Since written to a file that has a create and/or modify date, I think I would favor not having year on each line inside the file. https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:200: private void logInOutput(String message) { name confusing to me - why not just "log()"? https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:295: Log.wtf(TAG, "Cannot set log output file!"); consider just adding e arg here and not doing 2nd wtf below. https://codereview.chromium.org/2529603004/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:57: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); how about getting the coordinator once and holding onto it in a member variable?
addressed comments, but may I have more opinions about the design of this expansion and also the potential async file-writing issue please? Thanks! https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:184: new SimpleDateFormat("MM-dd HH:mm:ss.SSS", Locale.getDefault()); On 2016/11/28 23:02:44, dougarnett wrote: > On 2016/11/28 18:27:48, Pete Williamson wrote: > > Did we think about whether it makes sense to include the year, too? We could > be > > using this tool for several years. On the other hand, it makes the log string > > longer. I'm OK with either way, as long as you thought about it and picked > one > > intentionally. > > Since written to a file that has a create and/or modify date, I think I would > favor not having year on each line inside the file. Acknowledged. https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:200: private void logInOutput(String message) { On 2016/11/28 23:02:44, dougarnett wrote: > name confusing to me - why not just "log()"? Done. https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:295: Log.wtf(TAG, "Cannot set log output file!"); On 2016/11/28 23:02:44, dougarnett wrote: > consider just adding e arg here and not doing 2nd wtf below. Done. https://codereview.chromium.org/2529603004/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:57: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); On 2016/11/28 23:02:44, dougarnett wrote: > how about getting the coordinator once and holding onto it in a member variable? Done. https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/11/25 00:00:07, chili wrote: > So... if I understand it correctly, CustomLog and the Client allows a way for > classes to put the logged events elsewhere? If the only purpose is to write to > a file, I don't see why we can't have an open file handle passed in? Yes that was another option. The goal is 'we need to write logs to a file'. And this could happen on either Java or C++ side. Then the other side will need a method to call in order to have all the logs on the same side for consistency. Currently they're all on Java side. I don't really have a strong opinion where it should be, can you please elaborate the advantage of passing file handle to c++? Also I think it might end up hitting some async issues but I'm not 100% sure... (Even for this patch there might be async issues)
lgtm
Let me know if there's anything I misunderstood :) https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/11/30 00:15:10, romax wrote: > On 2016/11/25 00:00:07, chili wrote: > > So... if I understand it correctly, CustomLog and the Client allows a way for > > classes to put the logged events elsewhere? If the only purpose is to write > to > > a file, I don't see why we can't have an open file handle passed in? > > Yes that was another option. > The goal is 'we need to write logs to a file'. And this could happen on either > Java or C++ side. Then the other side will need a method to call in order to > have all the logs on the same side for consistency. Currently they're all on > Java side. > I don't really have a strong opinion where it should be, can you please > elaborate the advantage of passing file handle to c++? > Also I think it might end up hitting some async issues but I'm not 100% sure... > (Even for this patch there might be async issues) Keeping the file handle on Java sounds/looks ok to me. I think the main potential difference is: Right now if I understand right - - Java calls C++ log - C++ log calls Custom log - Custom log calls into Java - Java writes to file, returns to C++ call stack - finish log call This would occur with every single log message If the file handle is on C++, then Java would pass in file handle one time, and we have - - Java calls C++ log - C++ log writes to file - finish log call Having heard before that JNI boundary stuff is harder to test, having file writes on C++ side may be easier? https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:35: virtual void CustomLog(const std::string& message); nit - should this also have = 0?
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2529603004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:188: Log.e(TAG, e.getMessage(), e); Wouldn't it be easier (and more widely applicable) to use Log class everywhere and then learn to properly parse it... so that we can get relevant evaluation statements in production code and interpret any run, not just evaluation run? https://codereview.chromium.org/2529603004/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:415: log("ROMAX start writing results."); nit: ROMAX? https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:56: if (!coordinator_) { Since you only use this here, I don't think you need this as a field. Variable should do as well. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:58: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); what if your profile is null here, or incognito, and you are setting coordinator_ to null too? https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:218: offline_page_model->GetLogger()->SetClient(this); be consistent with filed vs. parameter usage. (see 2 lines above) https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:219: request_coordinator->GetLogger()->SetClient(this); chili@: I think what we are missing is the ability to swap loggers. Setting a client on the logger is compensating for it... romax@: You don't have to make changes related to this comment in this patch. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:16: void OfflineEventLogger::Clear() { Please explain why you are moving the code so much. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:28: void OfflineEventLogger::GetLogs(std::vector<std::string>* records) { I see you are moving this code, a few suggestions while you are at it. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:30: for (std::deque<std::string>::iterator it = activities_.begin(); you can probably use some form of auto here. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:31: it != activities_.end(); it++) { ++it https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:32: if (!it->empty()) This guard would be more reasonable in record activity I believe, before we put date in front of an empty activity, guaranteeing that the string here is not empty. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:64: SetIsLogging(true); what if client == nullptr? https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:15: std::string(__FILE__) + "(" + std::to_string(__LINE__) + ") :" Is this used anywhere? https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:20: static const size_t kMaxLogCount = 50; mark extern and move definition to the c++ file, please. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:33: class Client { virtual dtor missing https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:58: void RecordActivity(const std::string& activity); why is this no longer protected?
https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:20: static const size_t kMaxLogCount = 50; On 2016/11/30 17:30:47, fgorski wrote: > mark extern and move definition to the c++ file, please. Actually just move it inside c++ as I don't think we are using it outside.
https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:295: Log.wtf(TAG, "Cannot set log output file!"); On 2016/11/30 00:15:10, romax wrote: > On 2016/11/28 23:02:44, dougarnett wrote: > > consider just adding e arg here and not doing 2nd wtf below. > > Done. Don't see a related change uploaded for this https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/11/30 04:56:39, chili wrote: > On 2016/11/30 00:15:10, romax wrote: > > On 2016/11/25 00:00:07, chili wrote: > > > So... if I understand it correctly, CustomLog and the Client allows a way > for > > > classes to put the logged events elsewhere? If the only purpose is to write > > to > > > a file, I don't see why we can't have an open file handle passed in? > > > > Yes that was another option. > > The goal is 'we need to write logs to a file'. And this could happen on either > > Java or C++ side. Then the other side will need a method to call in order to > > have all the logs on the same side for consistency. Currently they're all on > > Java side. > > I don't really have a strong opinion where it should be, can you please > > elaborate the advantage of passing file handle to c++? > > Also I think it might end up hitting some async issues but I'm not 100% > sure... > > (Even for this patch there might be async issues) > > Keeping the file handle on Java sounds/looks ok to me. I think the main > potential difference is: > > Right now if I understand right - > - Java calls C++ log > - C++ log calls Custom log > - Custom log calls into Java > - Java writes to file, returns to C++ call stack > - finish log call > This would occur with every single log message > > If the file handle is on C++, then Java would pass in file handle one time, and > we have - > - Java calls C++ log > - C++ log writes to file > - finish log call > > Having heard before that JNI boundary stuff is harder to test, having file > writes on C++ side may be easier? It is a good point about hopping back and forth over JNI. This class is the one non-eval class in this CL though so I think I prefer not having a file handle in this class (interface seems better). Perhaps an open file/stream could be held in the eval native code though? I'm not sure if that is critical for this iteration. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:33: class Client { class comment here - Is this an optional custom logger? https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:54: // Sets client. a bit more - eg, what is client, required or optional? Really, this is like a tee of log output, you still send to main path and also tee to this client. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:64: // Whether we are currently recording logs or not. does this also control whether to send log messages to client?
sorry for the length replies. I'm happy to discuss offline if you have any insights/ideas/suggestions/criticism! I know the current patch looks shaky in design :( https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:295: Log.wtf(TAG, "Cannot set log output file!"); On 2016/11/30 18:05:25, dougarnett wrote: > On 2016/11/30 00:15:10, romax wrote: > > On 2016/11/28 23:02:44, dougarnett wrote: > > > consider just adding e arg here and not doing 2nd wtf below. > > > > Done. > > Don't see a related change uploaded for this Done. Sorry I lost the change. https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/11/30 04:56:39, chili wrote: > On 2016/11/30 00:15:10, romax wrote: > > On 2016/11/25 00:00:07, chili wrote: > > > So... if I understand it correctly, CustomLog and the Client allows a way > for > > > classes to put the logged events elsewhere? If the only purpose is to write > > to > > > a file, I don't see why we can't have an open file handle passed in? > > > > Yes that was another option. > > The goal is 'we need to write logs to a file'. And this could happen on either > > Java or C++ side. Then the other side will need a method to call in order to > > have all the logs on the same side for consistency. Currently they're all on > > Java side. > > I don't really have a strong opinion where it should be, can you please > > elaborate the advantage of passing file handle to c++? > > Also I think it might end up hitting some async issues but I'm not 100% > sure... > > (Even for this patch there might be async issues) > > Keeping the file handle on Java sounds/looks ok to me. I think the main > potential difference is: > > Right now if I understand right - > - Java calls C++ log > - C++ log calls Custom log > - Custom log calls into Java > - Java writes to file, returns to C++ call stack > - finish log call > This would occur with every single log message > > If the file handle is on C++, then Java would pass in file handle one time, and > we have > - Java calls C++ log > - C++ log writes to file > - finish log call > > Having heard before that JNI boundary stuff is harder to test, having file > writes on C++ side may be easier? Yes JNI is hard to test. The current logging process should look like: 1. Log on Java - Java calls EvalBridge.log() - EvalBridge.log() writes to file. - finish. 2. Log on C++ - C++ log calls Custom log - Custom log calls into Java - Java writes to file, returns to C++ call stack - finish. If we do the writing on C++ side (custom log writes to file?) 1. Log on Java - Java calls C++ log - C++ log calls Custom log - Custom log writes to file. - finish. 2. Log on C++ - C++ log calls custom log - Custom log writes to file. - finish. These two ways should look like mirror images in some sense... Oh I see what's missing... So in this patch, I'm not posting the Java logs to the log pool (so Java logs would not be shown in internals page). So Java logs wouldn't call C++ log methods. However I think it's a good thing to consider how the logger can be used through our code. Maybe we should combine OPM and RC into one and have the single service own the logger and policy controller... Regarding Doug's comment, I think the idea behind bridges are keeping them as simple bridge, doing some translation and calling. So I'm not sure if it's a good idea to keep the handle in the bridge. https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:35: virtual void CustomLog(const std::string& message); On 2016/11/30 04:56:40, chili wrote: > nit - should this also have = 0? Done. https://codereview.chromium.org/2529603004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:188: Log.e(TAG, e.getMessage(), e); On 2016/11/30 17:30:47, fgorski wrote: > Wouldn't it be easier (and more widely applicable) to use Log class everywhere > and then learn to properly parse it... so that we can get relevant evaluation > statements in production code and interpret any run, not just evaluation run? yes that's a more reasonable way and it would be easier to keep Logs even in production code. However this patch wasn't prepared for that scope and was only aiming for the evaluation run. I currently have no idea how the logs are used in Chromium production code. My quick thought is we're logging everything in logcat and we redirect the output stream to a file, then we parse the logs to get whatever we want(maybe with our TAGs). I guess in this way we might have to add many log statements in production code which I'm not seeing very often (maybe I just missed). Or am I totally misunderstanding your idea...? https://codereview.chromium.org/2529603004/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:415: log("ROMAX start writing results."); On 2016/11/30 17:30:47, fgorski wrote: > nit: ROMAX? Done. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:56: if (!coordinator_) { On 2016/11/30 17:30:47, fgorski wrote: > Since you only use this here, I don't think you need this as a field. Variable > should do as well. Acknowledged. yeah but this schedule might be called many times during a evaluation run, so i'd like to keep it as a member variable. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:58: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); On 2016/11/30 17:30:47, fgorski wrote: > what if your profile is null here, or incognito, and you are setting > coordinator_ to null too? I didn't think about this case since this scheduler is only used in test which would not have a null profile. And I don't think I have a way to handle it properly, maybe just let it crash... I'll add an assert here. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:218: offline_page_model->GetLogger()->SetClient(this); On 2016/11/30 17:30:47, fgorski wrote: > be consistent with filed vs. parameter usage. (see 2 lines above) Done. Not sure what you meant but my guess is that I wasn't using offline_page_model_. If so it's fixed. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:219: request_coordinator->GetLogger()->SetClient(this); On 2016/11/30 17:30:47, fgorski wrote: > chili@: > I think what we are missing is the ability to swap loggers. > Setting a client on the logger is compensating for it... > > romax@: > You don't have to make changes related to this comment in this patch. I think a broader vision should be when OfflinePageModel and RequestCoordinator moves under one single hood of service which would owns the client policy controller (and maybe the logger) it would be much easier to swap logger? https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:16: void OfflineEventLogger::Clear() { On 2016/11/30 17:30:47, fgorski wrote: > Please explain why you are moving the code so much. I thought the order of the implementation should be the same with header file so I'm moving a lot... https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:28: void OfflineEventLogger::GetLogs(std::vector<std::string>* records) { On 2016/11/30 17:30:47, fgorski wrote: > I see you are moving this code, a few suggestions while you are at it. Acknowledged. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:30: for (std::deque<std::string>::iterator it = activities_.begin(); On 2016/11/30 17:30:47, fgorski wrote: > you can probably use some form of auto here. Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:31: it != activities_.end(); it++) { On 2016/11/30 17:30:47, fgorski wrote: > ++it Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:32: if (!it->empty()) On 2016/11/30 17:30:47, fgorski wrote: > This guard would be more reasonable in record activity I believe, before we put > date in front of an empty activity, guaranteeing that the string here is not > empty. Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.cc:64: SetIsLogging(true); On 2016/11/30 17:30:47, fgorski wrote: > what if client == nullptr? then client_ would be set to nullptr. and nothing would be CustomLog'ed since it is checked when logging. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:15: std::string(__FILE__) + "(" + std::to_string(__LINE__) + ") :" On 2016/11/30 17:30:48, fgorski wrote: > Is this used anywhere? Yes it's being used in the evaluation_test_scheduler. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:20: static const size_t kMaxLogCount = 50; On 2016/11/30 17:32:53, fgorski wrote: > On 2016/11/30 17:30:47, fgorski wrote: > > mark extern and move definition to the c++ file, please. > > Actually just move it inside c++ as I don't think we are using it outside. Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/11/30 17:30:48, fgorski wrote: > virtual dtor missing Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:54: // Sets client. On 2016/11/30 18:05:25, dougarnett wrote: > a bit more - eg, what is client, required or optional? > > Really, this is like a tee of log output, you still send to main path and also > tee to this client. Done. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:58: void RecordActivity(const std::string& activity); On 2016/11/30 17:30:48, fgorski wrote: > why is this no longer protected? evaluation_test_bridge is calling this directly. I'm not sure if it's a good idea to implement a logger (like OfflinePageModelEventLogger and RCLogger) for each class living in c/b/offline_pages/ so I removed the protected and am calling the record directly. https://codereview.chromium.org/2529603004/diff/20001/components/offline_page... components/offline_pages/offline_event_logger.h:64: // Whether we are currently recording logs or not. On 2016/11/30 18:05:25, dougarnett wrote: > does this also control whether to send log messages to client? yes
LGTM minor comment for thought in later patches (maybe file cleanup bug for consideration?) https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/1/components/offline_pages/of... components/offline_pages/offline_event_logger.h:33: class Client { On 2016/12/01 01:47:52, romax wrote: > On 2016/11/30 04:56:39, chili wrote: > > On 2016/11/30 00:15:10, romax wrote: > > > On 2016/11/25 00:00:07, chili wrote: > > > > So... if I understand it correctly, CustomLog and the Client allows a way > > for > > > > classes to put the logged events elsewhere? If the only purpose is to > write > > > to > > > > a file, I don't see why we can't have an open file handle passed in? > > > > > > Yes that was another option. > > > The goal is 'we need to write logs to a file'. And this could happen on > either > > > Java or C++ side. Then the other side will need a method to call in order to > > > have all the logs on the same side for consistency. Currently they're all on > > > Java side. > > > I don't really have a strong opinion where it should be, can you please > > > elaborate the advantage of passing file handle to c++? > > > Also I think it might end up hitting some async issues but I'm not 100% > > sure... > > > (Even for this patch there might be async issues) > > > > Keeping the file handle on Java sounds/looks ok to me. I think the main > > potential difference is: > > > > Right now if I understand right - > > - Java calls C++ log > > - C++ log calls Custom log > > - Custom log calls into Java > > - Java writes to file, returns to C++ call stack > > - finish log call > > This would occur with every single log message > > > > If the file handle is on C++, then Java would pass in file handle one time, > and > > we have > > - Java calls C++ log > > - C++ log writes to file > > - finish log call > > > > Having heard before that JNI boundary stuff is harder to test, having file > > writes on C++ side may be easier? > > Yes JNI is hard to test. > The current logging process should look like: > 1. Log on Java > - Java calls EvalBridge.log() > - EvalBridge.log() writes to file. > - finish. > > 2. Log on C++ > - C++ log calls Custom log > - Custom log calls into Java > - Java writes to file, returns to C++ call stack > - finish. > > If we do the writing on C++ side (custom log writes to file?) > 1. Log on Java > - Java calls C++ log > - C++ log calls Custom log > - Custom log writes to file. > - finish. > > 2. Log on C++ > - C++ log calls custom log > - Custom log writes to file. > - finish. > > These two ways should look like mirror images in some sense... > Oh I see what's missing... So in this patch, I'm not posting the Java logs to > the log pool (so Java logs would not be shown in internals page). So Java logs > wouldn't call C++ log methods. > > However I think it's a good thing to consider how the logger can be used through > our code. Maybe we should combine OPM and RC into one and have the single > service own the logger and policy controller... > > Regarding Doug's comment, I think the idea behind bridges are keeping them as > simple bridge, doing some translation and calling. So I'm not sure if it's a > good idea to keep the handle in the bridge. I see. As I mentioned, I'm fine with the CL as is. In this particular case, I still think it might be worth it (in future) to ensure that the written log file is consistent with what our internal logger is keeping track of so we don't accidentally log more things on Java side expecting it to show up in internal page logs. I'm also generally supportive of doing log writing on C++ side. There exists some intention to also have our items work on other OS. I think it might be better in the long-term maintenance perspective to not have potentially OS-dependent log impls for something relatively common as writing to file.
https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:58: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); On 2016/12/01 01:47:52, romax wrote: > On 2016/11/30 17:30:47, fgorski wrote: > > what if your profile is null here, or incognito, and you are setting > > coordinator_ to null too? > > I didn't think about this case since this scheduler is only used in test which > would not have a null profile. And I don't think I have a way to handle it > properly, maybe just let it crash... I'll add an assert here. Go ahead. https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:219: request_coordinator->GetLogger()->SetClient(this); On 2016/12/01 01:47:52, romax wrote: > On 2016/11/30 17:30:47, fgorski wrote: > > chili@: > > I think what we are missing is the ability to swap loggers. > > Setting a client on the logger is compensating for it... > > > > romax@: > > You don't have to make changes related to this comment in this patch. > > I think a broader vision should be when OfflinePageModel and RequestCoordinator > moves under one single hood of service which would owns the client policy > controller (and maybe the logger) it would be much easier to swap logger? It's still doesn't solve the problem of having to set client on the logger. https://codereview.chromium.org/2529603004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.h (right): https://codereview.chromium.org/2529603004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.h:9: #include "components/offline_pages/offline_event_logger.h" this include is not used in this .h file, is it? https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:13: static const size_t kMaxLogCount = 50; you don't need static here. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:35: for (auto it = activities_.begin(); it != activities_.end(); ++it) Will this work? records->insert( records->end(), activities_.begin(), activities_.end()); https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:40: if (!is_logging_ || activity.empty()) { nit: {} not needed. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:14: #define OFFLINE_LOG_TAG \ move this to the file where it is used, please. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:30: // This client interface should be implemented by the class which provids the typo https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:32: // logs into a file). It's optional and use SetClient() to attach the client nit: optional and uses ?
Addressed comments, PTAL. Regarding the comment about setting a client can we please leave that for future discussion or redesign? https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc (right): https://codereview.chromium.org/2529603004/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.cc:58: RequestCoordinatorFactory::GetInstance()->GetForBrowserContext(profile); On 2016/12/01 21:42:52, fgorski wrote: > On 2016/12/01 01:47:52, romax wrote: > > On 2016/11/30 17:30:47, fgorski wrote: > > > what if your profile is null here, or incognito, and you are setting > > > coordinator_ to null too? > > > > I didn't think about this case since this scheduler is only used in test which > > would not have a null profile. And I don't think I have a way to handle it > > properly, maybe just let it crash... I'll add an assert here. > > Go ahead. Done. https://codereview.chromium.org/2529603004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.h (right): https://codereview.chromium.org/2529603004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/evaluation_test_scheduler.h:9: #include "components/offline_pages/offline_event_logger.h" On 2016/12/01 21:42:52, fgorski wrote: > this include is not used in this .h file, is it? Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:13: static const size_t kMaxLogCount = 50; On 2016/12/01 21:42:53, fgorski wrote: > you don't need static here. Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:35: for (auto it = activities_.begin(); it != activities_.end(); ++it) On 2016/12/01 21:42:53, fgorski wrote: > Will this work? > > records->insert( > records->end(), activities_.begin(), activities_.end()); Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.cc:40: if (!is_logging_ || activity.empty()) { On 2016/12/01 21:42:52, fgorski wrote: > > nit: {} not needed. Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... File components/offline_pages/offline_event_logger.h (right): https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:14: #define OFFLINE_LOG_TAG \ On 2016/12/01 21:42:53, fgorski wrote: > move this to the file where it is used, please. Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:30: // This client interface should be implemented by the class which provids the On 2016/12/01 21:42:53, fgorski wrote: > typo Done. https://codereview.chromium.org/2529603004/diff/40001/components/offline_page... components/offline_pages/offline_event_logger.h:32: // logs into a file). It's optional and use SetClient() to attach the client On 2016/12/01 21:42:53, fgorski wrote: > nit: optional and uses ? Done.
lgtm
lgtm Thanks for addressing all the comments.
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@fgorski, @chili: Sorry I was having build errors and missing tests. Added tests and made some minor changes to previous patches. PTAnL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
a few more comments https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger.cc:52: client_->CustomLog(date_string + ": " + activity); extract from here and line 57 https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... File components/offline_pages/offline_event_logger_unittest.cc (right): https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger_unittest.cc:40: std::vector<std::string> log; move declaration to where it is used. https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger_unittest.cc:43: for (size_t i = 0; i < kMaxLogCount + 1; ++i) { nit: remove {} Also, I don't believe putting the same message in the log makes for a good test of what happens when it overflows.
Addressed comments. PTAL https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... File components/offline_pages/offline_event_logger.cc (right): https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger.cc:52: client_->CustomLog(date_string + ": " + activity); On 2016/12/05 17:22:18, fgorski wrote: > extract from here and line 57 Done. https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... File components/offline_pages/offline_event_logger_unittest.cc (right): https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger_unittest.cc:40: std::vector<std::string> log; On 2016/12/05 17:22:18, fgorski wrote: > move declaration to where it is used. Done. https://codereview.chromium.org/2529603004/diff/100001/components/offline_pag... components/offline_pages/offline_event_logger_unittest.cc:43: for (size_t i = 0; i < kMaxLogCount + 1; ++i) { On 2016/12/05 17:22:18, fgorski wrote: > nit: remove {} > > Also, I don't believe putting the same message in the log makes for a good test > of what happens when it overflows. Done.
Yafei, are you able to land this now? Or still needs something?
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, chili@chromium.org, dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2529603004/#ps120001 (title: "more comments.")
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": 120001, "attempt_start_ts": 1481053749282870,
"parent_rev": "addbe2d4bd879396ebc59ca63ad70a700e1bbbed", "commit_rev":
"af546b9201b3ea9ce9140809716678911332c216"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Expanding event logger to be used by harness. Added some functionality in event logger to write log into files during test harness run. BUG=661411 ========== to ========== [Offline Pages] Expanding event logger to be used by harness. Added some functionality in event logger to write log into files during test harness run. BUG=661411 Committed: https://crrev.com/d7ce8a5c3be5ebe48eac93746c617f0ea9d45cd0 Cr-Commit-Position: refs/heads/master@{#436712} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d7ce8a5c3be5ebe48eac93746c617f0ea9d45cd0 Cr-Commit-Position: refs/heads/master@{#436712} |
