Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578973002:
Reload offline version on load failure (Closed)
DescriptionReload offline version on load failure
Change the way we trigger offline version.
The ReadingListWebStateObserver now listens for all page loads.
If there is an error or the page is too slow, and if an offline page is
available, show the native content instead.
Offline page is displayed by reloading on the offline URL.
BUG=671954, 671160, 671963
Committed: https://crrev.com/9d91357e728e0d6821b6fed7fdce60d3dfc95193
Cr-Commit-Position: refs/heads/master@{#440367}
Patch Set 1 #Patch Set 2 : clean #Patch Set 3 : create protocol #Patch Set 4 : clean #
Total comments: 2
Patch Set 5 : Reload #Patch Set 6 : clean #Patch Set 7 : cleaner #
Total comments: 8
Patch Set 8 : add dismiss #
Total comments: 4
Patch Set 9 : feedback #
Total comments: 13
Patch Set 10 : Downstream merge + feedback #Patch Set 11 : rebase #
Total comments: 17
Patch Set 12 : feedback #
Total comments: 31
Patch Set 13 : feedback #
Total comments: 2
Patch Set 14 : rebase + const #Depends on Patchset: Messages
Total messages: 48 (19 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, marq@chromium.org
Can we agree on a solution like this? Tab will implement ReadingListWebStateObserverDelegate and just pass the call to the web_controller.
https://codereview.chromium.org/2578973002/diff/60001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/60001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:181: [delegate_ displayOfflineVersionForURL:url]; Is this a call of CRWWebController method? https://codereview.chromium.org/2578973002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2578973002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:179: - (void)displayOfflineVersionForURL:(const GURL&)URL; Who will call this method? Please note that ios/chrome later should not be allowed to use CRWWebController.
Description was changed from ========== Add fallback to tab BUG= ========== to ========== Reload offline version on load failure BUG= ==========
olivierrobin@chromium.org changed reviewers: + kkhorimoto@chromium.org
What about this solution?
On 2016/12/16 14:43:11, Olivier Robin wrote: > What about this solution? General direction is reasonable, thank you for not punching holes in CRWWebController encapsulation. I left a few concrete comments and changing |currentNavigationURL| is probably a matter of a few separate CLs (first will remove |currentNavigationURL| and inline it's content and other CLs will replace GetVirtualURL with GetURL where necessary).
Moved URL -> VirtualURL to another CL. PTAL.
Sorry I wrote these comments last week, but did not publish because I replied w/o going through "Publish+Mail Comments" flow. https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: // The item of the current navigation. The logic behind this method is quite complicated and requires more comments. F.e. The result of this this should not be used in the Omnibox because Pending Item can only be displayed if it was not renderer initiated. https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); Should this just be GetVisibleItem instead? https://codereview.chromium.org/2578973002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2033: [self.nativeController willBeDismissed]; There are probably other cases when this needs to be called. Possibly |goToItemAtIndex:|, and places where |clearTransientContentView| is called.
Thanks. PTAL https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: // The item of the current navigation. On 2016/12/19 17:52:32, Eugene But wrote: > The logic behind this method is quite complicated and requires more comments. > F.e. The result of this this should not be used in the Omnibox because Pending > Item can only be displayed if it was not renderer initiated. Removed as GetVisibleItem seems to be what I need. https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/19 17:52:32, Eugene But wrote: > Should this just be GetVisibleItem instead? Yes, this seems the correct method. Replaced in code. https://codereview.chromium.org/2578973002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2033: [self.nativeController willBeDismissed]; On 2016/12/19 17:52:32, Eugene But wrote: > There are probably other cases when this needs to be called. Possibly > |goToItemAtIndex:|, and places where |clearTransientContentView| is called. Done.
Description was changed from ========== Reload offline version on load failure BUG= ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ==========
Could you please update CL description to fit 72 symbols https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:88: if (!reading_list_model_->loaded() || !web_state()) { nit: Do you want to create ShouldStopCheckingProgress method to simplify this method? if (ShouldStopCheckingProgress()) { StopCheckingProgress(); } else { GURL virtual_url = item->GetVirtualURL(); StartCheckingProgress(virtual_url.empty() ? item->GetURL() : virtual_url); } You will be able to reuse that method inside LoadReadingListDistilled as well. https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:102: if (virtual_url.is_empty()) { nit: Do you want to move |virtual_url| creation closer to the same place? https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:190: void ReadingListWebStateObserver::LoadReadingListDistilled() { How about LoadDistilledReadingListEntry or LoadOfflineReadingListEntry (not sure which is more important for this name offline or distilled)? https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:207: web_state()->GetNavigationManager()->Reload(NO); s/NO/false https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2087: [self.nativeController willBeDismissed]; It looks like |willBeDismissed| should be called only when |clearTransientContentView| is called. To make this more maintainable, could you please rename |clearTransientContentView| and move |willBeDismissed| call to that renamed method. https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2292: if ([self.nativeController respondsToSelector:@selector(willBeDismissed)]) |goToItemAtIndex:| calls |loadCurrentURL| or |loadWithParams|, and those 2 methods call willBeDismissed. So you probably don't need it here.
This looks good, except for a possible error condition around transient NavigationEntries. https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); I'm not sure that this will work correctly if there is an interstitial. Let's say we're loading a page that we had previously distilled, but whose certificate has subsequently expired. If that happened, we'd have a transient entry displaying the interstitial, and this would return that NavigationItem. When we try to load the offline version by rewriting the NavigationItem's URL, I think we might be discarding the transient entry before the reload occurs. Can you test that this doesn't happen by distilling expired.badssl.com?
https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/19 20:27:49, kkhorimoto_ wrote: > I'm not sure that this will work correctly if there is an interstitial. Let's > say we're loading a page that we had previously distilled, but whose certificate > has subsequently expired. If that happened, we'd have a transient entry > displaying the interstitial, and this would return that NavigationItem. When we > try to load the offline version by rewriting the NavigationItem's URL, I think > we might be discarding the transient entry before the reload occurs. Can you > test that this doesn't happen by distilling expired.badssl.com? Olivier uploaded a new patch which does not have this code anymore.
https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/19 20:49:11, Eugene But wrote: > On 2016/12/19 20:27:49, kkhorimoto_ wrote: > > I'm not sure that this will work correctly if there is an interstitial. Let's > > say we're loading a page that we had previously distilled, but whose > certificate > > has subsequently expired. If that happened, we'd have a transient entry > > displaying the interstitial, and this would return that NavigationItem. When > we > > try to load the offline version by rewriting the NavigationItem's URL, I think > > we might be discarding the transient entry before the reload occurs. Can you > > test that this doesn't happen by distilling expired.badssl.com? > Olivier uploaded a new patch which does not have this code anymore. The new patch uses NavigationManagerImpl::GetVisibleItem(), which poses the same problem of returning the transient item if it exists, so this is still something we would want to verify is behaving correctly. https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:102: if (virtual_url.is_empty()) { On 2016/12/19 18:47:04, Eugene But wrote: > nit: Do you want to move |virtual_url| creation closer to the same place? NavigationItemImpl::GetVirtualURL() will return GetURL() if the virtual URL is empty. This codepath is never excuted.
https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/19 18:26:38, Olivier Robin wrote: > On 2016/12/19 17:52:32, Eugene But wrote: > > Should this just be GetVisibleItem instead? > > Yes, this seems the correct method. > Replaced in code. I cannot use GetVisibleItem after all as it does not work on back forward. (crbug.com/675925) https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/140001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/19 21:18:50, kkhorimoto_ wrote: > On 2016/12/19 20:49:11, Eugene But wrote: > > On 2016/12/19 20:27:49, kkhorimoto_ wrote: > > > I'm not sure that this will work correctly if there is an interstitial. > Let's > > > say we're loading a page that we had previously distilled, but whose > > certificate > > > has subsequently expired. If that happened, we'd have a transient entry > > > displaying the interstitial, and this would return that NavigationItem. > When > > we > > > try to load the offline version by rewriting the NavigationItem's URL, I > think > > > we might be discarding the transient entry before the reload occurs. Can > you > > > test that this doesn't happen by distilling expired.badssl.com? > > Olivier uploaded a new patch which does not have this code anymore. > > The new patch uses NavigationManagerImpl::GetVisibleItem(), which poses the same > problem of returning the transient item if it exists, so this is still something > we would want to verify is behaving correctly. It is hard to trigger a case that has the problem with the current code as - If offline, the SSL certificate is not fetched - If online, the progress is often (always?) 1 when the interstitial is displayed - If network is bad, the timing must be perfect to display the offline version over the interstitial. I changed the code to force this situation to happen and if an interstitial reports a loadProgress < 0.75 (not sure if it is possible), the offline version will trigger a reload and dismiss the interstitial. I don't think this is WAI, so I added a IsShowingWebInterstitial() in the GetVisibleItem above. https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:88: if (!reading_list_model_->loaded() || !web_state()) { On 2016/12/19 18:47:04, Eugene But wrote: > nit: Do you want to create ShouldStopCheckingProgress method to simplify this > method? > > if (ShouldStopCheckingProgress()) { > StopCheckingProgress(); > } else { > GURL virtual_url = item->GetVirtualURL(); > StartCheckingProgress(virtual_url.empty() ? item->GetURL() : virtual_url); > } > > You will be able to reuse that method inside LoadReadingListDistilled as well. Created some methods to simplify checks https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:102: if (virtual_url.is_empty()) { On 2016/12/19 21:18:50, kkhorimoto_ wrote: > On 2016/12/19 18:47:04, Eugene But wrote: > > nit: Do you want to move |virtual_url| creation closer to the same place? > > NavigationItemImpl::GetVirtualURL() will return GetURL() if the virtual URL is > empty. This codepath is never excuted. Done. https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:190: void ReadingListWebStateObserver::LoadReadingListDistilled() { On 2016/12/19 18:47:05, Eugene But wrote: > How about LoadDistilledReadingListEntry or LoadOfflineReadingListEntry (not sure > which is more important for this name offline or distilled)? Done. https://codereview.chromium.org/2578973002/diff/160001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:207: web_state()->GetNavigationManager()->Reload(NO); On 2016/12/19 18:47:05, Eugene But wrote: > s/NO/false Done. https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2087: [self.nativeController willBeDismissed]; On 2016/12/19 18:47:05, Eugene But wrote: > It looks like |willBeDismissed| should be called only when > |clearTransientContentView| is called. To make this more maintainable, could you > please rename |clearTransientContentView| and move |willBeDismissed| call to > that renamed method. Done. https://codereview.chromium.org/2578973002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2292: if ([self.nativeController respondsToSelector:@selector(willBeDismissed)]) On 2016/12/19 18:47:05, Eugene But wrote: > |goToItemAtIndex:| calls |loadCurrentURL| or |loadWithParams|, and those 2 > methods call willBeDismissed. So you probably don't need it here. Done.
Description was changed from ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ==========
Description was changed from ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ==========
Description was changed from ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ==========
I have some questions regarding GetCurrentObservableIte and GetCurrentItem, but everything else looks good. I think having only one custom method which returns a NavigationItem would be simpler. In that method could you please comment why you need to return Transient/Pending/LastCommitted items. During the previous rounds I misunderstood your code and thought that you always need Visible item, which now I see is not the case. I understand why you want pending item, but still not sure why sometimes you want transient and sometimes last committed. https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: web::NavigationItem* item = manager->GetTransientItem(); On 2016/12/20 12:29:46, Olivier Robin wrote: > On 2016/12/19 18:26:38, Olivier Robin wrote: > > On 2016/12/19 17:52:32, Eugene But wrote: > > > Should this just be GetVisibleItem instead? > > > > Yes, this seems the correct method. > > Replaced in code. > > I cannot use GetVisibleItem after all as it does not work on back forward. > (crbug.com/675925) I think GetVisibleItem is WAI, but looks like it's not what you need here. GetVisibleItem returns LastCommittedItem in most cases, which is expected behavior. Is |GetCurrentObservableItem| the only place where you plan to use |CurrentItem|? If so then can you always use |PendingItem|? If you can't always use PendingItem then please explain the reason in the comment? https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // Load the offline version of the URL in place of the current page. s/Load/Loads Please fix everywhere https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool URLAvailableOffline(const GURL& url); nit: Ssould this be IsUrlAvailableOffline ? Please note that according to C++ Style Guide we should use Url not URL (though we are extremely inconsistent and you may keep all caps URL). https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: web::NavigationItem* GetCurrentObservableItem(); Do we even need 2 methods (GetCurrentItem and GetCurrentObservableItem)? The logic behind these methods is very tricky and I can't tell from the comments what they do. I think having one method and carefully documenting all the logic would make this simpler. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // Returns the current item being loaded. From this comment I have impression that GetCurrentItem always returns pending item, which is "the current item being loaded". But that's not always true. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: return item; Can this even happen? Transient Item is non null only if interstitial is displayed. But if it is displayed then GetCurrentObservableItem() always return null. Am I missing something? https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: return manager->GetLastCommittedItem(); In which cases do you need to return last committed item? Comments could be helpful here. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: NSLog(@"%s", item->GetURL().spec().c_str()); Please remove this. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: _webState->GetNavigationManager()->Reload(NO); s/NO/false
On 2016/12/20 18:11:04, Eugene But wrote: > I have some questions regarding GetCurrentObservableIte and GetCurrentItem, but > everything else looks good. > > I think having only one custom method which returns a NavigationItem would be > simpler. In that method could you please comment why you need to return > Transient/Pending/LastCommitted items. During the previous rounds I > misunderstood your code and thought that you always need Visible item, which now > I see is not the case. I understand why you want pending item, but still not > sure why sometimes you want transient and sometimes last committed. I will address the other comments. This is just an answer to first question. During my tests, I had problems having a reliable way to get the current entry. I decided to copy CRWSessionController::currentEntry which seems the safer way to have robust code. I don't think transientView will ever be returned, because at the moment it is only used in interstitials. ATM, pending entry is used in didStartLoading in two cases - tap on a RL entry in RL view controller - go back/forward. in other cases - All PageLoaded calls - didStartLoading on omnibox enter - didStartLoading on refresh the lastcommittedentry is used. I am not sure I understand exactly these different cases, so testing pending_entry seems the best way to do it at the moment. > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: > web::NavigationItem* item = manager->GetTransientItem(); > On 2016/12/20 12:29:46, Olivier Robin wrote: > > On 2016/12/19 18:26:38, Olivier Robin wrote: > > > On 2016/12/19 17:52:32, Eugene But wrote: > > > > Should this just be GetVisibleItem instead? > > > > > > Yes, this seems the correct method. > > > Replaced in code. > > > > I cannot use GetVisibleItem after all as it does not work on back forward. > > (crbug.com/675925) > I think GetVisibleItem is WAI, but looks like it's not what you need here. > GetVisibleItem returns LastCommittedItem in most cases, which is expected > behavior. > > Is |GetCurrentObservableItem| the only place where you plan to use > |CurrentItem|? If so then can you always use |PendingItem|? If you can't always > use PendingItem then please explain the reason in the comment? > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // Load > the offline version of the URL in place of the current page. > s/Load/Loads > > Please fix everywhere > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool > URLAvailableOffline(const GURL& url); > nit: Ssould this be IsUrlAvailableOffline ? > > Please note that according to C++ Style Guide we should use Url not URL (though > we are extremely inconsistent and you may keep all caps URL). > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: > web::NavigationItem* GetCurrentObservableItem(); > Do we even need 2 methods (GetCurrentItem and GetCurrentObservableItem)? The > logic behind these methods is very tricky and I can't tell from the comments > what they do. I think having one method and carefully documenting all the logic > would make this simpler. > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // Returns > the current item being loaded. > From this comment I have impression that GetCurrentItem always returns pending > item, which is "the current item being loaded". But that's not always true. > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: return > item; > Can this even happen? Transient Item is non null only if interstitial is > displayed. But if it is displayed then GetCurrentObservableItem() always return > null. Am I missing something? > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: return > manager->GetLastCommittedItem(); > In which cases do you need to return last committed item? Comments could be > helpful here. > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if > (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { > return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: > NSLog(@"%s", item->GetURL().spec().c_str()); > Please remove this. > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: > _webState->GetNavigationManager()->Reload(NO); > s/NO/false
On 2016/12/20 18:34:22, Olivier Robin wrote: > On 2016/12/20 18:11:04, Eugene But wrote: > > I have some questions regarding GetCurrentObservableIte and GetCurrentItem, > but > > everything else looks good. > > > > I think having only one custom method which returns a NavigationItem would be > > simpler. In that method could you please comment why you need to return > > Transient/Pending/LastCommitted items. During the previous rounds I > > misunderstood your code and thought that you always need Visible item, which > now > > I see is not the case. I understand why you want pending item, but still not > > sure why sometimes you want transient and sometimes last committed. > > I will address the other comments. This is just an answer to first question. > During my tests, I had problems having a reliable way to get the current entry. > I decided to copy CRWSessionController::currentEntry which seems the safer way > to have > robust code. > > I don't think transientView will ever be returned, because at the moment it is > only used in > interstitials. > ATM, pending entry is used in didStartLoading in two cases > - tap on a RL entry in RL view controller > - go back/forward. > > in other cases > - All PageLoaded calls > - didStartLoading on omnibox enter > - didStartLoading on refresh > the lastcommittedentry is used. > > I am not sure I understand exactly these different cases, so testing > pending_entry seems the best > way to do it at the moment. > > > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > (right): > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: > > web::NavigationItem* item = manager->GetTransientItem(); > > On 2016/12/20 12:29:46, Olivier Robin wrote: > > > On 2016/12/19 18:26:38, Olivier Robin wrote: > > > > On 2016/12/19 17:52:32, Eugene But wrote: > > > > > Should this just be GetVisibleItem instead? > > > > > > > > Yes, this seems the correct method. > > > > Replaced in code. > > > > > > I cannot use GetVisibleItem after all as it does not work on back forward. > > > (crbug.com/675925) > > I think GetVisibleItem is WAI, but looks like it's not what you need here. > > GetVisibleItem returns LastCommittedItem in most cases, which is expected > > behavior. > > > > Is |GetCurrentObservableItem| the only place where you plan to use > > |CurrentItem|? If so then can you always use |PendingItem|? If you can't > always > > use PendingItem then please explain the reason in the comment? > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.h > (right): > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // Load > > the offline version of the URL in place of the current page. > > s/Load/Loads > > > > Please fix everywhere > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool > > URLAvailableOffline(const GURL& url); > > nit: Ssould this be IsUrlAvailableOffline ? > > > > Please note that according to C++ Style Guide we should use Url not URL > (though > > we are extremely inconsistent and you may keep all caps URL). > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: > > web::NavigationItem* GetCurrentObservableItem(); > > Do we even need 2 methods (GetCurrentItem and GetCurrentObservableItem)? The > > logic behind these methods is very tricky and I can't tell from the comments > > what they do. I think having one method and carefully documenting all the > logic > > would make this simpler. > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // > Returns > > the current item being loaded. > > From this comment I have impression that GetCurrentItem always returns pending > > item, which is "the current item being loaded". But that's not always true. > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > (right): > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: return > > item; > > Can this even happen? Transient Item is non null only if interstitial is > > displayed. But if it is displayed then GetCurrentObservableItem() always > return > > null. Am I missing something? > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: return > > manager->GetLastCommittedItem(); > > In which cases do you need to return last committed item? Comments could be > > helpful here. > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if > > (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { > > return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: > > NSLog(@"%s", item->GetURL().spec().c_str()); > > Please remove this. > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > (right): > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: > > _webState->GetNavigationManager()->Reload(NO); > > s/NO/false Thanks Olivier! Sounds like we don't need to return transient item. All cases for pending entry usage make total sense to me. Let me comment on lastCommittedEntry cases: >> - All PageLoaded calls Using lastCommittedEntry makes sense here. >> - didStartLoading on omnibox enter >> - didStartLoading on refresh Can you rely on pending entry in this case? If no, then why? Sounds like inside PageLoaded() callback you have to always use lastCommittedItem, because load was completed and there is no pending entry. Sounds like inside DidStartLoading() callback you should be able to use pendingItem. If these statement are correct that you probably can drop GetCurrentItem / GetCurrentObservableItem and use NavigationManager directly. WDYT?
On 2016/12/20 19:24:06, Eugene But wrote: > On 2016/12/20 18:34:22, Olivier Robin wrote: > > On 2016/12/20 18:11:04, Eugene But wrote: > > > I have some questions regarding GetCurrentObservableIte and GetCurrentItem, > > but > > > everything else looks good. > > > > > > I think having only one custom method which returns a NavigationItem would > be > > > simpler. In that method could you please comment why you need to return > > > Transient/Pending/LastCommitted items. During the previous rounds I > > > misunderstood your code and thought that you always need Visible item, which > > now > > > I see is not the case. I understand why you want pending item, but still not > > > sure why sometimes you want transient and sometimes last committed. > > > > I will address the other comments. This is just an answer to first question. > > During my tests, I had problems having a reliable way to get the current > entry. > > I decided to copy CRWSessionController::currentEntry which seems the safer way > > to have > > robust code. > > > > I don't think transientView will ever be returned, because at the moment it is > > only used in > > interstitials. > > ATM, pending entry is used in didStartLoading in two cases > > - tap on a RL entry in RL view controller > > - go back/forward. > > > > in other cases > > - All PageLoaded calls > > - didStartLoading on omnibox enter > > - didStartLoading on refresh > > the lastcommittedentry is used. > > > > I am not sure I understand exactly these different cases, so testing > > pending_entry seems the best > > way to do it at the moment. > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > > (right): > > > > > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: > > > web::NavigationItem* item = manager->GetTransientItem(); > > > On 2016/12/20 12:29:46, Olivier Robin wrote: > > > > On 2016/12/19 18:26:38, Olivier Robin wrote: > > > > > On 2016/12/19 17:52:32, Eugene But wrote: > > > > > > Should this just be GetVisibleItem instead? > > > > > > > > > > Yes, this seems the correct method. > > > > > Replaced in code. > > > > > > > > I cannot use GetVisibleItem after all as it does not work on back forward. > > > > (crbug.com/675925) > > > I think GetVisibleItem is WAI, but looks like it's not what you need here. > > > GetVisibleItem returns LastCommittedItem in most cases, which is expected > > > behavior. > > > > > > Is |GetCurrentObservableItem| the only place where you plan to use > > > |CurrentItem|? If so then can you always use |PendingItem|? If you can't > > always > > > use PendingItem then please explain the reason in the comment? > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.h > > (right): > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // > Load > > > the offline version of the URL in place of the current page. > > > s/Load/Loads > > > > > > Please fix everywhere > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool > > > URLAvailableOffline(const GURL& url); > > > nit: Ssould this be IsUrlAvailableOffline ? > > > > > > Please note that according to C++ Style Guide we should use Url not URL > > (though > > > we are extremely inconsistent and you may keep all caps URL). > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: > > > web::NavigationItem* GetCurrentObservableItem(); > > > Do we even need 2 methods (GetCurrentItem and GetCurrentObservableItem)? The > > > logic behind these methods is very tricky and I can't tell from the comments > > > what they do. I think having one method and carefully documenting all the > > logic > > > would make this simpler. > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // > > Returns > > > the current item being loaded. > > > From this comment I have impression that GetCurrentItem always returns > pending > > > item, which is "the current item being loaded". But that's not always true. > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > > (right): > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: > return > > > item; > > > Can this even happen? Transient Item is non null only if interstitial is > > > displayed. But if it is displayed then GetCurrentObservableItem() always > > return > > > null. Am I missing something? > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: > return > > > manager->GetLastCommittedItem(); > > > In which cases do you need to return last committed item? Comments could be > > > helpful here. > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if > > > (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { > > > return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: > > > NSLog(@"%s", item->GetURL().spec().c_str()); > > > Please remove this. > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > (right): > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: > > > _webState->GetNavigationManager()->Reload(NO); > > > s/NO/false > Thanks Olivier! Sounds like we don't need to return transient item. All cases > for pending entry usage make total sense to me. Let me comment on > lastCommittedEntry cases: > > >> - All PageLoaded calls > Using lastCommittedEntry makes sense here. > > >> - didStartLoading on omnibox enter > >> - didStartLoading on refresh > Can you rely on pending entry in this case? If no, then why? > > Sounds like inside PageLoaded() callback you have to always use > lastCommittedItem, because load was completed and there is no pending entry. I think so. > Sounds like inside DidStartLoading() callback you should be able to use > pendingItem. If these statement are correct that you probably can drop > GetCurrentItem / GetCurrentObservableItem and use NavigationManager directly. > WDYT? pendingItem is in didStartLoading when refreshing or entering address from Omnibox, so I need to test it and use lastCommitted item on these cases. I can inline these methods, but I will need to make the tests anyway.
On 2016/12/20 21:09:45, Olivier Robin wrote: > On 2016/12/20 19:24:06, Eugene But wrote: > > On 2016/12/20 18:34:22, Olivier Robin wrote: > > > On 2016/12/20 18:11:04, Eugene But wrote: > > > > I have some questions regarding GetCurrentObservableIte and > GetCurrentItem, > > > but > > > > everything else looks good. > > > > > > > > I think having only one custom method which returns a NavigationItem would > > be > > > > simpler. In that method could you please comment why you need to return > > > > Transient/Pending/LastCommitted items. During the previous rounds I > > > > misunderstood your code and thought that you always need Visible item, > which > > > now > > > > I see is not the case. I understand why you want pending item, but still > not > > > > sure why sometimes you want transient and sometimes last committed. > > > > > > I will address the other comments. This is just an answer to first question. > > > During my tests, I had problems having a reliable way to get the current > > entry. > > > I decided to copy CRWSessionController::currentEntry which seems the safer > way > > > to have > > > robust code. > > > > > > I don't think transientView will ever be returned, because at the moment it > is > > > only used in > > > interstitials. > > > ATM, pending entry is used in didStartLoading in two cases > > > - tap on a RL entry in RL view controller > > > - go back/forward. > > > > > > in other cases > > > - All PageLoaded calls > > > - didStartLoading on omnibox enter > > > - didStartLoading on refresh > > > the lastcommittedentry is used. > > > > > > I am not sure I understand exactly these different cases, so testing > > > pending_entry seems the best > > > way to do it at the moment. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/120001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:92: > > > > web::NavigationItem* item = manager->GetTransientItem(); > > > > On 2016/12/20 12:29:46, Olivier Robin wrote: > > > > > On 2016/12/19 18:26:38, Olivier Robin wrote: > > > > > > On 2016/12/19 17:52:32, Eugene But wrote: > > > > > > > Should this just be GetVisibleItem instead? > > > > > > > > > > > > Yes, this seems the correct method. > > > > > > Replaced in code. > > > > > > > > > > I cannot use GetVisibleItem after all as it does not work on back > forward. > > > > > (crbug.com/675925) > > > > I think GetVisibleItem is WAI, but looks like it's not what you need here. > > > > GetVisibleItem returns LastCommittedItem in most cases, which is expected > > > > behavior. > > > > > > > > Is |GetCurrentObservableItem| the only place where you plan to use > > > > |CurrentItem|? If so then can you always use |PendingItem|? If you can't > > > always > > > > use PendingItem then please explain the reason in the comment? > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.h > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // > > Load > > > > the offline version of the URL in place of the current page. > > > > s/Load/Loads > > > > > > > > Please fix everywhere > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool > > > > URLAvailableOffline(const GURL& url); > > > > nit: Ssould this be IsUrlAvailableOffline ? > > > > > > > > Please note that according to C++ Style Guide we should use Url not URL > > > (though > > > > we are extremely inconsistent and you may keep all caps URL). > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: > > > > web::NavigationItem* GetCurrentObservableItem(); > > > > Do we even need 2 methods (GetCurrentItem and GetCurrentObservableItem)? > The > > > > logic behind these methods is very tricky and I can't tell from the > comments > > > > what they do. I think having one method and carefully documenting all the > > > logic > > > > would make this simpler. > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // > > > Returns > > > > the current item being loaded. > > > > From this comment I have impression that GetCurrentItem always returns > > pending > > > > item, which is "the current item being loaded". But that's not always > true. > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: > > return > > > > item; > > > > Can this even happen? Transient Item is non null only if interstitial is > > > > displayed. But if it is displayed then GetCurrentObservableItem() always > > > return > > > > null. Am I missing something? > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: > > return > > > > manager->GetLastCommittedItem(); > > > > In which cases do you need to return last committed item? Comments could > be > > > > helpful here. > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if > > > > (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { > > > > return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... > > > > ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: > > > > NSLog(@"%s", item->GetURL().spec().c_str()); > > > > Please remove this. > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > > > File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... > > > > ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: > > > > _webState->GetNavigationManager()->Reload(NO); > > > > s/NO/false > > Thanks Olivier! Sounds like we don't need to return transient item. All cases > > for pending entry usage make total sense to me. Let me comment on > > lastCommittedEntry cases: > > > > >> - All PageLoaded calls > > Using lastCommittedEntry makes sense here. > > > > >> - didStartLoading on omnibox enter > > >> - didStartLoading on refresh > > Can you rely on pending entry in this case? If no, then why? > > > > Sounds like inside PageLoaded() callback you have to always use > > lastCommittedItem, because load was completed and there is no pending entry. > I think so. > > > Sounds like inside DidStartLoading() callback you should be able to use > > pendingItem. If these statement are correct that you probably can drop > > GetCurrentItem / GetCurrentObservableItem and use NavigationManager directly. > > WDYT? > pendingItem is in didStartLoading when refreshing or entering address from > Omnibox, > so I need to test it and use lastCommitted item on these cases. > I can inline these methods, but I will need to make the tests anyway. Sorry > pendingItem is *null* in didStartLoading when refreshing or entering address from > Omnibox,
>> Sorry, pendingItem is *null* in didStartLoading when refreshing or entering address from Omnibox. Oh, this is a bug in CRWWebController (filed as crbug.com/676129). This is something that we can fix after shipping our newest Back-Forward navigation refactoring, but can't do right now. So I the meanwhile I would recommend to use pendingItem inside DidStartLoading and when it's null, then fallback to lastCommittedItem with TODO referencing crbug.com/676129.
https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:45: // Load the offline version of the URL in place of the current page. On 2016/12/20 18:11:03, Eugene But wrote: > s/Load/Loads > > Please fix everywhere Done. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool URLAvailableOffline(const GURL& url); On 2016/12/20 18:11:03, Eugene But wrote: > nit: Ssould this be IsUrlAvailableOffline ? > > Please note that according to C++ Style Guide we should use Url not URL (though > we are extremely inconsistent and you may keep all caps URL). Done. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:57: // Returns the current item being loaded. On 2016/12/20 18:11:03, Eugene But wrote: > From this comment I have impression that GetCurrentItem always returns pending > item, which is "the current item being loaded". But that's not always true. Discussed in CL comment. Removed method. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:91: return item; On 2016/12/20 18:11:04, Eugene But wrote: > Can this even happen? Transient Item is non null only if interstitial is > displayed. But if it is displayed then GetCurrentObservableItem() always return > null. Am I missing something? Discussed in CL comment. Removed method. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:97: return manager->GetLastCommittedItem(); On 2016/12/20 18:11:03, Eugene But wrote: > In which cases do you need to return last committed item? Comments could be > helpful here. Discussed in CL comment. Removed method. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:119: if (!entry || (entry->DistilledState() != ReadingListEntry::PROCESSED)) { On 2016/12/20 18:11:04, Eugene But wrote: > return entry && entry->DistilledState() == ReadingListEntry::PROCESSED; Done. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:128: NSLog(@"%s", item->GetURL().spec().c_str()); On 2016/12/20 18:11:03, Eugene But wrote: > Please remove this. Done. https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2578973002/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:79: _webState->GetNavigationManager()->Reload(NO); On 2016/12/20 18:11:04, Eugene But wrote: > s/NO/false Done.
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by olivierrobin@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.
Thank you for getting rid of custom GetCurrentItem method! lgtm https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:48: // Returns if the current page is |url| has an offline version that can be s/is/with ? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool IsUrlAvailableOffline(const GURL& url); Should this be |bool IsUrlAvailableOffline(const GURL& url) const;|? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:52: // Convenience method that checks it |item| should be observed or not. nit: s/Convenience method that checks/Checks https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:52: // Convenience method that checks it |item| should be observed or not. s/it/if ? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: bool ShouldObserveItem(web::NavigationItem* item); Should this be |bool ShouldObserveItem(web::NavigationItem* item); const| ? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:111: // TODO(crbug.com/676129): Remove this workaround once GetPendingItem() Optional nit Consider being more explicit: // manager->GetPendingItem() returns null on reload. TODO(crbug.com/676129): Remove this workaround once GetPendingItem() returns the correct value. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:124: // No need to launch the timer as we don't have offline version to show. nit: Please avoid "we" in the comments (go/avoidwe) https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:163: pending_url_ = GURL(); nit: This is unrelated to your change, but still would be nice to fix: s/GURL()/GURL::EmptyGURL() https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:175: // TODO(crbug.com/676129): Remove this workaround once GetPendingItem() Optional nit Consider being more explicit: // manager->GetPendingItem() returns null on reload. TODO(crbug.com/676129): Remove this workaround once GetPendingItem() returns the correct value. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:195: timer_->Stop(); nit: Do you want to call StopCheckingProgress() instead? If there is a value in keeping pending_url_ valid then I guess explanation comments could be useful. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:16: #include "ios/web/public/navigation_item.h" s/include/import https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:17: #include "ios/web/public/navigation_manager.h" ditto https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:18: #include "ios/web/public/web_state/web_state.h" ditto https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:25: // Restores the Navigation entry to its initial state. s/Navigation entry/last committed item https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:26: - (void)cleanup; Should this be |restoreOnlineURL| instead of |cleanup|? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:68: - (void)cleanup { nit: Could you please move this method to the end, to avoid mixing interface and implementation methods. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm:68: DCHECK_NE(firstEntry, nullptr); nit: DCHECK(firstEntry);
olivierrobin@chromium.org changed reviewers: + rohitrao@google.com
All done! Thanks rohitrao, can you take a look to tab? https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:48: // Returns if the current page is |url| has an offline version that can be On 2016/12/21 15:48:45, Eugene But wrote: > s/is/with ? Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:50: bool IsUrlAvailableOffline(const GURL& url); On 2016/12/21 15:48:45, Eugene But wrote: > Should this be |bool IsUrlAvailableOffline(const GURL& url) const;|? Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.h:55: bool ShouldObserveItem(web::NavigationItem* item); On 2016/12/21 15:48:45, Eugene But wrote: > Should this be |bool ShouldObserveItem(web::NavigationItem* item); const| ? Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:124: // No need to launch the timer as we don't have offline version to show. On 2016/12/21 15:48:46, Eugene But wrote: > nit: Please avoid "we" in the comments (go/avoidwe) Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:163: pending_url_ = GURL(); On 2016/12/21 15:48:46, Eugene But wrote: > nit: This is unrelated to your change, but still would be nice to fix: > s/GURL()/GURL::EmptyGURL() Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:175: // TODO(crbug.com/676129): Remove this workaround once GetPendingItem() On 2016/12/21 15:48:46, Eugene But wrote: > Optional nit Consider being more explicit: > // manager->GetPendingItem() returns null on reload. TODO(crbug.com/676129): > Remove this workaround once GetPendingItem() returns the correct value. Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:195: timer_->Stop(); On 2016/12/21 15:48:45, Eugene But wrote: > nit: Do you want to call StopCheckingProgress() instead? If there is a value in > keeping pending_url_ valid then I guess explanation comments could be useful. Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:16: #include "ios/web/public/navigation_item.h" On 2016/12/21 15:48:46, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:17: #include "ios/web/public/navigation_manager.h" On 2016/12/21 15:48:46, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:18: #include "ios/web/public/web_state/web_state.h" On 2016/12/21 15:48:46, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:25: // Restores the Navigation entry to its initial state. On 2016/12/21 15:48:46, Eugene But wrote: > s/Navigation entry/last committed item Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:26: - (void)cleanup; On 2016/12/21 15:48:46, Eugene But wrote: > Should this be |restoreOnlineURL| instead of |cleanup|? Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:68: - (void)cleanup { On 2016/12/21 15:48:46, Eugene But wrote: > nit: Could you please move this method to the end, to avoid mixing interface and > implementation methods. Done. https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm (right): https://codereview.chromium.org/2578973002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm:68: DCHECK_NE(firstEntry, nullptr); On 2016/12/21 15:48:46, Eugene But wrote: > nit: DCHECK(firstEntry); Done.
Almost forgot. Could you please add unit tests for your changes. There used to be tests in reading_list_entry_loading_util_unittest.
olivierrobin@chromium.org changed reviewers: + rohitrao@chromium.org - rohitrao@google.com
Tab lgtm https://codereview.chromium.org/2578973002/diff/260001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/260001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:160: web_state()->RemoveUserData(kObserverKey); This is what disconnects from the ReadingListModel, so we don't hold a pointer to it after it may be destroyed?
lgtm! Thanks for your patience through all the iterations of this CL!
Thanks. Adding unittests is clearly non trivial. There is no WSO unittests anywhere, and there is no TestNavigationManager. So It will likely be in a next CL. https://codereview.chromium.org/2578973002/diff/260001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2578973002/diff/260001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:160: web_state()->RemoveUserData(kObserverKey); On 2016/12/21 17:10:42, rohitrao wrote: > This is what disconnects from the ReadingListModel, so we don't hold a pointer > to it after it may be destroyed? No, this disconnect us from the webstate. RL model is a Browser state service so it will likely survive longer than the tab. I will likely add a RLModel observer for cb/676268 anyway so we will have cleaner unloading with that.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, kkhorimoto@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2578973002/#ps280001 (title: "rebase + const")
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": 280001, "attempt_start_ts": 1482394721042420,
"parent_rev": "c9e24b8a910b6a56fc6b6ff24131e8dde175b8cd", "commit_rev":
"234c66946cf80f670149cfc2ff1d42c8455a71d5"}
Message was sent while issue was closed.
Description was changed from ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 Review-Url: https://codereview.chromium.org/2578973002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 Review-Url: https://codereview.chromium.org/2578973002 ========== to ========== Reload offline version on load failure Change the way we trigger offline version. The ReadingListWebStateObserver now listens for all page loads. If there is an error or the page is too slow, and if an offline page is available, show the native content instead. Offline page is displayed by reloading on the offline URL. BUG=671954, 671160, 671963 Committed: https://crrev.com/9d91357e728e0d6821b6fed7fdce60d3dfc95193 Cr-Commit-Position: refs/heads/master@{#440367} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9d91357e728e0d6821b6fed7fdce60d3dfc95193 Cr-Commit-Position: refs/heads/master@{#440367} |
