Issue metadata
Sign in to add a comment
|
Find a maintainable solution to replace |-currentItem| |
||||||||||||||||||||||||
Issue description
As we are getting rid of |-currentItem| from CRWSessionController, the logic is being replaced by the following code:
web::NavigationItem* item = GetTransientItem();
if (!item)
item = GetPendingItem();
if (!item)
item = GetLastCommittedItem();
And this logic gets copied everywhere, which is error-prone and unmaintainable, so we need a better solution.
,
Mar 30 2017
BTW, do we know how Android handle this problem?
,
Mar 30 2017
I think we still need to wrap this logic inside NavigationManager(Impl), but with a better name. I have a few thoughts, which may or may not make sense: 1. |GetItemToBeReloaded|. In my understanding, the reason why "current item" is used extensively in our code is that "current item" is the item that is going to be reloaded, like reading list and user agent etc. If this is true, then |GetItemToBeReloaded| would be least confusing. 2. |GetMostRecentItem|. It seems that "current item" always refers to the item that is most recently added. If there is a transient item, then it must be the most recently added item because any navigation would discard the transient item; If there is a pending item, but no transient item, then pending item must be the most recently added item because committing a pending item discards the pending item.
,
Mar 31 2017
NavigationController has GetActiveEntry(). Seems like a reasonable name to me... https://cs.chromium.org/chromium/src/content/public/browser/navigation_controller.h?dr=CSs&l=249 https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?dr=CSs&l=471
,
Mar 31 2017
// THIS IS DEPRECATED. DO NOT USE. Use GetVisibleEntry instead. // See http://crbug.com/273710.
,
Mar 31 2017
Kurt, |GetActiveEntry| is deprecated, see comments: // THIS IS DEPRECATED. DO NOT USE. Use GetVisibleEntry instead. // See http://crbug.com/273710. Yuke, ios/web should not diverge from //content if possible. And in the case of Desktop UserAgent following //content seems possible.
,
Mar 31 2017
I'm very curious does Android reload current item or visible item? but I could't understand most of the code :(
,
Apr 13 2017
,
Apr 27 2017
I was recently poking around NavigationController, and their use GetCurrentEntryIndex(), which seems to serve similar function as currentItem in CRWSessionController: https://cs.chromium.org/chromium/src/content/public/browser/navigation_controller.h?l=265 https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?l=518 The behavior does not seem to be identical though. For example, it will not find new pending entry and transient entry because pending_entry_index_ and transient_entry_index_ are -1 in these cases respectively, whereas currentItem would return pendingItem or transientItem.
,
Jul 10 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by eugene...@chromium.org
, Mar 30 2017