Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2189353003:
Sent feedback reports must respect the product ID if any was provided (Closed)
|
Created:
4 years, 4 months ago by afakhry Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_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. |
DescriptionSent feedback reports must respect the product ID if any was provided
The product ID used to be ignored and the reports were sent using a default
value depending on either chrome or chromeos. The product ID has an integer type,
yet in the FeedbackInfo, it was never updated and still had a string type.
This CL fixes these issues.
BUG=629172
TEST=components_unittests --gtest_filter=FeedbackCommonTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/402fadf2aa66df46406f46dbde4d69a93d9a4a7e
Cr-Commit-Position: refs/heads/master@{#409641}
Patch Set 1 #Patch Set 2 : Fix comments #
Total comments: 6
Patch Set 3 : Rahul's comments #Patch Set 4 : Rahul's nit and adding myself to OWNERS #
Total comments: 5
Patch Set 5 : Devlin's comments #
Total comments: 4
Messages
Total messages: 40 (24 generated)
The CQ bit was checked by afakhry@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 checked by afakhry@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...
afakhry@chromium.org changed reviewers: + rkc@chromium.org
Rahul, could you please review? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2189353003/diff/20001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/20001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; Many, if not all, callers of the feedback UI set this to a string. Look at, https://cs.corp.google.com/piper///depot/google3/videoconf/gvc/hotrodapp/appl... You might want to talk to the Feedback team on how to map the product ids that these apps are giving us to what this integer value should have. Most probably the apps will need to populate this with their integer product ID, so we need to let all the whitelisted ones know. https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... File components/feedback/feedback_common.h (right): https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... components/feedback/feedback_common.h:69: bool HasProductId() const { return product_id_ != -1; } Could this be private? We can always move the friend test to private also.
Description was changed from ========== Sent feedback reports must respect the product ID if any was provided The product ID used to be ignored and the reports were sent using a default value depending on either chrome or chromeos. The product ID has an integer type, yet in the FeedbackInfo, it was never updated and still had a string type. This CL fixes these issues. BUG=629172 TEST=components_unittests --gtest_filter=FeedbackCommonTest.* ========== to ========== Sent feedback reports must respect the product ID if any was provided The product ID used to be ignored and the reports were sent using a default value depending on either chrome or chromeos. The product ID has an integer type, yet in the FeedbackInfo, it was never updated and still had a string type. This CL fixes these issues. BUG=629172 TEST=components_unittests --gtest_filter=FeedbackCommonTest.* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by afakhry@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...
https://codereview.chromium.org/2189353003/diff/20001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/20001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; On 2016/07/30 23:46:39, rkc wrote: > Many, if not all, callers of the feedback UI set this to a string. Look at, > https://cs.corp.google.com/piper///depot/google3/videoconf/gvc/hotrodapp/appl... > > You might want to talk to the Feedback team on how to map the product ids that > these apps are giving us to what this integer value should have. > > Most probably the apps will need to populate this with their integer product ID, > so we need to let all the whitelisted ones know. Thanks! As agreed, I added code to validate the type of the productId as an integer before we request to send it from the Feedback UI. I will also send a shout out email to those that use the private API directly shortly. https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... File components/feedback/feedback_common.h (right): https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... components/feedback/feedback_common.h:69: bool HasProductId() const { return product_id_ != -1; } On 2016/07/30 23:46:39, rkc wrote: > Could this be private? We can always move the friend test to private also. Done. I actually moved it to protected, just in case FeedbackData needs to check it?
lgtm % nit https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... File components/feedback/feedback_common.h (right): https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... components/feedback/feedback_common.h:69: bool HasProductId() const { return product_id_ != -1; } On 2016/08/02 00:49:36, afakhry wrote: > On 2016/07/30 23:46:39, rkc wrote: > > Could this be private? We can always move the friend test to private also. > > Done. > I actually moved it to protected, just in case FeedbackData needs to check it? If it does, we can move it to protected then :) We should attempt to maintain the maximum encapsulation we can given the current requirements of the code.
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 afakhry@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...
afakhry@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Rhul: Done. Devlin: Could you please review feedback_private.idl? Thanks! https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... File components/feedback/feedback_common.h (right): https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... components/feedback/feedback_common.h:69: bool HasProductId() const { return product_id_ != -1; } On 2016/08/02 00:57:58, rkc wrote: > On 2016/08/02 00:49:36, afakhry wrote: > > On 2016/07/30 23:46:39, rkc wrote: > > > Could this be private? We can always move the friend test to private also. > > > > Done. > > I actually moved it to protected, just in case FeedbackData needs to check it? > > If it does, we can move it to protected then :) > We should attempt to maintain the maximum encapsulation we can given the current > requirements of the code. Done.
On 2016/08/02 23:56:02, afakhry wrote: > Rhul: Done. Sorry, meant Rahul! :) > > Devlin: Could you please review feedback_private.idl? Thanks! > > https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... > File components/feedback/feedback_common.h (right): > > https://codereview.chromium.org/2189353003/diff/20001/components/feedback/fee... > components/feedback/feedback_common.h:69: bool HasProductId() const { return > product_id_ != -1; } > On 2016/08/02 00:57:58, rkc wrote: > > On 2016/08/02 00:49:36, afakhry wrote: > > > On 2016/07/30 23:46:39, rkc wrote: > > > > Could this be private? We can always move the friend test to private also. > > > > > > Done. > > > I actually moved it to protected, just in case FeedbackData needs to check > it? > > > > If it does, we can move it to protected then :) > > We should attempt to maintain the maximum encapsulation we can given the > current > > requirements of the code. > > Done.
re-lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:258: if (feedback_info.product_id.get()) optional nit: technically, all these .get()s are superfluous, because std::unique_ptr has a bool operator. I'll leave it up to you if you want to leave them in. https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... File chrome/browser/resources/feedback/js/feedback.js (right): https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... chrome/browser/resources/feedback/js/feedback.js:209: // For apps that still use a string value as the |productId|, we must clear Which apps are these? Do they all go through this flow? It looks like there are some whitelisted apps for feedback private, so if any of them aren't updated, the idl change will break them.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc (right): https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:258: if (feedback_info.product_id.get()) On 2016/08/03 18:04:36, Devlin wrote: > optional nit: technically, all these .get()s are superfluous, because > std::unique_ptr has a bool operator. I'll leave it up to you if you want to > leave them in. That's a very good nit. I was actually thinking about doing it, but I preferred to be consistent with the rest of the code here. Now that you mentioned it, done! https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... File chrome/browser/resources/feedback/js/feedback.js (right): https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... chrome/browser/resources/feedback/js/feedback.js:209: // For apps that still use a string value as the |productId|, we must clear On 2016/08/03 18:04:36, Devlin wrote: > Which apps are these? Do they all go through this flow? It looks like there > are some whitelisted apps for feedback private, so if any of them aren't > updated, the idl change will break them. For the whitelisted apps, I sent an alert email to the owners of these apps to inform them of this change. I forwarded that email to you as FYI.
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.
lgtm https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... File chrome/browser/resources/feedback/js/feedback.js (right): https://codereview.chromium.org/2189353003/diff/60001/chrome/browser/resource... chrome/browser/resources/feedback/js/feedback.js:209: // For apps that still use a string value as the |productId|, we must clear On 2016/08/03 20:16:10, afakhry wrote: > On 2016/08/03 18:04:36, Devlin wrote: > > Which apps are these? Do they all go through this flow? It looks like there > > are some whitelisted apps for feedback private, so if any of them aren't > > updated, the idl change will break them. > > For the whitelisted apps, I sent an alert email to the owners of these apps to > inform them of this change. I forwarded that email to you as FYI. Awesome, thanks! https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; Just FYI, we do actually support choices in APIs (in order to be able to say string | long). They're kind of a pain, so if we can be deterministic about it, it's usually better, but just in case you need it in the future.
https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; On 2016/08/03 22:11:50, Devlin wrote: > Just FYI, we do actually support choices in APIs (in order to be able to say > string | long). They're kind of a pain, so if we can be deterministic about it, > it's usually better, but just in case you need it in the future. Ahmed and I had discussed leaving this as a string and just handling it on our side. With the new kiosk permission though, and this API being a candidate, we want to fix this and make it clear to the users that this 'needs' to be an integer. Since this is a purely whitelisted API at this point, it is something we still can change, otherwise we'll forever be stuck with a productId that can be either an integer or a string.
https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; On 2016/08/03 22:11:50, Devlin wrote: > Just FYI, we do actually support choices in APIs (in order to be able to say > string | long). They're kind of a pain, so if we can be deterministic about it, > it's usually better, but just in case you need it in the future. Thanks! Is there any documentation for the supported features in IDL files that I can keep for reference?
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org Link to the patchset: https://codereview.chromium.org/2189353003/#ps80001 (title: "Devlin's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... File chrome/common/extensions/api/feedback_private.idl (right): https://codereview.chromium.org/2189353003/diff/80001/chrome/common/extension... chrome/common/extensions/api/feedback_private.idl:51: long? productId; On 2016/08/03 22:22:40, afakhry wrote: > On 2016/08/03 22:11:50, Devlin wrote: > > Just FYI, we do actually support choices in APIs (in order to be able to say > > string | long). They're kind of a pain, so if we can be deterministic about > it, > > it's usually better, but just in case you need it in the future. > > Thanks! Is there any documentation for the supported features in IDL files that > I can keep for reference? Sadly, no. At some point, I should write some up, but haven't had the time yet.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Sent feedback reports must respect the product ID if any was provided The product ID used to be ignored and the reports were sent using a default value depending on either chrome or chromeos. The product ID has an integer type, yet in the FeedbackInfo, it was never updated and still had a string type. This CL fixes these issues. BUG=629172 TEST=components_unittests --gtest_filter=FeedbackCommonTest.* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Sent feedback reports must respect the product ID if any was provided The product ID used to be ignored and the reports were sent using a default value depending on either chrome or chromeos. The product ID has an integer type, yet in the FeedbackInfo, it was never updated and still had a string type. This CL fixes these issues. BUG=629172 TEST=components_unittests --gtest_filter=FeedbackCommonTest.* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/402fadf2aa66df46406f46dbde4d69a93d9a4a7e Cr-Commit-Position: refs/heads/master@{#409641} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/402fadf2aa66df46406f46dbde4d69a93d9a4a7e Cr-Commit-Position: refs/heads/master@{#409641} |
