Issue 3038016: Stop Syncing Overlay and Import Data Overlay

Issue 3038016: Stop Syncing Overlay and Import Data Overlay (Closed)

Created:
10 years, 5 months ago by sargrass

Modified:
9 years, 7 months ago

Reviewers:
csilv, stuartmorgan, arv (Not doing code reviews), zel, Evan Martin

CC:
chromium-reviews, dhg, ben+cc_chromium.org, kuchhal, arv (Not doing code reviews)

Base URL:
http://src.chromium.org/git/chromium.git

Visibility:
Public.

Description

Stop Syncing Overlay and Import Data Overlay BUG=48883 TEST=Exercise Personal Stuff page via --enabled-tabbed-options Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54032

Patch Set 3 : merge conflict #

Patch Set 4 : display only setupsync after stop syncing #

Patch Set 5 : Revert clear browser data description #

Patch Set 6 : revert clear browser data #

Total comments: 14

Patch Set 7 : fixed function names and add import data overlay starting codes #

Patch Set 8 : move service into function. finding optionspage undefined error #

Patch Set 9 : Add import data #

Patch Set 10 : add from label #

Total comments: 8

Patch Set 12 : add start code for supported browser popup #

Patch Set 13 : Make get theme to href #

Patch Set 14 : started code for import function #

Total comments: 9

Patch Set 15 : order include #

Created: 10 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+397 lines, -24 lines)			Patch
M	chrome/browser/dom_ui/clear_browser_data_handler.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+0 lines, -1 line	0 comments	Download
A	chrome/browser/dom_ui/import_data_handler.h	View	9 10 11	1 chunk	+34 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/import_data_handler.cc	View	9 10 11 12 13	1 chunk	+71 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/options_ui.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/personal_options_handler.h	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/personal_options_handler.cc	View	1 2 3 4 5 6 7	3 chunks	+23 lines, -9 lines	0 comments	Download
A	chrome/browser/dom_ui/stop_syncing_handler.h	View	1 2 3 4 5 6 7	1 chunk	+28 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/stop_syncing_handler.cc	View	1 2 3 4 5 6 7	1 chunk	+48 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+10 lines, -1 line	0 comments	Download
A	chrome/browser/resources/options/import_data_overlay.html	View	9 10 11 12 13 14	1 chunk	+28 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/import_data_overlay.js	View	9 10 11 12 13	1 chunk	+64 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options/personal_options.html	View	2 3 4 5 6 7 8 9 10 11 12	4 chunks	+5 lines, -7 lines	0 comments	Download
M	chrome/browser/resources/options/personal_options.js	View	1 2 3 4 5 6 7 8 9 10 11 12	3 chunks	+25 lines, -5 lines	0 comments	Download
A	chrome/browser/resources/options/stop_syncing_overlay.html	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+8 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/stop_syncing_overlay.js	View	1 2 3 4 5 6	1 chunk	+38 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+5 lines, -1 line	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

csilv

http://codereview.chromium.org/3038016/diff/2001/3001 File chrome/browser/dom_ui/clear_browser_data_handler.h (right): http://codereview.chromium.org/3038016/diff/2001/3001#newcode10 chrome/browser/dom_ui/clear_browser_data_handler.h:10: // Chrome Clear Browser Data overlay UI handler. FYI, ...

10 years, 5 months ago (2010-07-23 01:58:30 UTC) #1

sargrass

10 years, 5 months ago (2010-07-23 22:09:34 UTC) #2

zel

http://codereview.chromium.org/3038016/diff/15001/1012 File chrome/browser/dom_ui/clear_browser_data_handler.h (left): http://codereview.chromium.org/3038016/diff/15001/1012#oldcode38 chrome/browser/dom_ui/clear_browser_data_handler.h:38: why did you remove this one? weren't our files ...

10 years, 5 months ago (2010-07-23 23:29:37 UTC) #3

csilv

http://codereview.chromium.org/3038016/diff/15001/1014 File chrome/browser/dom_ui/personal_options_handler.cc (right): http://codereview.chromium.org/3038016/diff/15001/1014#newcode100 chrome/browser/dom_ui/personal_options_handler.cc:100: service = dom_ui_->GetProfile()->GetProfileSyncService(); I'm not sure I see a ...

10 years, 5 months ago (2010-07-26 20:49:28 UTC) #4

http://codereview.chromium.org/3038016/diff/15001/1014
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/3038016/diff/15001/1014#newcode100
chrome/browser/dom_ui/personal_options_handler.cc:100: service =
dom_ui_->GetProfile()->GetProfileSyncService();
I'm not sure I see a value remembering the sync service.  I'd rather just call
GetProfileSyncService() when it's needed (in SetSyncStatusUIString.)  That way
we don't have to worry about what might happen the sync service were to change.

Also, GetProfileSyncService could return NULL.  So we need to account for that. 
If syncing id disabled, the UI should probably reflect that.  (May be necessary
to hide it altogether.)  See the implementation notes for
ProfileSyncService::IsSyncEnabled().  (We can disable sync via the command line
for testing.)

http://codereview.chromium.org/3038016/diff/15001/1015
File chrome/browser/dom_ui/personal_options_handler.h (right):

http://codereview.chromium.org/3038016/diff/15001/1015#newcode20
chrome/browser/dom_ui/personal_options_handler.h:20: virtual void
RegisterMessages();
Add an extra line and comment to indicate that RegisterMessages is a
DOMMessageHandler implementation.  See advanced_options_hander.h for example.

