|
|
Created:
4 years, 1 month ago by ltian Modified:
4 years ago Reviewers:
Peter Kasting CC:
chromium-reviews, extensions-reviews_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, chromium-apps-reviews_chromium.org, sync-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd field to monitor last visited time for each search engine
1. Add the last_visited field in template_url_data to monitor last
visited time of each search engine.
2. The last_visited will be updated to current time each time the OpenSearch
description document is detected.
3. For now, last_visited will not be synced if the template_url is
existed.
BUG=348360
Committed: https://crrev.com/8b5fc83dba9f525b0f707433a9fbb91465ca9bdd
Cr-Commit-Position: refs/heads/master@{#436079}
Patch Set 1 #Patch Set 2 : Add unit test for last_visited field. #
Total comments: 67
Patch Set 3 : Update based on Peter's comments. #Patch Set 4 : Gclient sync and fix patch failure caused by conflicts. #Patch Set 5 : Remove sync operations of last_visited field. #
Total comments: 19
Patch Set 6 : Update based on Peter's comments. #Patch Set 7 : Add comment. #
Total comments: 1
Patch Set 8 : Update Nit comment. #Depends on Patchset: Messages
Total messages: 55 (37 generated)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
ltian@chromium.org changed reviewers: + pkasting@chromium.org
Please review the changes. Thanks!
https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:115: data->last_visited = base::Time(); I don't really understand why |date_created| and |last_modified| were set to Time() here, but you don't really need to fix those in this CL as that's unrelated. I think setting |last_visited| to Time() is correct, but should be done in the constructor instead of here. See comments there. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:299: data.last_visited = Time::FromTimeT(100); It may be possible to remove most of the explicit sets of this field in this file if this value is defaulted to Time() in the constructor. Not sure. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:119: // date_created, the last_modified and the last_visit time. Neither pointer Nit: Shorter, and more future-proof: "It does not check the id or any time-related fields." https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:924: "http://icon1", false, "UTF-8;UTF-16", Time(), Time(), Time()); Nit: 80 columns (several places) (Running git cl format before uploading can help with this) https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1025: base::Time::Now(), base::Time::Now(), base::Time::Now()); Is it important that we set this to Time::Now()? Because if not, then I think otherwise we basically always pass Time() for this arg, and we could just eliminate it from both AddKeywordWithDate() and CreateKeywordWithDate(). https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1544: base::Time original_last_visited = original_url->last_visited(); Nit: Can be const (2 places) https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1546: TemplateURL* modified_url = model()->GetTemplateURLForKeyword( Nit: If you follow my next comment, then this could be inlined into the next statement, although feel free not to do this if it decreases readability overall (2 places) https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1549: EXPECT_NE(original_last_visited, modified_url->last_visited()); Nit: Use |modifed_last_visited| https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1551: TemplateURL* reloaded_url = model()->GetTemplateURLForKeyword( Nit: Prefer to linebreak after '=' rather than in the midst of the parameter list (I suspect git cl format would do this too, not sure) https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1554: // of precision when serializing and deserializing the timestamps. Nit: Comment not necessary (this is just explaining AssertTimesEqual() itself) https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1555: AssertTimesEqual(modifed_last_visited, reloaded_url ->last_visited()); Nit: No space before -> https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:32: // date_created, the last_modified time or the last_visited time. Neither Nit: See earlier comment about wording https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/keyword_table.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:88: // Column add in version 69. Nit: added https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:195: " last_visited INTEGER DEFAULT 0)"); Nit: No leading space https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:216: *update_compatible_version = true; Doesn't seem like you should set this, as we don't need to change the compatible version. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_data.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_data.cc:18: last_visited(base::Time::Now()), Seems wrong to set last_visited to Now() (instead of just to Time()) here, as then we can't distinguish between an engine which was never visited from one which has been. (To phrase differently, I think setting this this way causes every newly-created engine to appear to be "recently visited".) https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_data.h:87: // NOTE: Like data_created above, this may be 0. Nit: I'd change this to "NOTE: This may be 0 if the URL has never been visited." See also comment in .cc file. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:152: model->UpdateTemplateURLVisitTime(existing_url); I don't think the changes in this file are the right place to update the last_visited time. Basically, these changes will update the time whenever we would have re-fetched the TemplateURL. But I think we want to update instead whenever there's a history visit that matches the template_url in question. To implement this, you'd want to instead modify TemplateURLService::UpdateKeywordSearchTermsForURL() to update the timestamp. Behavioral differences, assuming some site like search_engine.com that has a page with an OSDD and a search box, and a search results page with no OSDD: * The existing patch would mark search_engine visited if the user navigates to search_engine.com and then closes the tab; my proposal does not * The existing patch would not mark search_engine visited if the user uses search_engine directly from the omnibox; my patch would * The existing patch would not mark search_engine visited if the user navigates directly to a search results page on search_engine; my patch would The second and third bullets above definitely seem like things we should change. The first is more arguable, but I would claim the user hasn't actually "used" search_engine in this case. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_prepopulate_data.cc:1016: data->last_visited = base::Time(); Nit: See comments in settings_overrides_api.cc https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:730: if(UpdateNoNotify(url, TemplateURL(data))) Nit: There must be a space between "if" and "("; see http://google.github.io/styleguide/cppguide.html#Conditionals . (Multiple places) https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); Nit: There are at least a couple other places that do "if (UpdateNoNotify(...)) NotifyObservers();". Might be worth adding a private helper called Update() that does this. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1275: se_specifics->set_last_visited(turl.last_visited().ToInternalValue()); I'm confused. I thought you weren't going to sync this field's value at all in this CL, and handle all sync issues in a followup CL? I think it would be less confusing to just not touch anything sync-related here than to add the field, but only restore it in certain cases. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:291: // Update the last_visited time of TemplateURL to current time. Nit: Comments should be declarative ("Updates") rather than imperative ("Update"); see http://google.github.io/styleguide/cppguide.html#Function_Comments Also, "current" -> "the current" https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... File components/sync/protocol/search_engine_specifics.proto (right): https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... components/sync/protocol/search_engine_specifics.proto:83: // units Nit: Broken line wrapping This usually happens if git cl format reformats a comment that was too long. If you haven't already done so, consider modifying your editor of choice so the 80-column mark is clearly visible and you'll be unlikely to write comments that exceed it. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... File components/webdata/common/web_database.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database.cc:22: const int kCompatibleVersionNumber = 69; I wouldn't update this; see comments elsewhere. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... File components/webdata/common/web_database_migration_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:170: Nit: Extra blank line https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:1075: // Tests delete show_in_default_list column in keywords table. Nit: While here: delete -> deletion of (I missed this in the previous CL) https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:1107: // Tests add last_visited column in keywords table. Nit: add -> addition of
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:115: data->last_visited = base::Time(); On 2016/11/21 03:35:07, Peter Kasting wrote: > I don't really understand why |date_created| and |last_modified| were set to > Time() here, but you don't really need to fix those in this CL as that's > unrelated. > > I think setting |last_visited| to Time() is correct, but should be done in the > constructor instead of here. See comments there. Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:299: data.last_visited = Time::FromTimeT(100); On 2016/11/21 03:35:07, Peter Kasting wrote: > It may be possible to remove most of the explicit sets of this field in this > file if this value is defaulted to Time() in the constructor. Not sure. I think these explicitly setting is used for some sync test cases in this file. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:119: // date_created, the last_modified and the last_visit time. Neither pointer On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Shorter, and more future-proof: "It does not check the id or any > time-related fields." Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:924: "http://icon1", false, "UTF-8;UTF-16", Time(), Time(), Time()); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: 80 columns (several places) > > (Running git cl format before uploading can help with this) Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1025: base::Time::Now(), base::Time::Now(), base::Time::Now()); On 2016/11/21 03:35:08, Peter Kasting wrote: > Is it important that we set this to Time::Now()? > > Because if not, then I think otherwise we basically always pass Time() for this > arg, and we could just eliminate it from both AddKeywordWithDate() and > CreateKeywordWithDate(). Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1544: base::Time original_last_visited = original_url->last_visited(); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Can be const (2 places) Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1546: TemplateURL* modified_url = model()->GetTemplateURLForKeyword( On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: If you follow my next comment, then this could be inlined into the next > statement, although feel free not to do this if it decreases readability overall > (2 places) git cl format does as you suggest. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1549: EXPECT_NE(original_last_visited, modified_url->last_visited()); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Use |modifed_last_visited| Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1551: TemplateURL* reloaded_url = model()->GetTemplateURLForKeyword( On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Prefer to linebreak after '=' rather than in the midst of the parameter > list (I suspect git cl format would do this too, not sure) Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1554: // of precision when serializing and deserializing the timestamps. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Comment not necessary (this is just explaining AssertTimesEqual() itself) Done. https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1555: AssertTimesEqual(modifed_last_visited, reloaded_url ->last_visited()); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: No space before -> Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:32: // date_created, the last_modified time or the last_visited time. Neither On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: See earlier comment about wording Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/keyword_table.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:88: // Column add in version 69. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: added Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:195: " last_visited INTEGER DEFAULT 0)"); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: No leading space Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:216: *update_compatible_version = true; On 2016/11/21 03:35:08, Peter Kasting wrote: > Doesn't seem like you should set this, as we don't need to change the compatible > version. Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_data.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_data.cc:18: last_visited(base::Time::Now()), On 2016/11/21 03:35:08, Peter Kasting wrote: > Seems wrong to set last_visited to Now() (instead of just to Time()) here, as > then we can't distinguish between an engine which was never visited from one > which has been. > > (To phrase differently, I think setting this this way causes every newly-created > engine to appear to be "recently visited".) I think it is related to how we define "recently visited"? If we think if a search engine is detected but is not used for search could also serve as a visit, then this setting is the right implementation, but if it is not counted as a visit, then we should set the filed to Time()? Change it to set as Time() and will also check with Kingston to make sure this is the right behavior. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_data.h:87: // NOTE: Like data_created above, this may be 0. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: I'd change this to "NOTE: This may be 0 if the URL has never been visited." > > See also comment in .cc file. Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:152: model->UpdateTemplateURLVisitTime(existing_url); On 2016/11/21 03:35:08, Peter Kasting wrote: > I don't think the changes in this file are the right place to update the > last_visited time. > > Basically, these changes will update the time whenever we would have re-fetched > the TemplateURL. But I think we want to update instead whenever there's a > history visit that matches the template_url in question. To implement this, > you'd want to instead modify > TemplateURLService::UpdateKeywordSearchTermsForURL() to update the timestamp. > > Behavioral differences, assuming some site like http://search_engine.com that has a > page with an OSDD and a search box, and a search results page with no OSDD: > * The existing patch would mark search_engine visited if the user navigates to > http://search_engine.com and then closes the tab; my proposal does not > * The existing patch would not mark search_engine visited if the user uses > search_engine directly from the omnibox; my patch would > * The existing patch would not mark search_engine visited if the user navigates > directly to a search results page on search_engine; my patch would > > The second and third bullets above definitely seem like things we should change. > The first is more arguable, but I would claim the user hasn't actually "used" > search_engine in this case. Thanks for the info, Peter. Originally I was trying to implement in that way but had no idea this function existed. Modifying TemplateURLService::UpdateKeywordSearchTermsForURL() is definitely better than my current implementation. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_prepopulate_data.cc:1016: data->last_visited = base::Time(); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: See comments in settings_overrides_api.cc Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:730: if(UpdateNoNotify(url, TemplateURL(data))) On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: There must be a space between "if" and "("; see > http://google.github.io/styleguide/cppguide.html#Conditionals . (Multiple > places) Done. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: There are at least a couple other places that do "if (UpdateNoNotify(...)) > NotifyObservers();". Might be worth adding a private helper called Update() > that does this. There are only two places simply doing "if (UpdateNoNotify(...)) NotifyObservers();", the rest places not only call NotifyObservers() but also do some customized operations in UpdateNoNotify(...), so I am not sure about how to add an Update() function to handle those cases. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1275: se_specifics->set_last_visited(turl.last_visited().ToInternalValue()); On 2016/11/21 03:35:08, Peter Kasting wrote: > I'm confused. I thought you weren't going to sync this field's value at all in > this CL, and handle all sync issues in a followup CL? > > I think it would be less confusing to just not touch anything sync-related here > than to add the field, but only restore it in certain cases. For sync part, there are operations for UPDATE and ADD. For ADD operation, I use the value from sync data to populate the new object. However, for UPDATE, it will not update |last_visited| from sync data. So what I do in this CL is having the field passed through sync data but not prevent updating it during synchronization. The next CL will add logic to sync separately for |last_visited| dealing with UPDATE operation. The reason I do it in this way is I am not quite sure if we add the field without any sync-related, whether it will crash the app for certain case. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:291: // Update the last_visited time of TemplateURL to current time. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Comments should be declarative ("Updates") rather than imperative > ("Update"); see > http://google.github.io/styleguide/cppguide.html#Function_Comments > > Also, "current" -> "the current" Done. https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... File components/sync/protocol/search_engine_specifics.proto (right): https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... components/sync/protocol/search_engine_specifics.proto:83: // units On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Broken line wrapping > > This usually happens if git cl format reformats a comment that was too long. If > you haven't already done so, consider modifying your editor of choice so the > 80-column mark is clearly visible and you'll be unlikely to write comments that > exceed it. I know Java's maximum column size is 100 and C/C++ one is 80. But do you have any idea what are the maximum column sizes for the other types of files? I cannot find reference of them. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... File components/webdata/common/web_database.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database.cc:22: const int kCompatibleVersionNumber = 69; On 2016/11/21 03:35:08, Peter Kasting wrote: > I wouldn't update this; see comments elsewhere. Sorry I am a little confused about when should code update this |kCompatibleVersionNumber| field? Say if the change is about addition of a new field, does |kCompatibleVersionNumber| have no need to be changed? Or it is also related to whether it is optional or not? If so, how to learn/decide whether the newly added field is optional or not? https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... File components/webdata/common/web_database_migration_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:170: On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: Extra blank line Done. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:1075: // Tests delete show_in_default_list column in keywords table. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: While here: delete -> deletion of > > (I missed this in the previous CL) Done. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database_migration_unittest.cc:1107: // Tests add last_visited column in keywords table. On 2016/11/21 03:35:08, Peter Kasting wrote: > Nit: add -> addition of Done.
https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); On 2016/11/28 22:08:02, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > Nit: There are at least a couple other places that do "if > (UpdateNoNotify(...)) > > NotifyObservers();". Might be worth adding a private helper called Update() > > that does this. > > There are only two places simply doing "if (UpdateNoNotify(...)) > NotifyObservers();", the rest places not only call NotifyObservers() but also do > some customized operations in UpdateNoNotify(...), so I am not sure about how to > add an Update() function to handle those cases. Hmm, I see four, plus the one you're adding: https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... For the second one, your Update() function would need to return a bool so the caller could still do the conditional MaybeUpdateDSEAfterSync() call. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1275: se_specifics->set_last_visited(turl.last_visited().ToInternalValue()); On 2016/11/28 22:08:02, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > I'm confused. I thought you weren't going to sync this field's value at all > in > > this CL, and handle all sync issues in a followup CL? > > > > I think it would be less confusing to just not touch anything sync-related > here > > than to add the field, but only restore it in certain cases. > > For sync part, there are operations for UPDATE and ADD. For ADD operation, I use > the value from sync data to populate the new object. However, for UPDATE, it > will not update |last_visited| from sync data. > So what I do in this CL is having the field passed through sync data but not > prevent updating it during synchronization. > > The next CL will add logic to sync separately for |last_visited| dealing with > UPDATE operation. > > The reason I do it in this way is I am not quite sure if we add the field > without any sync-related, whether it will crash the app for certain case. How would the app crash? Can you say more? I am extremely leery of writing half the sync code and not the rest. It seems like this has a higher possibility of putting things into an inconsistent state. https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... File components/sync/protocol/search_engine_specifics.proto (right): https://codereview.chromium.org/2498053002/diff/20001/components/sync/protoco... components/sync/protocol/search_engine_specifics.proto:83: // units On 2016/11/28 22:08:02, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > Nit: Broken line wrapping > > > > This usually happens if git cl format reformats a comment that was too long. > If > > you haven't already done so, consider modifying your editor of choice so the > > 80-column mark is clearly visible and you'll be unlikely to write comments > that > > exceed it. > > I know Java's maximum column size is 100 and C/C++ one is 80. But do you have > any idea what are the maximum column sizes for the other types of files? I > cannot find reference of them. I don't know. I would assume 80 columns in the absence of other info. https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... File components/webdata/common/web_database.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/webdata/comm... components/webdata/common/web_database.cc:22: const int kCompatibleVersionNumber = 69; On 2016/11/28 22:08:02, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > I wouldn't update this; see comments elsewhere. > > Sorry I am a little confused about when should code update this > |kCompatibleVersionNumber| field? Say if the change is about addition of a new > field, does |kCompatibleVersionNumber| have no need to be changed? Or it is also > related to whether it is optional or not? If so, how to learn/decide whether the > newly added field is optional or not? From a chat we had a few weeks ago: "The compatible version is the minimum version of Chrome that could safely read and write the data table from the current version." So, if we add this last_visited column, and then an old version of Chrome wants to use the data file, can it? The answer is yes: when reading data out of the file, it won't SELECT the last_visited column, so it won't read that data into anywhere. When writing data out, it won't update/reset this column, which is fine, since the only effect of that is that a visit made in an old client won't be recorded for the purpose of bumping that engine in the "recently visited" standings, and we can live with that. Basically, column removals should always bump the compatible version (since they will break old code trying to read them), but column additions may or may not bump it, depending on what the consequences are if old code adds or updates the overall entry without touching that column. Since in this case things will degrade reasonably gracefully, we're OK not bumping the version.
https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); On 2016/11/28 22:20:24, Peter Kasting wrote: > On 2016/11/28 22:08:02, ltian wrote: > > On 2016/11/21 03:35:08, Peter Kasting wrote: > > > Nit: There are at least a couple other places that do "if > > (UpdateNoNotify(...)) > > > NotifyObservers();". Might be worth adding a private helper called Update() > > > that does this. > > > > There are only two places simply doing "if (UpdateNoNotify(...)) > > NotifyObservers();", the rest places not only call NotifyObservers() but also > do > > some customized operations in UpdateNoNotify(...), so I am not sure about how > to > > add an Update() function to handle those cases. > > Hmm, I see four, plus the one you're adding: > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > For the second one, your Update() function would need to return a bool so the > caller could still do the conditional MaybeUpdateDSEAfterSync() call. Do you mean my update function also needs to call MaybeUpdateDSEAfterSync() after calling NotifyObservers()? I think MaybeUpdateDSEAfterSync() is used to deal with sync data, my update function only handles local update, so is this function call needed? https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1275: se_specifics->set_last_visited(turl.last_visited().ToInternalValue()); On 2016/11/28 22:20:24, Peter Kasting wrote: > On 2016/11/28 22:08:02, ltian wrote: > > On 2016/11/21 03:35:08, Peter Kasting wrote: > > > I'm confused. I thought you weren't going to sync this field's value at all > > in > > > this CL, and handle all sync issues in a followup CL? > > > > > > I think it would be less confusing to just not touch anything sync-related > > here > > > than to add the field, but only restore it in certain cases. > > > > For sync part, there are operations for UPDATE and ADD. For ADD operation, I > use > > the value from sync data to populate the new object. However, for UPDATE, it > > will not update |last_visited| from sync data. > > So what I do in this CL is having the field passed through sync data but not > > prevent updating it during synchronization. > > > > The next CL will add logic to sync separately for |last_visited| dealing with > > UPDATE operation. > > > > The reason I do it in this way is I am not quite sure if we add the field > > without any sync-related, whether it will crash the app for certain case. > > How would the app crash? Can you say more? > > I am extremely leery of writing half the sync code and not the rest. It seems > like this has a higher possibility of putting things into an inconsistent state. So for current code, the |last_visited| will be sent together with other fields in the sync data. But when populating a TemplateURL from sync data in CreateTemplateURLFromTemplateURLAndSyncData(), it will only be used when the |existing_turl| is nullptr, which means there is no matched TemplateURL to be updated (so a new TemplateURL needs to be created from sync data). The reason I do in this way is I am not sure if we totally unsynced |last_visited|, what value is appropriate to set when Chrome finds a new TemplateURL needs to be created from sync data. And from my understanding, this should not crash the app?
https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); On 2016/11/30 00:22:18, ltian wrote: > On 2016/11/28 22:20:24, Peter Kasting wrote: > > On 2016/11/28 22:08:02, ltian wrote: > > > On 2016/11/21 03:35:08, Peter Kasting wrote: > > > > Nit: There are at least a couple other places that do "if > > > (UpdateNoNotify(...)) > > > > NotifyObservers();". Might be worth adding a private helper called > Update() > > > > that does this. > > > > > > There are only two places simply doing "if (UpdateNoNotify(...)) > > > NotifyObservers();", the rest places not only call NotifyObservers() but > also > > do > > > some customized operations in UpdateNoNotify(...), so I am not sure about > how > > to > > > add an Update() function to handle those cases. > > > > Hmm, I see four, plus the one you're adding: > > > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > > For the second one, your Update() function would need to return a bool so the > > caller could still do the conditional MaybeUpdateDSEAfterSync() call. > > Do you mean my update function also needs to call MaybeUpdateDSEAfterSync() > after calling NotifyObservers()? No. The caller would do that. Your function would need to return a bool indicating whether an update happened (as UpdateNoNotify() currently does), so the caller can decide to do that. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1275: se_specifics->set_last_visited(turl.last_visited().ToInternalValue()); On 2016/11/30 00:22:18, ltian wrote: > On 2016/11/28 22:20:24, Peter Kasting wrote: > > On 2016/11/28 22:08:02, ltian wrote: > > > On 2016/11/21 03:35:08, Peter Kasting wrote: > > > > I'm confused. I thought you weren't going to sync this field's value at > all > > > in > > > > this CL, and handle all sync issues in a followup CL? > > > > > > > > I think it would be less confusing to just not touch anything sync-related > > > here > > > > than to add the field, but only restore it in certain cases. > > > > > > For sync part, there are operations for UPDATE and ADD. For ADD operation, I > > use > > > the value from sync data to populate the new object. However, for UPDATE, it > > > will not update |last_visited| from sync data. > > > So what I do in this CL is having the field passed through sync data but not > > > prevent updating it during synchronization. > > > > > > The next CL will add logic to sync separately for |last_visited| dealing > with > > > UPDATE operation. > > > > > > The reason I do it in this way is I am not quite sure if we add the field > > > without any sync-related, whether it will crash the app for certain case. > > > > How would the app crash? Can you say more? > > > > I am extremely leery of writing half the sync code and not the rest. It seems > > like this has a higher possibility of putting things into an inconsistent > state. > > So for current code, the |last_visited| will be sent together with other fields > in the sync data. But when populating a TemplateURL from sync data in > CreateTemplateURLFromTemplateURLAndSyncData(), it will only be used when the > |existing_turl| is nullptr, which means there is no matched TemplateURL to be > updated (so a new TemplateURL needs to be created from sync data). The reason I > do in this way is I am not sure if we totally unsynced |last_visited|, what > value is appropriate to set when Chrome finds a new TemplateURL needs to be > created from sync data. I would leave it as whatever the default value provided by the constructor is (i.e. Time()). > And from my understanding, this should not crash the > app? I don't see a reason to think things would crash.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/20001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1546: TemplateURL* modified_url = model()->GetTemplateURLForKeyword( On 2016/11/28 22:08:01, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > Nit: If you follow my next comment, then this could be inlined into the next > > statement, although feel free not to do this if it decreases readability > overall > > (2 places) > > git cl format does as you suggest. "Inlined into the next statement" isn't about line wrapping, but about replacing the lone use of |modified_url| in the subsequent statement with this expression that's used to initialize it. https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/keyword_table.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/keyword_table.cc:195: " last_visited INTEGER DEFAULT 0)"); On 2016/11/28 22:08:02, ltian wrote: > On 2016/11/21 03:35:08, Peter Kasting wrote: > > Nit: No leading space > > Done. No... I meant "no leading space inside the quotation mark on this last line". As for the indentation of the lines themselves, I have no idea; I'd have thought the previous indenting was better, is this what git cl format did? https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:731: NotifyObservers(); On 2016/11/30 00:39:37, Peter Kasting wrote: > On 2016/11/30 00:22:18, ltian wrote: > > On 2016/11/28 22:20:24, Peter Kasting wrote: > > > On 2016/11/28 22:08:02, ltian wrote: > > > > On 2016/11/21 03:35:08, Peter Kasting wrote: > > > > > Nit: There are at least a couple other places that do "if > > > > (UpdateNoNotify(...)) > > > > > NotifyObservers();". Might be worth adding a private helper called > > Update() > > > > > that does this. > > > > > > > > There are only two places simply doing "if (UpdateNoNotify(...)) > > > > NotifyObservers();", the rest places not only call NotifyObservers() but > > also > > > do > > > > some customized operations in UpdateNoNotify(...), so I am not sure about > > how > > > to > > > > add an Update() function to handle those cases. > > > > > > Hmm, I see four, plus the one you're adding: > > > > > > > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > > > > https://cs.chromium.org/chromium/src/components/search_engines/template_url_s... > > > > > > For the second one, your Update() function would need to return a bool so > the > > > caller could still do the conditional MaybeUpdateDSEAfterSync() call. > > > > Do you mean my update function also needs to call MaybeUpdateDSEAfterSync() > > after calling NotifyObservers()? > > No. The caller would do that. Your function would need to return a bool > indicating whether an update happened (as UpdateNoNotify() currently does), so > the caller can decide to do that. Note: This whole conversation was about the Update() method I was suggesting, NOT about UpdateTemplateURLVisitTime(). https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:32: // Checks that the two TemplateURLs are similar. Does not check the id and Nit: and -> or https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:33: // any time-related fields. Neither pointer should be NULL. Nit: While here: NULL -> null https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data.h:115: // NOTE: This might be 0 if the TemplateURL has never been visited.. Nit: Only a single trailing period https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_data_util.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data_util.cc:81: if (base::StringToInt64(last_modified_str, &last_modified)) { Woah, this looks like it was a flat-out existing bug. Can we add a test that would have caught this? (Maybe this fix + the test should be done in a separate CL so it's easy to review and/or merge back?) https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data_util.cc:86: if (base::StringToInt64(last_visited_str, &last_visited)) { Nit: This file is inconsistent about whether it uses {} or not on single-line conditional bodies. Here and in the two conditionals above we do, but in the innermost two conditional just below we don't. While here, I would make these consistent. I'd probably omit {} as that's more common within Chromium. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_fetcher.cc:204: TemplateURL* template_url = Restore the const here https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1806: visited_url = *i; Why copy this into a temp and then make the call below? Why not just do that directly here? https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1809: if (visited_url != nullptr) { Nit: No {} I would elide the "!= nullptr" as well. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.h:292: bool UpdateTemplateURLVisitTime(TemplateURL* url); This is only public due to being used in a test. I'm inclined to make it private instead and FRIEND_TEST the test, as we don't really want any non-test code calling this. I don't understand why this function returns a value that no one ever reads. I think you must have misunderstood my request that you add an Update() function that would do UpdateNoNotify() + NotifyObservers(), and instead thought I was talking about this function, and you added a return value to it? I'm very confused, and just trying to go off how there's an extra return value here, whereas there's no Update() (but maybe you intended to add that in a separate cleanup patch, which would be fine).
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:32: // Checks that the two TemplateURLs are similar. Does not check the id and On 2016/12/01 07:38:24, Peter Kasting wrote: > Nit: and -> or Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:33: // any time-related fields. Neither pointer should be NULL. On 2016/12/01 07:38:24, Peter Kasting wrote: > Nit: While here: NULL -> null Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data.h:115: // NOTE: This might be 0 if the TemplateURL has never been visited.. On 2016/12/01 07:38:24, Peter Kasting wrote: > Nit: Only a single trailing period Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_data_util.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data_util.cc:81: if (base::StringToInt64(last_modified_str, &last_modified)) { On 2016/12/01 07:38:24, Peter Kasting wrote: > Woah, this looks like it was a flat-out existing bug. Can we add a test that > would have caught this? (Maybe this fix + the test should be done in a separate > CL so it's easy to review and/or merge back?) Yeah, this is the reason caused the problem that the last_modified resets back once a TemplateURL is set as default. I will change it back and create a new CL to fix this bug and add a test for it. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_data_util.cc:86: if (base::StringToInt64(last_visited_str, &last_visited)) { On 2016/12/01 07:38:24, Peter Kasting wrote: > Nit: This file is inconsistent about whether it uses {} or not on single-line > conditional bodies. Here and in the two conditionals above we do, but in the > innermost two conditional just below we don't. > > While here, I would make these consistent. I'd probably omit {} as that's more > common within Chromium. Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_fetcher.cc:204: TemplateURL* template_url = On 2016/12/01 07:38:24, Peter Kasting wrote: > Restore the const here Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1806: visited_url = *i; On 2016/12/01 07:38:24, Peter Kasting wrote: > Why copy this into a temp and then make the call below? Why not just do that > directly here? Directly call it here will cause the error "attempt to increment a singular iterator." which seems mean the container of iterator is modified (maybe by UpdateNoNotify()?). Caching it and calling it later could avoid this problem. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1809: if (visited_url != nullptr) { On 2016/12/01 07:38:25, Peter Kasting wrote: > Nit: No {} > > I would elide the "!= nullptr" as well. Done. https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.h:292: bool UpdateTemplateURLVisitTime(TemplateURL* url); On 2016/12/01 07:38:25, Peter Kasting wrote: > This is only public due to being used in a test. I'm inclined to make it > private instead and FRIEND_TEST the test, as we don't really want any non-test > code calling this. > > I don't understand why this function returns a value that no one ever reads. I > think you must have misunderstood my request that you add an Update() function > that would do UpdateNoNotify() + NotifyObservers(), and instead thought I was > talking about this function, and you added a return value to it? I'm very > confused, and just trying to go off how there's an extra return value here, > whereas there's no Update() (but maybe you intended to add that in a separate > cleanup patch, which would be fine). Sorry I totally misunderstood of your thought. Will fix it.
https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2498053002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1806: visited_url = *i; On 2016/12/01 10:02:58, ltian wrote: > On 2016/12/01 07:38:24, Peter Kasting wrote: > > Why copy this into a temp and then make the call below? Why not just do that > > directly here? > > Directly call it here will cause the error "attempt to increment a singular > iterator." which seems mean the container of iterator is modified (maybe by > UpdateNoNotify()?). Caching it and calling it later could avoid this problem. I understand this now. You're correct that ultimately UpdateNoNotify will ask the host-to-url map to remove an entry, which will modify the container as we touch it. This can be fixed in a way that will save underlying work. If UpdateTemplateURLVisitTime() just changes the last visited time directly (instead of using UpdateNoNotify()), you should be fine putting that call here. This is legal because TemplateURLService is a friend of TemplateURL, and safe because you're not touching any fields that require the map updates UpdateNoNotify() performs.
On 2016/12/01 19:32:35, Peter Kasting wrote: > This can be fixed in a way that will save underlying work. If > UpdateTemplateURLVisitTime() just changes the last visited time directly > (instead of using UpdateNoNotify()), you should be fine putting that call here. > This is legal because TemplateURLService is a friend of TemplateURL, and safe > because you're not touching any fields that require the map updates > UpdateNoNotify() performs. Clarification. As you pointed out to me over IM, there is some work UpdateNoNotify() does that we should still do. Specifically, everything at the end of the function from "if (web_data_service_.get())" onwards also needs to be done in your case. There are several ways to handle this. (1) Leave the code as you have it today. (2) Pass some kind of conditional into UpdateNoNotify() about whether to do the other work above this point. (3) Have UpdateNoNotify() detect piecewise whether the new data differs form the old in various ways, and only run the parts of updating that make sense. I think this is mostly about keyword changes (if the keyword doesn't change, probably a lot of stuff doesn't have to run), but URL changes probably matter too. These are increasing amounts of work for potentially increasing amounts of payoff. For example, (3) could save time and effort during other types of updates as well, not just last visited changes. My suggestion to you would be to do (1) or (2) in this patch, and file a followup bug (possibly against yourself?) for (2) or (3).
On 2016/12/02 02:07:33, Peter Kasting wrote: > On 2016/12/01 19:32:35, Peter Kasting wrote: > > This can be fixed in a way that will save underlying work. If > > UpdateTemplateURLVisitTime() just changes the last visited time directly > > (instead of using UpdateNoNotify()), you should be fine putting that call > here. > > This is legal because TemplateURLService is a friend of TemplateURL, and safe > > because you're not touching any fields that require the map updates > > UpdateNoNotify() performs. > > Clarification. As you pointed out to me over IM, there is some work > UpdateNoNotify() does that we should still do. Specifically, everything at the > end of the function from "if (web_data_service_.get())" onwards also needs to be > done in your case. > > There are several ways to handle this. > > (1) Leave the code as you have it today. > (2) Pass some kind of conditional into UpdateNoNotify() about whether to do the > other work above this point. > (3) Have UpdateNoNotify() detect piecewise whether the new data differs form the > old in various ways, and only run the parts of updating that make sense. I > think this is mostly about keyword changes (if the keyword doesn't change, > probably a lot of stuff doesn't have to run), but URL changes probably matter > too. > > These are increasing amounts of work for potentially increasing amounts of > payoff. For example, (3) could save time and effort during other types of > updates as well, not just last visited changes. > > My suggestion to you would be to do (1) or (2) in this patch, and file a > followup bug (possibly against yourself?) for (2) or (3). For this CL, I prefer keeping with my current implementation. I will file a bug and create a CL to fix it in way (2) or (3) in future.
On 2016/12/02 02:23:28, ltian wrote: > For this CL, I prefer keeping with my current implementation. I will file a bug > and create > a CL to fix it in way (2) or (3) in future. I would definitely add comments (and a TODO?) for now then about why this has to be saved and then updated outside the loop.
LGTM https://codereview.chromium.org/2498053002/diff/120001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2498053002/diff/120001/components/search_engi... components/search_engines/template_url_service.h:572: // Updates the last_visited time of TemplateURL to the current time. Nit: TemplateURL -> |url|
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2498053002/#ps140001 (title: "Update Nit comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480721597718990, "parent_rev": "b2ba9bf2e63c2f159c356637d0fd851d1d53bfa7", "commit_rev": "3bf9e1e881d3c146c69aebd2614f0891732a5cc9"}
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add field to monitor last visited time for each search engine 1. Add the last_visited field in template_url_data to monitor last visited time of each search engine. 2. The last_visited will be updated to current time each time the OpenSearch description document is detected. 3. For now, last_visited will not be synced if the template_url is existed. BUG=348360 ========== to ========== Add field to monitor last visited time for each search engine 1. Add the last_visited field in template_url_data to monitor last visited time of each search engine. 2. The last_visited will be updated to current time each time the OpenSearch description document is detected. 3. For now, last_visited will not be synced if the template_url is existed. BUG=348360 Committed: https://crrev.com/8b5fc83dba9f525b0f707433a9fbb91465ca9bdd Cr-Commit-Position: refs/heads/master@{#436079} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8b5fc83dba9f525b0f707433a9fbb91465ca9bdd Cr-Commit-Position: refs/heads/master@{#436079} |