HTML UI implementation for the Google Feedback client for Chrome/ChromeOS....

Build fixes to the CL


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57019

[rkc1, 2010-08-09 23:19:18]

I am not quite able to figure out how to send incremental patch messages via
GCL, so here we go; changes done by the patches,
. merged code several times
. cleaned up style issues
. removed the previous (now redundant) BugReport dialog code from Linux and Mac
. added feedback notification for Chrome OS.

[tfarina, 2010-08-10 17:54:28]

http://codereview.chromium.org/3061044/diff/48001/49010
File chrome/browser/dom_ui/bug_report_ui.h (right):

http://codereview.chromium.org/3061044/diff/48001/49010#newcode8
chrome/browser/dom_ui/bug_report_ui.h:8: #include <string>
this and vector are not used.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode11
chrome/browser/dom_ui/bug_report_ui.h:11: #include
"chrome/browser/dom_ui/chrome_url_data_manager.h"
not used.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode13
chrome/browser/dom_ui/bug_report_ui.h:13: #include
"chrome/browser/tab_contents/tab_contents.h"
forward declare this.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode14
chrome/browser/dom_ui/bug_report_ui.h:14: #include "gfx/rect.h"
why do you need this include here?

http://codereview.chromium.org/3061044/diff/48001/49010#newcode17
chrome/browser/dom_ui/bug_report_ui.h:17: class Browser;
this and URLFetcher are not used here.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode23
chrome/browser/dom_ui/bug_report_ui.h:23: private:
add a blank line here.

