Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780703002:
Reload with empty navigation manager should not crash (Closed)
|
Created:
3 years, 9 months ago by liaoyuke Modified:
3 years, 8 months ago Reviewers:
Eugene But (OOO till 7-30) 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. |
DescriptionReload with empty navigation manager should not crash
Previously, the app crashes when user tries to reload when navigation
manager is empty.
This CL fixes the issue by adding an earl return to |Reload| in
NavigationManagerImpl so that it does nothing when its empty.
This CL also adds corresponding unit tests to prevent regressions.
BUG=705707
Review-Url: https://codereview.chromium.org/2780703002
Cr-Commit-Position: refs/heads/master@{#460464}
Committed: https://chromium.googlesource.com/chromium/src/+/e70e89bec8b805f1056ba85e1e14c9486341d138
Patch Set 1 #Patch Set 2 : Change include navigation_manager.h to import #
Total comments: 8
Patch Set 3 : Addressed comments #Patch Set 4 : Rebase #Messages
Total messages: 21 (14 generated)
Description was changed from ========== Reload with empty navigation manager should not crash Previously, the app crashes when user tries to reload when navigation manager is empty. This CL fixes the issue by adding an earl return to |Reload| in NavigationManagerImpl so that it does nothing when its empty. This CL also adds corresponding unit tests to prevent regressions. BUG=705707 ========== to ========== Reload with empty navigation manager should not crash Previously, the app crashes when user tries to reload when navigation manager is empty. This CL fixes the issue by adding an earl return to |Reload| in NavigationManagerImpl so that it does nothing when its empty. This CL also adds corresponding unit tests to prevent regressions. BUG=705707 ==========
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org
Hey Eugene, PTAL. Thank you very much!
https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:78: // Tests that reload with web::ReloadType::NORMAL will not crash when navigation s/will not crash/is no-op https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:82: ASSERT_TRUE(navigation_manager); WebState can not return null NM https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:88: false /* check_for_repost */); Could you please check that NM is still empty. https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:91: // Tests that reload with web::ReloadType::ORIGINAL_REQUEST_URL will not crash Same comments as from the previous method
PTAL. Thank you very much! https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:78: // Tests that reload with web::ReloadType::NORMAL will not crash when navigation On 2017/03/28 01:46:38, Eugene But wrote: > s/will not crash/is no-op Done. https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:82: ASSERT_TRUE(navigation_manager); On 2017/03/28 01:46:38, Eugene But wrote: > WebState can not return null NM Done. https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:88: false /* check_for_repost */); On 2017/03/28 01:46:38, Eugene But wrote: > Could you please check that NM is still empty. Done. https://codereview.chromium.org/2780703002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:91: // Tests that reload with web::ReloadType::ORIGINAL_REQUEST_URL will not crash On 2017/03/28 01:46:38, Eugene But wrote: > Same comments as from the previous method Done.
The CQ bit was checked by liaoyuke@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.
PTAL. Just wanted to make sure this CL is still in your radar, take your time to review. Thanks!
Sorry, I thought I lgtmed this already. Thank you for pinging, lgtm
The CQ bit was checked by liaoyuke@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 liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2780703002/#ps60001 (title: "Rebase")
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": 1490811397006610,
"parent_rev": "a3911c42a8ec4d0f164a77ff99b723d2fde12754", "commit_rev":
"e70e89bec8b805f1056ba85e1e14c9486341d138"}
Message was sent while issue was closed.
Description was changed from ========== Reload with empty navigation manager should not crash Previously, the app crashes when user tries to reload when navigation manager is empty. This CL fixes the issue by adding an earl return to |Reload| in NavigationManagerImpl so that it does nothing when its empty. This CL also adds corresponding unit tests to prevent regressions. BUG=705707 ========== to ========== Reload with empty navigation manager should not crash Previously, the app crashes when user tries to reload when navigation manager is empty. This CL fixes the issue by adding an earl return to |Reload| in NavigationManagerImpl so that it does nothing when its empty. This CL also adds corresponding unit tests to prevent regressions. BUG=705707 Review-Url: https://codereview.chromium.org/2780703002 Cr-Commit-Position: refs/heads/master@{#460464} Committed: https://chromium.googlesource.com/chromium/src/+/e70e89bec8b805f1056ba85e1e14... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e70e89bec8b805f1056ba85e1e14... |
