md-settings: add reset request origin to reset feedback proto.

md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a6e2ff7f9e7249663424e01a3d5aa31c9a8f9ee4
Cr-Commit-Position: refs/heads/master@{#425337}

[alito, 2016-10-13 00:07:21]

Description was changed from

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
==========

to

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[alito, 2016-10-13 16:41:32]

Patchset #1 (id:1) has been deleted

[alito, 2016-10-13 17:36:29]

alito@chromium.org changed reviewers:
+ dbeam@chromium.org

[alito, 2016-10-13 17:36:30]

Dan, PTAL.

This should be one of the final CLs that touches the settings code to set the
reset request origin in the feedback reports. Let me know what you think.

[Dan Beam, 2016-10-13 17:46:05]

Description was changed from

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-10-13 17:46:05]

dbeam@chromium.org changed reviewers:
+ hcarmona@chromium.org

[Dan Beam, 2016-10-13 17:46:38]

hey hcarmona@, can you help alito@ with a review and pass it back to me for
owners if needed?

[hcarmona, 2016-10-13 20:30:20]

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:35:
resetRequestOrigin_: {
Please change to:

resetRequestOrigin: String,

Having a default when it's not used can slow down the page if many are added.
This is because data binding will happen twice (on set default and on set with
actual value)

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:100: var
origin = window.location.hash.slice(1).toLowerCase() == 'cct' ?
Please add a comment explaining why 'cct' is important.

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right):

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:62: if
(!request_origin.empty())
Is an empty request_origin allowed?

[alito, 2016-10-13 21:27:28]

Thanks Hector. I've addressed your comments. PTAnL.

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:35:
resetRequestOrigin_: {
On 2016/10/13 20:30:19, Hector Carmona wrote:
> Please change to:
> 
> resetRequestOrigin: String,
> 
> Having a default when it's not used can slow down the page if many are added.
> This is because data binding will happen twice (on set default and on set with
> actual value)

Done.

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:100: var
origin = window.location.hash.slice(1).toLowerCase() == 'cct' ?
On 2016/10/13 20:30:19, Hector Carmona wrote:
> Please add a comment explaining why 'cct' is important.

Done.

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right):

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:62: if
(!request_origin.empty())
On 2016/10/13 20:30:20, Hector Carmona wrote:
> Is an empty request_origin allowed?

Yes. It signals that the origin is UNKNOWN.

[hcarmona, 2016-10-13 23:37:54]

LGTM

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right):

https://codereview.chromium.org/2411383003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.cc:62: if
(!request_origin.empty())
On 2016/10/13 21:27:28, alito wrote:
> On 2016/10/13 20:30:20, Hector Carmona wrote:
> > Is an empty request_origin allowed?
> 
> Yes. It signals that the origin is UNKNOWN.

Sounds good. :-)

[alito, 2016-10-13 23:44:38]

Thanks Hector!

Dan, I'll try to land this tomorrow, unless you have any comments.

/ali

[alito, 2016-10-13 23:47:03]

alito@chromium.org changed reviewers:
+ sky@chromium.org

[alito, 2016-10-13 23:47:03]

Adding sky@ for startup_browser_creator.cc.

[Dan Beam, 2016-10-13 23:52:01]

lgtm w/nits

https://codereview.chromium.org/2411383003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.h (right):

https://codereview.chromium.org/2411383003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.h:87: std::string
callback_id,
can callback_id be const-ref?

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:247: .then(function() {return
testResetRequestOrigin('');});
nit:

.then(function() { return testResetRequestOrigin(''); });
                  ^                                  ^

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:253: .then(function() {return
testResetRequestOrigin('userclick');});
spaces between { and } here as well

[alito, 2016-10-14 00:28:22]

Thanks Dan. All done.

https://codereview.chromium.org/2411383003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/reset_settings_handler.h (right):

https://codereview.chromium.org/2411383003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/reset_settings_handler.h:87: std::string
callback_id,
On 2016/10/13 23:52:01, Dan Beam wrote:
> can callback_id be const-ref?

AFICT, yes. Changed to const-ref.

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/reset_page_test.js (right):

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:247: .then(function() {return
testResetRequestOrigin('');});
On 2016/10/13 23:52:01, Dan Beam wrote:
> nit:
> 
> .then(function() { return testResetRequestOrigin(''); });
>                   ^                                  ^

Done.

https://codereview.chromium.org/2411383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/reset_page_test.js:253: .then(function() {return
testResetRequestOrigin('userclick');});
On 2016/10/13 23:52:01, Dan Beam wrote:
> spaces between { and } here as well

Done.

[alito, 2016-10-14 00:29:44]

The CQ bit was checked by alito@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 00:30:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 01:48:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 01:48:04]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-14 15:24:37]

LGTM

[alito, 2016-10-14 15:45:27]

The CQ bit was checked by alito@chromium.org

[alito, 2016-10-14 15:45:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2411383003/#ps80001
(title: "Addressed dbeam@'s comments")

[commit-bot: I haz the power, 2016-10-14 15:45:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 15:52:42]

Description was changed from

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-14 15:52:44]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-14 15:55:22]

Description was changed from

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: add reset request origin to reset feedback proto.

BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a6e2ff7f9e7249663424e01a3d5aa31c9a8f9ee4
Cr-Commit-Position: refs/heads/master@{#425337}
==========

[commit-bot: I haz the power, 2016-10-14 15:55:23]

Patchset 4 (id:??) landed as
https://crrev.com/a6e2ff7f9e7249663424e01a3d5aa31c9a8f9ee4
Cr-Commit-Position: refs/heads/master@{#425337}