http://codereview.chromium.org/3061044/diff/48001/49014
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/48001/49014#newcode111
chrome/browser/resources/bug_report.html:111: function $(o) {
please, use the function from resources/shared/js/util.js

http://codereview.chromium.org/3061044/diff/48001/49014#newcode173
chrome/browser/resources/bug_report.html:173: if ((outter_divs[i].id ==
outter_div_id) || ((outter_div_id == '') && (i == 0))) {
line > 80

http://codereview.chromium.org/3061044/diff/48001/49014#newcode188
chrome/browser/resources/bug_report.html:188: outter_div.onclick = (new
event_proxy_2(selectImage, id, outter_div.id)).invoke;
line > 80;

http://codereview.chromium.org/3061044/diff/48001/49014#newcode202
chrome/browser/resources/bug_report.html:202: //  large_image.style.width =
large_image.width * kPopupReduction + 'px';
remove ?

[tfarina, 2010-08-10 17:58:18]

http://codereview.chromium.org/3061044/diff/48001/49026
File chrome/browser/views/bug_report_view.cc (left):

http://codereview.chromium.org/3061044/diff/48001/49026#oldcode1
chrome/browser/views/bug_report_view.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
have you deleted this file? The status shows M instead of D.

[arv (Not doing code reviews), 2010-08-10 18:47:04]

http://codereview.chromium.org/3061044/diff/48001/49014
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/48001/49014#newcode6
chrome/browser/resources/bug_report.html:6: <style type="text/css">
type is by default text/css

http://codereview.chromium.org/3061044/diff/48001/49014#newcode7
chrome/browser/resources/bug_report.html:7: .bug_report_label {
use - for word separators in css

http://codereview.chromium.org/3061044/diff/48001/49014#newcode8
chrome/browser/resources/bug_report.html:8: text-align: left;
text-align: start;

for rtl support

http://codereview.chromium.org/3061044/diff/48001/49014#newcode13
chrome/browser/resources/bug_report.html:13: resize: none;
Why don't we want them to be able to resize this?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode20
chrome/browser/resources/bug_report.html:20: border:none;
Whitespace after :

http://codereview.chromium.org/3061044/diff/48001/49014#newcode27
chrome/browser/resources/bug_report.html:27: .thumbnail_list {
one empty line between each rule

http://codereview.chromium.org/3061044/diff/48001/49014#newcode28
chrome/browser/resources/bug_report.html:28: margin-left:1em;
-webkit-margin-start

http://codereview.chromium.org/3061044/diff/48001/49014#newcode69
chrome/browser/resources/bug_report.html:69: .image_thumbnail img{
missing whitespace before {

http://codereview.chromium.org/3061044/diff/48001/49014#newcode77
chrome/browser/resources/bug_report.html:77: top:130%; left:0%;
one property per line

http://codereview.chromium.org/3061044/diff/48001/49014#newcode78
chrome/browser/resources/bug_report.html:78: padding: 1;
missing unit. This property will be ignored. Either remove or add a unit

http://codereview.chromium.org/3061044/diff/48001/49014#newcode97
chrome/browser/resources/bug_report.html:97: var kIssueWithLabel;
const issueWithLabel = expr;

Constants needs to be assigned immediately, otherwise they are not constants

http://codereview.chromium.org/3061044/diff/48001/49014#newcode105
chrome/browser/resources/bug_report.html:105: // globals
Remove comment, it is obvious that these are globals

http://codereview.chromium.org/3061044/diff/48001/49014#newcode106
chrome/browser/resources/bug_report.html:106: var selected_thumbnail_div = '';
No underscores in JS

http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
chrome/browser/resources/bug_report.html:141: $("issue_with").innerText =
kIssueWithLabel;
use single quotes

http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
chrome/browser/resources/bug_report.html:141: $("issue_with").innerText =
kIssueWithLabel;
Use textContent. innerText has weird whitespace semantics

http://codereview.chromium.org/3061044/diff/48001/49014#newcode149
chrome/browser/resources/bug_report.html:149: for (i = 0; i <
kIssueTypeText.length; i++) {
missing var

http://codereview.chromium.org/3061044/diff/48001/49014#newcode153
chrome/browser/resources/bug_report.html:153: $('issue_with_combo').add(option,
null);
No need to pass null here

http://codereview.chromium.org/3061044/diff/48001/49014#newcode160
chrome/browser/resources/bug_report.html:160: function event_proxy_2(func,
param1, param2) {
Is this a constructor? Then it should be named EventProxy2

http://codereview.chromium.org/3061044/diff/48001/49014#newcode160
chrome/browser/resources/bug_report.html:160: function event_proxy_2(func,
param1, param2) {
Can you add JSDoc here and elsewhere?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode166
chrome/browser/resources/bug_report.html:166: }
missing semicolon

http://codereview.chromium.org/3061044/diff/48001/49014#newcode171
chrome/browser/resources/bug_report.html:171: selected_thumbnail_div = div_id;
id implies a string, div implies an element, so this looks like a type error.
Please rename.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode172
chrome/browser/resources/bug_report.html:172: for (i = 0; i <
outter_divs.length; i++) {
missing var

http://codereview.chromium.org/3061044/diff/48001/49014#newcode173
chrome/browser/resources/bug_report.html:173: if ((outter_divs[i].id ==
outter_div_id) || ((outter_div_id == '') && (i == 0))) {
too many parentheses

http://codereview.chromium.org/3061044/diff/48001/49014#newcode184
chrome/browser/resources/bug_report.html:184: outter_div =
document.createElement('div');
missing var here too

and in a lot of other places. Please go through all you assignments.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode188
chrome/browser/resources/bug_report.html:188: outter_div.onclick = (new
event_proxy_2(selectImage, id, outter_div.id)).invoke;
This can be done much simpler.

outerDiv.onclick = function(e) {
  selectImage(id, outerDiv.id);
};

http://codereview.chromium.org/3061044/diff/48001/49014#newcode203
chrome/browser/resources/bug_report.html:203: large_image.style.width = '500px';
move to CSS?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode210
chrome/browser/resources/bug_report.html:210: if (selected_thumbnail_id == '')
if (!selectedThumbnailId)

http://codereview.chromium.org/3061044/diff/48001/49014#newcode215
chrome/browser/resources/bug_report.html:215: if (screenshots.length < 1)
return;
Please invert the expression. No need for early returns when the function is
this trivial

http://codereview.chromium.org/3061044/diff/48001/49014#newcode220
chrome/browser/resources/bug_report.html:220: if (defaults.length < 1) return;
same here

http://codereview.chromium.org/3061044/diff/48001/49014#newcode226
chrome/browser/resources/bug_report.html:226: alert("Please select an issue
type");
i18n?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode227
chrome/browser/resources/bug_report.html:227: return false;
fix indentation

http://codereview.chromium.org/3061044/diff/48001/49014#newcode229
chrome/browser/resources/bug_report.html:229: if (selected_thumbnail_id != '')
if (selectedThumbnailId)

http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
chrome/browser/resources/bug_report.html:236: chrome.send("sendReport", ["" +
$('issue_with_combo').selectedIndex,
String($('issue_with_combo').selectedIndex)

http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
chrome/browser/resources/bug_report.html:236: chrome.send("sendReport", ["" +
$('issue_with_combo').selectedIndex,
Use single quotes

http://codereview.chromium.org/3061044/diff/48001/49014#newcode267
chrome/browser/resources/bug_report.html:267: <body onload='load();'
onselectstart='return false'>
Do not use window.onload. Use DOMContentReady

http://codereview.chromium.org/3061044/diff/48001/49014#newcode268
chrome/browser/resources/bug_report.html:268: <table style="">
remove empty attribute

http://codereview.chromium.org/3061044/diff/48001/49014#newcode292
chrome/browser/resources/bug_report.html:292: </tr><tr>
fix indentation

move <tr> to a new line

http://codereview.chromium.org/3061044/diff/48001/49014#newcode294
chrome/browser/resources/bug_report.html:294: <textarea id='description_text'
rows="10" maxlength=2000
Remove maxlength?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode295
chrome/browser/resources/bug_report.html:295: wrap="true"
class="bug_report_text"></textarea>
remove wrap?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode304
chrome/browser/resources/bug_report.html:304: value="none"
onClick="noneSelected()">
onclick but maybe move to code?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode311
chrome/browser/resources/bug_report.html:311: <br>
Don't use <br>. Use display block and margins instead

http://codereview.chromium.org/3061044/diff/48001/49014#newcode316
chrome/browser/resources/bug_report.html:316: <td align=left>
Move to CSS

http://codereview.chromium.org/3061044/diff/48001/49014#newcode319
chrome/browser/resources/bug_report.html:319: i18n-values="value:send_report"
onclick="sendReport()"/>
move to js?

http://codereview.chromium.org/3061044/diff/48001/49014#newcode319
chrome/browser/resources/bug_report.html:319: i18n-values="value:send_report"
onclick="sendReport()"/>
Invalid HTML. It should be

<input>

and not

<input/>

http://codereview.chromium.org/3061044/diff/48001/49015
File chrome/browser/resources/bug_report_cros.html (right):

http://codereview.chromium.org/3061044/diff/48001/49015#newcode1
chrome/browser/resources/bug_report_cros.html:1: <!DOCTYPE HTML>
This file looks more or less like a copy pasted version of the other one. Please
use external css and js to share code.

[rkc1, 2010-08-10 18:52:50]

tfarina's review comments incorporated.

http://codereview.chromium.org/3061044/diff/48001/49010
File chrome/browser/dom_ui/bug_report_ui.h (right):

http://codereview.chromium.org/3061044/diff/48001/49010#newcode8
chrome/browser/dom_ui/bug_report_ui.h:8: #include <string>
On 2010/08/10 17:54:28, tfarina wrote:
> this and vector are not used.

Done.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode11
chrome/browser/dom_ui/bug_report_ui.h:11: #include
"chrome/browser/dom_ui/chrome_url_data_manager.h"
On 2010/08/10 17:54:28, tfarina wrote:
> not used.

Done.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode13
chrome/browser/dom_ui/bug_report_ui.h:13: #include
"chrome/browser/tab_contents/tab_contents.h"
On 2010/08/10 17:54:28, tfarina wrote:
> forward declare this.

Done.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode14
chrome/browser/dom_ui/bug_report_ui.h:14: #include "gfx/rect.h"
On 2010/08/10 17:54:28, tfarina wrote:
> why do you need this include here?

Removed.
Done.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode17
chrome/browser/dom_ui/bug_report_ui.h:17: class Browser;
On 2010/08/10 17:54:28, tfarina wrote:
> this and URLFetcher are not used here.

Removed.
Done.

http://codereview.chromium.org/3061044/diff/48001/49010#newcode23
chrome/browser/dom_ui/bug_report_ui.h:23: private:
On 2010/08/10 17:54:28, tfarina wrote:
> add a blank line here.

Done.

http://codereview.chromium.org/3061044/diff/48001/49014
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/48001/49014#newcode111
chrome/browser/resources/bug_report.html:111: function $(o) {
On 2010/08/10 17:54:28, tfarina wrote:
> please, use the function from resources/shared/js/util.js

Changed in both bug_report and bug_report_cros
Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode173
chrome/browser/resources/bug_report.html:173: if ((outter_divs[i].id ==
outter_div_id) || ((outter_div_id == '') && (i == 0))) {
On 2010/08/10 17:54:28, tfarina wrote:
> line > 80

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode188
chrome/browser/resources/bug_report.html:188: outter_div.onclick = (new
event_proxy_2(selectImage, id, outter_div.id)).invoke;
On 2010/08/10 17:54:28, tfarina wrote:
> line > 80;

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode202
chrome/browser/resources/bug_report.html:202: //  large_image.style.width =
large_image.width * kPopupReduction + 'px';
On 2010/08/10 17:54:28, tfarina wrote:
> remove ?

Done.

http://codereview.chromium.org/3061044/diff/48001/49026
File chrome/browser/views/bug_report_view.cc (left):

http://codereview.chromium.org/3061044/diff/48001/49026#oldcode1
chrome/browser/views/bug_report_view.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/08/10 17:58:18, tfarina wrote:
> have you deleted this file? The status shows M instead of D.

svn st shows D on my end; seems like a GCL issue.
Trying again.

Done.

[rkc1, 2010-08-13 01:43:29]

Review comments incorporated; css and js extracted to a common file + overhaul
of the js.

http://codereview.chromium.org/3061044/diff/48001/49014
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/48001/49014#newcode6
chrome/browser/resources/bug_report.html:6: <style type="text/css">
On 2010/08/10 18:47:04, arv wrote:
> type is by default text/css

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode7
chrome/browser/resources/bug_report.html:7: .bug_report_label {
On 2010/08/10 18:47:04, arv wrote:
> use - for word separators in css

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode8
chrome/browser/resources/bug_report.html:8: text-align: left;
On 2010/08/10 18:47:04, arv wrote:
> text-align: start;
> 
> for rtl support

Looked up w3, it says that setting alignment to left aligns left for ltr and
right for rtl. Do we still need to use start?

http://www.w3.org/TR/CSS2/text.html#alignment-prop

http://codereview.chromium.org/3061044/diff/48001/49014#newcode13
chrome/browser/resources/bug_report.html:13: resize: none;
On 2010/08/10 18:47:04, arv wrote:
> Why don't we want them to be able to resize this?

Resizing this field throws off the overall look and feel of the page - it starts
to look weird.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode20
chrome/browser/resources/bug_report.html:20: border:none;
On 2010/08/10 18:47:04, arv wrote:
> Whitespace after :

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode27
chrome/browser/resources/bug_report.html:27: .thumbnail_list {
On 2010/08/10 18:47:04, arv wrote:
> one empty line between each rule

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode28
chrome/browser/resources/bug_report.html:28: margin-left:1em;
On 2010/08/10 18:47:04, arv wrote:
> -webkit-margin-start

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode69
chrome/browser/resources/bug_report.html:69: .image_thumbnail img{
On 2010/08/10 18:47:04, arv wrote:
> missing whitespace before {

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode77
chrome/browser/resources/bug_report.html:77: top:130%; left:0%;
On 2010/08/10 18:47:04, arv wrote:
> one property per line

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode78
chrome/browser/resources/bug_report.html:78: padding: 1;
On 2010/08/10 18:47:04, arv wrote:
> missing unit. This property will be ignored. Either remove or add a unit

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode97
chrome/browser/resources/bug_report.html:97: var kIssueWithLabel;
On 2010/08/10 18:47:04, arv wrote:
> const issueWithLabel = expr;
> 
> Constants needs to be assigned immediately, otherwise they are not constants

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode105
chrome/browser/resources/bug_report.html:105: // globals
On 2010/08/10 18:47:04, arv wrote:
> Remove comment, it is obvious that these are globals

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode106
chrome/browser/resources/bug_report.html:106: var selected_thumbnail_div = '';
On 2010/08/10 18:47:04, arv wrote:
> No underscores in JS

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
chrome/browser/resources/bug_report.html:141: $("issue_with").innerText =
kIssueWithLabel;
On 2010/08/10 18:47:04, arv wrote:
> use single quotes

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
chrome/browser/resources/bug_report.html:141: $("issue_with").innerText =
kIssueWithLabel;
On 2010/08/10 18:47:04, arv wrote:
> Use textContent. innerText has weird whitespace semantics

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode149
chrome/browser/resources/bug_report.html:149: for (i = 0; i <
kIssueTypeText.length; i++) {
On 2010/08/10 18:47:04, arv wrote:
> missing var

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode153
chrome/browser/resources/bug_report.html:153: $('issue_with_combo').add(option,
null);
On 2010/08/10 18:47:04, arv wrote:
> No need to pass null here

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode166
chrome/browser/resources/bug_report.html:166: }
On 2010/08/10 18:47:04, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode171
chrome/browser/resources/bug_report.html:171: selected_thumbnail_div = div_id;
On 2010/08/10 18:47:04, arv wrote:
> id implies a string, div implies an element, so this looks like a type error.
> Please rename.

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode172
chrome/browser/resources/bug_report.html:172: for (i = 0; i <
outter_divs.length; i++) {
On 2010/08/10 18:47:04, arv wrote:
> missing var

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode173
chrome/browser/resources/bug_report.html:173: if ((outter_divs[i].id ==
outter_div_id) || ((outter_div_id == '') && (i == 0))) {
On 2010/08/10 18:47:04, arv wrote:
> too many parentheses

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode184
chrome/browser/resources/bug_report.html:184: outter_div =
document.createElement('div');
On 2010/08/10 18:47:04, arv wrote:
> missing var here too
> 
> and in a lot of other places. Please go through all you assignments.

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode188
chrome/browser/resources/bug_report.html:188: outter_div.onclick = (new
event_proxy_2(selectImage, id, outter_div.id)).invoke;
On 2010/08/10 18:47:04, arv wrote:
> This can be done much simpler.
> 
> outerDiv.onclick = function(e) {
>   selectImage(id, outerDiv.id);
> };

Doesn't work; I've added comments explaining.
Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode203
chrome/browser/resources/bug_report.html:203: large_image.style.width = '500px';
On 2010/08/10 18:47:04, arv wrote:
> move to CSS?

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode210
chrome/browser/resources/bug_report.html:210: if (selected_thumbnail_id == '')
On 2010/08/10 18:47:04, arv wrote:
> if (!selectedThumbnailId)

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode215
chrome/browser/resources/bug_report.html:215: if (screenshots.length < 1)
return;
On 2010/08/10 18:47:04, arv wrote:
> Please invert the expression. No need for early returns when the function is
> this trivial

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode220
chrome/browser/resources/bug_report.html:220: if (defaults.length < 1) return;
On 2010/08/10 18:47:04, arv wrote:
> same here

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode226
chrome/browser/resources/bug_report.html:226: alert("Please select an issue
type");
On 2010/08/10 18:47:04, arv wrote:
> i18n?

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode227
chrome/browser/resources/bug_report.html:227: return false;
On 2010/08/10 18:47:04, arv wrote:
> fix indentation

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode229
chrome/browser/resources/bug_report.html:229: if (selected_thumbnail_id != '')
On 2010/08/10 18:47:04, arv wrote:
> if (selectedThumbnailId)

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
chrome/browser/resources/bug_report.html:236: chrome.send("sendReport", ["" +
$('issue_with_combo').selectedIndex,
On 2010/08/10 18:47:04, arv wrote:
> String($('issue_with_combo').selectedIndex)

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
chrome/browser/resources/bug_report.html:236: chrome.send("sendReport", ["" +
$('issue_with_combo').selectedIndex,
On 2010/08/10 18:47:04, arv wrote:
> Use single quotes

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode267
chrome/browser/resources/bug_report.html:267: <body onload='load();'
onselectstart='return false'>
On 2010/08/10 18:47:04, arv wrote:
> Do not use window.onload. Use DOMContentReady

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode268
chrome/browser/resources/bug_report.html:268: <table style="">
On 2010/08/10 18:47:04, arv wrote:
> remove empty attribute

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode292
chrome/browser/resources/bug_report.html:292: </tr><tr>
On 2010/08/10 18:47:04, arv wrote:
> fix indentation
> 
> move <tr> to a new line

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode294
chrome/browser/resources/bug_report.html:294: <textarea id='description_text'
rows="10" maxlength=2000
On 2010/08/10 18:47:04, arv wrote:
> Remove maxlength?

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode295
chrome/browser/resources/bug_report.html:295: wrap="true"
class="bug_report_text"></textarea>
On 2010/08/10 18:47:04, arv wrote:
> remove wrap?

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode304
chrome/browser/resources/bug_report.html:304: value="none"
onClick="noneSelected()">
On 2010/08/10 18:47:04, arv wrote:
> onclick but maybe move to code?

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode311
chrome/browser/resources/bug_report.html:311: <br>
On 2010/08/10 18:47:04, arv wrote:
> Don't use <br>. Use display block and margins instead

These <br>'s aren't for padding, they're for making sure the divs get pushed to
the next line and align correctly.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode316
chrome/browser/resources/bug_report.html:316: <td align=left>
On 2010/08/10 18:47:04, arv wrote:
> Move to CSS

Done.

http://codereview.chromium.org/3061044/diff/48001/49014#newcode319
chrome/browser/resources/bug_report.html:319: i18n-values="value:send_report"
onclick="sendReport()"/>
On 2010/08/10 18:47:04, arv wrote:
> Invalid HTML. It should be
> 
> <input>
> 
> and not
> 
> <input/>

Done.

http://codereview.chromium.org/3061044/diff/48001/49015
File chrome/browser/resources/bug_report_cros.html (right):

http://codereview.chromium.org/3061044/diff/48001/49015#newcode1
chrome/browser/resources/bug_report_cros.html:1: <!DOCTYPE HTML>
On 2010/08/10 18:47:04, arv wrote:
> This file looks more or less like a copy pasted version of the other one.
Please
> use external css and js to share code.

Done.

[arv1, 2010-08-13 02:00:49]

On Aug 12, 2010 6:43:29 PM PDT, Rahul Chaturvedi <rkc@google.com> wrote:
> Review comments incorporated; css and js extracted to a common file +
> overhaul
> of the js.

>> http://codereview.chromium.org/3061044/diff/48001/49014
>> File chrome/browser/resources/bug_report.html (right):
>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode6
>> chrome/browser/resources/bug_report.html:6:
>> On 2010/08/10 18:47:04, arv wrote:
>> type is by default text/css

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode7
> chrome/browser/resources/bug_report.html:7: .bug_report_label {

>> On 2010/08/10 18:47:04, arv wrote:
>> use - for word separators in css

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode8
> chrome/browser/resources/bug_report.html:8: text-align: left;

>> On 2010/08/10 18:47:04, arv wrote:
>> text-align: start;
>> for rtl support

> Looked up w3, it says that setting alignment to left aligns left for ltr
> and right for rtl. Do we still need to use start?

Yes, left and right does not flip in rtl in WebKit nor in CSS3.

http://www.w3.org/TR/css3-text/#text-align

If you tried it you would have known that they do not flip.

> http://www.w3.org/TR/CSS2/text.html#alignment-prop
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode13
> chrome/browser/resources/bug_report.html:13: resize: none;

>> On 2010/08/10 18:47:04, arv wrote:
>> Why don't we want them to be able to resize this?

> Resizing this field throws off the overall look and feel of the page -
> it starts to look weird.

That's sad.

> http://codereview.chromium.org/3061044/diff/48001/49014#newcode20
> chrome/browser/resources/bug_report.html:20: border:none;
> On 2010/08/10 18:47:04, arv wrote:
> Whitespace after :
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode27
> chrome/browser/resources/bug_report.html:27: .thumbnail_list {

>> On 2010/08/10 18:47:04, arv wrote:
>> one empty line between each rule

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode28
> chrome/browser/resources/bug_report.html:28: margin-left:1em;
> On 2010/08/10 18:47:04, arv wrote:
> -webkit-margin-start
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode69
> chrome/browser/resources/bug_report.html:69: .image_thumbnail img{
> On 2010/08/10 18:47:04, arv wrote:
> missing whitespace before {
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode77
> chrome/browser/resources/bug_report.html:77: top:130%; left:0%;
> On 2010/08/10 18:47:04, arv wrote:
> one property per line
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode78
> chrome/browser/resources/bug_report.html:78: padding: 1;

>> On 2010/08/10 18:47:04, arv wrote:
>> missing unit. This property will be ignored. Either remove or add a
>> unit

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode97
> chrome/browser/resources/bug_report.html:97: var kIssueWithLabel;

>> On 2010/08/10 18:47:04, arv wrote:
>> const issueWithLabel = expr;
>> Constants needs to be assigned immediately, otherwise they are not
>> constants

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode105
> chrome/browser/resources/bug_report.html:105: // globals

>> On 2010/08/10 18:47:04, arv wrote:
>> Remove comment, it is obvious that these are globals

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode106
>> chrome/browser/resources/bug_report.html:106: var selected_thumbnail_div

> On 2010/08/10 18:47:04, arv wrote:
> No underscores in JS
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
>> chrome/browser/resources/bug_report.html:141: $("issue_with").innerText
>> = kIssueWithLabel;

> On 2010/08/10 18:47:04, arv wrote:
> use single quotes
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode141
>> chrome/browser/resources/bug_report.html:141: $("issue_with").innerText
>> = kIssueWithLabel;
>> On 2010/08/10 18:47:04, arv wrote:
>> Use textContent. innerText has weird whitespace semantics

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode149
>> chrome/browser/resources/bug_report.html:149: for (i = 0; i <
>> kIssueTypeText.length; i++) {

> On 2010/08/10 18:47:04, arv wrote:
> missing var
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode153
>> chrome/browser/resources/bug_report.html:153:
>> $('issue_with_combo').add(option, null);

> On 2010/08/10 18:47:04, arv wrote:
> No need to pass null here
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode166
> chrome/browser/resources/bug_report.html:166: }
> On 2010/08/10 18:47:04, arv wrote:
> missing semicolon
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode171
>> chrome/browser/resources/bug_report.html:171: selected_thumbnail_div =
>> div_id;
>> On 2010/08/10 18:47:04, arv wrote:
>> id implies a string, div implies an element, so this looks like a type
>> error.
>> Please rename.

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode172
>> chrome/browser/resources/bug_report.html:172: for (i = 0; i <
>> outter_divs.length; i++) {

> On 2010/08/10 18:47:04, arv wrote:
> missing var

>> Done.
>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode173
>> chrome/browser/resources/bug_report.html:173: if ((outter_divs[i].id ==
>> outter_div_id) || ((outter_div_id == '') && (i == 0))) {

> On 2010/08/10 18:47:04, arv wrote:
> too many parentheses
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode184
>> chrome/browser/resources/bug_report.html:184: outter_div =
>> document.createElement('div');
>> On 2010/08/10 18:47:04, arv wrote:
>> missing var here too
>> and in a lot of other places. Please go through all you assignments.
>> Done.
>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode188
>> chrome/browser/resources/bug_report.html:188: outter_div.onclick = (new
>> event_proxy_2(selectImage, id, outter_div.id)).invoke;
>> On 2010/08/10 18:47:04, arv wrote:
>> This can be done much simpler.
>> outerDiv.onclick = function(e) {
>> selectImage(id, outerDiv.id);

> Doesn't work; I've added comments explaining.
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode203
>> chrome/browser/resources/bug_report.html:203: large_image.style.width =
>> '500px';

> On 2010/08/10 18:47:04, arv wrote:
> move to CSS?
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode210
>> chrome/browser/resources/bug_report.html:210: if (selected_thumbnail_id

> On 2010/08/10 18:47:04, arv wrote:
> if (!selectedThumbnailId)
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode215
>> chrome/browser/resources/bug_report.html:215: if (screenshots.length <
>> 1) return;
>> On 2010/08/10 18:47:04, arv wrote:
>> Please invert the expression. No need for early returns when the
>> function is
>> this trivial

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode220
>> chrome/browser/resources/bug_report.html:220: if (defaults.length < 1)
>> return;

> On 2010/08/10 18:47:04, arv wrote:
> same here
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode226
>> chrome/browser/resources/bug_report.html:226: alert("Please select an
>> issue type");

> On 2010/08/10 18:47:04, arv wrote:
> i18n?
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode227
> chrome/browser/resources/bug_report.html:227: return false;
> On 2010/08/10 18:47:04, arv wrote:
> fix indentation
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode229
>> chrome/browser/resources/bug_report.html:229: if (selected_thumbnail_id

> On 2010/08/10 18:47:04, arv wrote:
> if (selectedThumbnailId)
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
>> chrome/browser/resources/bug_report.html:236: chrome.send("sendReport",
>> ["" + $('issue_with_combo').selectedIndex,
>> On 2010/08/10 18:47:04, arv wrote:
>> String($('issue_with_combo').selectedIndex)

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode236
>> chrome/browser/resources/bug_report.html:236: chrome.send("sendReport",
>> ["" + $('issue_with_combo').selectedIndex,

> On 2010/08/10 18:47:04, arv wrote:
> Use single quotes
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode267
>> chrome/browser/resources/bug_report.html:267: <body onload='load();'
>> onselectstart='return false'>
>> On 2010/08/10 18:47:04, arv wrote:
>> Do not use window.onload. Use DOMContentReady

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode268
> chrome/browser/resources/bug_report.html:268:
> On 2010/08/10 18:47:04, arv wrote:
> remove empty attribute
> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode292
> chrome/browser/resources/bug_report.html:292:

>> On 2010/08/10 18:47:04, arv wrote:
>> fix indentation
>> move to a new line

> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode294
>> chrome/browser/resources/bug_report.html:294: <textarea
>> id='description_text' rows="10" maxlength=2000

> On 2010/08/10 18:47:04, arv wrote:
> Remove maxlength?
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode295
>> chrome/browser/resources/bug_report.html:295: wrap="true"
>> class="bug_report_text">

> On 2010/08/10 18:47:04, arv wrote:
> remove wrap?
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode304
>> chrome/browser/resources/bug_report.html:304: value="none"
>> onClick="noneSelected()">
>> On 2010/08/10 18:47:04, arv wrote:
>> onclick but maybe move to code?

> Done.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode311
> chrome/browser/resources/bug_report.html:311:

>> On 2010/08/10 18:47:04, arv wrote:
>> Don't use
. Use display block and margins instead

> These
's aren't for padding, they're for making sure the divs get
> pushed to the next line and align correctly.
> http://codereview.chromium.org/3061044/diff/48001/49014#newcode316
> chrome/browser/resources/bug_report.html:316:
> On 2010/08/10 18:47:04, arv wrote:
> Move to CSS
> Done.

>> http://codereview.chromium.org/3061044/diff/48001/49014#newcode319
>> chrome/browser/resources/bug_report.html:319:
>> i18n-values="value:send_report" onclick="sendReport()"/>
>> On 2010/08/10 18:47:04, arv wrote:
>> Invalid HTML. It should be

>> and not


> Done.

[arv (Not doing code reviews), 2010-08-13 02:19:51]

http://codereview.chromium.org/3061044/diff/73002/81014
File chrome/browser/resources/bug_report.css (right):

http://codereview.chromium.org/3061044/diff/73002/81014#newcode3
chrome/browser/resources/bug_report.css:3: vertical-align:text-top;
ws after colon

http://codereview.chromium.org/3061044/diff/73002/81014#newcode28
chrome/browser/resources/bug_report.css:28: z-index: 0;
z-index 0 is default

http://codereview.chromium.org/3061044/diff/73002/81014#newcode74
chrome/browser/resources/bug_report.css:74: position: relative;
why relative here?

http://codereview.chromium.org/3061044/diff/73002/81014#newcode82
chrome/browser/resources/bug_report.css:82: left:0%;
left: 0;

http://codereview.chromium.org/3061044/diff/73002/81015
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/73002/81015#newcode74
chrome/browser/resources/bug_report.html:74: <body onselectstart='return false'>
Move to css

body {
  -webkit-user-select: none;
}

http://codereview.chromium.org/3061044/diff/73002/81015#newcode111
chrome/browser/resources/bug_report.html:111: <input id="no-screenshot"
type="radio" name="screenshot-group"
Shouldn't this have a label? How is the user supposed to know what these radio
buttons do?

http://codereview.chromium.org/3061044/diff/73002/81016
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/3061044/diff/73002/81016#newcode77
chrome/browser/resources/bug_report.js:77: (new EventProxy2(selectImage, divId,
thumbnailDiv.id)).invoke;
I still don't understand this. The object itself is never referenced so .param1
and .param2 never changes and func never changes.

A simple closure should be enough here.

thumbnailDiv.onclick = function() {
  selectImage(divId, thumbnailDiv.id);
};

http://codereview.chromium.org/3061044/diff/73002/81016#newcode80
chrome/browser/resources/bug_report.js:80: innerDiv.className =
"image-thumbnail";
use single quotes

http://codereview.chromium.org/3061044/diff/73002/81016#newcode108
chrome/browser/resources/bug_report.js:108: alert(noIssueSelected);
Where did noIssueSelected come from? Can you access localStrings.getString here
directly?

http://codereview.chromium.org/3061044/diff/73002/81016#newcode130
chrome/browser/resources/bug_report.js:130: window.open(FEEDBACK_LANDING_PAGE,
'_blank');
I didn't think window.open worked from DOMUI.

http://codereview.chromium.org/3061044/diff/73002/81017
File chrome/browser/resources/bug_report_cros.html (right):

http://codereview.chromium.org/3061044/diff/73002/81017#newcode19
chrome/browser/resources/bug_report_cros.html:19: localStrings = new
LocalStrings();
missing var for all of these

http://codereview.chromium.org/3061044/diff/73002/81017#newcode20
chrome/browser/resources/bug_report_cros.html:20: issueWithLabel =
localStrings.getString('issue-with');
Can these be moved into the shared js?

http://codereview.chromium.org/3061044/diff/73002/81017#newcode32
chrome/browser/resources/bug_report_cros.html:32: issueTypeText[0] =
localStrings.getString('issue-choose');
Where does this come from?

Can you use an array literal?

http://codereview.chromium.org/3061044/diff/73002/81017#newcode43
chrome/browser/resources/bug_report_cros.html:43: $('issue-with').textContent =
issueWithLabel;
Any reason why these are not done in the markup?

http://codereview.chromium.org/3061044/diff/73002/81017#newcode46
chrome/browser/resources/bug_report_cros.html:46:
$('description-text').textContent = '';
Why?

http://codereview.chromium.org/3061044/diff/73002/81017#newcode85
chrome/browser/resources/bug_report_cros.html:85: // Call load when
DOMContentLoaded is fired
Useless comment

http://codereview.chromium.org/3061044/diff/73002/81017#newcode124
chrome/browser/resources/bug_report_cros.html:124: <span
id="sysinfo-label"></span> <a href='about:blank' id="sysinfo-url"
target="_blank"></a>
long line

http://codereview.chromium.org/3061044/diff/73002/81018
File chrome/browser/resources/bug_report_dev.html (right):

http://codereview.chromium.org/3061044/diff/73002/81018#newcode6
chrome/browser/resources/bug_report_dev.html:6: <style type="text/css">
remove type attribute

http://codereview.chromium.org/3061044/diff/73002/81018#newcode7
chrome/browser/resources/bug_report_dev.html:7: .bug_report_label {
bug-report-label

http://codereview.chromium.org/3061044/diff/73002/81018#newcode7
chrome/browser/resources/bug_report_dev.html:7: .bug_report_label {
Can the CSS be shared?

http://codereview.chromium.org/3061044/diff/73002/81018#newcode70
chrome/browser/resources/bug_report_dev.html:70: <body onload='load();'
onselectstart='return false'>
no such function "load"

[rkc1, 2010-08-13 21:19:06]

Review comments incorporated.

http://codereview.chromium.org/3061044/diff/73002/81014
File chrome/browser/resources/bug_report.css (right):

http://codereview.chromium.org/3061044/diff/73002/81014#newcode3
chrome/browser/resources/bug_report.css:3: vertical-align:text-top;
On 2010/08/13 02:19:51, arv wrote:
> ws after colon

Done.

http://codereview.chromium.org/3061044/diff/73002/81014#newcode28
chrome/browser/resources/bug_report.css:28: z-index: 0;
On 2010/08/13 02:19:51, arv wrote:
> z-index 0 is default

Done.

http://codereview.chromium.org/3061044/diff/73002/81014#newcode74
chrome/browser/resources/bug_report.css:74: position: relative;
On 2010/08/13 02:19:51, arv wrote:
> why relative here?
Not needed, static works fine; removed.

Done.

http://codereview.chromium.org/3061044/diff/73002/81014#newcode82
chrome/browser/resources/bug_report.css:82: left:0%;
On 2010/08/13 02:19:51, arv wrote:
> left: 0;

Done.

http://codereview.chromium.org/3061044/diff/73002/81015
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/3061044/diff/73002/81015#newcode74
chrome/browser/resources/bug_report.html:74: <body onselectstart='return false'>
On 2010/08/13 02:19:51, arv wrote:
> Move to css
> 
> body {
>   -webkit-user-select: none;
> }

Done.

http://codereview.chromium.org/3061044/diff/73002/81015#newcode111
chrome/browser/resources/bug_report.html:111: <input id="no-screenshot"
type="radio" name="screenshot-group"
On 2010/08/13 02:19:51, arv wrote:
> Shouldn't this have a label? How is the user supposed to know what these radio
> buttons do?

We set the text in the load function.

http://codereview.chromium.org/3061044/diff/73002/81016
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/3061044/diff/73002/81016#newcode77
chrome/browser/resources/bug_report.js:77: (new EventProxy2(selectImage, divId,
thumbnailDiv.id)).invoke;
On 2010/08/13 02:19:51, arv wrote:
> I still don't understand this. The object itself is never referenced so
.param1
> and .param2 never changes and func never changes.
> 
> A simple closure should be enough here.
> 
> thumbnailDiv.onclick = function() {
>   selectImage(divId, thumbnailDiv.id);
> };

Fixed, as per our discussion. Works now :)
Done.

http://codereview.chromium.org/3061044/diff/73002/81016#newcode80
chrome/browser/resources/bug_report.js:80: innerDiv.className =
"image-thumbnail";
On 2010/08/13 02:19:51, arv wrote:
> use single quotes

Done.

http://codereview.chromium.org/3061044/diff/73002/81016#newcode108
chrome/browser/resources/bug_report.js:108: alert(noIssueSelected);
On 2010/08/13 02:19:51, arv wrote:
> Where did noIssueSelected come from? Can you access localStrings.getString
here
> directly?

Done.

http://codereview.chromium.org/3061044/diff/73002/81016#newcode130
chrome/browser/resources/bug_report.js:130: window.open(FEEDBACK_LANDING_PAGE,
'_blank');
On 2010/08/13 02:19:51, arv wrote:
> I didn't think window.open worked from DOMUI.

Tested, works now.

http://codereview.chromium.org/3061044/diff/73002/81017
File chrome/browser/resources/bug_report_cros.html (right):

http://codereview.chromium.org/3061044/diff/73002/81017#newcode19
chrome/browser/resources/bug_report_cros.html:19: localStrings = new
LocalStrings();
On 2010/08/13 02:19:51, arv wrote:
> missing var for all of these

Done.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode20
chrome/browser/resources/bug_report_cros.html:20: issueWithLabel =
localStrings.getString('issue-with');
On 2010/08/13 02:19:51, arv wrote:
> Can these be moved into the shared js?

Removed; using the labels directly in markup.
Done.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode32
chrome/browser/resources/bug_report_cros.html:32: issueTypeText[0] =
localStrings.getString('issue-choose');
On 2010/08/13 02:19:51, arv wrote:
> Where does this come from?
> 
> Can you use an array literal?

Pulled from Chrome; the issues can vary in values and positioning between Chrome
and Chrome OS; this helps give good visibility to the order or issues.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode43
chrome/browser/resources/bug_report_cros.html:43: $('issue-with').textContent =
issueWithLabel;
On 2010/08/13 02:19:51, arv wrote:
> Any reason why these are not done in the markup?

Moved to markup.
Done.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode46
chrome/browser/resources/bug_report_cros.html:46:
$('description-text').textContent = '';
On 2010/08/13 02:19:51, arv wrote:
> Why?

For some reason, the default value in the description text seems to be a bunch
of spaces; this initializes it to empty.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode85
chrome/browser/resources/bug_report_cros.html:85: // Call load when
DOMContentLoaded is fired
On 2010/08/13 02:19:51, arv wrote:
> Useless comment

Removed.
Done.

http://codereview.chromium.org/3061044/diff/73002/81017#newcode124
chrome/browser/resources/bug_report_cros.html:124: <span
id="sysinfo-label"></span> <a href='about:blank' id="sysinfo-url"
target="_blank"></a>
On 2010/08/13 02:19:51, arv wrote:
> long line

Done.

http://codereview.chromium.org/3061044/diff/73002/81018
File chrome/browser/resources/bug_report_dev.html (right):

http://codereview.chromium.org/3061044/diff/73002/81018#newcode6
chrome/browser/resources/bug_report_dev.html:6: <style type="text/css">
On 2010/08/13 02:19:51, arv wrote:
> remove type attribute

Done.

http://codereview.chromium.org/3061044/diff/73002/81018#newcode7
chrome/browser/resources/bug_report_dev.html:7: .bug_report_label {
On 2010/08/13 02:19:51, arv wrote:
> bug-report-label

Done.

http://codereview.chromium.org/3061044/diff/73002/81018#newcode7
chrome/browser/resources/bug_report_dev.html:7: .bug_report_label {
On 2010/08/13 02:19:51, arv wrote:
> Can the CSS be shared?

This is a debug display HTML; ideally this could even be done in C++, but
currently I'm leaving this in here in case we want to add in more information in
the future.

Completely stripped the file and left only the essentials.

Done.

http://codereview.chromium.org/3061044/diff/73002/81018#newcode70
chrome/browser/resources/bug_report_dev.html:70: <body onload='load();'
onselectstart='return false'>
On 2010/08/13 02:19:51, arv wrote:
> no such function "load"

Done.

[zel, 2010-08-16 21:26:26]

http://codereview.chromium.org/3061044/diff/103001/50014
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/3061044/diff/103001/50014#newcode779
chrome/browser/app_controller_mac.mm:779: // TODO(rkc): Add code for Mac
do you want to keep the existing code here instead?

http://codereview.chromium.org/3061044/diff/103001/50023
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/3061044/diff/103001/50023#newcode66
chrome/browser/dom_ui/bug_report_ui.cc:66: static const char kAboutSystem[] =
"about:system";
all these and much more is defined in chrome/common/url_constants.h/.cc - please
move all your URLs there

http://codereview.chromium.org/3061044/diff/103001/50023#newcode84
chrome/browser/dom_ui/bug_report_ui.cc:84:
"https://feedback2-test.corp.google.com/tools/feedback/chrome/__submit";
we should get rid of local and test URLs from here

http://codereview.chromium.org/3061044/diff/103001/50023#newcode111
chrome/browser/dom_ui/bug_report_ui.cc:111: saved_screenshots->clear();
DCHECK your inputs

http://codereview.chromium.org/3061044/diff/103001/50036
File chrome/browser/resources/bug_report_test.html (right):

http://codereview.chromium.org/3061044/diff/103001/50036#newcode1
chrome/browser/resources/bug_report_test.html:1: <!DOCTYPE HTML>
what are these bug_report_*.html files for? can we nuke them?

[rkc1, 2010-08-18 17:45:05]

Waiting for old trybot's to finish to start new ones - have individually built
on all platforms as of the last patch.

http://codereview.chromium.org/3061044/diff/103001/50014
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/3061044/diff/103001/50014#newcode779
chrome/browser/app_controller_mac.mm:779: // TODO(rkc): Add code for Mac
On 2010/08/16 21:26:26, zel wrote:
> do you want to keep the existing code here instead?

Done.

http://codereview.chromium.org/3061044/diff/103001/50023
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/3061044/diff/103001/50023#newcode66
chrome/browser/dom_ui/bug_report_ui.cc:66: static const char kAboutSystem[] =
"about:system";
On 2010/08/16 21:26:27, zel wrote:
> all these and much more is defined in chrome/common/url_constants.h/.cc -
please
> move all your URLs there

Done.

http://codereview.chromium.org/3061044/diff/103001/50023#newcode84
chrome/browser/dom_ui/bug_report_ui.cc:84:
"https://feedback2-test.corp.google.com/tools/feedback/chrome/__submit";
On 2010/08/16 21:26:27, zel wrote:
> we should get rid of local and test URLs from here

Done.

http://codereview.chromium.org/3061044/diff/103001/50023#newcode111
chrome/browser/dom_ui/bug_report_ui.cc:111: saved_screenshots->clear();
On 2010/08/16 21:26:27, zel wrote:
> DCHECK your inputs

Do we really need a DCHECK for this input? It comes from our own functions which
sends the input from a local variable's address.

http://codereview.chromium.org/3061044/diff/103001/50036
File chrome/browser/resources/bug_report_test.html (right):

http://codereview.chromium.org/3061044/diff/103001/50036#newcode1
chrome/browser/resources/bug_report_test.html:1: <!DOCTYPE HTML>
On 2010/08/16 21:26:27, zel wrote:
> what are these bug_report_*.html files for? can we nuke them? 

Done.

[zel, 2010-08-18 19:56:37]

http://codereview.chromium.org/3061044/diff/118002/104016
File chrome/browser/resources/bug_report_cros.html (right):

http://codereview.chromium.org/3061044/diff/118002/104016#newcode1
chrome/browser/resources/bug_report_cros.html:1: <!DOCTYPE HTML>
I don't like the fact that we have a completely different file for ChromeOS.
That can be completely avoided by using the same sort of template filtering
mechanism we used for options DOMUI. The following CL probably illustrates this
the best - http://codereview.chromium.org/3190007/show.

[zel, 2010-08-18 20:04:37]

LGTM

you can address me previous concerns (html template merge) in a separate CL as
we agreed

[viettrungluu, 2010-08-19 18:16:59]

http://codereview.chromium.org/3061044/diff/118002/104005
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/3061044/diff/118002/104005#newcode43
chrome/browser/bug_report_util.cc:43: static char const kBugReportPostUrl[] =
Drive-by FYI:
- anonymous namespaces have local (i.e., static) linkage
- non-extern consts in C++ have local linkage

Therefore, "static" is the *third* redundancy here.