Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1794513002:
Fix sending multiple feedback reports within short durations of each other (Closed)
|
Created:
4 years, 9 months ago by afakhry Modified:
4 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix sending multiple feedback reports within short durations of each other
Those reports needed to be queued nicely and each report should be sent
with its correct respective system information not the one of the previous
report.
BUG=593452
Test=manually
Committed: https://crrev.com/9c678f28bc8b50041b8aaa700714c75cd02b19f4
Cr-Commit-Position: refs/heads/master@{#381768}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Xiyuan's comments #
Total comments: 2
Patch Set 3 : simplify a lot #
Total comments: 10
Patch Set 4 : more comments. #Patch Set 5 : self revision #
Messages
Total messages: 17 (4 generated)
afakhry@chromium.org changed reviewers: + rkc@chromium.org
Hi Rahul, I implemented a nice solution that works very well for queuing the reports. I hope you will like it. Please take a look.
afakhry@chromium.org changed reviewers: + xiyuan@chromium.org
Xiyuan, Rahul seems to be swamped at the moment. Could you please review this CL? Thanks!
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); It seems to me that we can bind the needed context via base::Bind and get rid of |system_information_callbacks_| and |send_feedback_requests_|. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.h (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.h:35: typedef int64_t RequestId; nit: new style is to use "using" using RequestId = int64; and update existing typedef since you are here. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:88: class FeedbackRequest { Are we allowed to use this ES6 feature? This seems to be the first js file using it. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:110: } Just to double check with you that changes here on |this.feedbackInfo_| would also be visible to |appWindow.contentWindow.feedbackInfo|, i.e. we can share js var instance between here and the inside the app content window. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:143: this.onSystemInfoReadyCallback_ = this.sendReportNow; Think we need to bind here, i.e. this.sendReportNow.bind(this). Otherwise, this.feedbackInfo_ etc is undefined when called from getSystemInformationCallback. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:248: feedbackRequestsMap.set(ID, new FeedbackRequest(ID, feedbackInfo)); It seems |feedbackRequestsMap| could be optimized out. I mean, we use |ID| as part of the closure here, why not use FeedbackRequest instance directly?
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); On 2016/03/14 23:47:28, xiyuan wrote: > It seems to me that we can bind the needed context via base::Bind and get rid of > |system_information_callbacks_| and |send_feedback_requests_|. I agree. Great idea. But I think we can only do this with system_information_callbacks_. For the send requests however, we need a place to store the different feedback_data of each report, that can be referred to and modified by either or both of the corresponding callbacks FeedbackService::AttachedFileCallback() and FeedbackService::ScreenshotCallback(). https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.h (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.h:35: typedef int64_t RequestId; On 2016/03/14 23:47:28, xiyuan wrote: > nit: new style is to use "using" > > using RequestId = int64; > > and update existing typedef since you are here. Done. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:88: class FeedbackRequest { On 2016/03/14 23:47:28, xiyuan wrote: > Are we allowed to use this ES6 feature? This seems to be the first js file using > it. I asked sky@ who referred me to estade@ who referred me to dbeam@, and he told me that yes we are allowed to use this feature on condition that the code doesn't run on iOS, and it passes both the linter and the closure compiler. I ran the closure compiler locally and found that it passes successfully. I'm not sure if it doesn't run on iOS though. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:110: } On 2016/03/14 23:47:28, xiyuan wrote: > Just to double check with you that changes here on |this.feedbackInfo_| would > also be visible to |appWindow.contentWindow.feedbackInfo|, i.e. we can share js > var instance between here and the inside the app content window. Yes I made sure they refer to the same thing, and that when this.feedbackInfo_ changes the changes are seen by the sys info page. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:143: this.onSystemInfoReadyCallback_ = this.sendReportNow; On 2016/03/14 23:47:28, xiyuan wrote: > Think we need to bind here, i.e. this.sendReportNow.bind(this). Otherwise, > this.feedbackInfo_ etc is undefined when called from > getSystemInformationCallback. It works without this bind, since it will be called from this.getSystemInformationCallback() which is bound to the same object. The only bind that I found absolutely necessary is the one in: chrome.feedbackPrivate.getSystemInformation(boundCallback); inside getSystemInformation() above. Since this boundCallback will be called from C++ and needs to be bound to a particular instance of FeedbackRequest. Please let me know if it's really necessary so that I can change it. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:248: feedbackRequestsMap.set(ID, new FeedbackRequest(ID, feedbackInfo)); On 2016/03/14 23:47:28, xiyuan wrote: > It seems |feedbackRequestsMap| could be optimized out. I mean, we use |ID| as > part of the closure here, why not use FeedbackRequest instance directly? Yes, thanks for the suggestion. Done. However, we still need the |feedbackRequestsMap| as we need to keep the various FeedbackRequest instances alive long after their corresponding windows are closed and this closure is removed, until the report is sent.
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); On 2016/03/15 19:25:38, afakhry wrote: > On 2016/03/14 23:47:28, xiyuan wrote: > > It seems to me that we can bind the needed context via base::Bind and get rid > of > > |system_information_callbacks_| and |send_feedback_requests_|. > > I agree. Great idea. But I think we can only do this with > system_information_callbacks_. For the send requests however, we need a place to > store the different feedback_data of each report, that can be referred to and > modified by either or both of the corresponding callbacks > FeedbackService::AttachedFileCallback() and > FeedbackService::ScreenshotCallback(). feedback_data is a ref-counted object. It would be available as long as there is an outstanding reference. base::Bind would cause a reference added to it and FeedbackService::AttachedFileCallback can take it as a scoped_refptr<>. i.e. ... base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), feedback_data)); ... void FeedbackService::AttachedFileCallback( scoped_refptr<FeedbackData> feedback_data, scoped_ptr<std::string> data, int64_t /* total_blob_length */) { ... } https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:88: class FeedbackRequest { On 2016/03/15 19:25:38, afakhry wrote: > On 2016/03/14 23:47:28, xiyuan wrote: > > Are we allowed to use this ES6 feature? This seems to be the first js file > using > > it. > > I asked sky@ who referred me to estade@ who referred me to dbeam@, and he told > me that yes we are allowed to use this feature on condition that the code > doesn't run on iOS, and it passes both the linter and the closure compiler. > > I ran the closure compiler locally and found that it passes successfully. I'm > not sure if it doesn't run on iOS though. Thank you for digging into this. I am fine as long as dbeam@ says okay. https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:143: this.onSystemInfoReadyCallback_ = this.sendReportNow; On 2016/03/15 19:25:38, afakhry wrote: > On 2016/03/14 23:47:28, xiyuan wrote: > > Think we need to bind here, i.e. this.sendReportNow.bind(this). Otherwise, > > this.feedbackInfo_ etc is undefined when called from > > getSystemInformationCallback. > > It works without this bind, since it will be called from > this.getSystemInformationCallback() which is bound to the same object. > > The only bind that I found absolutely necessary is the one in: > chrome.feedbackPrivate.getSystemInformation(boundCallback); > > inside getSystemInformation() above. Since this boundCallback will be called > from C++ and needs to be bound to a particular instance of FeedbackRequest. > > Please let me know if it's really necessary so that I can change it. Interesting. Just tried it and it works as you described. So ignore my comments and no need to change. There is always something to learn in JS. :) https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:248: feedbackRequestsMap.set(ID, new FeedbackRequest(ID, feedbackInfo)); On 2016/03/15 19:25:38, afakhry wrote: > On 2016/03/14 23:47:28, xiyuan wrote: > > It seems |feedbackRequestsMap| could be optimized out. I mean, we use |ID| as > > part of the closure here, why not use FeedbackRequest instance directly? > > Yes, thanks for the suggestion. Done. > > However, we still need the |feedbackRequestsMap| as we need to keep the various > FeedbackRequest instances alive long after their corresponding windows are > closed and this closure is removed, until the report is sent. I don't think we need to manage this explicitly. If the request needs to be kept alive, it means we are waiting for chrome.feedbackPrivate.getSystemInformation to come back. This should be enough to keep the FeedbackRequest instances alive. https://codereview.chromium.org/1794513002/diff/20001/chrome/browser/resource... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/20001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:248: /** @const */ var REQUEST = new FeedbackRequest(ID, feedbackInfo); |REQUEST| is not really a const. Let remove @const annoation and call it |request|.
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); On 2016/03/15 19:54:06, xiyuan wrote: > On 2016/03/15 19:25:38, afakhry wrote: > > On 2016/03/14 23:47:28, xiyuan wrote: > > > It seems to me that we can bind the needed context via base::Bind and get > rid > > of > > > |system_information_callbacks_| and |send_feedback_requests_|. > > > > I agree. Great idea. But I think we can only do this with > > system_information_callbacks_. For the send requests however, we need a place > to > > store the different feedback_data of each report, that can be referred to and > > modified by either or both of the corresponding callbacks > > FeedbackService::AttachedFileCallback() and > > FeedbackService::ScreenshotCallback(). > > feedback_data is a ref-counted object. It would be available as long as there is > an outstanding reference. base::Bind would cause a reference added to it and > FeedbackService::AttachedFileCallback can take it as a scoped_refptr<>. > > i.e. > > ... > base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), > feedback_data)); > ... > > void FeedbackService::AttachedFileCallback( > scoped_refptr<FeedbackData> feedback_data, > scoped_ptr<std::string> data, > int64_t /* total_blob_length */) { > ... > } > Right, right. Sorry I forgot that FeedbackData is RefCounted. This definitely makes it much simpler. FeedbackService doesn't have any state now, we can convert it to a bunch of static methods, and get rid of the SupportsWeakPtr. What do you think? https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/resources/fe... chrome/browser/resources/feedback/js/event_handler.js:248: feedbackRequestsMap.set(ID, new FeedbackRequest(ID, feedbackInfo)); On 2016/03/15 19:54:06, xiyuan wrote: > On 2016/03/15 19:25:38, afakhry wrote: > > On 2016/03/14 23:47:28, xiyuan wrote: > > > It seems |feedbackRequestsMap| could be optimized out. I mean, we use |ID| > as > > > part of the closure here, why not use FeedbackRequest instance directly? > > > > Yes, thanks for the suggestion. Done. > > > > However, we still need the |feedbackRequestsMap| as we need to keep the > various > > FeedbackRequest instances alive long after their corresponding windows are > > closed and this closure is removed, until the report is sent. > > I don't think we need to manage this explicitly. If the request needs to be kept > alive, it means we are waiting for chrome.feedbackPrivate.getSystemInformation > to come back. This should be enough to keep the FeedbackRequest instances alive. Right. I removed the map, and it works. My brain needs to adjust a little to the JS world. https://codereview.chromium.org/1794513002/diff/20001/chrome/browser/resource... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/20001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:248: /** @const */ var REQUEST = new FeedbackRequest(ID, feedbackInfo); On 2016/03/15 19:54:07, xiyuan wrote: > |REQUEST| is not really a const. Let remove @const annoation and call it > |request|. Done.
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); On 2016/03/16 00:45:34, afakhry wrote: > FeedbackService doesn't have any state now, we can > convert it to a bunch of static methods, and get rid of the SupportsWeakPtr. > What do you think? Up to you. I am okay to do it now or in a later CL. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_service.h (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_service.h:10: #include <map> nit: no longer in use. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:90: this.id_ = requestId; This is no longer needed, right? Can we get rid of this and |lastUsedId| etc? https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:163: var that = this; unused? https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:256: var reportIsBeingSent = false; nit: This can be part of FeedbackRequest. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... File chrome/browser/resources/feedback/js/feedback.js (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/feedback.js:186: feedbackInfo.systemInformation = null; It is probably safer to remember useSystemInfo in FeedbackRequest.sendReport in and do this in FeedbackRequest.sendReportNow. Just in case, we clear it too early here while the getSystemInformation call is still in fly.
https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right): https://codereview.chromium.org/1794513002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/feedback_private/feedback_service.cc:76: base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id)); On 2016/03/16 16:39:28, xiyuan wrote: > On 2016/03/16 00:45:34, afakhry wrote: > > FeedbackService doesn't have any state now, we can > > convert it to a bunch of static methods, and get rid of the SupportsWeakPtr. > > What do you think? > > Up to you. I am okay to do it now or in a later CL. Let's keep the scope of this CL small and do it in a followup CL. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_service.h (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_service.h:10: #include <map> On 2016/03/16 16:39:28, xiyuan wrote: > nit: no longer in use. Done. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:90: this.id_ = requestId; On 2016/03/16 16:39:28, xiyuan wrote: > This is no longer needed, right? Can we get rid of this and |lastUsedId| etc? As discussed offline, we use it for logging that the report was sent for Request with this particular ID. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:163: var that = this; On 2016/03/16 16:39:28, xiyuan wrote: > unused? Oops, left overs from testing. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/event_handler.js:256: var reportIsBeingSent = false; On 2016/03/16 16:39:28, xiyuan wrote: > nit: This can be part of FeedbackRequest. Done. https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... File chrome/browser/resources/feedback/js/feedback.js (right): https://codereview.chromium.org/1794513002/diff/40001/chrome/browser/resource... chrome/browser/resources/feedback/js/feedback.js:186: feedbackInfo.systemInformation = null; On 2016/03/16 16:39:28, xiyuan wrote: > It is probably safer to remember useSystemInfo in FeedbackRequest.sendReport in > and do this in FeedbackRequest.sendReportNow. Just in case, we clear it too > early here while the getSystemInformation call is still in fly. Thanks for catching this. Done.
LGTM Thank you for making the changes. :)
lgtm
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1794513002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1794513002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix sending multiple feedback reports within short durations of each other Those reports needed to be queued nicely and each report should be sent with its correct respective system information not the one of the previous report. BUG=593452 Test=manually ========== to ========== Fix sending multiple feedback reports within short durations of each other Those reports needed to be queued nicely and each report should be sent with its correct respective system information not the one of the previous report. BUG=593452 Test=manually Committed: https://crrev.com/9c678f28bc8b50041b8aaa700714c75cd02b19f4 Cr-Commit-Position: refs/heads/master@{#381768} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9c678f28bc8b50041b8aaa700714c75cd02b19f4 Cr-Commit-Position: refs/heads/master@{#381768} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
