Profile Reset dialog: the new section with detailed feedback information

Profile Reset dialog: the new section with detailed feedback information.

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

[vasilii, 2013-09-05 16:15:35]

Hi guys,

please review the CL:
Dominic: everything
Evan: WebUI (or everything optionally)

You can find the screenshots in the bugtracker.

[Evan Stade, 2013-09-06 01:05:37]

lgtm

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/reset_profile_settings_overlay.html
(right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/reset_profile_settings_overlay.html:30: <div
id='expandFeedback'></div>
ids are supposed to be hyphenated

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/reset_profile_settings_overlay.js:42: if
(feedbackTemplate.hidden)
feedbackTemplate.hidden = !feedbackTemplate.hidden;

[battre, 2013-09-06 08:42:45]

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:191:
ASCIIToUTF16(kProfileResetFeedbackBucket));
I would remove this, it is a technicality.

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:249:
(extension_ids += *i)  += ' ';
nit: two spaces before +=

[vasilii, 2013-09-06 12:39:22]

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:191:
ASCIIToUTF16(kProfileResetFeedbackBucket));
On 2013/09/06 08:42:45, battre wrote:
> I would remove this, it is a technicality.

Done.

https://codereview.chromium.org/23450021/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:249:
(extension_ids += *i)  += ' ';
On 2013/09/06 08:42:45, battre wrote:
> nit: two spaces before +=

Done.

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/reset_profile_settings_overlay.html
(right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/reset_profile_settings_overlay.html:30: <div
id='expandFeedback'></div>
On 2013/09/06 01:05:37, Evan Stade wrote:
> ids are supposed to be hyphenated

Done.

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://codereview.chromium.org/23450021/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/reset_profile_settings_overlay.js:42: if
(feedbackTemplate.hidden)
On 2013/09/06 01:05:37, Evan Stade wrote:
> feedbackTemplate.hidden = !feedbackTemplate.hidden;

Done.

[battre, 2013-09-12 14:03:13]

lgtm

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:260:
extension_ids.erase(extension_ids.end() - 1);
do you want to do that for startup_urls as well?

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.h (right):

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.h:19: typedef
std::vector<std::pair<std::string, std::string> > ExtensionList;
Can you please describe the components of the pair? Alternatively you can do the
following:

typedef std::string ExtensionId;
typedef std::string ExtensionName;
typedef std::vector<std::pair<ExtensionId, ExtensionName> > ExtensionList;

[battre, 2013-09-13 08:59:01]

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:155:
list->AppendString(i->first + ";" + i->second);
One more request: Can you strip quotes (") from i->second so that the JSON
parsing becomes easier?

[vasilii, 2013-09-16 13:58:02]

Please check again

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:155:
list->AppendString(i->first + ";" + i->second);
On 2013/09/13 08:59:02, battre wrote:
> One more request: Can you strip quotes (") from i->second so that the JSON
> parsing becomes easier?

Done.

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:260:
extension_ids.erase(extension_ids.end() - 1);
On 2013/09/12 14:03:14, battre wrote:
> do you want to do that for startup_urls as well?

Done.

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.h (right):

https://codereview.chromium.org/23450021/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.h:19: typedef
std::vector<std::pair<std::string, std::string> > ExtensionList;
On 2013/09/12 14:03:14, battre wrote:
> Can you please describe the components of the pair? Alternatively you can do
the
> following:
> 
> typedef std::string ExtensionId;
> typedef std::string ExtensionName;
> typedef std::vector<std::pair<ExtensionId, ExtensionName> > ExtensionList;

Done.

[battre, 2013-09-16 14:25:44]

https://codereview.chromium.org/23450021/diff/30001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/30001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:158:
std::back_inserter(ext), '\"', '\'');
I would prefer this:
std::string& ext_id = i->first;
std::string ext_name;
ReplaceChars(i->second, "\"", "\'", &ext_name);
list->Append(ext_id + ";" + name);

[vasilii, 2013-09-16 14:38:01]

https://codereview.chromium.org/23450021/diff/30001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/resettable_settings_snapshot.cc (right):

https://codereview.chromium.org/23450021/diff/30001/chrome/browser/profile_re...
chrome/browser/profile_resetter/resettable_settings_snapshot.cc:158:
std::back_inserter(ext), '\"', '\'');
On 2013/09/16 14:25:44, battre wrote:
> I would prefer this:
> std::string& ext_id = i->first;
> std::string ext_name;
> ReplaceChars(i->second, "\"", "\'", &ext_name);
> list->Append(ext_id + ";" + name);

Done.

[battre, 2013-09-16 14:41:52]

Thanks. LGTM

[commit-bot: I haz the power, 2013-09-17 07:39:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/23450021/42001

[commit-bot: I haz the power, 2013-09-17 11:52:15]

Change committed as 223580

[Vitaly Pavlenko, 2014-09-08 22:20:31]

vitalyp@chromium.org changed reviewers:
+ vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-08 22:20:32]

https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:85: var input
= new JsEvalContext(feedbackListData);
I'm trying to understand, what does JsEvalContext means at this point? When I
just open chrome://settings, JsEvalContext is undefined. Could you point me to
the place where it's defined?

[Evan Stade, 2014-09-08 22:26:34]

https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:85: var input
= new JsEvalContext(feedbackListData);
On 2014/09/08 22:20:32, Vitaly Pavlenko wrote:
> I'm trying to understand, what does JsEvalContext means at this point? When I
> just open chrome://settings, JsEvalContext is undefined.

that's probably because your console is in the context of the top frame. Try
switching the dropdown right above the console prompt from <top frame> to
settings( settings-frame/ ), or navigate to chrome://settings-frame instead of
chrome://settings.

[Vitaly Pavlenko, 2014-09-08 22:27:29]

On 2014/09/08 22:26:34, Evan Stade wrote:
>
https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
> File chrome/browser/resources/options/reset_profile_settings_overlay.js
(right):
> 
>
https://chromiumcodereview.appspot.com/23450021/diff/42001/chrome/browser/res...
> chrome/browser/resources/options/reset_profile_settings_overlay.js:85: var
input
> = new JsEvalContext(feedbackListData);
> On 2014/09/08 22:20:32, Vitaly Pavlenko wrote:
> > I'm trying to understand, what does JsEvalContext means at this point? When
I
> > just open chrome://settings, JsEvalContext is undefined.
> 
> that's probably because your console is in the context of the top frame. Try
> switching the dropdown right above the console prompt from <top frame> to
> settings( settings-frame/ ), or navigate to chrome://settings-frame instead of
> chrome://settings.

Thank you, that works for me.