http://codereview.chromium.org/3038016/diff/15001/1016
File chrome/browser/dom_ui/stop_syncing_handler.cc (right):

http://codereview.chromium.org/3038016/diff/15001/1016#newcode31
chrome/browser/dom_ui/stop_syncing_handler.cc:31:
localized_strings->SetString(L"cancel",
No need to define cancel, this is one of the common strings that are available
for all dialogs.  See core_options_handler.cc for complete list.

http://codereview.chromium.org/3038016/diff/15001/1017
File chrome/browser/dom_ui/stop_syncing_handler.h (right):

http://codereview.chromium.org/3038016/diff/15001/1017#newcode19
chrome/browser/dom_ui/stop_syncing_handler.h:19: virtual void
RegisterMessages();
Add comment // DOMMessageHandler implementation.

sargrass

10 years, 5 months ago (2010-07-26 20:50:07 UTC) #5

sargrass

10 years, 5 months ago (2010-07-26 22:42:58 UTC) #6

http://codereview.chromium.org/3038016/diff/15001/1014
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/3038016/diff/15001/1014#newcode100
chrome/browser/dom_ui/personal_options_handler.cc:100: service =
dom_ui_->GetProfile()->GetProfileSyncService();
On 2010/07/26 20:49:29, csilv wrote:
> I'm not sure I see a value remembering the sync service.  I'd rather just call
> GetProfileSyncService() when it's needed (in SetSyncStatusUIString.)  That way
> we don't have to worry about what might happen the sync service were to
change.
> 
> Also, GetProfileSyncService could return NULL.  So we need to account for
that. 
> If syncing id disabled, the UI should probably reflect that.  (May be
necessary
> to hide it altogether.)  See the implementation notes for
> ProfileSyncService::IsSyncEnabled().  (We can disable sync via the command
line
> for testing.)

Done.

http://codereview.chromium.org/3038016/diff/15001/1015
File chrome/browser/dom_ui/personal_options_handler.h (right):

http://codereview.chromium.org/3038016/diff/15001/1015#newcode20
chrome/browser/dom_ui/personal_options_handler.h:20: virtual void
RegisterMessages();
On 2010/07/26 20:49:29, csilv wrote:
> Add an extra line and comment to indicate that RegisterMessages is a
> DOMMessageHandler implementation.  See advanced_options_hander.h for example.

Done.

http://codereview.chromium.org/3038016/diff/15001/1016
File chrome/browser/dom_ui/stop_syncing_handler.cc (right):

http://codereview.chromium.org/3038016/diff/15001/1016#newcode31
chrome/browser/dom_ui/stop_syncing_handler.cc:31:
localized_strings->SetString(L"cancel",
On 2010/07/26 20:49:29, csilv wrote:
> No need to define cancel, this is one of the common strings that are available
> for all dialogs.  See core_options_handler.cc for complete list.

Done.

http://codereview.chromium.org/3038016/diff/15001/1017
File chrome/browser/dom_ui/stop_syncing_handler.h (right):

http://codereview.chromium.org/3038016/diff/15001/1017#newcode19
chrome/browser/dom_ui/stop_syncing_handler.h:19: virtual void
RegisterMessages();
On 2010/07/26 20:49:29, csilv wrote:
> Add comment // DOMMessageHandler implementation.

Done.

arv (Not doing code reviews)

FYI http://codereview.chromium.org/3038016/diff/29001/30010 File chrome/browser/resources/options/import_data_overlay.html (right): http://codereview.chromium.org/3038016/diff/29001/30010#newcode3 chrome/browser/resources/options/import_data_overlay.html:3: <table> There does not seem to be any ...

10 years, 5 months ago (2010-07-26 22:53:31 UTC) #7

sargrass

http://codereview.chromium.org/3038016/diff/29001/30010 File chrome/browser/resources/options/import_data_overlay.html (right): http://codereview.chromium.org/3038016/diff/29001/30010#newcode3 chrome/browser/resources/options/import_data_overlay.html:3: <table> On 2010/07/26 22:53:31, arv wrote: > There does ...

10 years, 5 months ago (2010-07-26 23:21:45 UTC) #8

stuartmorgan

http://codereview.chromium.org/3038016/diff/38001/31010 File chrome/browser/resources/options.html (right): http://codereview.chromium.org/3038016/diff/38001/31010#newcode43 chrome/browser/resources/options.html:43: <script src="options/import_data_overlay.js"></script> I just alphabetized all of these so ...

10 years, 4 months ago (2010-07-28 21:22:08 UTC) #9

csilv

http://codereview.chromium.org/3038016/diff/38001/31002 File chrome/browser/dom_ui/clear_browser_data_handler.h (right): http://codereview.chromium.org/3038016/diff/38001/31002#newcode1 chrome/browser/dom_ui/clear_browser_data_handler.h:1: // Copyright (c) 2010 The Chromium Authors. All rights ...

10 years, 4 months ago (2010-07-28 21:49:51 UTC) #10

sargrass

10 years, 4 months ago (2010-07-28 21:55:05 UTC) #11

Evan Martin

Did you run this through the try bot? It failed to compile on the build ...

10 years, 4 months ago (2010-07-28 22:05:06 UTC) #12

Evan Martin

10 years, 4 months ago (2010-07-28 22:27:15 UTC) #13

On 2010/07/28 22:05:06, Evan Martin wrote:
> Did you run this through the try bot?  It failed to compile on the build bot.

It looks like you just forgot a file, but since it broke the build I reverted
it.

Expand Messages | Collapse Messages