Refactor and fix feedback

Refactor and fix feedback
This CL does the following things,

.) Removes user-facing categories from the UI completely.
.) Changes categories that are still used (Autofill for example) use category tags instead of the category enum (which has been deprecated)
.) Updates the protocol buffers
.) Refactors the code to change bug report to feedback and re-organizes the file naming/layout
.) Cleans up the unused feedback strings, files and code
.) Cleans up the code at a few places
.) Makes a few UI changes to get closer to the mock given in chromium:98738

This seems like a really huge CL but in reality it just just a couple of hundred lines of 'code' changes; a lot of this is just code being deleted


BUG=chromium:98738
TEST=Tested sending reports.

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

[rkc, 2011-12-22 01:21:39]

Ready for review :)

[tfarina, 2011-12-22 01:47:39]

http://codereview.chromium.org/9006003/diff/1055/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/9006003/diff/1055/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_window_gtk.cc:75: #include
"chrome/browser/ui/webui/feedback_ui.h"
Why do you need to include this here? I've removed bug_report_util.h in another
CL because it's unused in BrowserWindowGtk. And I don't see any usage of
feedback_ui.h here.

[rkc, 2011-12-22 01:54:50]

Removed extra include from browser_window_gtk.cc

http://codereview.chromium.org/9006003/diff/1055/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/9006003/diff/1055/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_window_gtk.cc:75: #include
"chrome/browser/ui/webui/feedback_ui.h"
On 2011/12/22 01:47:40, tfarina wrote:
> Why do you need to include this here? I've removed bug_report_util.h in
another
> CL because it's unused in BrowserWindowGtk. And I don't see any usage of
> feedback_ui.h here.

It came back in with a local merge. Fixed.
Done.

