Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699243002:
Remove CRWSessionEntry from CRWSessionController's unittests. (Closed)
|
Created:
3 years, 10 months ago by kkhorimoto Modified:
3 years, 9 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, Eugene But (OOO till 7-30) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove CRWSessionEntry from CRWSessionController's unittests.
BUG=454984
Review-Url: https://codereview.chromium.org/2699243002
Cr-Commit-Position: refs/heads/master@{#455029}
Committed: https://chromium.googlesource.com/chromium/src/+/36f8c56e394d7feddcdbf6084ca30e1b096c2c12
Patch Set 1 #
Total comments: 19
Patch Set 2 : rebase + eugene's comments #Patch Set 3 : used EmptyGURL() #Messages
Total messages: 11 (5 generated)
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org
lgtm https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:35: return GURL(); nit: GURL::EmptyURL() and you can keep the only reference signature https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:39: return self.currentItem ? self.currentItem->GetURL() : GURL(); ditto https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:70: EXPECT_EQ(nullptr, [session_controller_ currentItem]); nit: EXPECT_FALSE https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:229: EXPECT_EQ(nullptr, [session_controller_ currentItem]); ditto https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:294: EXPECT_EQ(nullptr, [session_controller_ currentItem]); ditto https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:373: web::NavigationItem* pendingItem = [session_controller_ pendingItem]; Do you want to drop this variable? https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:374: ASSERT_NE(nullptr, pendingItem); nit: EXPECT_TRUE https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:394: EXPECT_EQ(nullptr, [session_controller_ currentItem]); nit: EXPECT_FALSE https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:419: EXPECT_EQ(nullptr, [session_controller_ currentItem]); ditto
https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:35: return GURL(); On 2017/02/23 16:55:17, Eugene But wrote: > nit: GURL::EmptyURL() and you can keep the only reference signature Maybe this was removed, but I can't seem to find GURL::EmptyURL() anywhere in the codebase. Leaving as is for now. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:39: return self.currentItem ? self.currentItem->GetURL() : GURL(); On 2017/02/23 16:55:17, Eugene But wrote: > ditto Acknowledged. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:70: EXPECT_EQ(nullptr, [session_controller_ currentItem]); On 2017/02/23 16:55:17, Eugene But wrote: > nit: EXPECT_FALSE Done. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:229: EXPECT_EQ(nullptr, [session_controller_ currentItem]); On 2017/02/23 16:55:17, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:294: EXPECT_EQ(nullptr, [session_controller_ currentItem]); On 2017/02/23 16:55:17, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:373: web::NavigationItem* pendingItem = [session_controller_ pendingItem]; On 2017/02/23 16:55:17, Eugene But wrote: > Do you want to drop this variable? It's used below as well. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:374: ASSERT_NE(nullptr, pendingItem); On 2017/02/23 16:55:17, Eugene But wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:394: EXPECT_EQ(nullptr, [session_controller_ currentItem]); On 2017/02/23 16:55:17, Eugene But wrote: > nit: EXPECT_FALSE Done. https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:419: EXPECT_EQ(nullptr, [session_controller_ currentItem]); On 2017/02/23 16:55:17, Eugene But wrote: > ditto Done.
https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2699243002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller_unittest.mm:35: return GURL(); On 2017/03/02 01:11:48, kkhorimoto_ wrote: > On 2017/02/23 16:55:17, Eugene But wrote: > > nit: GURL::EmptyURL() and you can keep the only reference signature > > Maybe this was removed, but I can't seem to find GURL::EmptyURL() anywhere in > the codebase. Leaving as is for now. Sorry, EmptyGURL
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2699243002/#ps40001 (title: "used EmptyGURL()")
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": 40001, "attempt_start_ts": 1488855104130770,
"parent_rev": "8c80b233da12ca81bece40f77b990a84a13c75a2", "commit_rev":
"36f8c56e394d7feddcdbf6084ca30e1b096c2c12"}
Message was sent while issue was closed.
Description was changed from ========== Remove CRWSessionEntry from CRWSessionController's unittests. BUG=454984 ========== to ========== Remove CRWSessionEntry from CRWSessionController's unittests. BUG=454984 Review-Url: https://codereview.chromium.org/2699243002 Cr-Commit-Position: refs/heads/master@{#455029} Committed: https://chromium.googlesource.com/chromium/src/+/36f8c56e394d7feddcdbf6084ca3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/36f8c56e394d7feddcdbf6084ca3... |
