Add a checkbox on stop-sync dialog to allow user to choose whether to delete

Add a checkbox on stop-sync dialog to allow user to choose whether to delete
all their data(profile), too. If checked, user will be prompted again to
confirm profile deletion.

BUG=363272
NOTRY=true

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

[Nicolas Zea, 2014-04-24 21:33:16]

You'll need an owner to stamp btw, as I don't have ownership for the UI code

https://codereview.chromium.org/235943011/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/235943011/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:87:
chrome.send('SyncSetupStopSyncing', [deleteProfile]);
It seems odd that we're calling into the C++ code to have it trigger a
javascript method. Can we not invoke that from here if the box was checked?

Also, why send an array vs just the variable?

[haitaol1, 2014-04-25 18:19:04]

https://codereview.chromium.org/235943011/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/235943011/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:87:
chrome.send('SyncSetupStopSyncing', [deleteProfile]);
On 2014/04/24 21:33:17, Nicolas Zea wrote:
> It seems odd that we're calling into the C++ code to have it trigger a
> javascript method. Can we not invoke that from here if the box was checked?

That was my first try but it didn't work because signin manager refreshes
setting page when handling signing out, which destroys the profile-deletion
confirmation popup.

> 
> Also, why send an array vs just the variable?

The message callback handler requires list argument.

[Nicolas Zea, 2014-04-25 19:47:10]

LGTM from a sync perspective then

[haitaol1, 2014-04-25 19:59:44]

Roger: please review chrome/browser/ui/webui/sync_setup_handler.cc

Raghu: please review
chrome/browser/resources/sync_setup_overlay*

James: please review
chrome/browser/resources/options/browser_options.js

[Nicolas Zea, 2014-04-25 20:02:32]

On 2014/04/25 19:59:44, haitaol1 wrote:
> Roger: please review chrome/browser/ui/webui/sync_setup_handler.cc
> 
> Raghu: please review
> chrome/browser/resources/sync_setup_overlay*
> 
> James: please review
> chrome/browser/resources/options/browser_options.js

FYI Raghu is no longer working on Chrome (we should probably remove him from the
owners files)

[Roger Tawa OOO till Jul 10th, 2014-04-25 21:42:58]

Lgtm

[haitaol1, 2014-04-29 16:43:41]

Friendly ping.

[James Hawkins, 2014-04-30 19:15:33]

https://codereview.chromium.org/235943011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/235943011/diff/60001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:224: <div class="content-area">
Why did you introduce a second content area?

[haitaol1, 2014-04-30 20:16:01]

https://codereview.chromium.org/235943011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/235943011/diff/60001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:224: <div class="content-area">
Done. I guess I was using a different class.

On 2014/04/30 19:15:34, James Hawkins wrote:
> Why did you introduce a second content area?

[James Hawkins, 2014-04-30 20:19:32]

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:223: <p/>
What is this for?

[haitaol1, 2014-04-30 20:34:15]

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:223: <p/>
To put checkbox on a new line and put some space between them. I'm not a UI guy,
so please advice if there's better way.

On 2014/04/30 20:19:32, James Hawkins wrote:
> What is this for?

[James Hawkins, 2014-04-30 20:35:37]

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/235943011/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:223: <p/>
Wrap the <label> in a <div>.  Give the div and ID and use that ID as a selector
in the CSS to add appropriate margin.  Please send me a screenshot of the UI
when that's done.

On 2014/04/30 20:34:15, haitaol1 wrote:
> To put checkbox on a new line and put some space between them. I'm not a UI
guy,
> so please advice if there's better way.
> 
> On 2014/04/30 20:19:32, James Hawkins wrote:
> > What is this for?
>

[chromium-reviews, 2014-04-30 21:05:06]

On Wed, Apr 30, 2014 at 1:35 PM, <jhawkins@chromium.org> wrote:

>
> https://codereview.chromium.org/235943011/diff/80001/
> chrome/browser/resources/sync_setup_overlay.html
> File chrome/browser/resources/sync_setup_overlay.html (right):
>
> https://codereview.chromium.org/235943011/diff/80001/
> chrome/browser/resources/sync_setup_overlay.html#newcode223
> chrome/browser/resources/sync_setup_overlay.html:223: <p/>
> Wrap the <label> in a <div>.  Give the div and ID and use that ID as a
> selector in the CSS to add appropriate margin.  Please send me a
> screenshot of the UI when that's done.


Done.  Screenshot attached.

>
>
> On 2014/04/30 20:34:15, haitaol1 wrote:
>
>> To put checkbox on a new line and put some space between them. I'm not
>>
> a UI guy,
>
>> so please advice if there's better way.
>>
>
>  On 2014/04/30 20:19:32, James Hawkins wrote:
>> > What is this for?
>>
>
>
> https://codereview.chromium.org/235943011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Hawkins, 2014-04-30 21:17:53]

