Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925673002:
Call reload when user initiate reload using the omnibox (Closed)
DescriptionCall reload when user initiate reload using the omnibox
BUG=730192
Review-Url: https://codereview.chromium.org/2925673002
Cr-Commit-Position: refs/heads/master@{#478004}
Committed: https://chromium.googlesource.com/chromium/src/+/a84d5a044c5401028b18d84c738ac1f7fdfc2c2c
Patch Set 1 : consistent reload path #Patch Set 2 : add todo #
Total comments: 2
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by mrefaat@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...
Description was changed from ========== Call reload when user initiate reload using the omnibox BUG= ========== to ========== Call reload when user initiate reload using the omnibox BUG=676129 ==========
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 mrefaat@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mrefaat@chromium.org changed reviewers: + kkhorimoto@chromium.org
lgtm
Can you write a unittest for this change? Thanks
On 2017/06/06 16:55:32, kkhorimoto_ wrote: > Can you write a unittest for this change? Thanks Per our discussion, adding unit test won't test much instead, so instead we should add a DCHECK to make sure that when we have 2 same url the transition type is reload.
mrefaat@chromium.org changed reviewers: + sdefresne@chromium.org
mrefaat@chromium.org changed reviewers: + marq@chromium.org
https://codereview.chromium.org/2925673002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2925673002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3766: // TODO(crbug.com/730192): Add DCHECK to verify that whenever urlToLood is the Rather than leave this hanging as a bug, can you just add the DCHECK on line 3768? It's just: DCHECK(urlToLoad == [_model currentTab].lastCommitedURL) right?
lgtm
Description was changed from ========== Call reload when user initiate reload using the omnibox BUG=676129 ========== to ========== Call reload when user initiate reload using the omnibox BUG=730192 ==========
https://codereview.chromium.org/2925673002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2925673002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3766: // TODO(crbug.com/730192): Add DCHECK to verify that whenever urlToLood is the On 2017/06/08 09:16:25, marq (ping after 24h) wrote: > Rather than leave this hanging as a bug, can you just add the DCHECK on line > 3768? It's just: > > DCHECK(urlToLoad == [_model currentTab].lastCommitedURL) > > right? It's a todo as we are not 100% sure that this will be always the case, so i'll go through the code around more before i decide how to proceed with that.
The CQ bit was checked by mrefaat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2925673002/#ps60001 (title: "add todo")
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": 1496941320641460,
"parent_rev": "fa514a94aae583c9c3067eecd0f4255853b6cd0b", "commit_rev":
"a84d5a044c5401028b18d84c738ac1f7fdfc2c2c"}
Message was sent while issue was closed.
Description was changed from ========== Call reload when user initiate reload using the omnibox BUG=730192 ========== to ========== Call reload when user initiate reload using the omnibox BUG=730192 Review-Url: https://codereview.chromium.org/2925673002 Cr-Commit-Position: refs/heads/master@{#478004} Committed: https://chromium.googlesource.com/chromium/src/+/a84d5a044c5401028b18d84c738a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a84d5a044c5401028b18d84c738a... |
