Integrate UI and reset logic into AutomaticProfileResetter.

Integrate UI and reset logic into AutomaticProfileResetter.

BUG=298036
NOTRY=true

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

[engedy, 2013-11-06 23:51:07]

Dan, Marc-André, Dominic: Please take a good look, with added focus on object
lifetimes and the somewhat unorthodox way of how the WebUI communicates with the
BrowserContextKeyedService.

Normally, I would say that the Service should not know about the "WebUI" per se
(not even in method names), but in this case, its logic is fundamentally
affected by knowing exactly specific actions are performed by the user on the
WebUI. Still, please let me know if there is a better way to do this.

Regarding tests, I will add unittests for AutomaticProfileResetter and
AutomaticProfileResetterDelegate tomorrow in the next patch set. I was unsure
about what would be the best way to test the changes to
ResetProfileSettingsHandler.

[Dan Beam, 2013-11-07 00:01:17]

lgtm for c/b/ui/webui/

https://codereview.chromium.org/62193002/diff/60001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/60001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:280: if (state_ ==
STATE_AWAITING_USER_CHOICE) {
nit: no curlies here or on L288-230

[engedy, 2013-11-08 02:56:40]

Marc-André, Dominic: the unit tests have arrived, making the patch complete,
apart from some wordsmithing/cleaning up. Please take a look.

There are one or two parts for which I was unsure how add tests (or if we need
at all). Please let me know what you think.

https://codereview.chromium.org/62193002/diff/60001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/60001/chrome/browser/profile_re...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:280: if (state_ ==
STATE_AWAITING_USER_CHOICE) {
On 2013/11/07 00:01:18, Dan Beam wrote:
> nit: no curlies here or on L288-230

Ahh, done.

[engedy, 2013-11-08 02:59:26]

Furthermore, the amount of boilerplate in apr_unittest is becoming somewhat
extreme, I will try to do something about that.

[engedy, 2013-11-10 22:35:48]

@Dominic, PTAL.

[battre, 2013-11-11 02:38:21]

only two nits. LGTM if you fix the shutdown problem. Feel free to ping me if
this requires bigger changes.

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:297: // about it
when the prompt is currently active.
nit: -2 spaces.

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_reset_global_error.cc (right):

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_reset_global_error.cc:22: // The URL
that goes to directly opens the WebUI reset dialog on .
This sentence needs to be fixed.

[MAD, 2013-11-11 13:26:11]

Some minor comments I had about comments on an earlier patch...

Will look at the latest one now...

BYE
MAD

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:53: // action.
Then, however, the user initiated or refrained from (resp.) doing
resp. ???

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:80: // Called by
ProfileResetGlobalError in case the user chooses to reset their
Not sure if we should document here who calls this...

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:100: // dismiss the
menu item in the wrench menu. This should be always followed by
Not sure we need the word "already" here... and I would switch "be always" to
"always be".

[MAD, 2013-11-11 16:38:31]

LGTM with a few format/comments nits...

Thanks!

BYE
MAD

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:297: // about it
when the prompt is currently active.
indent -2

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:59: // already
being shown at the time. Regardless, however, the user initiated
Is another bubble (will always be) the only reason to not show the reset bubble?
If not, we should add e.g., (or for example).

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:145: // can only
possible received (synchronous) call-backs from it here.
"can only possible received"?

please rephrase...

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc
(right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:142:
brandcoded_defaults_fetched_event_.is_signaled())
add {} when condition is more than 1 line.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_reset_global_error.cc (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_reset_global_error.cc:22: // The URL
that goes to directly opens the WebUI reset dialog on .
"dialog on ." ???

[engedy, 2013-11-13 00:40:43]

Addressed all comments, and fixed the shut-down issues, but it turned out to be
some heavy lifting involved.

@Dominic: PTAL.

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:53: // action.
Then, however, the user initiated or refrained from (resp.) doing
On 2013/11/11 13:26:11, MAD wrote:
> resp. ???

Done.

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:80: // Called by
ProfileResetGlobalError in case the user chooses to reset their
On 2013/11/11 13:26:11, MAD wrote:
> Not sure if we should document here who calls this...

Removed, also 4 in instances below.

https://codereview.chromium.org/62193002/diff/290001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:100: // dismiss the
menu item in the wrench menu. This should be always followed by
On 2013/11/11 13:26:11, MAD wrote:
> Not sure we need the word "already" here... and I would switch "be always" to
> "always be".

Done.

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:297: // about it
when the prompt is currently active.
On 2013/11/11 02:38:22, battre wrote:
> nit: -2 spaces.

Done.

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_reset_global_error.cc (right):

https://codereview.chromium.org/62193002/diff/310001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_reset_global_error.cc:22: // The URL
that goes to directly opens the WebUI reset dialog on .
On 2013/11/11 02:38:22, battre wrote:
> This sentence needs to be fixed.

Done.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.cc:297: // about it
when the prompt is currently active.
On 2013/11/11 16:38:32, MAD wrote:
> indent -2

Done.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:59: // already
being shown at the time. Regardless, however, the user initiated
On 2013/11/11 16:38:32, MAD wrote:
> Is another bubble (will always be) the only reason to not show the reset
bubble?
> If not, we should add e.g., (or for example).

For the foreseeable future, yes. Regarding later, I would prefer not adding
uncertainty to comments, and trusting that whomever will modify this invariant
will update this as well.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:145: // can only
possible received (synchronous) call-backs from it here.
On 2013/11/11 16:38:32, MAD wrote:
> "can only possible received"?
> 
> please rephrase... 

Reworked this part.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc
(right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:142:
brandcoded_defaults_fetched_event_.is_signaled())
On 2013/11/11 16:38:32, MAD wrote:
> add {} when condition is more than 1 line.

Hmm. Other code in this directory seems to following a different guidelines. If
I am not mistaken, this is not strictly prescribed by the style guide, so I
would propose to just be consistent here.

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/profile_reset_global_error.cc (right):

https://codereview.chromium.org/62193002/diff/470001/chrome/browser/profile_r...
chrome/browser/profile_resetter/profile_reset_global_error.cc:22: // The URL
that goes to directly opens the WebUI reset dialog on .
On 2013/11/11 16:38:32, MAD wrote:
> "dialog on ." ???

Fixed, sorry.

[battre, 2013-11-13 06:17:11]

still LGTM

https://codereview.chromium.org/62193002/diff/740001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/740001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:148:
STATE_HAS_SHOWN_BUBBLE,
These two names are not 100% clear to me.  what is the difference of the two? I
see three states that the user can be in that can be expressed by these states:
- we have created the GlobalError but the prompt is still waiting for a chance
to show up
- we have started displaying the bubble and we are not saying anything whether
it has been closed
- we have closed the bubble and the process of showing it has concluded.
Maybe you can add a comment here. I think then the names are ok.

[engedy, 2013-11-13 13:24:30]

https://codereview.chromium.org/62193002/diff/740001/chrome/browser/profile_r...
File chrome/browser/profile_resetter/automatic_profile_resetter.h (right):

https://codereview.chromium.org/62193002/diff/740001/chrome/browser/profile_r...
chrome/browser/profile_resetter/automatic_profile_resetter.h:148:
STATE_HAS_SHOWN_BUBBLE,
On 2013/11/13 06:17:11, battre wrote:
> These two names are not 100% clear to me.  what is the difference of the two?
I
> see three states that the user can be in that can be expressed by these
states:
> - we have created the GlobalError but the prompt is still waiting for a chance
> to show up
> - we have started displaying the bubble and we are not saying anything whether
> it has been closed
> - we have closed the bubble and the process of showing it has concluded.
> Maybe you can add a comment here. I think then the names are ok.

Done.

[commit-bot: I haz the power, 2013-11-13 14:59:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/960001

[commit-bot: I haz the power, 2013-11-13 15:41:26]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-13 16:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1220001

[commit-bot: I haz the power, 2013-11-13 16:43:11]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-14 18:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1660001

[commit-bot: I haz the power, 2013-11-14 21:03:11]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-11-14 23:43:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1880001

[commit-bot: I haz the power, 2013-11-15 01:36:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1880001

[commit-bot: I haz the power, 2013-11-15 03:57:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1880001

[commit-bot: I haz the power, 2013-11-15 06:28:56]

Retried try job too often on win_rel for step(s) content_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 07:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1880001

[commit-bot: I haz the power, 2013-11-15 09:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/62193002/1880001

[commit-bot: I haz the power, 2013-11-15 10:09:46]

Change committed as 235309