[xiyuan, 2011-12-22 02:18:22]

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_feedback_infobar_delegate.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_feedback_infobar_delegate.cc:16: const char
kCategoryTagAutofill[] = "Autofill";
"static" or anonymous namespace if we don't use it outside this file.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_util.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_util.cc:338: // 
feedback_data.set_category_tag(category_tag);
Is this line needed?

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/prot...
File chrome/browser/feedback/proto/annotations.proto (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/prot...
chrome/browser/feedback/proto/annotations.proto:11: package userfeedback;
do we need to update the package name here as well?

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/views/sad_...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/views/sad_...
chrome/browser/ui/views/sad_tab_view.cc:34: const char kCategoryTagCrash[] =
"Crash";
"static" if we don't use it outside this file.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:17: #include
"chrome/browser/ui/webui/feedback_ui.h"
sort alphabetically.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/feed...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/feed...
chrome/browser/ui/webui/feedback_ui.cc:422: (*i++)->GetAsString(&page_url);
Since we removed all (++i == list_value->end()) check, suggested to add some
check at function entry. Just in case js code is messed up, this would help to
pin-point the error.

if (list_value.GetSize() < 5) {
  LOG(ERROR) << "...";
  return;
}

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.cc:29: const char kChromeUIFeedbackURL[] =
"chrome://feedback/";
ditto.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.cc:126: const char kChromeUIFeedbackHost[] =
"feedback";
ditto.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.h:24: extern const char kChromeUIFeedbackURL[];
sort alphabetically.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.h:115: extern const char kChromeUIFeedbackHost[];
sort alphabetically.

[tfarina, 2011-12-22 02:20:43]

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_feedback_infobar_delegate.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_feedback_infobar_delegate.cc:16: const char
kCategoryTagAutofill[] = "Autofill";
On 2011/12/22 02:18:23, xiyuan wrote:
> "static" or anonymous namespace if we don't use it outside this file.

const has internal linkage so it doesn't need either static or unnamed namespace
in C++. If still you want to go ahead, wrap it into an unnamed namespace.

namespace {
...
}  // namespace

[rkc, 2011-12-22 22:41:45]

Review changes incorporated and TestBugReportPage browser test renamed and
fixed.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_feedback_infobar_delegate.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_feedback_infobar_delegate.cc:16: const char
kCategoryTagAutofill[] = "Autofill";
On 2011/12/22 02:20:43, tfarina wrote:
> On 2011/12/22 02:18:23, xiyuan wrote:
> > "static" or anonymous namespace if we don't use it outside this file.
> 
> const has internal linkage so it doesn't need either static or unnamed
namespace
> in C++. If still you want to go ahead, wrap it into an unnamed namespace.
> 
> namespace {
> ...
> }  // namespace

Confirmed that the C++ standard does specify internal linkage for const; also
tested on gcc to confirm so leaving it as is.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_util.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_util.cc:338: // 
feedback_data.set_category_tag(category_tag);
On 2011/12/22 02:18:23, xiyuan wrote:
> Is this line needed?
Oops. Yes, just need to check to see if a category tag was specified. Category
tags are still needed for things like Autofill and crash feedback.

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/prot...
File chrome/browser/feedback/proto/annotations.proto (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/feedback/prot...
chrome/browser/feedback/proto/annotations.proto:11: package userfeedback;
On 2011/12/22 02:18:23, xiyuan wrote:
> do we need to update the package name here as well?

This package name comes from the protobuf on the backend, which is why I left it
as is. The protobuf files are as close to their backend counterparts as possible
with only the completely unused optional fields removed.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/views/sad_...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/views/sad_...
chrome/browser/ui/views/sad_tab_view.cc:34: const char kCategoryTagCrash[] =
"Crash";
On 2011/12/22 02:18:23, xiyuan wrote:
> "static" if we don't use it outside this file.

const = internal linkage, static is not needed.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:17: #include
"chrome/browser/ui/webui/feedback_ui.h"
On 2011/12/22 02:18:23, xiyuan wrote:
> sort alphabetically.

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:169: if (url.host() ==
chrome::kChromeUIFeedbackHost)
Sorted this alphabetically also.

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/feed...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/browser/ui/webui/feed...
chrome/browser/ui/webui/feedback_ui.cc:422: (*i++)->GetAsString(&page_url);
On 2011/12/22 02:18:23, xiyuan wrote:
> Since we removed all (++i == list_value->end()) check, suggested to add some
> check at function entry. Just in case js code is messed up, this would help to
> pin-point the error.
> 
> if (list_value.GetSize() < 5) {
>   LOG(ERROR) << "...";
>   return;
> }

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.cc:29: const char kChromeUIFeedbackURL[] =
"chrome://feedback/";
On 2011/12/22 02:18:23, xiyuan wrote:
> ditto.

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.cc:126: const char kChromeUIFeedbackHost[] =
"feedback";
On 2011/12/22 02:18:23, xiyuan wrote:
> ditto.

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.h:24: extern const char kChromeUIFeedbackURL[];
On 2011/12/22 02:18:23, xiyuan wrote:
> sort alphabetically.

Done.

http://codereview.chromium.org/9006003/diff/3004/chrome/common/url_constants....
chrome/common/url_constants.h:115: extern const char kChromeUIFeedbackHost[];
On 2011/12/22 02:18:23, xiyuan wrote:
> sort alphabetically.

Done.

[xiyuan, 2011-12-22 22:50:28]

LGTM

Think you also need blessing from browser/resources/OWNERS.

[rkc, 2011-12-28 23:34:57]

Added all three owners from browser/resources in case one logs on during the
holidays :)

On 2011/12/22 22:50:28, xiyuan wrote:
> LGTM
> 
> Think you also need blessing from browser/resources/OWNERS.

[James Hawkins, 2012-01-01 00:55:01]

LGTM with some feedback. *ahem*.

http://codereview.chromium.org/9006003/diff/11001/chrome/browser/resources/fe...
File chrome/browser/resources/feedback.css (right):

http://codereview.chromium.org/9006003/diff/11001/chrome/browser/resources/fe...
chrome/browser/resources/feedback.css:169: padding-top: 24px;
Not going to ask it in this CL, but these properties need to be alphabetized.

http://codereview.chromium.org/9006003/diff/11001/chrome/browser/ui/webui/fee...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/9006003/diff/11001/chrome/browser/ui/webui/fee...
chrome/browser/ui/webui/feedback_ui.cc:420: if (list_value->GetSize() != 6) {
Ick, this seems wrong.  Please add a TODO to figure out a cleaner way to handle
this.

[commit-bot: I haz the power, 2012-01-05 04:39:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/9006003/21001

[commit-bot: I haz the power, 2012-01-05 04:39:20]

Can't process patch for file chrome/browser/resources/bug_report_invalid.html.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-01-05 05:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/9006003/28003

[commit-bot: I haz the power, 2012-01-05 05:01:36]

Can't process patch for file chrome/browser/resources/bug_report_invalid.html.
File's status is None, patchset upload is incomplete.

[rkc, 2012-01-05 22:17:12]

Added sky@ for the browser/ui OWNERS review.

[sky, 2012-01-05 22:20:15]

LGTM

http://codereview.chromium.org/9006003/diff/28003/chrome/browser/ui/webui/fee...
File chrome/browser/ui/webui/feedback_ui.h (right):

http://codereview.chromium.org/9006003/diff/28003/chrome/browser/ui/webui/fee...
chrome/browser/ui/webui/feedback_ui.h:17: const std::string& category_tag);
nit: fix spacing on lines 16/17.