Provide a better way of injecting extra diagnostics in feedbacks

Provide a better way of injecting extra diagnostics in feedbacks

Instead of adding them to the body of the feedback description, inject them
in the feedback's system information.

BUG=708511
TEST=browser_tests --gtest_filter=FeedbackTest.ExtraDiagnostics

Review-Url: https://codereview.chromium.org/2934303002
Cr-Commit-Position: refs/heads/master@{#479761}
Committed: https://chromium.googlesource.com/chromium/src/+/a28874006914c75dd23d022efc192ecdb9f9213b

[afakhry, 2017-06-14 18:37:54]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 18:38:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-06-14 18:48:47]

afakhry@chromium.org changed reviewers:
+ pkasting@chromium.org

[afakhry, 2017-06-14 18:48:47]

pkasting@ please take a look. Thank you!

[commit-bot: I haz the power, 2017-06-14 19:54:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 19:54:27]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-06-14 20:41:05]

LGTM

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_feedback.cc (right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_feedback.cc:256: description_, "Login",
std::string() /* extra_diagnostics */, GURL(),
Nit: While there's no formal rule up or down on these sorts of comments, I'm not
generally a fan of inserting them if existing code isn't already using them. 
They call attention to the arg when, often, the reader doesn't need to be
thinking about that arg; and it's generally easy to find param names in the
IDE/codesearch.  There's a number of places in this CL that do this; up to you
what to do with this feedback.

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc
(right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc:93: "" /*
extra_diagnostics */);
Nit: Use std::string() in place of "" everywhere

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc:152: //
Makes sure that for feedbacks requested given extra diagnostics, the
Nit: Grammar; how about "Ensures that when extra diagnostics are provided with
feedback, they are injected properly in the system information."?

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:52:
constexpr char kExtraDiagnosticsKey[] = "EXTRA_DIAGNOSTICS";
Nit: I suggest declaring this locally below right where it's used, in keeping
with the sentiment of
http://google.github.io/styleguide/cppguide.html#Local_Variables .

[afakhry, 2017-06-14 22:05:34]

Patchset #2 (id:20001) has been deleted

[afakhry, 2017-06-14 22:10:17]

afakhry@chromium.org changed reviewers:
+ pam@chromium.org, xiyuan@chromium.org

[afakhry, 2017-06-14 22:10:18]

pam@chromium.org: Please review changes in:
  supervised_user_interstitial.cc

xiyuan@chromium.org: Please review changes in:
  login_feedback.cc

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_feedback.cc (right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_feedback.cc:256: description_, "Login",
std::string() /* extra_diagnostics */, GURL(),
On 2017/06/14 20:41:04, Peter Kasting wrote:
> Nit: While there's no formal rule up or down on these sorts of comments, I'm
not
> generally a fan of inserting them if existing code isn't already using them. 
> They call attention to the arg when, often, the reader doesn't need to be
> thinking about that arg; and it's generally easy to find param names in the
> IDE/codesearch.  There's a number of places in this CL that do this; up to you
> what to do with this feedback.

I kept them only in the code that was already using them, and removed them from
the code that doesn't.

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc
(right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc:93: "" /*
extra_diagnostics */);
On 2017/06/14 20:41:04, Peter Kasting wrote:
> Nit: Use std::string() in place of "" everywhere

Done.

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc:152: //
Makes sure that for feedbacks requested given extra diagnostics, the
On 2017/06/14 20:41:04, Peter Kasting wrote:
> Nit: Grammar; how about "Ensures that when extra diagnostics are provided with
> feedback, they are injected properly in the system information."?

Done.

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(right):

https://codereview.chromium.org/2934303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:52:
constexpr char kExtraDiagnosticsKey[] = "EXTRA_DIAGNOSTICS";
On 2017/06/14 20:41:05, Peter Kasting wrote:
> Nit: I suggest declaring this locally below right where it's used, in keeping
> with the sentiment of
> http://google.github.io/styleguide/cppguide.html#Local_Variables .

In that case I don't think we need it at all. I replaced it with a literal.

[xiyuan, 2017-06-14 22:13:04]

login_feedback.cc lgtm

[Pam (message me for reviews), 2017-06-15 16:28:26]

supervised_user_interstitial.cc LGTM

[afakhry, 2017-06-15 17:00:49]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-06-15 17:00:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2934303002/#ps40001
(title: "pkasting's comments")

[commit-bot: I haz the power, 2017-06-15 17:01:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 18:24:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497546049864890,
"parent_rev": "e9450f8c3d9767b43d61e670aa17dde9cda52990", "commit_rev":
"e401574d8204f14e80f89f6854950c57217ec584"}

[commit-bot: I haz the power, 2017-06-15 18:24:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497546049864890,
"parent_rev": "502d45ed84592a1f2a4ea40f0dd06edc370800e3", "commit_rev":
"a28874006914c75dd23d022efc192ecdb9f9213b"}

[commit-bot: I haz the power, 2017-06-15 18:24:52]

Description was changed from

==========
Provide a better way of injecting extra diagnostics in feedbacks

Instead of adding them to the body of the feedback description, inject them
in the feedback's system information.

BUG=708511
TEST=browser_tests --gtest_filter=FeedbackTest.ExtraDiagnostics
==========

to

==========
Provide a better way of injecting extra diagnostics in feedbacks

Instead of adding them to the body of the feedback description, inject them
in the feedback's system information.

BUG=708511
TEST=browser_tests --gtest_filter=FeedbackTest.ExtraDiagnostics

Review-Url: https://codereview.chromium.org/2934303002
Cr-Commit-Position: refs/heads/master@{#479761}
Committed:
https://chromium.googlesource.com/chromium/src/+/a28874006914c75dd23d022efc19...
==========

[commit-bot: I haz the power, 2017-06-15 18:24:55]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a28874006914c75dd23d022efc19...