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

Issue 2690913003: Revert "Updated ownership of NavigationItems within CRWSessionController." (Closed)

Created:
3 years, 10 months ago by kkhorimoto
Modified:
3 years, 10 months ago
CC:
chromium-reviews, marq+watch_chromium.org, Eugene But (OOO till 7-30), pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert "Updated ownership of NavigationItems within CRWSessionController." The original CL updated behavior in a couple ways that were introducing many bugs: - New CRWSessionEntries were vended every time a session entry property was accessed, which which broke logic comparing CRWSessionEntries. - CRWSessionEntries held an unowned pointer to their corresponding NavigationItems, making it possible for the CRWSessionEntry to outlive its backing NavigationItem. The CL will be re-landed once callers have been updated to reflect this new behavior. This reverts 3 CLs: https://codereview.chromium.org/2672953005 (original CL) https://codereview.chromium.org/2690973002 (hacky fix) https://codereview.chromium.org/2680313004 (hacky fix) BUG=689358, 691634, 545227, 691492, 691469 Review-Url: https://codereview.chromium.org/2690913003 Cr-Commit-Position: refs/heads/master@{#450156} Committed: https://chromium.googlesource.com/chromium/src/+/2b55257be75ef0790299a2a239fbb8d33ec4e805

Patch Set 1 #

Patch Set 2 : Revert olivier's CLs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+703 lines, -632 lines) Patch
M ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm View 4 chunks +60 lines, -46 lines 0 comments Download
M ios/web/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M ios/web/navigation/crw_session_controller.h View 3 chunks +5 lines, -5 lines 0 comments Download
M ios/web/navigation/crw_session_controller.mm View 20 chunks +320 lines, -315 lines 0 comments Download
M ios/web/navigation/crw_session_controller_unittest.mm View 40 chunks +248 lines, -207 lines 0 comments Download
M ios/web/navigation/crw_session_entry.h View 3 chunks +5 lines, -2 lines 0 comments Download
M ios/web/navigation/crw_session_entry.mm View 1 1 chunk +25 lines, -17 lines 0 comments Download
M ios/web/navigation/crw_session_entry_unittest.mm View 1 chunk +8 lines, -8 lines 0 comments Download
M ios/web/navigation/navigation_item_impl_list.h View 1 chunk +0 lines, -4 lines 0 comments Download
M ios/web/navigation/navigation_item_impl_list.mm View 1 chunk +0 lines, -9 lines 0 comments Download
M ios/web/navigation/session_storage_builder.mm View 1 chunk +2 lines, -1 line 0 comments Download
M ios/web/public/navigation_item_list.h View 1 chunk +5 lines, -0 lines 0 comments Download
A ios/web/public/navigation_item_list.mm View 1 chunk +17 lines, -0 lines 0 comments Download
M ios/web/web_state/ui/crw_web_controller.mm View 1 2 chunks +1 line, -12 lines 0 comments Download
M ios/web/web_state/ui/crw_web_controller_unittest.mm View 1 chunk +6 lines, -6 lines 0 comments Download

Messages

Total messages: 21 (17 generated)
kkhorimoto
I'm testing whether this definitively fixes all the bugs reported this morning; will update the ...
3 years, 10 months ago (2017-02-13 22:41:20 UTC) #10
Eugene But (OOO till 7-30)
lgtm
3 years, 10 months ago (2017-02-13 23:12:24 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690913003/20001
3 years, 10 months ago (2017-02-13 23:45:40 UTC) #18
commit-bot: I haz the power
3 years, 10 months ago (2017-02-13 23:53:17 UTC) #21
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2b55257be75ef0790299a2a239fb...

Powered by Google App Engine
This is Rietveld 408576698