Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2059513002:
Remove obsoleted function and add test (Closed)
|
Created:
4 years, 6 months ago by Vivian Modified:
4 years, 6 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove obsoleted function and add test
Remove OfflinePageUtils::MaybeGetOfflineURLForOnlineURL
Add OfflinePageUtilsTest::GetOfflineURLForOnlineURL
BUG=618732
Committed: https://crrev.com/61ebf56ef4cba2a43cf7a67091fc4f188f7716e4
Cr-Commit-Position: refs/heads/master@{#399255}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add RunUntilIdle and Style fix #
Total comments: 14
Patch Set 3 : Formatting and edit unittest #Patch Set 4 : Remove backup file #
Total comments: 4
Patch Set 5 : Edit parameter of helper function #
Total comments: 2
Patch Set 6 : Remove empty lines #
Messages
Total messages: 27 (10 generated)
weiran@google.com changed reviewers: + dewittj@chromium.org
ptal
thanks for working on this! https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_utils.h (right): https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils.h:27: // Deprecated. Use |GetOfflineURLForOnlineURL|. Please remove the deprecation line and the blank line so that this comment becomes a comment for |GetOfflineURLForOnlineURL|. https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:74: GURL url() const { return url_; } nit: const GURL& https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:202: OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())); It doesn't look like this is waiting for the call to complete so url() will never be set by the time you call it. Looks like you need a RunUntilIdle(); here, but I'd recommend making a helper function : GURL OfflinePageUtilsTest::GetOfflineURLForOnlineURL(GURL online_url, base::Callback<void(const GURL&)> callback) that does the actual OfflinePageUtils call, runs until idle, and then returns url(). That'd make the actual test code more straightforward.
On 2016/06/09 22:13:58, dewittj wrote: > thanks for working on this! > > https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/offline_page_utils.h (right): > > https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/offline_page_utils.h:27: // Deprecated. > Use |GetOfflineURLForOnlineURL|. > Please remove the deprecation line and the blank line so that this comment > becomes a comment for |GetOfflineURLForOnlineURL|. > > https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc > (right): > > https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:74: GURL > url() const { return url_; } > nit: const GURL& > > https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:202: > OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), kTestPage1Url, > base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())); > It doesn't look like this is waiting for the call to complete so url() will > never be set by the time you call it. > > Looks like you need a RunUntilIdle(); here, but I'd recommend making a helper > function : > > GURL OfflinePageUtilsTest::GetOfflineURLForOnlineURL(GURL online_url, > base::Callback<void(const GURL&)> callback) > > that does the actual OfflinePageUtils call, runs until idle, and then returns > url(). That'd make the actual test code more straightforward. Thanks for your comment! Made some change according to the suggestions.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
Vivian, Nice patch. Thanks for working on it. General comment: use the formatting tool and update the setup of your editor. Also, I think you included a backup file or something else you were not supposed to. I can help you get rid of it tomorrow. My comments are mostly nits. I included link to style guide, but you don't have to look at it until after you are done with this patch. Thanks. Filip https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:57: base::Callback<void(const GURL&)> callback); nit: you need to set up your editor to convert tabs to spaces properly Another trick to get it right is to run "git cl format" before submitting https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:81: const GURL& url() { return url_; } nit: Signature of this method can be updated. const GURL& url() const { return url_; } Adding the second const means that the method does not change the state of the class and hence can be called when you only have const reference to it. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:149: base::Callback<void(const GURL&)> callback) { This line is too long. Again "git cl format" handles that for you. Chromium allows 80 chars per line in C++ code and 100 chars per line in Java code. Please take a look here: http://www.chromium.org/developers/coding-style after you are done with this patch and start familiarizing yourself with the coding style (C++ and Java). Consistency will help you read the code later on (once you get used to it). https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:152: return url(); return url_; This should work for you. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:233: kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()))); Lines in this test are too long as well. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:239: kTestPage3Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()))); Please use GURL::EmptyGURL() in place of GURL() Also, it seems that you need to add one more EXPECT_EQ based on the rebased version on the left. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:242: nit: use one empty line only to separate methods.
Just a quick note: it's customary to reply to each comment that a reviewer makes, so that it's easy to tell which things have been resolved and which haven't. Even if it's just "Done." that really helps. Thanks!
Patchset #4 (id:60001) has been deleted
Thanks for the comments. I made some change accordingly, did format before upload, and removed the backup file. https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_utils.h (right): https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils.h:27: // Deprecated. Use |GetOfflineURLForOnlineURL|. On 2016/06/09 22:13:58, dewittj wrote: > Please remove the deprecation line and the blank line so that this comment > becomes a comment for |GetOfflineURLForOnlineURL|. Done. https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:74: GURL url() const { return url_; } On 2016/06/09 22:13:58, dewittj wrote: > nit: const GURL& Done. https://codereview.chromium.org/2059513002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:202: OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())); On 2016/06/09 22:13:58, dewittj wrote: > It doesn't look like this is waiting for the call to complete so url() will > never be set by the time you call it. > > Looks like you need a RunUntilIdle(); here, but I'd recommend making a helper > function : > > GURL OfflinePageUtilsTest::GetOfflineURLForOnlineURL(GURL online_url, > base::Callback<void(const GURL&)> callback) > > that does the actual OfflinePageUtils call, runs until idle, and then returns > url(). That'd make the actual test code more straightforward. Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:57: base::Callback<void(const GURL&)> callback); On 2016/06/10 04:46:24, fgorski wrote: > nit: you need to set up your editor to convert tabs to spaces properly > > Another trick to get it right is to run "git cl format" before submitting Done. I used git cl format for now. Will set up editor later. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:81: const GURL& url() { return url_; } On 2016/06/10 04:46:24, fgorski wrote: > nit: Signature of this method can be updated. > const GURL& url() const { return url_; } > > Adding the second const means that the method does not change the state of the > class and hence can be called when you only have const reference to it. Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:149: base::Callback<void(const GURL&)> callback) { On 2016/06/10 04:46:25, fgorski wrote: > This line is too long. Again "git cl format" handles that for you. > > Chromium allows 80 chars per line in C++ code and 100 chars per line in Java > code. > > Please take a look here: http://www.chromium.org/developers/coding-style after > you are done with this patch and start familiarizing yourself with the coding > style (C++ and Java). > Consistency will help you read the code later on (once you get used to it). Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:152: return url(); On 2016/06/10 04:46:24, fgorski wrote: > return url_; > > This should work for you. Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:233: kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()))); On 2016/06/10 04:46:25, fgorski wrote: > Lines in this test are too long as well. Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:239: kTestPage3Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()))); On 2016/06/10 04:46:24, fgorski wrote: > Please use GURL::EmptyGURL() in place of GURL() > > Also, it seems that you need to add one more EXPECT_EQ based on the rebased > version on the left. Done. https://codereview.chromium.org/2059513002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:242: On 2016/06/10 04:46:25, fgorski wrote: > nit: use one empty line only to separate methods. Done.
almost there. Just a few comments https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:81: const GURL& url() const { return url_; } I think this is unused... can remove it. https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:150: base::Callback<void(const GURL&)> callback) { No need to pass in callback, you can just refer to it directly in the call in this method.
Thanks for pointing it out! I'll watch out my coding style:) https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:81: const GURL& url() const { return url_; } On 2016/06/10 17:46:10, dewittj wrote: > I think this is unused... can remove it. Done. https://codereview.chromium.org/2059513002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:150: base::Callback<void(const GURL&)> callback) { On 2016/06/10 17:46:10, dewittj wrote: > No need to pass in callback, you can just refer to it directly in the call in > this method. Done.
lgtm Since fgorski is also reviewing, please wait for him to give lgtm before you check the commit queue box.
lgtm with nit. https://codereview.chromium.org/2059513002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:232: nit: you don't need empty lines. Other tests are written without them.
The CQ bit was checked by weiran@google.com to run a CQ dry run
The CQ bit was unchecked by weiran@google.com
The CQ bit was checked by weiran@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2059513002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Deleted empty lines. Passed dry run. Proceed to commit. https://codereview.chromium.org/2059513002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2059513002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:232: On 2016/06/10 18:03:47, fgorski wrote: > nit: you don't need empty lines. Other tests are written without them. Done.
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2059513002/#ps120001 (title: "Remove empty lines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2059513002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Remove obsoleted function and add test Remove OfflinePageUtils::MaybeGetOfflineURLForOnlineURL Add OfflinePageUtilsTest::GetOfflineURLForOnlineURL BUG=618732 ========== to ========== Remove obsoleted function and add test Remove OfflinePageUtils::MaybeGetOfflineURLForOnlineURL Add OfflinePageUtilsTest::GetOfflineURLForOnlineURL BUG=618732 Committed: https://crrev.com/61ebf56ef4cba2a43cf7a67091fc4f188f7716e4 Cr-Commit-Position: refs/heads/master@{#399255} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/61ebf56ef4cba2a43cf7a67091fc4f188f7716e4 Cr-Commit-Position: refs/heads/master@{#399255} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