It looks like this styling [1] is not being applied for some reason.  I
suggest digging through the inspector to figure out why.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...


On Wed, Apr 30, 2014 at 2:05 PM, Haitao Liu <haitaol@google.com> wrote:

>
>
>
> On Wed, Apr 30, 2014 at 1:35 PM, <jhawkins@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/235943011/diff/80001/
>> chrome/browser/resources/sync_setup_overlay.html
>> File chrome/browser/resources/sync_setup_overlay.html (right):
>>
>> https://codereview.chromium.org/235943011/diff/80001/
>> chrome/browser/resources/sync_setup_overlay.html#newcode223
>> chrome/browser/resources/sync_setup_overlay.html:223: <p/>
>> Wrap the <label> in a <div>.  Give the div and ID and use that ID as a
>> selector in the CSS to add appropriate margin.  Please send me a
>> screenshot of the UI when that's done.
>
>
> Done.  Screenshot attached.
>
>>
>>
>> On 2014/04/30 20:34:15, haitaol1 wrote:
>>
>>> To put checkbox on a new line and put some space between them. I'm not
>>>
>> a UI guy,
>>
>>> so please advice if there's better way.
>>>
>>
>>  On 2014/04/30 20:19:32, James Hawkins wrote:
>>> > What is this for?
>>>
>>
>>
>> https://codereview.chromium.org/235943011/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-04-30 23:07:41]

Looks like I need to set class="checkbox" on the div. New screenshot is
attached.


On Wed, Apr 30, 2014 at 2:17 PM, James Hawkins <jhawkins@chromium.org>wrote:

> It looks like this styling [1] is not being applied for some reason.  I
> suggest digging through the inspector to figure out why.
>
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
>
>
> On Wed, Apr 30, 2014 at 2:05 PM, Haitao Liu <haitaol@google.com> wrote:
>
>>
>>
>>
>> On Wed, Apr 30, 2014 at 1:35 PM, <jhawkins@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/235943011/diff/80001/
>>> chrome/browser/resources/sync_setup_overlay.html
>>> File chrome/browser/resources/sync_setup_overlay.html (right):
>>>
>>> https://codereview.chromium.org/235943011/diff/80001/
>>> chrome/browser/resources/sync_setup_overlay.html#newcode223
>>> chrome/browser/resources/sync_setup_overlay.html:223: <p/>
>>> Wrap the <label> in a <div>.  Give the div and ID and use that ID as a
>>> selector in the CSS to add appropriate margin.  Please send me a
>>> screenshot of the UI when that's done.
>>
>>
>> Done.  Screenshot attached.
>>
>>>
>>>
>>> On 2014/04/30 20:34:15, haitaol1 wrote:
>>>
>>>> To put checkbox on a new line and put some space between them. I'm not
>>>>
>>> a UI guy,
>>>
>>>> so please advice if there's better way.
>>>>
>>>
>>>  On 2014/04/30 20:19:32, James Hawkins wrote:
>>>> > What is this for?
>>>>
>>>
>>>
>>> https://codereview.chromium.org/235943011/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Hawkins, 2014-05-01 17:54:29]

https://codereview.chromium.org/235943011/diff/100001/chrome/browser/resource...
File chrome/browser/resources/sync_setup_overlay.css (right):

https://codereview.chromium.org/235943011/diff/100001/chrome/browser/resource...
chrome/browser/resources/sync_setup_overlay.css:102: padding-top: 20px;
Use margin not padding.

[James Hawkins, 2014-05-01 17:55:24]

On 2014/05/01 17:54:29, James Hawkins wrote:
>
https://codereview.chromium.org/235943011/diff/100001/chrome/browser/resource...
> File chrome/browser/resources/sync_setup_overlay.css (right):
> 
>
https://codereview.chromium.org/235943011/diff/100001/chrome/browser/resource...
> chrome/browser/resources/sync_setup_overlay.css:102: padding-top: 20px;
> Use margin not padding.

Looks stale, sorry.

LGTM

[tim (not reviewing), 2014-05-01 18:25:52]

sync_setup_overlay LGTM.  Whether I'm a good owner for that file or not is a
separate question... but lg for now :p

[haitaol1, 2014-05-01 19:12:01]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-01 19:12:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/235943011/140001

[commit-bot: I haz the power, 2014-05-01 20:01:32]

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

[commit-bot: I haz the power, 2014-05-01 20:01:32]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[haitaol1, 2014-05-02 17:01:21]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-02 17:01:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/235943011/160001

[commit-bot: I haz the power, 2014-05-02 17:34:17]

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

[commit-bot: I haz the power, 2014-05-02 17:34:17]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[haitaol1, 2014-05-05 21:36:57]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-05 21:37:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/235943011/180001

[commit-bot: I haz the power, 2014-05-05 21:42:32]

Change committed as 268276