Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840103002:
Add new API function: feedbackPrivate.readLogSource (Closed)
DescriptionAdd new API function: feedbackPrivate.readLogSource
This function from a single log source (e.g. a log file). If the log source is a
log file, this function can continuously read new lines from it. The function
also contains mechanisms to prevent excessive log source access.
BUG=715263
Review-Url: https://codereview.chromium.org/2840103002
Cr-Commit-Position: refs/heads/master@{#479160}
Committed: https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953c6d21e9b3813
Patch Set 1 #
Total comments: 12
Patch Set 2 : Add LogSource enum, updated API function to use readerId #Patch Set 3 : Fully functional now #Patch Set 4 : Add comments to SingleLogSourceFactory; Add new histogram enum #
Total comments: 35
Patch Set 5 : Addressed comments from Patch Set 4 #
Total comments: 21
Patch Set 6 : Addressed tbarzic's comments from Patch Set 4 and 5 #
Total comments: 7
Patch Set 7 : Add API unit test; create CrOS-specific test module; minor fixes/refactors #
Total comments: 1
Patch Set 8 : Add more unit tests; delete API tests; update permissions #Patch Set 9 : Rebased; updated histogram enum #
Total comments: 58
Patch Set 10 : addressed comments from patch set 9 #Patch Set 11 : Moved most functionality to LogSourceAccessManager; Minor refactors #Patch Set 12 : Remove LogSourceAccessManagerTest #Patch Set 13 : Remove unused function #Patch Set 14 : Refactor passing of params from API into Log Source #
Total comments: 64
Patch Set 15 : Responded to comments from Patch Set 14 #
Total comments: 22
Patch Set 16 : Get profile() directly; Call RunFunctionAndReturn*(); Separate success and error function calls #Patch Set 17 : Addressed Comments from patch set 15 #Patch Set 18 : Missed a few things; Check max number of readers per source #Patch Set 19 : Rebased. #
Total comments: 1
Patch Set 20 : Remove TODO #Patch Set 21 : Rebased #Patch Set 22 : Rebased #Patch Set 23 : Add proper ifdefs for non-CrOS platforms #Messages
Total messages: 70 (22 generated)
rkc@chromium.org changed reviewers: + rkc@chromium.org
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset Instead of reading any arbitrary filename, can we restrict this to known log sources, selected based on an enum? https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, This is not quite a logging API anymore. Instead of getting into things like offset and number of bytes, can we instead just have the API keep track of how much you've read? How about something like, readLogSource(LogSource source, bool incremental, ReadCallback callback) ?
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset On 2017/04/26 03:08:43, rkc wrote: > Instead of reading any arbitrary filename, can we restrict this to known log > sources, selected based on an enum? That's a good idea. I was originally going to use an internal whitelist, in C++ code. And this was going to be CrOS-specific. The list of files would be something like: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/system_lo... Would it still make sense to put an enum def here, given that this will only work on Chrome OS, and that the enums represent log file paths on Chrome OS? https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, On 2017/04/26 03:08:43, rkc wrote: > This is not quite a logging API anymore. Instead of getting into things like > offset and number of bytes, can we instead just have the API keep track of how > much you've read? > > How about something like, > > readLogSource(LogSource source, bool incremental, ReadCallback callback) > > ? I realized the following problem... If there is a tracking mechanism, what happens when it gets called from multiple apps? A few possibilities: 1. Assume that it will only be called from one app. Not a very secure assumption. 2. Provide a token to the first caller that it must subsequently pass in each time. This allows only the first caller to have access to a particular file over this API. 3. Similar to #2 but allow multiple callers to track a single file independently (up to a limited number of tracking counters), each with its own token.
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset On 2017/04/26 14:08:19, Simon Que wrote: > On 2017/04/26 03:08:43, rkc wrote: > > Instead of reading any arbitrary filename, can we restrict this to known log > > sources, selected based on an enum? > > That's a good idea. I was originally going to use an internal whitelist, in C++ > code. And this was going to be CrOS-specific. The list of files would be > something like: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/system_lo... > > Would it still make sense to put an enum def here, given that this will only > work on Chrome OS, and that the enums represent log file paths on Chrome OS? This API will only ever work on Chrome OS. Apps on non-chrome OS platforms are deprecated. https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, On 2017/04/26 14:08:19, Simon Que wrote: > On 2017/04/26 03:08:43, rkc wrote: > > This is not quite a logging API anymore. Instead of getting into things like > > offset and number of bytes, can we instead just have the API keep track of how > > much you've read? > > > > How about something like, > > > > readLogSource(LogSource source, bool incremental, ReadCallback callback) > > > > ? > > I realized the following problem... > > If there is a tracking mechanism, what happens when it gets called from multiple > apps? A few possibilities: > 1. Assume that it will only be called from one app. Not a very secure > assumption. > 2. Provide a token to the first caller that it must subsequently pass in each > time. This allows only the first caller to have access to a particular file over > this API. > 3. Similar to #2 but allow multiple callers to track a single file independently > (up to a limited number of tracking counters), each with its own token. This will either be a private API or be only run in Kiosk Mode, so it is unlikely that two apps will call this API (it is still possible, but then both apps would be provided by the enterprise admin, since the only way you can have multiple apps in Kiosk mode is via having dependent apps/extensions). Still, having a tracking ID for the read in progress is a good idea regardless - so maybe something like an optional "readerId" that you can pass in to subsequent calls.
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset What about extensions? feedbackPrivate API is supported for extensions. https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, feedbackPrivate API is whitelisted for both the Hotrod app and the feedback UI.
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset On 2017/04/26 18:42:44, Simon Que wrote: > What about extensions? feedbackPrivate API is supported for extensions. The 'rest' of this API will continue to work on other platforms, I mean this function specifically. In either case, this is a private API, we control the usage. There are no plans on using this from any other app. https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, On 2017/04/26 18:42:44, Simon Que wrote: > feedbackPrivate API is whitelisted for both the Hotrod app and the feedback UI. Right, the feedback UI will not use this API. There is no reason for it to.
https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:122: // Reads from a log file |filename|, starting from byte offset On 2017/04/26 18:46:33, rkc wrote: > On 2017/04/26 18:42:44, Simon Que wrote: > > What about extensions? feedbackPrivate API is supported for extensions. > > The 'rest' of this API will continue to work on other platforms, I mean this > function specifically. > > In either case, this is a private API, we control the usage. There are no plans > on using this from any other app. Done. https://codereview.chromium.org/2840103002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/feedback_private.idl:131: long offsetBytes, On 2017/04/26 18:46:33, rkc wrote: > On 2017/04/26 18:42:44, Simon Que wrote: > > feedbackPrivate API is whitelisted for both the Hotrod app and the feedback > UI. > > Right, the feedback UI will not use this API. There is no reason for it to. Done.
Description was changed from ========== WIP: chrome: Add feedbackPrivate.readLinesFromFile API func This function reads a range of lines from a text file on disk. TODO: Fill in more details BUG=chromium:715263 ========== to ========== Add new API function: feedbackPrivate.readLogSource This function from a single log source (e.g. a log file). If the log source is a log file, this function can continuously read new lines from it. The function also contains mechanisms to prevent excessive log source access. BUG=chromium:715263 ==========
sque@chromium.org changed reviewers: + tbarzic@chromium.org
Sorry, I didn't realize this was up for review. (you should use "Publish+Mail Comments" when a CL is ready for review, so the reviewers get email notifications). Also, BUG= does not require chromium: prefix https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: LogSourceResourceManager::GetFactoryInstance() { Can this be added to FeedbackAPI (similar to LogSourceAccessManager) https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:309: } // namespace can you move this up, with the rest of the anonymous namespace vars? https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:336: return EmptyResponse(); I'd go with returning an error instead. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:348: // If caller passed in |reader_id| == 0, create a new SingleLogSource. I'd extract this and if (reader_id > 0) part to a separate method: LogSourceResource* GetLogSourceResource(reader_id, source) {} https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:360: reader_id = resource_manager->Add(new_resource); can you use unique_ptr as Add method argument (to have a self-documented ownership relationship). https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:371: log_source_manager->GetLastExtensionAccessTime(source_and_extension); can this be checked by log_source_manager->AccessSourceFromExtension()? e.g. if the timeout hasn't passed at the time, the method could return false/ or some error status. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:373: return EmptyResponse(); return proper error https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:384: return EmptyResponse(); return RespondNow(Error("Not supported")); https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:398: int readerId, nit: reader_id https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:47: base::TimeDelta::FromMilliseconds(100)); I think you should use 0 delay here, that should be enough to simulate async call (and avoid holding up the test without a reason). If you want to test that the callback timing respects the timeout value, you should use unittests, where you can mock message loop time. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:64: int call_count_; add DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:96: const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(2000)); we should not hold up tests for 2 seconds :/ https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:100: base::Bind(&TestSingleLogSource::Create); can you do this in FeedbackApiTest::SetUp? https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:26: using SourceAndExtension = I'd prefer to have this defined as a struct {LogSource source; std::string extension_id;} https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.h (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/single_log_source_factory.h:21: // ownership of the returned object. return unique_ptr https://codereview.chromium.org/2840103002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/feedback_private/read_log_source/test.js (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/feedback_private/read_log_source/test.js:84: chrome.feedbackPrivate.readLogSource( hm, this seems like it could be flaky - is it guaranteed that the first readLogSource function implementation won't finish before the second one starts? https://codereview.chromium.org/2840103002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/feedback_private/read_log_source/test.js:156: console.log("Callback with count " + count); generally, try to avoid unnecessary log statements. If you think this info could be useful for debugging, you should add a message to chrome.test.asserts (I think chrome.test.assertEq(a, b, 'Error message'); should work) https://codereview.chromium.org/2840103002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/feedback_private/read_log_source/test.js:191: {delayInMinutes: 0.0, periodInMinutes: period}); I'm a bit skeptical about using alarms here, especially ones that are 0.5 second long. Test that rely on a particular timing, or use timeouts, should generally be avoided, as they're likely to be flaky. (also, we should not intentionally add delays to test run-times, those can add up)
Let's revisit the apitest -- perhaps it should be a API unit test instead. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: LogSourceResourceManager::GetFactoryInstance() { On 2017/05/22 23:02:54, tbarzic wrote: > Can this be added to FeedbackAPI (similar to LogSourceAccessManager) I don't think so. I assume you mean something like: BrowserContextKeyedAPIFactory<LogSourceResourceManager>* LogSourceResourceManager::GetFactoryInstance() { return FeedbackPrivateAPI::GetFactoryInstance() ->Get(browser_context) ->GetLogSourceResourceManagerFactory(); } But there is no browser context available here. This GetFactoryInstance() function is not explicitly called from anywhere in my code. It's called from the API Resource System somewhere. Plus, api_resource_manager.h specifically says to define this in the .cc file as a static global. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:309: } // namespace On 2017/05/22 23:02:54, tbarzic wrote: > can you move this up, with the rest of the anonymous namespace vars? Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:336: return EmptyResponse(); On 2017/05/22 23:02:54, tbarzic wrote: > I'd go with returning an error instead. Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:348: // If caller passed in |reader_id| == 0, create a new SingleLogSource. On 2017/05/22 23:02:54, tbarzic wrote: > I'd extract this and if (reader_id > 0) part to a separate method: > > LogSourceResource* GetLogSourceResource(reader_id, source) {} Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:360: reader_id = resource_manager->Add(new_resource); On 2017/05/22 23:02:54, tbarzic wrote: > can you use unique_ptr as Add method argument (to have a self-documented > ownership relationship). Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:371: log_source_manager->GetLastExtensionAccessTime(source_and_extension); On 2017/05/22 23:02:54, tbarzic wrote: > can this be checked by log_source_manager->AccessSourceFromExtension()? > e.g. if the timeout hasn't passed at the time, the method could return false/ or > some error status. Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:373: return EmptyResponse(); On 2017/05/22 23:02:54, tbarzic wrote: > return proper error This is not meant to be an error. The caller does not need to keep track of how much time has elapsed since the last read. From the caller's perspective, the log source behaves as if there is no new data available yet. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:384: return EmptyResponse(); On 2017/05/22 23:02:54, tbarzic wrote: > return RespondNow(Error("Not supported")); Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:398: int readerId, On 2017/05/22 23:02:54, tbarzic wrote: > nit: reader_id Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:47: base::TimeDelta::FromMilliseconds(100)); On 2017/05/22 23:02:55, tbarzic wrote: > I think you should use 0 delay here, that should be enough to simulate async > call (and avoid holding up the test without a reason). > > If you want to test that the callback timing respects the timeout value, you > should use unittests, where you can mock message loop time. Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:64: int call_count_; On 2017/05/22 23:02:55, tbarzic wrote: > add DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:96: const base::TimeDelta timeout(base::TimeDelta::FromMilliseconds(2000)); On 2017/05/22 23:02:55, tbarzic wrote: > we should not hold up tests for 2 seconds :/ Agreed, but the alarm API used by the test app doesn't seem to fire more than once per second. Maybe we should switch to C++-based API unittest? https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:100: base::Bind(&TestSingleLogSource::Create); On 2017/05/22 23:02:55, tbarzic wrote: > can you do this in FeedbackApiTest::SetUp? Done. Not sure if I'm doing it right though. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:26: using SourceAndExtension = On 2017/05/22 23:02:55, tbarzic wrote: > I'd prefer to have this defined as a struct {LogSource source; std::string > extension_id;} Done. https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.h (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/single_log_source_factory.h:21: // ownership of the returned object. On 2017/05/22 23:02:55, tbarzic wrote: > return unique_ptr Done.
https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. I should document that it is only supported on Chrome OS.
rkc@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/24 19:30:47, Simon Que wrote: > I should document that it is only supported on Chrome OS. This should self-document via the docserver parsing the _permission_features.json file. Is this correct Devlin?
m https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/24 19:58:11, rkc wrote: > On 2017/05/24 19:30:47, Simon Que wrote: > > I should document that it is only supported on Chrome OS. > > This should self-document via the docserver parsing the > _permission_features.json file. Is this correct Devlin? I don't see any reference to Chrome OS in that file: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission...
If this call isn't restricted to Chrome OS in _permission_features, then this API isn't really restricted to Chrome OS :) If you want this API to be Chrome OS specific only, then you need to enforce that via _permission_features, rather than depending on compliance with the honor system from the app :)
On 2017/05/24 20:09:56, rkc wrote: > If this call isn't restricted to Chrome OS in _permission_features, then this > API isn't really restricted to Chrome OS :) > > If you want this API to be Chrome OS specific only, then you need to enforce > that via _permission_features, rather than depending on compliance with the > honor system from the app :) The existing feedbackPrivate API has no Chrome OS-specific enforcement. Is that what is intended? As for the new function readLogSource(), I am already enforcing its Chrome OS-specificity in the code. On other systems, the API returns an error. My comment in the IDL was just that the new function needs to be documented as supported in CrOS only. Similarly, my CPU temperature feature was only supported on Chrome OS, but there was no restriction on the function being called from other systems. Thus I had to document it in the IDL https://codereview.chromium.org/2802593005/diff/120001/extensions/common/api/...
On 2017/05/24 19:58:12, rkc wrote: > https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... > File chrome/common/extensions/api/feedback_private.idl (right): > > https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... > chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log > source indicated by |source|. > On 2017/05/24 19:30:47, Simon Que wrote: > > I should document that it is only supported on Chrome OS. > > This should self-document via the docserver parsing the > _permission_features.json file. Is this correct Devlin? Sadly, no. Docs are generated from the API specification files (.json or .idl), but don't take the features files into account.
https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/24 20:07:17, Simon Que wrote: > On 2017/05/24 19:58:11, rkc wrote: > > On 2017/05/24 19:30:47, Simon Que wrote: > > > I should document that it is only supported on Chrome OS. > > > > This should self-document via the docserver parsing the > > _permission_features.json file. Is this correct Devlin? > > I don't see any reference to Chrome OS in that file: > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission... So we should definitely restrict this via _permission_features.json (you can have permissions for individual functions within an API) and document it in the IDL. Preventing it from the API implementation is not really a great way of doing this. API permissions should be enforced in one place. Since we already enforce them via _permission_features.json, we should enforce the permissions for this function also via that file. The code at best should have a LOG(ERROR) and/or a DCHECK() for this. Toni, WDYT?
On 2017/05/24 20:27:44, rkc wrote: > https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... > File chrome/common/extensions/api/feedback_private.idl (right): > > https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... > chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log > source indicated by |source|. > On 2017/05/24 20:07:17, Simon Que wrote: > > On 2017/05/24 19:58:11, rkc wrote: > > > On 2017/05/24 19:30:47, Simon Que wrote: > > > > I should document that it is only supported on Chrome OS. > > > > > > This should self-document via the docserver parsing the > > > _permission_features.json file. Is this correct Devlin? > > > > I don't see any reference to Chrome OS in that file: > > > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission... > > So we should definitely restrict this via _permission_features.json (you can > have permissions for individual functions within an API) and document it in the > IDL. > > Preventing it from the API implementation is not really a great way of doing > this. API permissions should be enforced in one place. Since we already enforce > them via _permission_features.json, we should enforce the permissions for this > function also via that file. > > The code at best should have a LOG(ERROR) and/or a DCHECK() for this. > > Toni, WDYT? Yeah, I think that's reasonable, as long as the set of platform specific methods is small.
https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: LogSourceResourceManager::GetFactoryInstance() { On 2017/05/23 20:01:19, Simon Que wrote: > On 2017/05/22 23:02:54, tbarzic wrote: > > Can this be added to FeedbackAPI (similar to LogSourceAccessManager) > > I don't think so. I assume you mean something like: > > BrowserContextKeyedAPIFactory<LogSourceResourceManager>* > LogSourceResourceManager::GetFactoryInstance() { > return FeedbackPrivateAPI::GetFactoryInstance() > ->Get(browser_context) > ->GetLogSourceResourceManagerFactory(); > } > > But there is no browser context available here. This GetFactoryInstance() > function is not explicitly called from anywhere in my code. It's called from the > API Resource System somewhere. > > Plus, api_resource_manager.h specifically says to define this in the .cc file as > a static global. I was thinking something along the lines: class FeedbackPrivateAPI { FeedbackPrivateAPI(browser_context) : log_source_resources_(browser_context) {} LogSourceResourceManager* log_source_resources() {return &log_source_resources_;} private: LogSourceResourceManager log_source_resources_; }; So to get the manager you'd do: FeedbackPrivateAPI::GetFactoryInstance()->Get(browser_context())->log_source_resources(); Though, I didn't notice that LogSourceResourceManager is just an alias for ApiResourceManager<LogSourceResource>, rather than a browser context keyed service you created yourself. Can you move this factory to log_source_resource.cc? And I'd get rid of using LogSourceResourceManager = ApiResourceManager<LogSourceResource>; https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:140: g_rate_limiting_timeout ? *g_rate_limiting_timeout can you just move this const to LogSourceAccessManager, and add LogSourceAccessManager::Set*ForTesting https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:344: LogSourceAccessManager::SourceAndExtension(source, extension_id()))) add {} https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:374: return nullptr; Shouldn't you just return resource here? i.e. if (reader_id > 0) return resource_manager->Get(extension_id(), reader_id); https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:394: std::unique_ptr<LogSourceResource> new_resource(new LogSourceResource( prefer MakeUnique over new https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:400: reader_id = resource_manager->Add(new_resource.release()); you don't use this anywhere https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:33: LogSourceAccessManager* GetLogSourceAccessManager() { for simple getters/setters this would be preferred: LogSourceAccessmanager* log_source_access_manager() { return log_source_access_manager_; } https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:57: if (last.is_null() || base::Time::Now() > last + min_time_between_reads_) { you should use TimeTicks (as Time is not guaranteed to increase monotonically) Also, I'd add a TickClock as a member to this class - that way you can easily mock passage of the time in tests. E.g. in production, use DefaultTickClock, but enable test to inject a test tick clock (e.g. SimpleTestTickClock - https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?type=...) Then replace base::Time::Now() with tick_clock_->NowTicks(); And in tests use SimpleTestTickClock::Advance when required.
https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: LogSourceResourceManager::GetFactoryInstance() { On 2017/05/25 22:46:44, tbarzic wrote: > On 2017/05/23 20:01:19, Simon Que wrote: > > On 2017/05/22 23:02:54, tbarzic wrote: > > > Can this be added to FeedbackAPI (similar to LogSourceAccessManager) > > > > I don't think so. I assume you mean something like: > > > > BrowserContextKeyedAPIFactory<LogSourceResourceManager>* > > LogSourceResourceManager::GetFactoryInstance() { > > return FeedbackPrivateAPI::GetFactoryInstance() > > ->Get(browser_context) > > ->GetLogSourceResourceManagerFactory(); > > } > > > > But there is no browser context available here. This GetFactoryInstance() > > function is not explicitly called from anywhere in my code. It's called from > the > > API Resource System somewhere. > > > > Plus, api_resource_manager.h specifically says to define this in the .cc file > as > > a static global. > > I was thinking something along the lines: > > class FeedbackPrivateAPI { > > FeedbackPrivateAPI(browser_context) : log_source_resources_(browser_context) {} > > LogSourceResourceManager* log_source_resources() {return > &log_source_resources_;} > > private: > LogSourceResourceManager log_source_resources_; > }; > > So to get the manager you'd do: > FeedbackPrivateAPI::GetFactoryInstance()->Get(browser_context())->log_source_resources(); > > Though, I didn't notice that LogSourceResourceManager is just an alias for > ApiResourceManager<LogSourceResource>, rather than a browser context keyed > service you created yourself. > > Can you move this factory to log_source_resource.cc? And I'd get rid of > using LogSourceResourceManager = ApiResourceManager<LogSourceResource>; Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:140: g_rate_limiting_timeout ? *g_rate_limiting_timeout On 2017/05/25 22:46:44, tbarzic wrote: > can you just move this const to LogSourceAccessManager, and add > LogSourceAccessManager::Set*ForTesting Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:344: LogSourceAccessManager::SourceAndExtension(source, extension_id()))) On 2017/05/25 22:46:44, tbarzic wrote: > add {} Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:374: return nullptr; On 2017/05/25 22:46:44, tbarzic wrote: > Shouldn't you just return resource here? > i.e. > if (reader_id > 0) > return resource_manager->Get(extension_id(), reader_id); > Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:394: std::unique_ptr<LogSourceResource> new_resource(new LogSourceResource( On 2017/05/25 22:46:45, tbarzic wrote: > prefer MakeUnique over new Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:400: reader_id = resource_manager->Add(new_resource.release()); On 2017/05/25 22:46:45, tbarzic wrote: > you don't use this anywhere Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:33: LogSourceAccessManager* GetLogSourceAccessManager() { On 2017/05/25 22:46:45, tbarzic wrote: > for simple getters/setters this would be preferred: > LogSourceAccessmanager* log_source_access_manager() { > return log_source_access_manager_; > } > Done. https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:57: if (last.is_null() || base::Time::Now() > last + min_time_between_reads_) { On 2017/05/25 22:46:45, tbarzic wrote: > you should use TimeTicks (as Time is not guaranteed to increase monotonically) > > Also, I'd add a TickClock as a member to this class - that way you can easily > mock passage of the time in tests. > > E.g. in production, use DefaultTickClock, but enable test to inject a test tick > clock (e.g. SimpleTestTickClock - > https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?type=...) > > Then replace base::Time::Now() with tick_clock_->NowTicks(); > And in tests use SimpleTestTickClock::Advance when required. Done.
Description was changed from ========== Add new API function: feedbackPrivate.readLogSource This function from a single log source (e.g. a log file). If the log source is a log file, this function can continuously read new lines from it. The function also contains mechanisms to prevent excessive log source access. BUG=chromium:715263 ========== to ========== Add new API function: feedbackPrivate.readLogSource This function from a single log source (e.g. a log file). If the log source is a log file, this function can continuously read new lines from it. The function also contains mechanisms to prevent excessive log source access. BUG=715263 ==========
I also think I should rewrite the API test into a unit test using the test override functions I've since added. https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/24 20:27:43, rkc wrote: > On 2017/05/24 20:07:17, Simon Que wrote: > > On 2017/05/24 19:58:11, rkc wrote: > > > On 2017/05/24 19:30:47, Simon Que wrote: > > > > I should document that it is only supported on Chrome OS. > > > > > > This should self-document via the docserver parsing the > > > _permission_features.json file. Is this correct Devlin? > > > > I don't see any reference to Chrome OS in that file: > > > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission... > > So we should definitely restrict this via _permission_features.json (you can > have permissions for individual functions within an API) and document it in the > IDL. > > Preventing it from the API implementation is not really a great way of doing > this. API permissions should be enforced in one place. Since we already enforce > them via _permission_features.json, we should enforce the permissions for this > function also via that file. > > The code at best should have a LOG(ERROR) and/or a DCHECK() for this. > > Toni, WDYT? Toni, got any opinion on this?
yes, I think testing more subtle behavior (e.g. concurrent requests) with unittests would make sense. Though, api tests have some value too (e.g. to exercise renderer side bindings), so I'd keep some of it too - e.g. a simple test that fetches logs once or twice in succession. (note: I'll make a more thorough pass through the cl later today) https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/30 17:52:54, Simon Que wrote: > On 2017/05/24 20:27:43, rkc wrote: > > On 2017/05/24 20:07:17, Simon Que wrote: > > > On 2017/05/24 19:58:11, rkc wrote: > > > > On 2017/05/24 19:30:47, Simon Que wrote: > > > > > I should document that it is only supported on Chrome OS. > > > > > > > > This should self-document via the docserver parsing the > > > > _permission_features.json file. Is this correct Devlin? > > > > > > I don't see any reference to Chrome OS in that file: > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission... > > > > So we should definitely restrict this via _permission_features.json (you can > > have permissions for individual functions within an API) and document it in > the > > IDL. > > > > Preventing it from the API implementation is not really a great way of doing > > this. API permissions should be enforced in one place. Since we already > enforce > > them via _permission_features.json, we should enforce the permissions for this > > function also via that file. > > > > The code at best should have a LOG(ERROR) and/or a DCHECK() for this. > > > > Toni, WDYT? > > Toni, got any opinion on this? I'd be fine with restricting this using features system (though, it would have to be _api_features.json rather than _permission_features.json), provided that this is the only thing that's restricted to Chrome OS. Though, to be honest I don't see that much problem here, as the reason for restricting this to Chrome OS seems to be a consequence of this function not being really relevant to other platforms, rather than being not allowed on other platforms - i.e. it's more akin to this method simply not being implemented on other platforms. Alternatively, I'd consider creating a separate log fetching API - this does not seem to be directly tied to feedback reporting (at least not to the feedback reporting app). https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc (right): https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:77: #if defined(OS_CHROMEOS) can you move the Chrome OS specific test code into feedback_private_chromeos_apitest.cc https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:50: static void SetRateLimitingTimeoutForTesting(const base::TimeDelta* timeout); do you still need this, given that you can just advance time using test tick clock? https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:87: void set_tick_clock(base::TickClock* clock) { tick_clock_.reset(clock); } pass in std::unique_ptr<base::TickClock>, to make ownership transfer evident.
On 2017/05/30 19:35:35, tbarzic wrote: > yes, I think testing more subtle behavior (e.g. concurrent requests) with > unittests would make sense. > Though, api tests have some value too (e.g. to exercise renderer side bindings), > so I'd keep some of it too - e.g. a simple test that fetches logs once or twice > in succession. I'll address your per-line comments later. For now, I'm finding the existing testing infrastructure to be too restrictive. I'm using ApiUnitTest to run individual functions but it seems like the class is currently set up to return only the first value in a ListValue result. I'd have add a new function to ApiUnitTest to get the entire ListValue result (or refactor it since it might not actually be doing what the author intended). Another problem is that there is no way to asynchronously run the API function (except for maybe ShellApiTest::LoadApp). Without that, I can't test concurrent requests, nor am I able to use a TestTickClock to advance time manually.
I encountered more problems as I attempted to move the testing to an
ApiUnitTest.
===============================================
1. There is no generic "Run API function and return entire result" function. I
think the intent was to implement one, but it was incorrectly done. I had to
make the following change for the new unit test to work:
--- a/extensions/browser/api_test_utils.cc
+++ b/extensions/browser/api_test_utils.cc
@@ -155,7 +155,7 @@ std::unique_ptr<base::Value>
RunFunctionWithDelegateAndReturnSingleResult(
const base::Value* single_result = NULL;
if (function->GetResultList() != NULL &&
function->GetResultList()->Get(0, &single_result)) {
- return single_result->CreateDeepCopy();
+ return function->GetResultList()->CreateDeepCopy();
}
return NULL;
}
===============================================
2. The API Resource Manager gets destroyed between separate unit tests, and
never restored. This is probably because it has the "DestructorAtExit" property.
Should it be made into a leaky instance instead?
https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc
(right):
https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/feedback_private/feedback_private_apitest.cc:77:
#if defined(OS_CHROMEOS)
On 2017/05/30 19:35:35, tbarzic wrote:
> can you move the Chrome OS specific test code into
> feedback_private_chromeos_apitest.cc
Done.
https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h
(right):
https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:50:
static void SetRateLimitingTimeoutForTesting(const base::TimeDelta* timeout);
On 2017/05/30 19:35:35, tbarzic wrote:
> do you still need this, given that you can just advance time using test tick
> clock?
I'll deal with this once I figure out the larger problems going on with unit
testing.
https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:87:
void set_tick_clock(base::TickClock* clock) { tick_clock_.reset(clock); }
On 2017/05/30 19:35:35, tbarzic wrote:
> pass in std::unique_ptr<base::TickClock>, to make ownership transfer evident.
Done.
PTAL https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:150: // Reads from a log source indicated by |source|. On 2017/05/30 19:35:35, tbarzic wrote: > On 2017/05/30 17:52:54, Simon Que wrote: > > On 2017/05/24 20:27:43, rkc wrote: > > > On 2017/05/24 20:07:17, Simon Que wrote: > > > > On 2017/05/24 19:58:11, rkc wrote: > > > > > On 2017/05/24 19:30:47, Simon Que wrote: > > > > > > I should document that it is only supported on Chrome OS. > > > > > > > > > > This should self-document via the docserver parsing the > > > > > _permission_features.json file. Is this correct Devlin? > > > > > > > > I don't see any reference to Chrome OS in that file: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_permission... > > > > > > So we should definitely restrict this via _permission_features.json (you can > > > have permissions for individual functions within an API) and document it in > > the > > > IDL. > > > > > > Preventing it from the API implementation is not really a great way of doing > > > this. API permissions should be enforced in one place. Since we already > > enforce > > > them via _permission_features.json, we should enforce the permissions for > this > > > function also via that file. > > > > > > The code at best should have a LOG(ERROR) and/or a DCHECK() for this. > > > > > > Toni, WDYT? > > > > Toni, got any opinion on this? > > I'd be fine with restricting this using features system (though, it would have > to be _api_features.json rather than _permission_features.json), provided that > this is the only thing that's restricted to Chrome OS. > > Though, to be honest I don't see that much problem here, as the reason for > restricting this to Chrome OS seems to be a consequence of this function not > being really relevant to other platforms, rather than being not allowed on other > platforms - i.e. it's more akin to this method simply not being implemented on > other platforms. > > Alternatively, I'd consider creating a separate log fetching API - this does not > seem to be directly tied to feedback reporting (at least not to the feedback > reporting app). > Done. https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:50: static void SetRateLimitingTimeoutForTesting(const base::TimeDelta* timeout); On 2017/05/31 00:21:44, Simon Que wrote: > On 2017/05/30 19:35:35, tbarzic wrote: > > do you still need this, given that you can just advance time using test tick > > clock? > > I'll deal with this once I figure out the larger problems going on with unit > testing. See the new API unit tests. If I got rid of this, that code would be a lot more verbose.
https://codereview.chromium.org/2840103002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:350: I'd pass SourceAndExtension as a parameter (you'll have to calculate it after this method returns either way) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: } NOTREACHED() << "Unknown log source type."; return SingleLogSource::SupportedSource::kMessages; https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:316: DCHECK(log_source_manager); I don't think you need dcheck here https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:331: return RespondNow(Error("API function is not supported.")); Can you add a NOTREACHED now that you've restricted the function to Chrome OS (using api_features file) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:371: SingleLogSourceFactory::CreateSingleLogSource(source_type).release(), I wonder if design here would be simpler if more logic was moved to LogSourceManager. I'm concerned that LogSourceManager exposes too much of it's implementation to be usable with resource manager. In particular I don't like GetUnregisterCallback being exposed, and LogAccessManager not having control over whether the access time actually matches last log source fetch done by the extension. Maybe this could be avoided by moving resource manager handling to LogSourceManager? For example if you introduce something like: bool LogSourceManager::FetchResource(int reader_id, SourceAndExtension source, callback) { int resource_id = reader_id > 0 ? reader_id : CreateResource(source); if (resource_id <= 0) return false; resource = resource_manager->Get(source.extension_id, reader_id); if (!resource) return false; if (!UpdateSourceAccessTime(source)) return false; resource->GetLogSource()->Fetch(Bind(&OnLogSourceFetched, callback, reader_id)); } Here, there is no need to publicly expose UnregisterCallback (given that resource creation is done internally to LogSourceManager); and LogSourceManager does not have to rely on it's client to keep last access time in sync with last resource fetch. WDYT? https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:395: if (!incremental) { Can you update the comment to note that incremental not being set indicates that any existing incremental reader has to be closed (and not only that the requested access is non-incremental) - otherwise one would think that just not registering resource with the resource manage would be enough to handle incremental == false. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:399: reader_id = 0; Instead of changing |reader_id|, can you introduce a variable that will be set to the final reader_id value accordingly. For example: int final_reader_id = incremental ? reader_id : -1; Respond(ArgumentList( feedback_private::ReadLogSource::Results::Create(final_reader_id, result))); (also, I think -1 would be better value for "invalid" reader ID) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:65: LogSourceAccessManager* log_source_access_manager_; Does this have to be a pointer, the object lifetime seems to be equal to FeedbackPrivateAPI. You should at least change it to unique_ptr. (actually, same goes for service_) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: update the year https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:93: // To avoid the ApiResourceManager being destroyed each time TearDown() is Can you clarify the comment a little? Destroying ApiResourceManager is something that would be expected in tear down (though, it should be recreated in subsequent tests). https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:103: SingleLogSourceFactory::InitForTesting(nullptr); nit: Can you rename this to |SetForTesting| - it seems surprising to call Init during tear down :) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:156: } can you add new lines around methods https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:57: auto iter = active_keys_.find(key); I'd prefer something like: return active_keys_.erase(key) > 0; or if (active_keys_.count(key) == 0) return false; active_keys_.erase(key); return true; https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:87: return false; nit: I'd switch this as: if (!last.is_null() && now < last + GetMinTimeBetweenReads()) return false; last_access_times_[key] = now; return true; https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:80: // Returns a closure (callback with no args) that removes |key| from this nit: remove (callback with no args) https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:86: // Takes ownership of |clock|. no need for the comment, it's evident from argument type https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:88: tick_clock_.reset(clock.release()); nit: tick_clock_ = std::move(clock); https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_resource.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_resource.h:25: system_logs::SingleLogSource* source, // Takes ownership std::unique<SignelLogSource> https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc:21: g_callback ? g_callback->Run(type) : new SingleLogSource(type)); use MakeUnique instead of new https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:455: "feedbackPrivate.readLogSource": { platforms: [chromeos], session_types:[kiosk] should be enough; the rest will be inherited from feedbackPrivate https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:100: // /var/log/messages Chrome OS system messages. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:103: // /var/log/ui/ui.LATEST Latest Chrome OS UI logs. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:104: ui_latest uiLatest or latestUi https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:114: // continue where it left off. readLogSource() can be called with s/readLogSource()/$(ref:readLogSource)/ https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:115: // incremental=true repeatedly. To subsequently close the file handle, pass <code>incremental = true</code> https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:132: callback ReadLogSourceCallback = void (long readerId, DOMString[] logLines); I'd consider returning result as object { optional long readerId; DOMString[] lines; } https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:157: // - Returns the entire contents of the log file. put lists in <ul></ul> (so docs render better, you can run chrome/common/extensions/docs/server2/preview.py and check docs at http://localhost:8000/extensions/feedbackPrivate) https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:165: // If |incremental| is true, and a valid non-zero |readerId| is provided: <code></code> where you use ||
https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:104: } On 2017/06/01 19:23:45, tbarzic wrote: > NOTREACHED() << "Unknown log source type."; > return SingleLogSource::SupportedSource::kMessages; Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:316: DCHECK(log_source_manager); On 2017/06/01 19:23:45, tbarzic wrote: > I don't think you need dcheck here Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:331: return RespondNow(Error("API function is not supported.")); On 2017/06/01 19:23:45, tbarzic wrote: > Can you add a NOTREACHED now that you've restricted the function to Chrome OS > (using api_features file) Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:371: SingleLogSourceFactory::CreateSingleLogSource(source_type).release(), On 2017/06/01 19:23:45, tbarzic wrote: > I wonder if design here would be simpler if more logic was moved to > LogSourceManager. > > I'm concerned that LogSourceManager exposes too much of it's implementation to > be usable with resource manager. In particular I don't like > GetUnregisterCallback being exposed, and LogAccessManager not having control > over whether the access time actually matches last log source fetch done by the > extension. Maybe this could be avoided by moving resource manager handling to > LogSourceManager? > > For example if you introduce something like: > bool LogSourceManager::FetchResource(int reader_id, SourceAndExtension source, > callback) { > int resource_id = > reader_id > 0 ? reader_id : CreateResource(source); > if (resource_id <= 0) > return false; > resource = resource_manager->Get(source.extension_id, reader_id); > if (!resource) > return false; > > if (!UpdateSourceAccessTime(source)) > return false; > resource->GetLogSource()->Fetch(Bind(&OnLogSourceFetched, callback, > reader_id)); > } > > Here, there is no need to publicly expose UnregisterCallback (given that > resource creation is done internally to LogSourceManager); and LogSourceManager > does not have to rely on it's client to keep last access time in sync with last > resource fetch. > > WDYT? I like the idea. But your description still leaves some open questions: - How is LogSourceManager going to access the API resource manager? It will have to provide a browser context when accessing it. That means LogSourceManager will have to depend on BrowserContext (which is a possible way to go). - Does UnregisterCallback still get passed to API manager, but only in a context that is private to LogSourceManager? https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:395: if (!incremental) { On 2017/06/01 19:23:45, tbarzic wrote: > Can you update the comment to note that incremental not being set indicates that > any existing incremental reader has to be closed (and not only that the > requested access is non-incremental) - otherwise one would think that just not > registering resource with the resource manage would be enough to handle > incremental == false. Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:399: reader_id = 0; On 2017/06/01 19:23:45, tbarzic wrote: > Instead of changing |reader_id|, can you introduce a variable that will be set > to the final reader_id value accordingly. > > For example: > int final_reader_id = incremental ? reader_id : -1; > Respond(ArgumentList( > feedback_private::ReadLogSource::Results::Create(final_reader_id, result))); > > (also, I think -1 would be better value for "invalid" reader ID) Done. I'd normally agree with -1 over 0, but 0 is the invalid value as used in (and returned by) ApiResourceManager. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:65: LogSourceAccessManager* log_source_access_manager_; On 2017/06/01 19:23:46, tbarzic wrote: > Does this have to be a pointer, the object lifetime seems to be equal to > FeedbackPrivateAPI. You should at least change it to unique_ptr. > > (actually, same goes for service_) Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/06/01 19:23:46, tbarzic wrote: > nit: update the year Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:93: // To avoid the ApiResourceManager being destroyed each time TearDown() is On 2017/06/01 19:23:46, tbarzic wrote: > Can you clarify the comment a little? > Destroying ApiResourceManager is something that would be expected in tear down > (though, it should be recreated in subsequent tests). Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:103: SingleLogSourceFactory::InitForTesting(nullptr); On 2017/06/01 19:23:46, tbarzic wrote: > nit: Can you rename this to |SetForTesting| - it seems surprising to call Init > during tear down :) Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:156: } On 2017/06/01 19:23:46, tbarzic wrote: > can you add new lines around methods Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:57: auto iter = active_keys_.find(key); On 2017/06/01 19:23:46, tbarzic wrote: > I'd prefer something like: > > return active_keys_.erase(key) > 0; > > or > > if (active_keys_.count(key) == 0) > return false; > active_keys_.erase(key); > return true; Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:87: return false; On 2017/06/01 19:23:46, tbarzic wrote: > nit: I'd switch this as: > if (!last.is_null() && now < last + GetMinTimeBetweenReads()) > return false; > > last_access_times_[key] = now; > return true; Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:80: // Returns a closure (callback with no args) that removes |key| from this On 2017/06/01 19:23:46, tbarzic wrote: > nit: remove (callback with no args) Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:86: // Takes ownership of |clock|. On 2017/06/01 19:23:46, tbarzic wrote: > no need for the comment, it's evident from argument type Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:88: tick_clock_.reset(clock.release()); On 2017/06/01 19:23:46, tbarzic wrote: > nit: > tick_clock_ = std::move(clock); Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_resource.h (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_resource.h:25: system_logs::SingleLogSource* source, // Takes ownership On 2017/06/01 19:23:46, tbarzic wrote: > std::unique<SignelLogSource> Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc:21: g_callback ? g_callback->Run(type) : new SingleLogSource(type)); On 2017/06/01 19:23:46, tbarzic wrote: > use MakeUnique instead of new Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:455: "feedbackPrivate.readLogSource": { On 2017/06/01 19:23:46, tbarzic wrote: > platforms: [chromeos], > session_types:[kiosk] > should be enough; the rest will be inherited from feedbackPrivate Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:100: // /var/log/messages On 2017/06/01 19:23:46, tbarzic wrote: > Chrome OS system messages. Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:103: // /var/log/ui/ui.LATEST On 2017/06/01 19:23:46, tbarzic wrote: > Latest Chrome OS UI logs. Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:104: ui_latest On 2017/06/01 19:23:47, tbarzic wrote: > uiLatest > > or latestUi Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:114: // continue where it left off. readLogSource() can be called with On 2017/06/01 19:23:47, tbarzic wrote: > s/readLogSource()/$(ref:readLogSource)/ Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:115: // incremental=true repeatedly. To subsequently close the file handle, pass On 2017/06/01 19:23:47, tbarzic wrote: > <code>incremental = true</code> Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:132: callback ReadLogSourceCallback = void (long readerId, DOMString[] logLines); On 2017/06/01 19:23:46, tbarzic wrote: > I'd consider returning result as object > { > optional long readerId; > DOMString[] lines; > } > How do I define the object format in the IDL, if at all? https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:157: // - Returns the entire contents of the log file. On 2017/06/01 19:23:47, tbarzic wrote: > put lists in <ul></ul> > > > (so docs render better, you can run > chrome/common/extensions/docs/server2/preview.py and check docs at > http://localhost:8000/extensions/feedbackPrivate) Done. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:165: // If |incremental| is true, and a valid non-zero |readerId| is provided: On 2017/06/01 19:23:47, tbarzic wrote: > <code></code> where you use || Done.
https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:371: SingleLogSourceFactory::CreateSingleLogSource(source_type).release(), On 2017/06/01 21:49:09, Simon Que wrote: > On 2017/06/01 19:23:45, tbarzic wrote: > > I wonder if design here would be simpler if more logic was moved to > > LogSourceManager. > > > > I'm concerned that LogSourceManager exposes too much of it's implementation to > > be usable with resource manager. In particular I don't like > > GetUnregisterCallback being exposed, and LogAccessManager not having control > > over whether the access time actually matches last log source fetch done by > the > > extension. Maybe this could be avoided by moving resource manager handling to > > LogSourceManager? > > > > For example if you introduce something like: > > bool LogSourceManager::FetchResource(int reader_id, SourceAndExtension source, > > callback) { > > int resource_id = > > reader_id > 0 ? reader_id : CreateResource(source); > > if (resource_id <= 0) > > return false; > > resource = resource_manager->Get(source.extension_id, reader_id); > > if (!resource) > > return false; > > > > if (!UpdateSourceAccessTime(source)) > > return false; > > resource->GetLogSource()->Fetch(Bind(&OnLogSourceFetched, callback, > > reader_id)); > > } > > > > Here, there is no need to publicly expose UnregisterCallback (given that > > resource creation is done internally to LogSourceManager); and > LogSourceManager > > does not have to rely on it's client to keep last access time in sync with > last > > resource fetch. > > > > WDYT? > > I like the idea. But your description still leaves some open questions: > - How is LogSourceManager going to access the API resource manager? It will have > to provide a browser context when accessing it. That means LogSourceManager will > have to depend on BrowserContext (which is a possible way to go). Yes, depending on browser context would probably be a good option > - Does UnregisterCallback still get passed to API manager, but only in a context > that is private to LogSourceManager? Yes, provided it's needed. https://codereview.chromium.org/2840103002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:399: reader_id = 0; On 2017/06/01 21:49:09, Simon Que wrote: > On 2017/06/01 19:23:45, tbarzic wrote: > > Instead of changing |reader_id|, can you introduce a variable that will be set > > to the final reader_id value accordingly. > > > > For example: > > int final_reader_id = incremental ? reader_id : -1; > > Respond(ArgumentList( > > feedback_private::ReadLogSource::Results::Create(final_reader_id, > result))); > > > > (also, I think -1 would be better value for "invalid" reader ID) > > Done. > > I'd normally agree with -1 over 0, but 0 is the invalid value as used in (and > returned by) ApiResourceManager. Acknowledged. https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:132: callback ReadLogSourceCallback = void (long readerId, DOMString[] logLines); On 2017/06/01 21:49:10, Simon Que wrote: > On 2017/06/01 19:23:46, tbarzic wrote: > > I'd consider returning result as object > > { > > optional long readerId; > > DOMString[] lines; > > } > > > > How do I define the object format in the IDL, if at all? Same as for ReadLogSourceParams
Now the new FeedbackPrivateReadLogSourceFunction class is just a wrapper around LogSourceAccessManager. It no longer makes sense to have a unit test for each class. Would it make more sense to keep LogSourceAccessManagerTest or FeedbackApiUnittest? The test coverage in the current FeedbackApiUnittest is sufficient, I believe. Currently, I can delete LogSourceAccessManagerTest and keep FeedbackApiUnittest. But since the API function is just a wrapper, you could also make the case that LogSourceAccessManagerTest should be the one to keep. What do you think? https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:132: callback ReadLogSourceCallback = void (long readerId, DOMString[] logLines); On 2017/06/01 21:57:23, tbarzic wrote: > On 2017/06/01 21:49:10, Simon Que wrote: > > On 2017/06/01 19:23:46, tbarzic wrote: > > > I'd consider returning result as object > > > { > > > optional long readerId; > > > DOMString[] lines; > > > } > > > > > > > How do I define the object format in the IDL, if at all? > > Same as for ReadLogSourceParams Done.
On 2017/06/02 22:54:32, Simon Que wrote: > Now the new FeedbackPrivateReadLogSourceFunction class is just a wrapper around > LogSourceAccessManager. It no longer makes sense to have a unit test for each > class. Would it make more sense to keep LogSourceAccessManagerTest or > FeedbackApiUnittest? > > The test coverage in the current FeedbackApiUnittest is sufficient, I believe. > Currently, I can delete LogSourceAccessManagerTest and keep FeedbackApiUnittest. > > But since the API function is just a wrapper, you could also make the case that > LogSourceAccessManagerTest should be the one to keep. > > What do you think? Another option is to keep both: - Move the current test functionality (fine-grained) into LogSourceAccessManagerTest. - FeedbackApiUnittest will have simple test coverage to make sure the wrapper layer is working properly.
On 2017/06/02 22:58:16, Simon Que wrote: > On 2017/06/02 22:54:32, Simon Que wrote: > > Now the new FeedbackPrivateReadLogSourceFunction class is just a wrapper > around > > LogSourceAccessManager. It no longer makes sense to have a unit test for each > > class. Would it make more sense to keep LogSourceAccessManagerTest or > > FeedbackApiUnittest? > > > > The test coverage in the current FeedbackApiUnittest is sufficient, I believe. > > Currently, I can delete LogSourceAccessManagerTest and keep > FeedbackApiUnittest. > > > > But since the API function is just a wrapper, you could also make the case > that > > LogSourceAccessManagerTest should be the one to keep. > > > > What do you think? > > > Another option is to keep both: > - Move the current test functionality (fine-grained) into > LogSourceAccessManagerTest. > - FeedbackApiUnittest will have simple test coverage to make sure the wrapper > layer is working properly. I think keepign just FeedbackApiUnittest should be fine
On 2017/06/02 23:00:34, tbarzic wrote: > On 2017/06/02 22:58:16, Simon Que wrote: > > On 2017/06/02 22:54:32, Simon Que wrote: > > > Now the new FeedbackPrivateReadLogSourceFunction class is just a wrapper > > around > > > LogSourceAccessManager. It no longer makes sense to have a unit test for > each > > > class. Would it make more sense to keep LogSourceAccessManagerTest or > > > FeedbackApiUnittest? > > > > > > The test coverage in the current FeedbackApiUnittest is sufficient, I > believe. > > > Currently, I can delete LogSourceAccessManagerTest and keep > > FeedbackApiUnittest. > > > > > > But since the API function is just a wrapper, you could also make the case > > that > > > LogSourceAccessManagerTest should be the one to keep. > > > > > > What do you think? > > > > > > Another option is to keep both: > > - Move the current test functionality (fine-grained) into > > LogSourceAccessManagerTest. > > - FeedbackApiUnittest will have simple test coverage to make sure the wrapper > > layer is working properly. > > I think keepign just FeedbackApiUnittest should be fine Done.
https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:276: scoped_refptr<FeedbackPrivateReadLogSourceFunction> function = this; you should not need this - just pass |this| to Bind(&OnCompleted) https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:18: #include "base/time/time.h" nit: can you clean these up. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:20: #include "chrome/browser/extensions/api/feedback_private/log_source_access_manager.h" nit: you can forward declare this one https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:36: return log_source_access_manager_.get(); nit: can rename this to GetLockSourceAcceeManager (to be consistent with GetService). (or maybe do it other way around, rename GetService to service() const, and inline it here) https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:40: ~TestSingleLogSource() override {} = default https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:48: CHECK_GT(result.size(), 0U); ASSERT_GT https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:57: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( You can use PostTask instead PostDelayedTask(..., FromMilliseconds(0)); https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( I think you can use RunFunctionAndReturnValue instead of this now. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:204: << "Result not in valid ReadLogSourceResult format: " I think you can use base::Value directly for logging. if (!ReafLogSourceResult::Populate()) { return AssertionFailure() << "Invalid ReadLogSourceResult " << *result_value; } https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:51: } NOTREACHED(); return SingleLogSource::SupportedSource::kMessages; https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:130: const base::Closure& cleanup_callback, instead of passing in cleanup_callback and testing whether it's set, can you pass bool remove_resource. I think it would make it easier to figure out what this method does (i.e. that cleanup_callback removes a key). https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:149: // entry from sources_. Otherwise ApiResourceManager:Remove() will call this this seems very fragile.. Could we avoid having RemoveSource call into ApiResourceManager? (e.g. replace calls to RemoveSource with ApiResourceManager<>::Get()->Remove()) https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:184: weak_factory_.GetWeakPtr(), key); I'd inline this https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:200: auto iter = last_access_times_.find(key); const auto? https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:23: // Provides bookkeepping for the rules of surrounding the use of "Provides bookkeeping for SingleLogSource usage. It ensures that:" https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:42: void set_tick_clock(std::unique_ptr<base::TickClock> clock) { is this supposed to be used in tests only? If so, can you rename it to SetTickClockForTesting https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:48: bool FetchFromSource(api::feedback_private::ReadLogSourceParams& params, nit: const & https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:53: // Every source/extension_id pair is linked to a unique SingleLogSource. nit: this comment seems out of place. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:69: return SourceAndExtension(source, extension_id); nit: you can probably do without this method. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:110: // If it was never accessed by the extension, returns an empty base::Time nit: base::TimeTicks https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:122: // explicitly call AccessSourceFromExtension() at the time of accessing the update the comment :) https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_resource.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_resource.cc:13: ApiResourceManager<LogSourceResource>>>::DestructorAtExit Can this be leaky instead? (see https://cs.chromium.org/chromium/src/base/lazy_instance.h?rcl=66008232510835b...) https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc:23: return std::unique_ptr<SingleLogSource>(g_callback->Run(type)); Can't g_callback return unique_ptr directly? https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:24: "accessibilityPrivate.onTwoFingerTouchStart": { is this intentional? https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:116: // handle, pass in incremental=false. <code>incremental=false</code> https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:120: // that was returned from a previous readLogSource() call. The file handle $(ref:readLogSoruce) https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:131: // <code>incremental=false</false>, this is always set to 0. If the call was s/</false>/</code> https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:169: // <br>If <code>incremental</code> is false: prefer <p></p> over <br> https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:171: // <li>Returns <code>readerId</code> value of 0 to callback.</li></ul> can you format this so it's a bit more readable from the IDL e.g <ul> <li>xxxxxxxx</li> <li> xxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxx </li> </ul> Foo <ul> </ul>
https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:276: scoped_refptr<FeedbackPrivateReadLogSourceFunction> function = this; On 2017/06/06 20:27:25, tbarzic wrote: > you should not need this - just pass |this| to Bind(&OnCompleted) Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:18: #include "base/time/time.h" On 2017/06/06 20:27:25, tbarzic wrote: > nit: can you clean these up. Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:20: #include "chrome/browser/extensions/api/feedback_private/log_source_access_manager.h" On 2017/06/06 20:27:25, tbarzic wrote: > nit: you can forward declare this one Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:36: return log_source_access_manager_.get(); On 2017/06/06 20:27:25, tbarzic wrote: > nit: can rename this to GetLockSourceAcceeManager (to be consistent with > GetService). > > (or maybe do it other way around, rename GetService to service() const, and > inline it here) Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:40: ~TestSingleLogSource() override {} On 2017/06/06 20:27:25, tbarzic wrote: > = default Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:48: CHECK_GT(result.size(), 0U); On 2017/06/06 20:27:25, tbarzic wrote: > ASSERT_GT Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:57: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2017/06/06 20:27:25, tbarzic wrote: > You can use PostTask instead PostDelayedTask(..., FromMilliseconds(0)); Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( On 2017/06/06 20:27:25, tbarzic wrote: > I think you can use RunFunctionAndReturnValue instead of this now. That function still ends up with an EXPECT that the result is valid. https://cs.chromium.org/chromium/src/extensions/browser/api_test_utils.cc?l=138 https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:204: << "Result not in valid ReadLogSourceResult format: " On 2017/06/06 20:27:25, tbarzic wrote: > I think you can use base::Value directly for logging. > > if (!ReafLogSourceResult::Populate()) { > return AssertionFailure() << "Invalid ReadLogSourceResult " << *result_value; > } Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:51: } On 2017/06/06 20:27:26, tbarzic wrote: > NOTREACHED(); > return SingleLogSource::SupportedSource::kMessages; Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:130: const base::Closure& cleanup_callback, On 2017/06/06 20:27:26, tbarzic wrote: > instead of passing in cleanup_callback and testing whether it's set, can you > pass bool remove_resource. I think it would make it easier to figure out what > this method does (i.e. that cleanup_callback removes a key). Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:149: // entry from sources_. Otherwise ApiResourceManager:Remove() will call this On 2017/06/06 20:27:26, tbarzic wrote: > this seems very fragile.. > Could we avoid having RemoveSource call into ApiResourceManager? > (e.g. replace calls to RemoveSource with ApiResourceManager<>::Get()->Remove()) Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:184: weak_factory_.GetWeakPtr(), key); On 2017/06/06 20:27:26, tbarzic wrote: > I'd inline this Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:200: auto iter = last_access_times_.find(key); On 2017/06/06 20:27:26, tbarzic wrote: > const auto? Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.h (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:23: // Provides bookkeepping for the rules of surrounding the use of On 2017/06/06 20:27:26, tbarzic wrote: > "Provides bookkeeping for SingleLogSource usage. It ensures that:" Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:42: void set_tick_clock(std::unique_ptr<base::TickClock> clock) { On 2017/06/06 20:27:26, tbarzic wrote: > is this supposed to be used in tests only? If so, can you rename it to > SetTickClockForTesting Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:48: bool FetchFromSource(api::feedback_private::ReadLogSourceParams& params, On 2017/06/06 20:27:26, tbarzic wrote: > nit: const & Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:53: // Every source/extension_id pair is linked to a unique SingleLogSource. On 2017/06/06 20:27:26, tbarzic wrote: > nit: this comment seems out of place. Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:69: return SourceAndExtension(source, extension_id); On 2017/06/06 20:27:26, tbarzic wrote: > nit: you can probably do without this method. Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:110: // If it was never accessed by the extension, returns an empty base::Time On 2017/06/06 20:27:26, tbarzic wrote: > nit: base::TimeTicks Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.h:122: // explicitly call AccessSourceFromExtension() at the time of accessing the On 2017/06/06 20:27:26, tbarzic wrote: > update the comment :) Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_resource.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_resource.cc:13: ApiResourceManager<LogSourceResource>>>::DestructorAtExit On 2017/06/06 20:27:26, tbarzic wrote: > Can this be leaky instead? > (see > https://cs.chromium.org/chromium/src/base/lazy_instance.h?rcl=66008232510835b...) It doesn't work, results in a crash. DestructorAtExit is used in other instances of ApiResourceManager too, e.g.: https://cs.chromium.org/chromium/src/extensions/browser/api/socket/tcp_socket.cc https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/single_log_source_factory.cc:23: return std::unique_ptr<SingleLogSource>(g_callback->Run(type)); On 2017/06/06 20:27:26, tbarzic wrote: > Can't g_callback return unique_ptr directly? Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:24: "accessibilityPrivate.onTwoFingerTouchStart": { On 2017/06/06 20:27:26, tbarzic wrote: > is this intentional? No. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:116: // handle, pass in incremental=false. On 2017/06/06 20:27:26, tbarzic wrote: > <code>incremental=false</code> Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:120: // that was returned from a previous readLogSource() call. The file handle On 2017/06/06 20:27:26, tbarzic wrote: > $(ref:readLogSoruce) Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:131: // <code>incremental=false</false>, this is always set to 0. If the call was On 2017/06/06 20:27:26, tbarzic wrote: > s/</false>/</code> Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:169: // <br>If <code>incremental</code> is false: On 2017/06/06 20:27:26, tbarzic wrote: > prefer <p></p> over <br> Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/common/extensio... chrome/common/extensions/api/feedback_private.idl:171: // <li>Returns <code>readerId</code> value of 0 to callback.</li></ul> On 2017/06/06 20:27:26, tbarzic wrote: > can you format this so it's a bit more readable from the IDL > > e.g > <ul> > <li>xxxxxxxx</li> > <li> xxxxxxxxxxxxxxxxxxxxxx > xxxxxxxxxxxxx > </li> > </ul> > > Foo > <ul> > </ul> Done.
https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( On 2017/06/06 22:29:14, Simon Que wrote: > On 2017/06/06 20:27:25, tbarzic wrote: > > I think you can use RunFunctionAndReturnValue instead of this now. > > That function still ends up with an EXPECT that the result is valid. > > https://cs.chromium.org/chromium/src/extensions/browser/api_test_utils.cc?l=138 When ApiUnitTest and api_test_utils get refactored and possibly combined, there should be a flag to disable the EXPECTs. If that feature were added For now, I can add a TODO(tbarzic?) to do as you suggested, once that has been done.
https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( On 2017/06/07 01:43:24, Simon Que wrote: > On 2017/06/06 22:29:14, Simon Que wrote: > > On 2017/06/06 20:27:25, tbarzic wrote: > > > I think you can use RunFunctionAndReturnValue instead of this now. > > > > That function still ends up with an EXPECT that the result is valid. > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api_test_utils.cc?l=138 > > When ApiUnitTest and api_test_utils get refactored and possibly combined, there > should be a flag to disable the EXPECTs. If that feature were added > > For now, I can add a TODO(tbarzic?) to do as you suggested, once that has been > done. *If that feature were added, we can remove the local implementation of RunFunctionAndReturnValue().
https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:98: ->SetTestingFactoryAndUse(browser()->profile(), use profile() instead of browser()->profile() https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( On 2017/06/07 01:57:51, Simon Que wrote: > On 2017/06/07 01:43:24, Simon Que wrote: > > On 2017/06/06 22:29:14, Simon Que wrote: > > > On 2017/06/06 20:27:25, tbarzic wrote: > > > > I think you can use RunFunctionAndReturnValue instead of this now. > > > > > > That function still ends up with an EXPECT that the result is valid. > > > > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api_test_utils.cc?l=138 > > > > When ApiUnitTest and api_test_utils get refactored and possibly combined, > there > > should be a flag to disable the EXPECTs. If that feature were added > > > > For now, I can add a TODO(tbarzic?) to do as you suggested, once that has been > > done. > > *If that feature were added, we can remove the local implementation of > RunFunctionAndReturnValue(). That feature kinda already exists - instead of using RunFunctionAnrReturnDictionary, you'd have to use RunFunctionAndReturnError when the function is expected to fail (which would also enable you to test which error was encounterred, not only whether there was an error). https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:83: class FeedbackApiUnittest : public ExtensionApiUnittest { nit: I'd rename this to FeedbackPrivateApiTest https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:397: EXPECT_FALSE( it would be nice if the test distinguished between the function returning an error and the function returning empty logs (this test would pass in both cases)
few more nits and thought on tests, but generally looks good https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:31: return log_source_access_manager_.get(); nit: this should now go to .cc https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:63: #endif // defined(OS_CHROMEOS) nit: add DISALLOW_COPY_AND_ASSIGN(FeedbackPrivateAPI); https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:271: EXPECT_EQ(1, result_reader_id); reader IDs being incremental seems like implementation detail of the ApiResourceManager. I'd consider testing it's > 0 here, and later compare later ID to the one returned here? https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:306: ReadLogSourceParams params1a; Can you make the param var names more meaningful? e.g. first_read_params, repeated_first_read_params, second_read_params, repeated_second_read_params Or maybe do something like: int first_reader_id = -1; { ReadLogParams params; params.source = ... EXPECT_TRUE(RunLogSourceFunction(params, &first_reader_id, result_string)); EXPECT_GT(first_reader_id, 0); } https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:312: params1a.reader_id.reset(new int(result_reader_id)); nit: use base::MakeUnique here. though, I'd prefer to keep this info in a designated variable (e.g. first_reader_id). https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:376: auto FromMilliseconds = &base::TimeDelta::FromMilliseconds; I'd prefer using TimeDelta::FromMilliseconds. (with using base::TimeDelta at the beginning of the file) https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:130: bool delete_source, nit: rename to delete_resource https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:153: auto iter = sources_.find(key); this can be just: sources_.erase(key); https://codereview.chromium.org/2840103002/diff/280001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4128: sources += [ "../browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc" ] I think you can move this to line 3953 (under if (enable_extensions) - chromeos in the name should restrict the file to chromeos automatically)
Hold off on that LGTM, there's a few more corner cases I need to cover... https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:98: ->SetTestingFactoryAndUse(browser()->profile(), On 2017/06/07 03:39:02, tbarzic wrote: > use profile() instead of browser()->profile() Done. https://codereview.chromium.org/2840103002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:167: std::unique_ptr<base::ListValue> RunFunctionAndReturnValue( On 2017/06/07 03:39:02, tbarzic wrote: > On 2017/06/07 01:57:51, Simon Que wrote: > > On 2017/06/07 01:43:24, Simon Que wrote: > > > On 2017/06/06 22:29:14, Simon Que wrote: > > > > On 2017/06/06 20:27:25, tbarzic wrote: > > > > > I think you can use RunFunctionAndReturnValue instead of this now. > > > > > > > > That function still ends up with an EXPECT that the result is valid. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api_test_utils.cc?l=138 > > > > > > When ApiUnitTest and api_test_utils get refactored and possibly combined, > > there > > > should be a flag to disable the EXPECTs. If that feature were added > > > > > > For now, I can add a TODO(tbarzic?) to do as you suggested, once that has > been > > > done. > > > > *If that feature were added, we can remove the local implementation of > > RunFunctionAndReturnValue(). > > That feature kinda already exists - instead of using > RunFunctionAnrReturnDictionary, you'd have to use RunFunctionAndReturnError > when the function is expected to fail (which would also enable you to test which > error was encounterred, not only whether there was an error). Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api.h (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:31: return log_source_access_manager_.get(); On 2017/06/07 17:58:38, tbarzic wrote: > nit: this should now go to .cc Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api.h:63: #endif // defined(OS_CHROMEOS) On 2017/06/07 17:58:38, tbarzic wrote: > nit: add DISALLOW_COPY_AND_ASSIGN(FeedbackPrivateAPI); Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:83: class FeedbackApiUnittest : public ExtensionApiUnittest { On 2017/06/07 03:39:02, tbarzic wrote: > nit: I'd rename this to FeedbackPrivateApiTest Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:271: EXPECT_EQ(1, result_reader_id); On 2017/06/07 17:58:38, tbarzic wrote: > reader IDs being incremental seems like implementation detail of the > ApiResourceManager. > I'd consider testing it's > 0 here, and later compare later ID to the one > returned here? Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:306: ReadLogSourceParams params1a; On 2017/06/07 17:58:38, tbarzic wrote: > Can you make the param var names more meaningful? > e.g. first_read_params, repeated_first_read_params, second_read_params, > repeated_second_read_params > > Or maybe do something like: > > int first_reader_id = -1; > { > ReadLogParams params; > params.source = ... > > EXPECT_TRUE(RunLogSourceFunction(params, &first_reader_id, result_string)); > EXPECT_GT(first_reader_id, 0); > } Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:312: params1a.reader_id.reset(new int(result_reader_id)); On 2017/06/07 17:58:38, tbarzic wrote: > nit: use base::MakeUnique here. > > though, I'd prefer to keep this info in a designated variable (e.g. > first_reader_id). Done. This needs to be stored back into the params so that the same log source reader object can be used in the next call. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:376: auto FromMilliseconds = &base::TimeDelta::FromMilliseconds; On 2017/06/07 17:58:38, tbarzic wrote: > I'd prefer using TimeDelta::FromMilliseconds. > (with using base::TimeDelta at the beginning of the file) Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc:397: EXPECT_FALSE( On 2017/06/07 03:39:02, tbarzic wrote: > it would be nice if the test distinguished between the function returning an > error and the function returning empty logs (this test would pass in both cases) Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:130: bool delete_source, On 2017/06/07 17:58:38, tbarzic wrote: > nit: rename to delete_resource Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:153: auto iter = sources_.find(key); On 2017/06/07 17:58:38, tbarzic wrote: > this can be just: > sources_.erase(key); Done. https://codereview.chromium.org/2840103002/diff/280001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2840103002/diff/280001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4128: sources += [ "../browser/extensions/api/feedback_private/feedback_private_api_chromeos_unittest.cc" ] On 2017/06/07 17:58:39, tbarzic wrote: > I think you can move this to line 3953 (under if (enable_extensions) - chromeos > in the name should restrict the file to chromeos automatically) Done.
The check for max number of active readers per source got lost in the refactor. I have restored it.
Ping, I'd like to get this review done soon, so it can unblock some other things. https://codereview.chromium.org/2840103002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc (right): https://codereview.chromium.org/2840103002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc:66: // TODO(sque): Use std::move? I measured this -- the difference was about 160 ms vs 170 ms, and this part was not a bottleneck either. It's not worth optimizing.
lgtm
rkc, can I get an OWNER approval for chrome/browser/extensions/api/feedback_private/?
The CQ bit was checked by sque@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by sque@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sque@chromium.org changed reviewers: + mpearson@chromium.org - rdevlin.cronin@chromium.org, rkc@chromium.org
mpearson, can I get approval for extensions/browser/extension_function_histogram_value.h?
enums.xml lgtm
The CQ bit was checked by sque@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 sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2840103002/#ps440001 (title: "Add proper ifdefs for non-CrOS platforms")
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": 440001, "attempt_start_ts": 1497389018492230,
"parent_rev": "f3fccd6c63f904e99c987636f981d33044e5a048", "commit_rev":
"055672cc4d3da15292acfc7d6953c6d21e9b3813"}
Message was sent while issue was closed.
Description was changed from ========== Add new API function: feedbackPrivate.readLogSource This function from a single log source (e.g. a log file). If the log source is a log file, this function can continuously read new lines from it. The function also contains mechanisms to prevent excessive log source access. BUG=715263 ========== to ========== Add new API function: feedbackPrivate.readLogSource This function from a single log source (e.g. a log file). If the log source is a log file, this function can continuously read new lines from it. The function also contains mechanisms to prevent excessive log source access. BUG=715263 Review-Url: https://codereview.chromium.org/2840103002 Cr-Commit-Position: refs/heads/master@{#479160} Committed: https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953...
Message was sent while issue was closed.
On 2017/06/13 21:31:16, commit-bot: I haz the power wrote: > Committed patchset #23 (id:440001) as > https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953... Can you please add me as a reviewer of such CLs or at least CC me? I'm trying to review your other change and I'm so out of context because I see a lot of newly-added files that I've never seen before!
Message was sent while issue was closed.
On 2017/06/23 23:26:57, afakhry wrote: > On 2017/06/13 21:31:16, commit-bot: I haz the power wrote: > > Committed patchset #23 (id:440001) as > > > https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953... > > Can you please add me as a reviewer of such CLs or at least CC me? I'm trying to > review your other change and I'm so out of context because I see a lot of > newly-added files that I've never seen before! No problem.
Message was sent while issue was closed.
On 2017/06/23 23:26:57, afakhry wrote: > On 2017/06/13 21:31:16, commit-bot: I haz the power wrote: > > Committed patchset #23 (id:440001) as > > > https://chromium.googlesource.com/chromium/src/+/055672cc4d3da15292acfc7d6953... > > Can you please add me as a reviewer of such CLs or at least CC me? I'm trying to > review your other change and I'm so out of context because I see a lot of > newly-added files that I've never seen before! No problem. |
