Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2327083002:
Ntp: restore scroll position. (Closed)
|
Created:
4 years, 3 months ago by Michael van Ouwerkerk Modified:
4 years, 2 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNtp: restore scroll position.
* The scroll position is stored as extra data on the NavigationEntry.
* The main use case handled is when the user clicks on a suggested
article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
* It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
* Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.
BUG=606356
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
Cr-Commit-Position: refs/heads/master@{#420075}
Patch Set 1 #Patch Set 2 : Use adapter position and store it in Tab. #Patch Set 3 : Store scroll position in extra data. #Patch Set 4 : Cleanups. #
Total comments: 12
Patch Set 5 : Address review comments from bauerb. Fix test build. #Patch Set 6 : fix build (const) #
Total comments: 4
Patch Set 7 : Adrress review comments from clamy. #
Total comments: 3
Patch Set 8 : Rebase. #
Total comments: 6
Patch Set 9 : Address review comments from tedchoc. #Patch Set 10 : Reland: Ntp: restore scroll position. #Messages
Total messages: 64 (41 generated)
The CQ bit was checked by mvanouwerkerk@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...
mvanouwerkerk@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, this is the simple approach, which doesn't work. I would probably need to hook into the NavigationEntry at a different time, when the PageState is still writable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by mvanouwerkerk@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.
Description was changed from ========== Ntp: restore scroll position. BUG=606356 ========== to ========== Ntp: restore scroll position. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by mvanouwerkerk@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 checked by mvanouwerkerk@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...
Hi Bernhard, this now uses the extra data storage in NavigationEntry itself. It no longer touches PageState. It also actually works :-) Please take a look.
Nice! https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:835: return 0; Would it be slightly nicer to return NO_POSITION here to indicate the absence of a value? https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:269: if (position == NO_POSITION) position = 0; Actually, we could probably return NO_POSITION in this case as well, and then just not write it to the NavigationEntry. https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1447: NavigationEntry entry = getWebContents().getNavigationController().getEntryAtIndex( Extract the NavigationController to a local variable? https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1462: nativeSetLastCommittedNavigationEntryExtraData(mNativeTabAndroid, key, value); Would it make sense to move the JNI call to NavigationController so it's a bit more symmetric with the getter above? https://codereview.chromium.org/2327083002/diff/60001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/browser/android/... chrome/browser/android/tab_android.cc:671: if (jkey) I don't think we expect either of these to be null. https://codereview.chromium.org/2327083002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2327083002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:143: void GetExtraData(std::map<std::string, base::string16>* data) override; You could just return a const ref to the map...
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_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Thanks! Please take another look. https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:835: return 0; On 2016/09/16 15:02:50, Bernhard Bauer wrote: > Would it be slightly nicer to return NO_POSITION here to indicate the absence of > a value? Done. https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:269: if (position == NO_POSITION) position = 0; On 2016/09/16 15:02:50, Bernhard Bauer wrote: > Actually, we could probably return NO_POSITION in this case as well, and then > just not write it to the NavigationEntry. Done. https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1447: NavigationEntry entry = getWebContents().getNavigationController().getEntryAtIndex( On 2016/09/16 15:02:50, Bernhard Bauer wrote: > Extract the NavigationController to a local variable? Done. https://codereview.chromium.org/2327083002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1462: nativeSetLastCommittedNavigationEntryExtraData(mNativeTabAndroid, key, value); On 2016/09/16 15:02:50, Bernhard Bauer wrote: > Would it make sense to move the JNI call to NavigationController so it's a bit > more symmetric with the getter above? Done. https://codereview.chromium.org/2327083002/diff/60001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2327083002/diff/60001/chrome/browser/android/... chrome/browser/android/tab_android.cc:671: if (jkey) On 2016/09/16 15:02:50, Bernhard (slow until Sep 27) wrote: > I don't think we expect either of these to be null. Done. https://codereview.chromium.org/2327083002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2327083002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:143: void GetExtraData(std::map<std::string, base::string16>* data) override; On 2016/09/16 15:02:50, Bernhard (slow until Sep 27) wrote: > You could just return a const ref to the map... Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mvanouwerkerk@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, could you take a look please?
Description was changed from ========== Ntp: restore scroll position. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ntp: restore scroll position. * The scroll position is stored as extra data on the NavigationEntry. * The main use case handled is when the user clicks on a suggested article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here. * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed. * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by mvanouwerkerk@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...
mvanouwerkerk@chromium.org changed reviewers: + clamy@chromium.org
mvanouwerkerk@chromium.org changed reviewers: + clamy@chromium.org
Camille, could you take a look please as owner of /frame_host/ ?
Camille, could you take a look please as owner of /frame_host/ ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... content/public/browser/navigation_entry.h:210: virtual const std::map<std::string, base::string16>& GetExtraData() const = 0; I cannot see any usage outside of content/ of this function. The content/ public API policy is not to expose any function that is not used outside of content/. So this should be removed. Additionally, why do you need to expose the map when you have getters and setters to interact with it?
https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... content/public/browser/navigation_entry.h:210: virtual const std::map<std::string, base::string16>& GetExtraData() const = 0; On 2016/09/19 09:49:23, clamy wrote: > I cannot see any usage outside of content/ of this function. The content/ public > API policy is not to expose any function that is not used outside of content/. > So this should be removed. > > Additionally, why do you need to expose the map when you have getters and > setters to interact with it? Hi, are you suggesting that I should move this method to NavigationEntryImpl to avoid defining it in the public content API? CreateJavaNavigationEntry in navigation_controller_android.cc would then have to cast from NavigationEntry to NavigationEntryImpl. That would feel like a strange design to me. I need this method because CreateJavaNavigationEntry makes a Java copy of the entry, and it does not know what the keys are.
https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... content/public/browser/navigation_entry.h:210: virtual const std::map<std::string, base::string16>& GetExtraData() const = 0; On 2016/09/19 14:27:40, Michael van Ouwerkerk wrote: > On 2016/09/19 09:49:23, clamy wrote: > > I cannot see any usage outside of content/ of this function. The content/ > public > > API policy is not to expose any function that is not used outside of content/. > > So this should be removed. > > > > Additionally, why do you need to expose the map when you have getters and > > setters to interact with it? > > Hi, are you suggesting that I should move this method to NavigationEntryImpl to > avoid defining it in the public content API? CreateJavaNavigationEntry in > navigation_controller_android.cc would then have to cast from NavigationEntry to > NavigationEntryImpl. That would feel like a strange design to me. Yes precisely. This is the design used all over content: anything not needed outside of content/ is defined in the impl version of the class. Then classes inside content/ freely use the impl version. This is done either by casting to the FooImpl version or by having the implementation of functions of the public API return the Imp version. Example: NavigationEntry NavigationController::GetEntryAtIndex is actually implemented as NavigationEntryImpl NavigationControllerImpl::GetEntryAsIndex (in navigation_controller_impl.h). For your use case, NavigationControllerAndroid should just ensured it's keeping a pointer to a NavigationControllerImpl, and then all NavigationEntries returned by this controller can be directly used a NavigationEntryImpls. Alternatively, you can safely cast the NavigationEntry to a NavigationEntryImpl. > > I need this method because CreateJavaNavigationEntry makes a Java copy of the > entry, and it does not know what the keys are. Acknowledged.
The CQ bit was checked by mvanouwerkerk@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...
Thanks! Please take another look. https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... content/public/browser/navigation_entry.h:210: virtual const std::map<std::string, base::string16>& GetExtraData() const = 0; On 2016/09/19 15:25:27, clamy wrote: > On 2016/09/19 14:27:40, Michael van Ouwerkerk wrote: > > On 2016/09/19 09:49:23, clamy wrote: > > > I cannot see any usage outside of content/ of this function. The content/ > > public > > > API policy is not to expose any function that is not used outside of > content/. > > > So this should be removed. > > > > > > Additionally, why do you need to expose the map when you have getters and > > > setters to interact with it? > > > > Hi, are you suggesting that I should move this method to NavigationEntryImpl > to > > avoid defining it in the public content API? CreateJavaNavigationEntry in > > navigation_controller_android.cc would then have to cast from NavigationEntry > to > > NavigationEntryImpl. That would feel like a strange design to me. > > Yes precisely. This is the design used all over content: anything not needed > outside of content/ is defined in the impl version of the class. Then classes > inside content/ freely use the impl version. This is done either by casting to > the FooImpl version or by having the implementation of functions of the public > API return the Imp version. Example: > > NavigationEntry NavigationController::GetEntryAtIndex > is actually implemented as > NavigationEntryImpl NavigationControllerImpl::GetEntryAsIndex (in > navigation_controller_impl.h). For your use case, NavigationControllerAndroid > should just ensured it's keeping a pointer to a NavigationControllerImpl, and > then all NavigationEntries returned by this controller can be directly used a > NavigationEntryImpls. > > Alternatively, you can safely cast the NavigationEntry to a NavigationEntryImpl. > > > > > I need this method because CreateJavaNavigationEntry makes a Java copy of the > > entry, and it does not know what the keys are. > > Acknowledged. Done.
Thanks! Please take another look. https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2327083002/diff/100001/content/public/browser... content/public/browser/navigation_entry.h:210: virtual const std::map<std::string, base::string16>& GetExtraData() const = 0; On 2016/09/19 15:25:27, clamy wrote: > On 2016/09/19 14:27:40, Michael van Ouwerkerk wrote: > > On 2016/09/19 09:49:23, clamy wrote: > > > I cannot see any usage outside of content/ of this function. The content/ > > public > > > API policy is not to expose any function that is not used outside of > content/. > > > So this should be removed. > > > > > > Additionally, why do you need to expose the map when you have getters and > > > setters to interact with it? > > > > Hi, are you suggesting that I should move this method to NavigationEntryImpl > to > > avoid defining it in the public content API? CreateJavaNavigationEntry in > > navigation_controller_android.cc would then have to cast from NavigationEntry > to > > NavigationEntryImpl. That would feel like a strange design to me. > > Yes precisely. This is the design used all over content: anything not needed > outside of content/ is defined in the impl version of the class. Then classes > inside content/ freely use the impl version. This is done either by casting to > the FooImpl version or by having the implementation of functions of the public > API return the Imp version. Example: > > NavigationEntry NavigationController::GetEntryAtIndex > is actually implemented as > NavigationEntryImpl NavigationControllerImpl::GetEntryAsIndex (in > navigation_controller_impl.h). For your use case, NavigationControllerAndroid > should just ensured it's keeping a pointer to a NavigationControllerImpl, and > then all NavigationEntries returned by this controller can be directly used a > NavigationEntryImpls. > > Alternatively, you can safely cast the NavigationEntry to a NavigationEntryImpl. > > > > > I need this method because CreateJavaNavigationEntry makes a Java copy of the > > entry, and it does not know what the keys are. > > Acknowledged. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...)
https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:362: return extra_data_; Sorry for my delay, but I have a couple general questions. 1.) What is extra data used for (not in the scope of this cl)? Do we expect things to be stored there normally? Many things? What is the lifetime of them? Is it entirely an in-memory map that we expect to get thrown away? 2.) Do we actually need to expose this? Don't we only need to access one bit of extra data? Could we not just expose GetExtraDataForIndex(index, key) to avoid the need for a new method, potentially unnecessary serialization across to JNI that is only ever used once.
https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:362: return extra_data_; On 2016/09/19 16:00:43, Ted C wrote: > Sorry for my delay, but I have a couple general questions. > > 1.) What is extra data used for (not in the scope of this cl)? Do we expect > things to be stored there normally? Many things? What is the lifetime of them? > Is it entirely an in-memory map that we expect to get thrown away? SetExtraData is used from only a few places currently. These are DataUseTabModel::OnNavigationEvent and SetPasswordStateInNavigation. Only the password state is written into SerializedNavigationEntry. The extra_data_ map has this doc: // Used to store extra data to support browser features. This member is not // persisted, unless specific data is taken out/put back in at save/restore // time (see TabNavigation for an example of this). From that (and the general concept of a NavigationEntry) it seems our use case is suitable. Persistence is optional, and for this feature not desirable because restoring scroll position after a long period would be confusing to the user. Therefore the lifetime of the data is that of the NavigationEntryImpl. There is currently very little of this data. > 2.) Do we actually need to expose this? Don't we only need to access one bit of > extra data? Could we not just expose GetExtraDataForIndex(index, key) to avoid > the need for a new method, potentially unnecessary serialization across to JNI > that is only ever used once. In Java we just have a copy of the NavigationEntry, it has no JNI of its own, or a pointer to its C++ counterpart. I think this CL is in line with the existing design. Creating a JNI method to go with NavigationController(Impl).java is possible, but seems a bit counter to the existing class design.
The CQ bit was checked by mvanouwerkerk@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_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Overall this looks very reasonable to me, but I'd like to find some way to keep get/set next to each other in the code. https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2327083002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:362: return extra_data_; On 2016/09/19 16:42:02, Michael van Ouwerkerk wrote: > On 2016/09/19 16:00:43, Ted C wrote: > > Sorry for my delay, but I have a couple general questions. > > > > 1.) What is extra data used for (not in the scope of this cl)? Do we expect > > things to be stored there normally? Many things? What is the lifetime of > them? > > Is it entirely an in-memory map that we expect to get thrown away? > > SetExtraData is used from only a few places currently. These are > DataUseTabModel::OnNavigationEvent and SetPasswordStateInNavigation. Only the > password state is written into SerializedNavigationEntry. > > The extra_data_ map has this doc: > > // Used to store extra data to support browser features. This member is not > // persisted, unless specific data is taken out/put back in at save/restore > // time (see TabNavigation for an example of this). > > From that (and the general concept of a NavigationEntry) it seems our use case > is suitable. Persistence is optional, and for this feature not desirable because > restoring scroll position after a long period would be confusing to the user. > Therefore the lifetime of the data is that of the NavigationEntryImpl. There is > currently very little of this data. Cool...thanks for looking into this and describing it. > > > > 2.) Do we actually need to expose this? Don't we only need to access one bit > of > > extra data? Could we not just expose GetExtraDataForIndex(index, key) to > avoid > > the need for a new method, potentially unnecessary serialization across to JNI > > that is only ever used once. > > In Java we just have a copy of the NavigationEntry, it has no JNI of its own, or > a pointer to its C++ counterpart. I think this CL is in line with the existing > design. Creating a JNI method to go with NavigationController(Impl).java is > possible, but seems a bit counter to the existing class design. > That is a fair point, but we seem to already be doing that for setEntryExtraData, which is exposed on the Controller vs the Entry. While I don't have a strong preference whether that goes into the Controller or the Entry, I think have symmetry of set and get is quite important. https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:680: int index = controller.getLastCommittedEntryIndex(); I would add an assert here that controller.getEntryAtIndex(index).getUrl() is an NTP URL just to make sure we don't ever mix this up. https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:869: String scrollPositionData = entry.getExtraData(NAVIGATION_ENTRY_SCROLL_POSITION_KEY); I would check for null here and then make the exception handler be a case where an error actually occurred (where invalid data was stored for some reason). https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1183: public int getScrollPosition() { javadoc (probably worth noting that it does not necessarily reflect the internal scroll Y position but just a scroll value that allows this to return to a close approximation of the previous state)
The CQ bit was checked by mvanouwerkerk@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...
https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:680: int index = controller.getLastCommittedEntryIndex(); On 2016/09/19 19:59:08, Ted C wrote: > I would add an assert here that controller.getEntryAtIndex(index).getUrl() is an > NTP URL just to make sure we don't ever mix this up. Done. https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:869: String scrollPositionData = entry.getExtraData(NAVIGATION_ENTRY_SCROLL_POSITION_KEY); On 2016/09/19 19:59:08, Ted C wrote: > I would check for null here and then make the exception handler be a case where > an error actually occurred (where invalid data was stored for some reason). Done. https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2327083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1183: public int getScrollPosition() { On 2016/09/19 19:59:08, Ted C wrote: > javadoc (probably worth noting that it does not necessarily reflect the internal > scroll Y position but just a scroll value that allows this to return to a close > approximation of the previous state) Done.
> > > 2.) Do we actually need to expose this? Don't we only need to access one > bit > > of > > > extra data? Could we not just expose GetExtraDataForIndex(index, key) to > > avoid > > > the need for a new method, potentially unnecessary serialization across to > JNI > > > that is only ever used once. > > > > In Java we just have a copy of the NavigationEntry, it has no JNI of its own, > or > > a pointer to its C++ counterpart. I think this CL is in line with the existing > > design. Creating a JNI method to go with NavigationController(Impl).java is > > possible, but seems a bit counter to the existing class design. > > > > That is a fair point, but we seem to already be doing that for > setEntryExtraData, which is exposed on the Controller vs the Entry. While I > don't have a strong preference whether that goes into the Controller or the > Entry, I think have symmetry of set and get is quite important. Alright, today is symmetry day. I've moved it to the controller.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks! It looks nicer this way, lgtm.
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Ntp: restore scroll position. * The scroll position is stored as extra data on the NavigationEntry. * The main use case handled is when the user clicks on a suggested article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here. * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed. * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ntp: restore scroll position. * The scroll position is stored as extra data on the NavigationEntry. * The main use case handled is when the user clicks on a suggested article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here. * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed. * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Ntp: restore scroll position. * The scroll position is stored as extra data on the NavigationEntry. * The main use case handled is when the user clicks on a suggested article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here. * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed. * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ntp: restore scroll position. * The scroll position is stored as extra data on the NavigationEntry. * The main use case handled is when the user clicks on a suggested article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here. * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed. * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content. BUG=606356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6 Cr-Commit-Position: refs/heads/master@{#420075} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6 Cr-Commit-Position: refs/heads/master@{#420075}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2363483002/ by hush@chromium.org. The reason for reverting is: Broke org.chromium.chrome.browser.ntp.NewTabPageTest.*. |
