Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809563002:
[ios clean] Fixes Toolbar navigation crash. (Closed)
Description[ios clean] Fixes Toolbar navigation crash.
Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation
arrow, the App would crash. That was caused since the button doesn't get disabled fast enough after there's no more pages left to navigate.
This CL fixes that crash.
BUG=683793
Review-Url: https://codereview.chromium.org/2809563002
Cr-Commit-Position: refs/heads/master@{#463304}
Committed: https://chromium.googlesource.com/chromium/src/+/8a8f6656dc6031ec1007082eff44893addd12f94
Patch Set 1 #
Total comments: 3
Messages
Total messages: 13 (6 generated)
Description was changed from ========== [ios clean] Fixes Toolbar navigation crash. BUG=683793 ========== to ========== [ios clean] Fixes Toolbar navigation crash. Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation arrow, the App will crash. This CL fixes that crash. BUG=683793 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, lpromero@chromium.org, marq@chromium.org, rohitrao@chromium.org
PTAL I was planning on adding this fix when creating the new navigation object, but since the navigation object will probably bring up some discussion I though it would be better to just fix it in a separate CL.
Description was changed from ========== [ios clean] Fixes Toolbar navigation crash. Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation arrow, the App will crash. This CL fixes that crash. BUG=683793 ========== to ========== [ios clean] Fixes Toolbar navigation crash. Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation arrow, the App would crash. That was caused since the button doesn't get disabled fast enough after there's no more pages left to navigate. This CL fixes that crash. BUG=683793 ==========
https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:117: self.webState->GetNavigationManager()->GoBack(); I wonder if the GoBack() method should simply no-op if you can't go back.
LGTM to fix the crash, but see if the navigation API can be cleaned up. https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:117: self.webState->GetNavigationManager()->GoBack(); On 2017/04/10 05:13:15, edchin wrote: > I wonder if the GoBack() method should simply no-op if you can't go back. If it's a programming error to call GoBack() when CanGoBack() is false, then the public interface for those functions should say as much, and it should be enforced by DCHECK. If it's not a programming error, then it should silently no-op. Ask eugenebut@ which approach he would prefer.
https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:117: self.webState->GetNavigationManager()->GoBack(); On 2017/04/10 11:14:09, marq wrote: > On 2017/04/10 05:13:15, edchin wrote: > > I wonder if the GoBack() method should simply no-op if you can't go back. > > If it's a programming error to call GoBack() when CanGoBack() is false, then the > public interface for those functions should say as much, and it should be > enforced by DCHECK. > > If it's not a programming error, then it should silently no-op. > > Ask eugenebut@ which approach he would prefer. I would prefer that GoBack() fail loudly if CanGoBack() returns false. This is kind of area where silently doing nothing is more likely to cause bugs than DCHECKing. (As another point in favor, //content also has a NOTREACHED() in this codepath.) Another option would be to DCHECK(CanGoBack()) here and make sure that callers don't invoke this command. That's probably not a good idea, because UI is weird. But it would be nice to understand exactly why it's possible to mash fast enough to call this command multiple times before the button updates and gets disabled.
On 2017/04/10 12:47:04, rohitrao wrote: > https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... > File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): > > https://codereview.chromium.org/2809563002/diff/1/ios/clean/chrome/browser/ui... > ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:117: > self.webState->GetNavigationManager()->GoBack(); > On 2017/04/10 11:14:09, marq wrote: > > On 2017/04/10 05:13:15, edchin wrote: > > > I wonder if the GoBack() method should simply no-op if you can't go back. > > > > If it's a programming error to call GoBack() when CanGoBack() is false, then > the > > public interface for those functions should say as much, and it should be > > enforced by DCHECK. > > > > If it's not a programming error, then it should silently no-op. > > > > Ask eugenebut@ which approach he would prefer. > > I would prefer that GoBack() fail loudly if CanGoBack() returns false. This is > kind of area where silently doing nothing is more likely to cause bugs than > DCHECKing. (As another point in favor, //content also has a NOTREACHED() in > this codepath.) > > Another option would be to DCHECK(CanGoBack()) here and make sure that callers > don't invoke this command. That's probably not a good idea, because UI is > weird. But it would be nice to understand exactly why it's possible to mash > fast enough to call this command multiple times before the button updates and > gets disabled. Thanks everyone for the thoughts. Will commit this and follow up with Eugene so we can improve this.
The CQ bit was checked by sczs@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": 1, "attempt_start_ts": 1491843829468790, "parent_rev":
"3e81a3f2ab68a0f96e7b1ef8b32606b7ffef30cd", "commit_rev":
"8a8f6656dc6031ec1007082eff44893addd12f94"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Fixes Toolbar navigation crash. Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation arrow, the App would crash. That was caused since the button doesn't get disabled fast enough after there's no more pages left to navigate. This CL fixes that crash. BUG=683793 ========== to ========== [ios clean] Fixes Toolbar navigation crash. Ed kindly pointer out that If you quickly tap on the forward or back toolbar navigation arrow, the App would crash. That was caused since the button doesn't get disabled fast enough after there's no more pages left to navigate. This CL fixes that crash. BUG=683793 Review-Url: https://codereview.chromium.org/2809563002 Cr-Commit-Position: refs/heads/master@{#463304} Committed: https://chromium.googlesource.com/chromium/src/+/8a8f6656dc6031ec1007082eff44... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/8a8f6656dc6031ec1007082eff44... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
