Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10802060:
Ignore file URLs from being synced (Closed)
|
Created:
8 years, 5 months ago by carloschilazo Modified:
8 years, 4 months ago CC:
chromium-reviews, Raghu Simha, ncarter (slow), akalin, tim (not reviewing) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionIgnore local file URLs from being synced
BUG=100356
Test=Unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151095
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : #
Total comments: 3
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 20 (0 generated)
+atwilson: Drew, could you take a look at this? I'm willing to handle getting it committed if you're OK with the change.
Is there any update on this? @atwilson: Could I get feedback about the patch?
On 2012/07/30 07:19:45, carloschilazo wrote: > Is there any update on this? > @atwilson: Could I get feedback about the patch? Sorry, missed the original email. Thanks for the patch! There are two classes that handle syncing: a) TypedUrlChangeProcessor handles changes to typed URLs after chrome is already running (either changes sent down from the server, or changes initiated client-side). b) TypedUrlModelAssociator handles brings the client and server typed URL data sets into sync when chrome starts up. As it stands, this patch would merely delay the syncing of file URLs that happened on the local client - on the next chrome restart, the URLs would be synced by TypedUrlModelAssociator. I suspect that adding a similar clause to TypedUrlModelAssociator::ShouldIgnoreUrl() would suffice to fix this. Similarly, we need to add code to ignore changes being sent down from the server for file URLs (in case you have an older client that doesn't have this fix yet, for example). So we need to add code to TypedUrlChangeProcessor::ApplyChangesFromSyncModel() for this. I would typically require a unit test with a patch like this, but since you are a third-party contributor I think we can accept a patch without one, given the complexity of the unit test framework. That said, if you want to add a test to typed_url_change_processor_unittest.cc and profile_sync_service_typed_url_unittest.cc, I'd be thrilled to review it :)
On 2012/07/30 16:41:18, Andrew T Wilson wrote: > On 2012/07/30 07:19:45, carloschilazo wrote: > > Is there any update on this? > > @atwilson: Could I get feedback about the patch? > > Sorry, missed the original email. Thanks for the patch! > > There are two classes that handle syncing: > > a) TypedUrlChangeProcessor handles changes to typed URLs after chrome is already > running (either changes sent down from the server, or changes initiated > client-side). > b) TypedUrlModelAssociator handles brings the client and server typed URL data > sets into sync when chrome starts up. > > As it stands, this patch would merely delay the syncing of file URLs that > happened on the local client - on the next chrome restart, the URLs would be > synced by TypedUrlModelAssociator. I suspect that adding a similar clause to > TypedUrlModelAssociator::ShouldIgnoreUrl() would suffice to fix this. > > Similarly, we need to add code to ignore changes being sent down from the server > for file URLs (in case you have an older client that doesn't have this fix yet, > for example). So we need to add code to > TypedUrlChangeProcessor::ApplyChangesFromSyncModel() for this. > > I would typically require a unit test with a patch like this, but since you are > a third-party contributor I think we can accept a patch without one, given the > complexity of the unit test framework. That said, if you want to add a test to > typed_url_change_processor_unittest.cc and > profile_sync_service_typed_url_unittest.cc, I'd be thrilled to review it :) @atwilson: I have added the checks where you mentioned and decided to take a dive into the unit test framework and added two unit tests. Thank you very much!
LGTM with one suggestion below. Richard, you said you were willing to drive these through the try bots and land it? http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:466: EXPECT_EQ(1U, new_visits.size()); We should probably make sure the one item is "yey.com".
On 2012/08/03 21:59:27, Andrew T Wilson wrote: > LGTM with one suggestion below. Richard, you said you were willing to drive > these through the try bots and land it? > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:466: > EXPECT_EQ(1U, new_visits.size()); > We should probably make sure the one item is "yey.com". That check is now part of the unit test in the latest patch set. I believe is ready to be sent to the trybot
On 2012/08/04 00:46:35, carloschilazo wrote: > On 2012/08/03 21:59:27, Andrew T Wilson wrote: > > LGTM with one suggestion below. Richard, you said you were willing to drive > > these through the try bots and land it? > > > > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > > File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): > > > > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > > chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:466: > > EXPECT_EQ(1U, new_visits.size()); > > We should probably make sure the one item is "yey.com". > > That check is now part of the unit test in the latest patch set. > > I believe is ready to be sent to the trybot @rlarocque or @atwilson, any updates?
On 2012/08/07 16:52:02, carloschilazo wrote: > On 2012/08/04 00:46:35, carloschilazo wrote: > > On 2012/08/03 21:59:27, Andrew T Wilson wrote: > > > LGTM with one suggestion below. Richard, you said you were willing to drive > > > these through the try bots and land it? > > > > > > > > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > > > File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc > (right): > > > > > > > > > http://codereview.chromium.org/10802060/diff/14002/chrome/browser/sync/glue/t... > > > chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:466: > > > EXPECT_EQ(1U, new_visits.size()); > > > We should probably make sure the one item is "yey.com". > > > > That check is now part of the unit test in the latest patch set. > > > > I believe is ready to be sent to the trybot > > @rlarocque or @atwilson, any updates? Sorry for the delay, I've been away on vacation for the past few days. I'll send this at the trybots today.
I saw a few minor style issues while uploading this patch. http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... File chrome/browser/sync/glue/typed_url_model_associator.cc (right): http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... chrome/browser/sync/glue/typed_url_model_associator.cc:122: // Ignore local file URLs nit: Comments should end with a period. http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:471: nit: Our tools don't like this extra whitespace. Can you remove it? http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:975: nit: Trailing whitespace.
On 2012/08/07 17:50:30, rlarocque wrote: > I saw a few minor style issues while uploading this patch. > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > File chrome/browser/sync/glue/typed_url_model_associator.cc (right): > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > chrome/browser/sync/glue/typed_url_model_associator.cc:122: // Ignore local file > URLs > nit: Comments should end with a period. > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:471: > nit: Our tools don't like this extra whitespace. Can you remove it? > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... > File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... > chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:975: > nit: Trailing whitespace. New patches addresses the style issues mentioned thanks!
@rlarocque: I have investigated and fixed what caused the trybot test failure. I did not catched that failure locally because I was running them locally on Release build. It has been fixed and I no longer get the failure (even on debug) Could you please submit to trybots the latest patch set again? Explanation for why it failed if anyone interested: I was hitting a DCHECK on TypedUrlModelAssociator::MergeUrls because I was creating a copy of an existing HistoryURL element and at some point trying to merge the new copy with the original during the test. I decided to expand my own test on: profile_sync_service_typed_url_unittest.cc and It now covers the following: -Make sure that when we update from syncdb, we only apply updates to non-local file urls. -If we get a change from syncdb for a new local file url, we ignore it. On 2012/08/07 18:26:29, carloschilazo wrote: > On 2012/08/07 17:50:30, rlarocque wrote: > > I saw a few minor style issues while uploading this patch. > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > > File chrome/browser/sync/glue/typed_url_model_associator.cc (right): > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > > chrome/browser/sync/glue/typed_url_model_associator.cc:122: // Ignore local > file > > URLs > > nit: Comments should end with a period. > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > > File chrome/browser/sync/glue/typed_url_model_associator_unittest.cc (right): > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/glue/ty... > > chrome/browser/sync/glue/typed_url_model_associator_unittest.cc:471: > > nit: Our tools don't like this extra whitespace. Can you remove it? > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... > > File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): > > > > > http://codereview.chromium.org/10802060/diff/7006/chrome/browser/sync/profile... > > chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:975: > > nit: Trailing whitespace. > > New patches addresses the style issues mentioned thanks!
LGTM with two style nits. http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:980: &updated_visits)); Need to indent this one more space. http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:982: "dog", 20, 15, false, indentation for this and the next line is off
Thank you, style nits addressed in latest patchset. On 2012/08/09 16:55:02, Andrew T Wilson wrote: > LGTM with two style nits. > > http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... > File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): > > http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... > chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:980: > &updated_visits)); > Need to indent this one more space. > > http://codereview.chromium.org/10802060/diff/10007/chrome/browser/sync/profil... > chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:982: "dog", 20, > 15, false, > indentation for this and the next line is off
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carloschilazo@gmail.com/10802060/5011
Try job failure for 10802060-5011 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Is it possible that the failed test is not because of the patch? I was checking to see if they could be related but I cannot find any relation between the patch and the failed test (OldPanelBrowserTest.AnimateBounds for example). On 2012/08/10 00:36:03, I haz the power (commit-bot) wrote: > Try job failure for 10802060-5011 (retry) on linux_rel for step > "interactive_ui_tests". > It's a second try, previously, step "interactive_ui_tests" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2012/08/10 15:10:17, carloschilazo wrote: > Is it possible that the failed test is not because of the patch? > I was checking to see if they could be related but I cannot find any relation > between the patch and the failed test (OldPanelBrowserTest.AnimateBounds for > example). > > > On 2012/08/10 00:36:03, I haz the power (commit-bot) wrote: > > Try job failure for 10802060-5011 (retry) on linux_rel for step > > "interactive_ui_tests". > > It's a second try, previously, step "interactive_ui_tests" failed. > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Yes, that happens sometimes. The fact that it failed differently on each linux_rel try run makes me think it's a flake. Let's try again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carloschilazo@gmail.com/10802060/5011
Change committed as 151095 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
