Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(412)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 7093004: Sync: Refactor the ProfileSyncService and sync setup flow to remove use of WebUI from PSS. (Closed)

Created:
9 years, 6 months ago by James Hawkins
Modified:
9 years, 6 months ago
CC:
chromium-reviews, ncarter (slow), idana, Raghu Simha, achuith+watch_chromium.org, rginda+watch_chromium.org, arv (Not doing code reviews), estade+watch_chromium.org, tim (not reviewing)
Visibility:
Public.

Description

Sync: Refactor the ProfileSyncService and sync setup flow to remove use of WebUI from PSS. Also in this CL: * Only call didShowPage on pages/overlays if they're not already visible. * Remove buggy and unused logic in shouldClosePage. BUG=85935 TEST=none R=csilv@chromium.org,binji@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88941

Patch Set 1 #

Patch Set 2 : Remove shouldCloseOverlay. #

Patch Set 3 : Cleanup. #

Patch Set 4 : Win fix. #

Total comments: 4

Patch Set 5 : Fix logic. #

Patch Set 6 : Add tests, fixes. #

Total comments: 10

Patch Set 7 : Add TODO. #

Patch Set 8 : Review fixes and cleanups. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+380 lines, -347 lines) Patch
M chrome/browser/profiles/profile_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/options_page.js View 1 6 chunks +12 lines, -26 lines 0 comments Download
M chrome/browser/resources/options/personal_options.js View 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/resources/options/sync_setup_overlay.js View 1 2 3 4 5 4 chunks +15 lines, -20 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.h View 2 chunks +8 lines, -14 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 2 4 chunks +14 lines, -30 lines 0 comments Download
M chrome/browser/sync/sync_setup_flow.h View 1 2 3 4 5 2 chunks +1 line, -6 lines 0 comments Download
M chrome/browser/sync/sync_setup_flow.cc View 1 2 3 4 5 6 7 9 chunks +46 lines, -24 lines 0 comments Download
M chrome/browser/sync/sync_setup_wizard.h View 1 2 3 4 5 6 2 chunks +8 lines, -3 lines 0 comments Download
M chrome/browser/sync/sync_setup_wizard.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/sync_setup_wizard_unittest.cc View 1 2 3 4 5 6 7 11 chunks +226 lines, -169 lines 0 comments Download
M chrome/browser/sync/sync_ui_util.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/gtk/bookmarks/bookmark_bar_gtk.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/personal_options_handler.h View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/options/personal_options_handler.cc View 2 chunks +0 lines, -28 lines 0 comments Download
M chrome/browser/ui/webui/options/sync_setup_handler.h View 1 2 3 4 5 6 7 1 chunk +19 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/sync_setup_handler.cc View 1 2 3 4 5 6 7 3 chunks +21 lines, -13 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
James Hawkins
9 years, 6 months ago (2011-06-08 18:49:25 UTC) #1
James Hawkins
csilv: For lgtm review. binji: Please add review comments.
9 years, 6 months ago (2011-06-08 18:49:52 UTC) #2
James Hawkins
+tim for Sync changes.
9 years, 6 months ago (2011-06-08 18:50:17 UTC) #3
binji
http://codereview.chromium.org/7093004/diff/6002/chrome/browser/ui/webui/options/sync_setup_handler.cc File chrome/browser/ui/webui/options/sync_setup_handler.cc (right): http://codereview.chromium.org/7093004/diff/6002/chrome/browser/ui/webui/options/sync_setup_handler.cc#newcode426 chrome/browser/ui/webui/options/sync_setup_handler.cc:426: sync_service->get_wizard().Step(state); This logic seems strange to me. Why do ...
9 years, 6 months ago (2011-06-08 20:19:56 UTC) #4
James Hawkins
http://codereview.chromium.org/7093004/diff/6002/chrome/browser/ui/webui/options/sync_setup_handler.cc File chrome/browser/ui/webui/options/sync_setup_handler.cc (right): http://codereview.chromium.org/7093004/diff/6002/chrome/browser/ui/webui/options/sync_setup_handler.cc#newcode426 chrome/browser/ui/webui/options/sync_setup_handler.cc:426: sync_service->get_wizard().Step(state); On 2011/06/08 20:19:56, binji wrote: > This logic ...
9 years, 6 months ago (2011-06-08 22:33:39 UTC) #5
tim (not reviewing)
Thank you for doing this. http://codereview.chromium.org/7093004/diff/6002/chrome/browser/sync/sync_setup_wizard.h File chrome/browser/sync/sync_setup_wizard.h (right): http://codereview.chromium.org/7093004/diff/6002/chrome/browser/sync/sync_setup_wizard.h#newcode39 chrome/browser/sync/sync_setup_wizard.h:39: NONFATAL_ERROR, Please add unittests ...
9 years, 6 months ago (2011-06-09 14:50:36 UTC) #6
csilv
LGTM
9 years, 6 months ago (2011-06-13 18:11:26 UTC) #7
James Hawkins
Can you guys take another look? I've made some changes to accommodate the tests.
9 years, 6 months ago (2011-06-13 20:44:55 UTC) #8
csilv
LGTM again. See comments below. http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_setup_wizard_unittest.cc File chrome/browser/sync/sync_setup_wizard_unittest.cc (right): http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_setup_wizard_unittest.cc#newcode184 chrome/browser/sync/sync_setup_wizard_unittest.cc:184: //set_window(test_window_); If this line ...
9 years, 6 months ago (2011-06-13 22:20:57 UTC) #9
tim (not reviewing)
New test looks nice, thanks. LGTM, with one question. Also, please file a bug + ...
9 years, 6 months ago (2011-06-13 22:32:01 UTC) #10
binji
http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_setup_wizard_unittest.cc File chrome/browser/sync/sync_setup_wizard_unittest.cc (right): http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_setup_wizard_unittest.cc#newcode184 chrome/browser/sync/sync_setup_wizard_unittest.cc:184: //set_window(test_window_); remove commented code http://codereview.chromium.org/7093004/diff/4023/chrome/browser/ui/webui/options/sync_setup_handler.h File chrome/browser/ui/webui/options/sync_setup_handler.h (right): http://codereview.chromium.org/7093004/diff/4023/chrome/browser/ui/webui/options/sync_setup_handler.h#newcode48 ...
9 years, 6 months ago (2011-06-13 22:32:51 UTC) #11
James Hawkins
9 years, 6 months ago (2011-06-13 23:14:52 UTC) #12 
http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:732: if (WizardIsVisible()) {
On 2011/06/13 22:32:01, timsteele wrote:
> I like that some of the complexity here moved into the flow rather than the
> service, though I'm a bit surprised that nothing has to happen if the wizard
is
> visible? Did you actually try out (manually) the different permutations with
> passphrases here?

I don't know why or if this is still necessary, only that this is what all of
the other ShowX() calls do on this object. Someone more familiar should take a
look at some point.

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard_unittest.cc:184:
//set_window(test_window_);
On 2011/06/13 22:32:52, binji wrote:
> remove commented code

Done.

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard_unittest.cc:184:
//set_window(test_window_);
On 2011/06/13 22:20:57, csilv wrote:
> If this line isn't commented out as part of the TODO above, please kill it.

Done.

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard_unittest.cc:229: #if defined(OS_MACOSX)
On 2011/06/13 22:20:57, csilv wrote:
> We may not need to skip these tests anymore.  The bug says that the issue is a
> limitation of BrowserShowHtmlDialog which the new UI doesn't use anymore.

I'll do that in a followup CL.

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/sync_setup_handler.h (right):

http://codereview.chromium.org/7093004/diff/4023/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/sync_setup_handler.h:48: // Callbacks from the
page. Protected in order to be called by the
On 2011/06/13 22:32:52, binji wrote:
> finish comment

Done.

Powered by Google App Engine
This is Rietveld 408576698