Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2037693004:
Change field_trials type to hashes in permission report proto (Closed)
|
Created:
4 years, 6 months ago by stefanocs Modified:
4 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange field_trials type to hashes in permission report proto
BUG=613883
Committed: https://crrev.com/e56277cf28fc073104cbae644fcda0d6607652fc
Cr-Commit-Position: refs/heads/master@{#400564}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 2
Patch Set 4 : Remove origin of FieldTrial comment #Messages
Total messages: 28 (10 generated)
stefanocs@google.com changed reviewers: + kcarattini@chromium.org
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:92: // enrolled in. nit: fill 80 char lines https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:95: // This message is copied from nit you can add a new line above this for a new paragraph https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. Hmm do you know why this is? Is it hard to fix? Should we file a bug about it? https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:99: // The name of the field trial, as a 32-bit identifier. nit: fill 80 chars
https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:92: // enrolled in. On 2016/06/08 03:00:11, raymes wrote: > nit: fill 80 char lines Done. https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:95: // This message is copied from On 2016/06/08 03:00:11, raymes wrote: > nit you can add a new line above this for a new paragraph Done. https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. On 2016/06/08 03:00:11, raymes wrote: > Hmm do you know why this is? Is it hard to fix? Should we file a bug about it? Yes, I discussed this issue with Johan yesterday. We think the problem is that the build system seems to be using a python script located at //src/tools/protoc_wrapper/protoc_wrapper.py to compile the proto files and this script only set the proto_path argument (which is the import path) to the directory of the containing folder and not to the //src. Maybe we should file a bug about this issue. https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:99: // The name of the field trial, as a 32-bit identifier. On 2016/06/08 03:00:11, raymes wrote: > nit: fill 80 chars Done.
https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. On 2016/06/08 03:47:22, stefanocs wrote: > On 2016/06/08 03:00:11, raymes wrote: > > Hmm do you know why this is? Is it hard to fix? Should we file a bug about it? > > > Yes, I discussed this issue with Johan yesterday. We think the problem is that > the build system seems to be using a python script located at > //src/tools/protoc_wrapper/protoc_wrapper.py to compile the proto files and this > script only set the proto_path argument (which is the import path) to the > directory of the containing folder and not to the //src. Maybe we should file a > bug about this issue. Do you think it would be hard to change the script? Maybe we should even just add a hardcoded list of paths to the script for cases like this?
https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/20001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. On 2016/06/09 01:44:17, raymes wrote: > On 2016/06/08 03:47:22, stefanocs wrote: > > On 2016/06/08 03:00:11, raymes wrote: > > > Hmm do you know why this is? Is it hard to fix? Should we file a bug about > it? > > > > > > Yes, I discussed this issue with Johan yesterday. We think the problem is that > > the build system seems to be using a python script located at > > //src/tools/protoc_wrapper/protoc_wrapper.py to compile the proto files and > this > > script only set the proto_path argument (which is the import path) to the > > directory of the containing folder and not to the //src. Maybe we should file > a > > bug about this issue. > > Do you think it would be hard to change the script? Maybe we should even just > add a hardcoded list of paths to the script for cases like this? I have tried a simple fix by adding //src to proto_path, it was able to find the imported file but it wasn't able to build the proto because the generated function name is different. For example when importing "components/metrics/proto/system_profile.proto" in this proto: A forward declaration of a function named "protobuf_AddDesc_components_2fmetrics_2fproto_2fsystem_5fprofile_2eproto" is generated by permission_report.proto. But the function implementation generated by system_profile.proto is named "protobuf_AddDesc_system_5fprofile_2eproto". The cause is possibly because the protos are not compiled within the same level. A more complicated changes to build system might be needed.
You need to fix the typo in both the cl Title and description (trails ->trials).
Description was changed from ========== Change field_trails type to hashes in permission report proto BUG=613883 ========== to ========== Change field_trials type to hashes in permission report proto BUG=613883 ==========
Fixed.
lgtm
lgtm https://codereview.chromium.org/2037693004/diff/40001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. I think we can get rid of this part of the paragraph and just maintain our own fork (for now at least).
https://codereview.chromium.org/2037693004/diff/40001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2037693004/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:97: // since the file cannot be imported by the current build system. On 2016/06/15 02:27:02, raymes wrote: > I think we can get rid of this part of the paragraph and just maintain our own > fork (for now at least). Done.
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2037693004/#ps60001 (title: "Remove origin of FieldTrial comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037693004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
stefanocs@google.com changed reviewers: + nparker@chromium.org
+nparker@chromium.org Hi Nathan, please review changes in the permission proto file. Thanks
lgtm
The CQ bit was checked by stefanocs@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037693004/60001
Message was sent while issue was closed.
Description was changed from ========== Change field_trials type to hashes in permission report proto BUG=613883 ========== to ========== Change field_trials type to hashes in permission report proto BUG=613883 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change field_trials type to hashes in permission report proto BUG=613883 ========== to ========== Change field_trials type to hashes in permission report proto BUG=613883 Committed: https://crrev.com/e56277cf28fc073104cbae644fcda0d6607652fc Cr-Commit-Position: refs/heads/master@{#400564} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e56277cf28fc073104cbae644fcda0d6607652fc Cr-Commit-Position: refs/heads/master@{#400564} |
