Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884973005:
Look up for pending navigation contexts instead of creating new ones. (Closed)
|
Created:
3 years, 7 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 7 months ago Reviewers:
kkhorimoto CC:
chromium-reviews, Eugene But (OOO till 7-30), ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLook up for pending navigation contexts instead of creating new ones.
Use -[CRWWKNavigationStates pendingNavigations] to get all pending
navigations and match pending context by URL.
BUG=713836
Review-Url: https://codereview.chromium.org/2884973005
Cr-Commit-Position: refs/heads/master@{#472892}
Committed: https://chromium.googlesource.com/chromium/src/+/1ba5140bd27b3ad9542e029ded6068a79dc56a3c
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : I can spell navigation, I promise #Patch Set 4 : Rebased #
Total comments: 4
Messages
Total messages: 19 (12 generated)
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org
lgtm https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4948: // comparing to the previous URL. Can we restructure this if statement since this condition doesn't contain any code anymore?
https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4948: // comparing to the previous URL. On 2017/05/17 23:05:37, kkhorimoto_ wrote: > Can we restructure this if statement since this condition doesn't contain any > code anymore? We could but, I wanted to keep a comment to explain the case when context exists. How do you want to restructure it?
https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4948: // comparing to the previous URL. On 2017/05/18 14:47:29, Eugene But wrote: > On 2017/05/17 23:05:37, kkhorimoto_ wrote: > > Can we restructure this if statement since this condition doesn't contain any > > code anymore? > We could but, I wanted to keep a comment to explain the case when context > exists. How do you want to restructure it? Hmm not a lot of great ways to do it. Maybe we could have a separate variable that's just a raw NavigationContextImpl*, and we could store |-contextForPendingNavigationWithURL:|'s return value there. Then if we need to create a new context, we store it in the unique ptr, then copy its value into the other stack variable? That way we could avoid needing to call it again later below. I think that might be more confusing than just having a no-op condition though, so feel free to land as is.
https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2884973005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4948: // comparing to the previous URL. On 2017/05/18 18:32:04, kkhorimoto_ wrote: > On 2017/05/18 14:47:29, Eugene But wrote: > > On 2017/05/17 23:05:37, kkhorimoto_ wrote: > > > Can we restructure this if statement since this condition doesn't contain > any > > > code anymore? > > We could but, I wanted to keep a comment to explain the case when context > > exists. How do you want to restructure it? > > Hmm not a lot of great ways to do it. Maybe we could have a separate variable > that's just a raw NavigationContextImpl*, and we could store > |-contextForPendingNavigationWithURL:|'s return value there. Then if we need to > create a new context, we store it in the unique ptr, then copy its value into > the other stack variable? That way we could avoid needing to call it again > later below. I think that might be more confusing than just having a no-op > condition though, so feel free to land as is. Thanks! Initially I had raw NavigationContextImpl*, but it required to have 2 variables (scoped_ptr and raw ptr), which looked confusing and required explanation comment. Landing as it is.
The CQ bit was checked by eugenebut@chromium.org
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": 60001, "attempt_start_ts": 1495133300097360,
"parent_rev": "f6f47b5a77854b023c3ccd6c57d1584051f2280a", "commit_rev":
"1ba5140bd27b3ad9542e029ded6068a79dc56a3c"}
Message was sent while issue was closed.
Description was changed from ========== Look up for pending navigation contexts instead of creating new ones. Use -[CRWWKNavigationStates pendingNavigations] to get all pending navigations and match pending context by URL. BUG=713836 ========== to ========== Look up for pending navigation contexts instead of creating new ones. Use -[CRWWKNavigationStates pendingNavigations] to get all pending navigations and match pending context by URL. BUG=713836 Review-Url: https://codereview.chromium.org/2884973005 Cr-Commit-Position: refs/heads/master@{#472892} Committed: https://chromium.googlesource.com/chromium/src/+/1ba5140bd27b3ad9542e029ded60... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1ba5140bd27b3ad9542e029ded60... |
