Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470913007:
[ios] Refactored back-forward navigation in CRWSessionController. (Closed)
|
Created:
4 years, 1 month ago by Eugene But (OOO till 7-30) Modified:
4 years, 1 month ago CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Refactored back-forward navigation in CRWSessionController.
Notable changes:
- Added public -[CRWSessionController indexOfEntryForDelta:] which will
be used in future BF navigation refactorings (crbug.com/661858). This
API will allow to tell the future navigation index w/o making an
actual navigation and pass that future entry to
|isSameDocumentNavigationBetweenEntry:andEntry:| call.
- Made goDelta: no-op if delta is out of bounds. This make
CRWSessionController implementation better conform to w3 spec:
https://www.w3.org/TR/html5/browsers.html#the-history-interface
- Added public canGoDelta: API and use it in canGoBack/canGoForward.
- goBack and goForward now implemented by calling goDelta:-1 and
goDelta:1. This way the implementation is more efficient (no extra
calls of |discardNonCommittedEntries| and |discardTransientEntry|)
and cleaner.
- Do not change previousNavigationIndex if navigation did not happen.
- Added more unit tests.
Further cleanups (like reusing indexOfEntryForDelta: and
isRedirectTransitionForEntryAtIndex:) will be done in separate CLs.
BUG=661820
Committed: https://crrev.com/44dc0832429cf6754c254739d4939382e5ee3b71
Cr-Commit-Position: refs/heads/master@{#430435}
Patch Set 1 #Patch Set 2 : Self review #
Total comments: 6
Patch Set 3 : Addressed review comments #
Total comments: 2
Patch Set 4 : Use NSNotFound #
Total comments: 5
Patch Set 5 : Fixed typos #
Dependent Patchsets: Messages
Total messages: 28 (16 generated)
Description was changed from ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Added more unit tests. BUG=661820 ========== to ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta:) will be done in separate CLs. BUG=661820 ==========
Description was changed from ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta:) will be done in separate CLs. BUG=661820 ========== to ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ==========
Description was changed from ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ========== to ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Do not change previousNavigationIndex if navigation did not happen. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ==========
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org
https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.h (right): https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.h:159: // specified |delta| skipping redirect navigation items. The index returned is NIT: comma after |delta| https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:648: base::RecordAction(UserMetricsAction("Back")); From the user's perspective, a |-goDelta:| call is perceived as a single action. Are we overcounting by recording a UMA metric for each intermediary page in one of these navigations? https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:821: return count; I'm confused as to why you're returning |count| here. Could you clarify in a comment here & in the header?
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.h (right): https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.h:159: // specified |delta| skipping redirect navigation items. The index returned is On 2016/11/03 22:12:18, kkhorimoto_ wrote: > NIT: comma after |delta| Done. https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:648: base::RecordAction(UserMetricsAction("Back")); On 2016/11/03 22:12:19, kkhorimoto_ wrote: > From the user's perspective, a |-goDelta:| call is perceived as a single action. > Are we overcounting by recording a UMA metric for each intermediary page in one > of these navigations? That's the current behavior (goDelta just used to call goBack/goForward multiple times). However you brought a good point that we probably over-counting and this should probably be a single action recording. Once you happy with CL I'm going to send it to Rohit, and ask his opinion, but I'm actually inclined ditching this for loop here. https://codereview.chromium.org/2470913007/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:821: return count; On 2016/11/03 22:12:19, kkhorimoto_ wrote: > I'm confused as to why you're returning |count| here. Could you clarify in a > comment here & in the header? Added explanation comments inline and changed to NSIntegerMax (which by itself should lead to less confusion). I would prefer to avoid implementation specific comments in the header. Header comments already state that "The index returned is not guaranteed to be valid" and clients should not really care if it's |count| or NSIntegerMax or the other positive number out of range. Out of range number means that there is no valid for forward navigation. Also I added this code to preserve the current behavior and I'm actually going to remove when we add support for pendingIndex.
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.
lgtm https://codereview.chromium.org/2470913007/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:826: return NSIntegerMax; Great, this is much clearer now. One nit: we should use NSNotFound since it's an index.
eugenebut@chromium.org changed reviewers: + rohitrao@chromium.org
Thanks, Kurt! Rohit, can you take a look? https://codereview.chromium.org/2470913007/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:826: return NSIntegerMax; On 2016/11/04 00:45:20, kkhorimoto_ wrote: > Great, this is much clearer now. One nit: we should use NSNotFound since it's > an index. Done. https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:648: base::RecordAction(UserMetricsAction("Back")); Rohit, we probably over-counting Actions here and goDelta should actually report "Back" or "Forward" only once. Do you think this was done on purpose or I can just drop for-loop here?
Rohit?
lgtm Based on the tests, it sounds like there are no functional changes in this CL, aside from maybe previousNavigationEntry. Is that true? Would the new unittests pass if run on the old code? I'm very excited about unifying goBack and goDelta. It always seemed like a bad thing that we had three different implementations of this. https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:648: base::RecordAction(UserMetricsAction("Back")); On 2016/11/04 01:39:35, Eugene But wrote: > Rohit, we probably over-counting Actions here and goDelta should actually report > "Back" or "Forward" only once. Do you think this was done on purpose or I can > just drop for-loop here? I'd suggest staying as close to the original behavior in this CL. We can follow up to change it in a separate CL if we want. Maybe check with mardini to see if he looks at the Back/Forward metrics for anything? https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:695: // non-redirect enries. 2 typos: entries.
Thanks! > Based on the tests, it sounds like there are no functional changes in this CL, > aside from maybe previousNavigationEntry. Is that true? Would the new > unittests pass if run on the old code? There is one additional change mentioned in CL description: I made |goDelta| no-op if delta is out of bounds (which is correct behavior according to w3c spec). So new tests will fail with the old code, which goes back to first or last item if delta is out of bounds. Preserving old behavior for this CL would be tricky. https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:648: base::RecordAction(UserMetricsAction("Back")); On 2016/11/07 21:28:00, rohitrao wrote: > On 2016/11/04 01:39:35, Eugene But wrote: > > Rohit, we probably over-counting Actions here and goDelta should actually > report > > "Back" or "Forward" only once. Do you think this was done on purpose or I can > > just drop for-loop here? > > I'd suggest staying as close to the original behavior in this CL. We can follow > up to change it in a separate CL if we want. Maybe check with mardini to see if > he looks at the Back/Forward metrics for anything? Leaving as it is for this CL. Filed 663072 to figure out if we should report only one action. https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2470913007/diff/60001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:695: // non-redirect enries. On 2016/11/07 21:28:00, rohitrao wrote: > 2 typos: entries. Done.
The CQ bit was checked by eugenebut@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 eugenebut@chromium.org
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2470913007/#ps80001 (title: "Fixed typos")
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 ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Do not change previousNavigationIndex if navigation did not happen. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ========== to ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Do not change previousNavigationIndex if navigation did not happen. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Do not change previousNavigationIndex if navigation did not happen. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 ========== to ========== [ios] Refactored back-forward navigation in CRWSessionController. Notable changes: - Added public -[CRWSessionController indexOfEntryForDelta:] which will be used in future BF navigation refactorings (crbug.com/661858). This API will allow to tell the future navigation index w/o making an actual navigation and pass that future entry to |isSameDocumentNavigationBetweenEntry:andEntry:| call. - Made goDelta: no-op if delta is out of bounds. This make CRWSessionController implementation better conform to w3 spec: https://www.w3.org/TR/html5/browsers.html#the-history-interface - Added public canGoDelta: API and use it in canGoBack/canGoForward. - goBack and goForward now implemented by calling goDelta:-1 and goDelta:1. This way the implementation is more efficient (no extra calls of |discardNonCommittedEntries| and |discardTransientEntry|) and cleaner. - Do not change previousNavigationIndex if navigation did not happen. - Added more unit tests. Further cleanups (like reusing indexOfEntryForDelta: and isRedirectTransitionForEntryAtIndex:) will be done in separate CLs. BUG=661820 Committed: https://crrev.com/44dc0832429cf6754c254739d4939382e5ee3b71 Cr-Commit-Position: refs/heads/master@{#430435} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/44dc0832429cf6754c254739d4939382e5ee3b71 Cr-Commit-Position: refs/heads/master@{#430435}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2488553004/ by eugenebut@chromium.org. The reason for reverting is: GoDelta test fails on 32-bit platform. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
