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

Issue 4158009: Session sync integration tests. Left out many client for now, since even... (Closed)

Created:
10 years, 1 month ago by Nicolas Zea
Modified:
9 years, 7 months ago
CC:
chromium-reviews, ncarter (slow), idana, ben+cc_chromium.org, Nicolas Zea, tim (not reviewing)
Visibility:
Public.

Description

Session sync integration tests. Left out many client for now, since even without doing anything in the test it was timing out. BUG=30519 TEST=self Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65051

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 56

Patch Set 3 : Comments + fixed some broken helpers and multi-client test #

Patch Set 4 : unit_test typo fix #

Total comments: 22

Patch Set 5 : Comments #

Total comments: 4

Patch Set 6 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+588 lines, -20 lines) Patch
M chrome/browser/dom_ui/foreign_session_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sessions/session_service.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sessions/session_service_test_helper.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sessions/session_types.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sessions/session_types.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/glue/session_model_associator.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/session_model_associator.cc View 1 2 4 chunks +23 lines, -15 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_session_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 chunks +7 lines, -0 lines 0 comments Download
A chrome/test/live_sync/live_sessions_sync_test.h View 1 2 3 4 5 1 chunk +418 lines, -0 lines 0 comments Download
M chrome/test/live_sync/live_sync_test.cc View 1 chunk +5 lines, -0 lines 0 comments Download
A chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc View 1 2 3 4 5 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/test/live_sync/single_client_live_sessions_sync_test.cc View 1 2 3 4 5 1 chunk +30 lines, -0 lines 0 comments Download
A chrome/test/live_sync/two_client_live_sessions_sync_test.cc View 1 2 3 4 5 1 chunk +62 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Nicolas Zea
Mind taking a look? I'm planning to add many client in later, as well as ...
10 years, 1 month ago (2010-10-29 07:55:22 UTC) #1
Paweł Hajdan Jr.
Drive-by with a test comment. No need to wait for another review by me. http://codereview.chromium.org/4158009/diff/13001/14009 ...
10 years, 1 month ago (2010-10-29 07:58:37 UTC) #2
Raghu Simha
This is a pretty good start. I've made a bunch of comments. In general, test ...
10 years, 1 month ago (2010-10-29 19:41:19 UTC) #3
Nicolas Zea
Changes made. In addition, I noticed some bugs I had left in, which involved a ...
10 years, 1 month ago (2010-11-02 21:52:50 UTC) #4
Raghu Simha
Nice work! The test cases look really good now, and are easy to write, read ...
10 years, 1 month ago (2010-11-03 03:13:51 UTC) #5
Nicolas Zea
Comments. http://codereview.chromium.org/4158009/diff/43001/44010 File chrome/test/live_sync/live_sessions_sync_test.h (right): http://codereview.chromium.org/4158009/diff/43001/44010#newcode257 chrome/test/live_sync/live_sessions_sync_test.h:257: static bool SessionWindowSorter(SessionWindow* lhs, SessionWindow* rhs) { On ...
10 years, 1 month ago (2010-11-03 20:59:13 UTC) #6
Raghu Simha
LGTM, with minor nits. Yay! Note: I'm still seeing lint errors on some of the ...
10 years, 1 month ago (2010-11-03 21:53:01 UTC) #7
Raghu Simha
10 years, 1 month ago (2010-11-03 22:26:41 UTC) #8
Posting my comments for real this time :)

http://codereview.chromium.org/4158009/diff/58001/59010
File chrome/test/live_sync/live_sessions_sync_test.h (right):

http://codereview.chromium.org/4158009/diff/58001/59010#newcode262
chrome/test/live_sync/live_sessions_sync_test.h:262: // Sort session windows
based on their first tab's url.
nit: update comment to reflect new method name. Document what a return value of
true means.

http://codereview.chromium.org/4158009/diff/58001/59010#newcode285
chrome/test/live_sync/live_sessions_sync_test.h:285: // Sorts a foreign session
based on the first session window.
nit: same as above -- update this comment.

http://codereview.chromium.org/4158009/diff/58001/59010#newcode335
chrome/test/live_sync/live_sessions_sync_test.h:335: // with a reference
SessionWindow list.
nit: Add to the comment saying what a return value of true signifies.

http://codereview.chromium.org/4158009/diff/58001/59012
File chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc (right):

http://codereview.chromium.org/4158009/diff/58001/59012#newcode30
chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc:30:
ASSERT_TRUE(CheckForeignSessionsAgainst(i, client_windows));
Awesome!

Powered by Google App Engine
This is Rietveld 408576698