UI Revamp + several fixes.

UI Revamp + several fixes.
.) UI matched to the Chrome settings DOM UI
.) User-mail is now unchangeable
.) Changed the screenshot input to a nicer toggle system
.) Saved screenshots are only now loaded when the user choses to see them
.) Check boxes for everything
.) Privacy policy added
.) Text changed around as per request by Chrome OS marketing

BUG=chromium-os:8836, chromium:61847
TEST=Ran the new UI, checking and unchecking various boxes, trying different screenshots and submitting. More tests being done by the feedback team.

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

[oshima, 2010-11-29 19:09:21]

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
chrome/browser/dom_ui/bug_report_ui.cc:65: static const size_t
kMaxSavedScreenshots = 2;
while we're at it,can you change the above static consts
to const in anonymous namespace? e.g:

namespace {
const char kScreenshotBaseUrl[] = "...";
const char ...;
}

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
chrome/browser/dom_ui/bug_report_ui.cc:576: #if defined(OS_CHROMEOS)
Don't you have to "ifdefined" the declaration and usage of this function as
well?

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.html:104: if (defaults[0] == "")
single quote for strings.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.html:109: if (defaults[2] == "") {
same here

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.js:106: userEmail = "";
single quote

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.js:187:
localStrings.getString('choose-original-screenshot'));
This looks a bit hacky. can't this be done via css (like altering visibility)?

[rkc, 2010-11-29 22:41:08]

All done; also fixed an issue with e-mail address handling that our test
engineer found.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
chrome/browser/dom_ui/bug_report_ui.cc:65: static const size_t
kMaxSavedScreenshots = 2;
On 2010/11/29 19:09:22, oshima wrote:
> while we're at it,can you change the above static consts
> to const in anonymous namespace? e.g:
> 
> namespace {
> const char kScreenshotBaseUrl[] = "...";
> const char ...;
> }

Done.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/dom_ui/bug_repor...
chrome/browser/dom_ui/bug_report_ui.cc:576: #if defined(OS_CHROMEOS)
On 2010/11/29 19:09:22, oshima wrote:
> Don't you have to "ifdefined" the declaration and usage of this function as
> well?
Fixed (all other places)

Done.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.html:104: if (defaults[0] == "")
On 2010/11/29 19:09:22, oshima wrote:
> single quote for strings.

Done.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.html:109: if (defaults[2] == "") {
On 2010/11/29 19:09:22, oshima wrote:
> same here

Done.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.js:106: userEmail = "";
On 2010/11/29 19:09:22, oshima wrote:
> single quote

Done.

http://codereview.chromium.org/5271007/diff/1/chrome/browser/resources/bug_re...
chrome/browser/resources/bug_report.js:187:
localStrings.getString('choose-original-screenshot'));
On 2010/11/29 19:09:22, oshima wrote:
> This looks a bit hacky. can't this be done via css (like altering visibility)?

Done.

[oshima, 2010-11-29 23:00:21]

LGTM with following changes. please run trybot so that this won't be reverted.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/dom_ui/bug_re...
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/dom_ui/bug_re...
chrome/browser/dom_ui/bug_report_ui.cc:57: const char kScreenshotBaseUrl[] =
"chrome://screenshots/";
Sorry i wasn't clear. please move this into anonymous namespace. (but without
static)

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:134: from there -->
If this is a copy from other file, please say from which file, and probably
should say this is to reuse options_page.css?

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.js:19: var privacyMode = false;
please add comment and explain what this is.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.js:166:
$('screenshot-label-saved').style.display = 'inline';
can you add TODO to use a class?

[rkc, 2010-11-29 23:15:58]

Done, try bots running.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/dom_ui/bug_re...
File chrome/browser/dom_ui/bug_report_ui.cc (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/dom_ui/bug_re...
chrome/browser/dom_ui/bug_report_ui.cc:57: const char kScreenshotBaseUrl[] =
"chrome://screenshots/";
On 2010/11/29 23:00:22, oshima wrote:
> Sorry i wasn't clear. please move this into anonymous namespace. (but without
> static)

Done.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:134: from there -->
On 2010/11/29 23:00:22, oshima wrote:
> If this is a copy from other file, please say from which file, and probably
> should say this is to reuse options_page.css?

Done.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.js (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.js:19: var privacyMode = false;
On 2010/11/29 23:00:22, oshima wrote:
> please add comment and explain what this is.

Removed, was left over from code I had added then removed.
Done.

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.js:166:
$('screenshot-label-saved').style.display = 'inline';
On 2010/11/29 23:00:22, oshima wrote:
> can you add TODO to use a class?

Done.

[arv (Not doing code reviews), 2010-11-29 23:20:34]

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.css (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.css:170: padding: 0px 0px;
padding: 0;

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
File chrome/browser/resources/bug_report.html (right):

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:38: // maintained individually between
in these two sections.
<del>in</del>

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:80: $('saved-screenshots').innerText =
textContent

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:91: for (i = 0; i < screenshots.length;
++i)
missing var

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:140: <tbody>
thead seems more correct here

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:164: <table
style="-webkit-border-vertical-spacing: 0px;">
move to css

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:168: <table class="bug-report-table">
Do we really need all these tables? tables makes layout hard to maintain

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:171: <input id="page-url-checkbox"
type="checkbox"
Missing label.

All input[type=checkbox] and input[type=radio] must have a label

http://codereview.chromium.org/5271007/diff/7001/chrome/browser/resources/bug...
chrome/browser/resources/bug_report.html:270: <input id="send-report-button"
type="submit"
I prefer <button> for buttons