Issue metadata
Sign in to add a comment
|
Unresponsive stop(x) button: Pages would never stop loading, even after pressing the stop button |
||||||||||||||||||||
Issue descriptionVersion: 53.0.2774.2 iOS Version: 9.3.3 Device: iPad, not sure about iPhone. 1. Surf, create tabs, restore closed tabs, go back and forth, open links in new tabs (No specific information about which websites to surf on yet) 2. At some point, tabs would not finish loading (or would take minutes to finish) and pressing the stop button does absolutely nothing. 3. The icon remains the X and the tab's spinner is still spinning Actual behavior: Pages would never stop loading, even after pressing the stop button. Expected behavior: Pages should stop loading on pressing stop button. This is not consistently reproducible and we don't know for sure whether it is a regression or not.
,
Aug 16 2016
On tapping "Stop Loading" button, the icon does not change to "Reload". The icon remains the X and the tab's spinner is still spinning.
,
Aug 16 2016
Steps to reproduce: 1. Load https://www.specialized.com/us/en/bikes/road/ 2. Tap Stop before load is completed
,
Aug 16 2016
WebView actually stops loading. Either embedder is not notified that the load was stopped or has UI bug.
,
Aug 16 2016
This is M53 regression, which happened after ToolbarModelImplIOS::IsLoading refactoring.
,
Aug 17 2016
,
Aug 17 2016
,
Aug 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16e4de4de55c5c946b8bdfa47bfee274afc930f2 commit 16e4de4de55c5c946b8bdfa47bfee274afc930f2 Author: eugenebut <eugenebut@chromium.org> Date: Wed Aug 17 00:09:15 2016 [ios] Set WebState::IsLoading flag to false if load is cancelled. Before this change WebState::IsLoading was returning false which caused Toolbar to think that page did not finish loading. In M52 and earlier toolbar was relying to CRWWebController.loadPhase API to check the load state so this bug is M53 regression. Keeping WebState::IsLoading in sync with CRWWebController.loadPhase fixes toobar bug and is generally a reasonable thing to do. BUG= 638277 Review-Url: https://codereview.chromium.org/2250563003 Cr-Commit-Position: refs/heads/master@{#412391} [modify] https://crrev.com/16e4de4de55c5c946b8bdfa47bfee274afc930f2/ios/web/web_state/ui/crw_web_controller.mm
,
Aug 17 2016
Issue 629351 has been merged into this issue.
,
Aug 17 2016
The fix did not get into the latest Canary
,
Aug 17 2016
Is there a unit test or EG test to ensure this behavior doesn't regress in the future?
,
Aug 17 2016
There is no test for that. This is something that is tricky to test with our local http server. Claude added this to manual test cases (this is really easy to test manually, you just hit stop button before page finish its loading). Functionality has regressed because WebState had a bug probably since we wrote this class. So if something will regress during the future refactoring (specifically moving from private web API to public web API), this will be a different thing. I think we should automate things which are not hard to automate, and give us significant coverage (like Navigation tests which we are writing in Q3). Also I think that we should have better unit tests, but for that we need to refactor our WebController god class, which we have plans to do in the future quarters. Writing tests for past regressions is great if they are easy to write, which is not the case for this bug. Hopefully it answers your question.
,
Aug 17 2016
Is the local HTTP server asynchronous? Can we write a test page that contains an IMG that never finishes loading?
,
Aug 17 2016
Server is asynchronous, and writing test is possible but tricky. Taking into account the reason of this regression, I just don't feel that spending time on this specific test will pay back (especially taking into account that it has been added to manual test cases). If you strongly feel that we should test this I will write this test it's not a problem, I just feel that we can do different things to improve the quality (like refactor the code so we can finally write useful unit tests or write test suites for areas where regressions are very common). Your questions suggested that we should go through all web regression bugs (we have the history of those in bugtracker) and identify which areas we should cover with EG tests (not hard to test, frequent regressions) and write those tests as a planned effort. And this is something that we can and probably should prioritize for Q4. WDYT?
,
Aug 18 2016
There's no way to write a test that when we tell the webview to stop that the button is in the right state?
,
Aug 18 2016
There was no canary since Aug 17. The test will be written. Per Comment #12 and #14 there is a way to write this test.
,
Aug 18 2016
Work fine in Chrome 54.0.2832.0 Canary.
,
Aug 18 2016
[Automated comment] Less than 2 weeks to go before AppStore submit on M53, manual review required.
,
Aug 18 2016
Verified on 54.0.2832.0 canary iPad Air 2 iOS 9.3.2, iPad mini 9.3.3, iPad Pro iOS 9.2.1 Page stop loading on pressing stop button (specialized.com) Looks good.
,
Aug 18 2016
,
Aug 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa1c693955107d0e4d43ed59ebabe974ee698665 commit aa1c693955107d0e4d43ed59ebabe974ee698665 Author: Eugene But <eugenebut@google.com> Date: Thu Aug 18 21:00:53 2016 [ios] Set WebState::IsLoading flag to false if load is cancelled. Before this change WebState::IsLoading was returning false which caused Toolbar to think that page did not finish loading. In M52 and earlier toolbar was relying to CRWWebController.loadPhase API to check the load state so this bug is M53 regression. Keeping WebState::IsLoading in sync with CRWWebController.loadPhase fixes toobar bug and is generally a reasonable thing to do. BUG= 638277 Review-Url: https://codereview.chromium.org/2250563003 Cr-Commit-Position: refs/heads/master@{#412391} (cherry picked from commit 16e4de4de55c5c946b8bdfa47bfee274afc930f2) Review URL: https://codereview.chromium.org/2260003002 . Cr-Commit-Position: refs/branch-heads/2785@{#668} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/aa1c693955107d0e4d43ed59ebabe974ee698665/ios/web/web_state/ui/crw_web_controller.mm
,
Aug 19 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/2cc803f425524b0c974d6bfbcefc70f0c40661ce commit 2cc803f425524b0c974d6bfbcefc70f0c40661ce Author: eugenebut <eugenebut@google.com> Date: Fri Aug 19 19:01:10 2016
,
Aug 19 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/2cc803f425524b0c974d6bfbcefc70f0c40661ce commit 2cc803f425524b0c974d6bfbcefc70f0c40661ce Author: eugenebut <eugenebut@google.com> Date: Fri Aug 19 19:01:10 2016
,
Aug 24 2016
Verified on 53.0.2785.80 beta iPad Air 2 iOS 10 beta 7, iPad Pro iOS 9.3.3, following the steps mentioned in comment # 0 Page stop loading on pressing stop button. Looks good. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 Deleted