Attach performance tracing data to feedback reports

Attach performance tracing data to feedback reports

BUG=248357
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216873

[Zachary Kuznia, 2013-08-08 22:40:31]

Please take a look

[rkc, 2013-08-09 21:15:44]

https://codereview.chromium.org/22695004/diff/1/chrome/browser/feedback/feedb...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/feedback/feedb...
chrome/browser/feedback/feedback_data.cc:107: return;
Nit: Document this early return, this confused me a bit.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.html (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.html:80: <input id="performance-info-checkbox"
type="checkbox" checked>
Should this maybe be unchecked to start with?

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:188: console.log(traceId);
Do we want this log? If so, might want to prepend a string describing what it is
that you are logging.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:313: <if expr="pp_ifdef('chromeos')">
We don't need the <if expr.. here, having the extra key in here won't hurt but
the <if looks pretty ugly :)

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:369: <if expr="pp_ifdef('chromeos')">
Same, we can leave this code in non-chromeOS also.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:371: console.log(traceId);
Same as above, either prepend a description to the log, or remove.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
File chrome/browser/ui/webui/feedback_ui.cc (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
chrome/browser/ui/webui/feedback_ui.cc:242: if (TracingManager* manager =
TracingManager::Get()) {
Nit: Add a comment explaining that this if will only be entered if tracing is
turned on.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
chrome/browser/ui/webui/feedback_ui.cc:623: int trace_id =
atoi(trace_id_str.c_str());
Use StringToInt from base/string_number_conversions.h here.

[Zachary Kuznia, 2013-08-09 21:45:28]

https://codereview.chromium.org/22695004/diff/1/chrome/browser/feedback/feedb...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/feedback/feedb...
chrome/browser/feedback/feedback_data.cc:107: return;
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Nit: Document this early return, this confused me a bit.

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.html (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.html:80: <input id="performance-info-checkbox"
type="checkbox" checked>
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Should this maybe be unchecked to start with?

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:188: console.log(traceId);
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Do we want this log? If so, might want to prepend a string describing what it
is
> that you are logging.
Leftover debugging.  Removed.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:313: <if expr="pp_ifdef('chromeos')">
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> We don't need the <if expr.. here, having the extra key in here won't hurt but
> the <if looks pretty ugly :)

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:369: <if expr="pp_ifdef('chromeos')">
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Same, we can leave this code in non-chromeOS also.

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:371: console.log(traceId);
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Same as above, either prepend a description to the log, or remove.

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
File chrome/browser/ui/webui/feedback_ui.cc (right):

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
chrome/browser/ui/webui/feedback_ui.cc:242: if (TracingManager* manager =
TracingManager::Get()) {
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Nit: Add a comment explaining that this if will only be entered if tracing is
> turned on.

Done.

https://codereview.chromium.org/22695004/diff/1/chrome/browser/ui/webui/feedb...
chrome/browser/ui/webui/feedback_ui.cc:623: int trace_id =
atoi(trace_id_str.c_str());
On 2013/08/09 21:15:44, Rahul Chaturvedi wrote:
> Use StringToInt from base/string_number_conversions.h here.

Done.

[rkc, 2013-08-09 21:52:45]

https://codereview.chromium.org/22695004/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/22695004/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/feedback.js:388: if (traceId != 0) {
add a && ($('performance-info-area')) to the if to handle non-ChromeOS builds.

[Zachary Kuznia, 2013-08-09 22:10:12]

https://codereview.chromium.org/22695004/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/22695004/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/feedback.js:388: if (traceId != 0) {
On 2013/08/09 21:52:45, Rahul Chaturvedi wrote:
> add a && ($('performance-info-area')) to the if to handle non-ChromeOS builds.

Done.

[Zachary Kuznia, 2013-08-09 22:12:15]

Hello xiyuan,

Could you do an OWNERS review of this CL?

Thanks,
-Zach

[rkc, 2013-08-09 22:17:34]

lgtm

[xiyuan, 2013-08-09 22:19:58]

chrome/browser/ui/webui/feedback_ui.cc LGTM

[commit-bot: I haz the power, 2013-08-09 22:53:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22695004/24001

[commit-bot: I haz the power, 2013-08-09 23:28:31]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-09 23:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22695004/24001

[commit-bot: I haz the power, 2013-08-09 23:57:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-10 00:23:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22695004/24001

[commit-bot: I haz the power, 2013-08-10 15:41:04]

Change committed as 216873