Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628533002:
Fix tabs are not rendered properly when restore tabs after crash on iPad (Closed)
|
Created:
3 years, 11 months ago by liaoyuke Modified:
3 years, 11 months ago Reviewers:
rohitrao (ping after 24h) CC:
chromium-reviews, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix tabs are not rendered properly when restoring tabs after crash on iPad
On iPad, tabs are only rendered within half of the tabs strip when restoring all
tabs after the app crashed with four or more tabs open.
Background: During tabs restoration, old tabs are inserted before the current
tabs are removed, and after old tabs are inserted, the autoscroll amount of the
tab strip view is calculated based on all tabs, then removing current tabs
should not only remove the tabs, it is also expected to make sure that the
autoscroll amount is updated correctly, and this is done by shifting the
subviews by the amount of content offset that has changed before and after
removing the tabs.
The issue was happening because scrollRectToVisible with animation was called to
calculate and set the autoscroll amount, however, scrollRectToVisible with
animation is a lazy setter, which doesn't update the content offset of the tab
strip view until animation has actually finished. Meanwhile, removing current
tabs happened too soon, and to update the autoscroll amount, it referred to some
out-of-date content offset, which finally resulted in the mess.
This CL fixes the issue by changing from scrollRectToVisible with animation to
setContentOffset to set autoscroll amount if newly inserted tab is the rightmost
one, and this is because the tabs inserted during restoration are always at the
rightmost position.
BUG=679107
Review-Url: https://codereview.chromium.org/2628533002
Cr-Commit-Position: refs/heads/master@{#446042}
Committed: https://chromium.googlesource.com/chromium/src/+/0a4e2e647f579fc30f46b88acd1a63fafeca9058
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed feedback #
Total comments: 6
Patch Set 3 : Addressed feedback #Messages
Total messages: 23 (11 generated)
liaoyuke@chromium.org changed reviewers: + rohitrao@chromium.org
Hi Rohit, Please take a look and let me know how you think about this fix. Thank you very much!
On 2017/01/10 02:22:21, liaoyuke wrote: > Hi Rohit, > > Please take a look and let me know how you think about this fix. > > Thank you very much! PING?
On 2017/01/11 17:30:42, liaoyuke wrote: > On 2017/01/10 02:22:21, liaoyuke wrote: > > Hi Rohit, > > > > Please take a look and let me know how you think about this fix. > > > > Thank you very much! > > PING? Hi Rohit, I was wondering if you are able to take a look by the end of this week? If you are too busy, please let me know and I'll try to find someone else to help review. Thank you very much!
Description was changed from ========== Fix tabs are not rendered properly when restore tabs after crash on iPad on iPad, tabs are only rendered within half of the tabs strip when restore all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 ========== to ========== Fix tabs are not rendered properly when restoring tabs after crash on iPad on iPad, tabs are only rendered within half of the tabs strip when restore all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 ==========
Description was changed from ========== Fix tabs are not rendered properly when restoring tabs after crash on iPad on iPad, tabs are only rendered within half of the tabs strip when restore all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 ========== to ========== Fix tabs are not rendered properly when restoring tabs after crash on iPad On iPad, tabs are only rendered within half of the tabs strip when restoring all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 ==========
This new code will also fire whenever we create a new tab using the new tab button, right? Are there any behavior changes in that case? https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: if (tabIndex == [_tabArray count] - 1) { If crash restore adds 100 tabs to the tabstrip, will we shift every tab view 100 times? If you try with a smaller number of tabs, say 20, is there a noticeable delay in restoring? https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1281: for (UIView* view in [_tabStripView subviews]) { Is there any way to share this code with the other callsite at line 1200 above? https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1287: return; This function now has two early returns, which makes me think there's probably a better way to organize the code. Should we split this up into three separate functions, and call one of the three based on which case we are in?
Fixed a couple small typos in the CL description, but in general that was a really good description of the problem. Thanks for being so detailed!
Than you very much for fixing the typos! PTAL. https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: if (tabIndex == [_tabArray count] - 1) { On 2017/01/19 01:44:22, rohitrao wrote: > If crash restore adds 100 tabs to the tabstrip, will we shift every tab view 100 > times? If you try with a smaller number of tabs, say 20, is there a noticeable > delay in restoring? Yes, the complexity is n^2, every tab view will be shifted 100 times. I didn't see any noticeable delay for 20 tabs. While for 100 tabs, there is a noticeable delay both with or without the change, maybe a little bit longer with the change. https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1281: for (UIView* view in [_tabStripView subviews]) { On 2017/01/19 01:44:22, rohitrao wrote: > Is there any way to share this code with the other callsite at line 1200 above? Done. https://codereview.chromium.org/2628533002/diff/1/ios/chrome/browser/ui/tabs/... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1287: return; On 2017/01/19 01:44:22, rohitrao wrote: > This function now has two early returns, which makes me think there's probably a > better way to organize the code. Should we split this up into three separate > functions, and call one of the three based on which case we are in? From my point of view, I think it would be non-trivial to come up with good names for those three functions, I tried to think about some names, but I found them too confusing. Please let me know if you have some good names in mind. I agree that we should have a better way to organize the code, so I tentatively moved the new code into a separate function while left the old code unchanged. Once the old code is deleted, we can move it back. Please let me know how you think about this.
lgtm When you tested performance, what device did you test on? https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:289: // shift all of the tab strip subviews by an amount equal to the content offset Capitalize "Shift". https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:324: - (void)AutoScrollForNewTab:(NSUInteger)tabIndex; Lowercase "a" to start the method name. https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: - (void)updateContentOffsetForLastTab { Is this needed?
On 2017/01/24 14:19:24, rohitrao wrote: > lgtm > > When you tested performance, what device did you test on? > > https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... > File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tabs/tab_strip_controller.mm:289: // shift all of the tab > strip subviews by an amount equal to the content offset > Capitalize "Shift". > > https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tabs/tab_strip_controller.mm:324: - > (void)AutoScrollForNewTab:(NSUInteger)tabIndex; > Lowercase "a" to start the method name. > > https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: - > (void)updateContentOffsetForLastTab { > Is this needed? I tested performance both on iPad2 and iPad Mini 4. Do you feel like I need to test on more devices? Thank you for reviewing!
I've tested on two more devices in our cabinet, and I didn't see any noticeable performance degradation. So, I'll just land this CL. Thank you very much for reviewing! https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:289: // shift all of the tab strip subviews by an amount equal to the content offset On 2017/01/24 14:19:23, rohitrao wrote: > Capitalize "Shift". Done. https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:324: - (void)AutoScrollForNewTab:(NSUInteger)tabIndex; On 2017/01/24 14:19:23, rohitrao wrote: > Lowercase "a" to start the method name. Done. https://codereview.chromium.org/2628533002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: - (void)updateContentOffsetForLastTab { On 2017/01/24 14:19:23, rohitrao wrote: > Is this needed? Done.
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2628533002/#ps40001 (title: "Addressed feedback")
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 liaoyuke@chromium.org
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2628533002/#ps60001 (title: "Addressed feedback")
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": 1485363368098390,
"parent_rev": "b4054e8b83b60019c8cdcc9e9025fc6138725cf4", "commit_rev":
"0a4e2e647f579fc30f46b88acd1a63fafeca9058"}
Message was sent while issue was closed.
Description was changed from ========== Fix tabs are not rendered properly when restoring tabs after crash on iPad On iPad, tabs are only rendered within half of the tabs strip when restoring all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 ========== to ========== Fix tabs are not rendered properly when restoring tabs after crash on iPad On iPad, tabs are only rendered within half of the tabs strip when restoring all tabs after the app crashed with four or more tabs open. Background: During tabs restoration, old tabs are inserted before the current tabs are removed, and after old tabs are inserted, the autoscroll amount of the tab strip view is calculated based on all tabs, then removing current tabs should not only remove the tabs, it is also expected to make sure that the autoscroll amount is updated correctly, and this is done by shifting the subviews by the amount of content offset that has changed before and after removing the tabs. The issue was happening because scrollRectToVisible with animation was called to calculate and set the autoscroll amount, however, scrollRectToVisible with animation is a lazy setter, which doesn't update the content offset of the tab strip view until animation has actually finished. Meanwhile, removing current tabs happened too soon, and to update the autoscroll amount, it referred to some out-of-date content offset, which finally resulted in the mess. This CL fixes the issue by changing from scrollRectToVisible with animation to setContentOffset to set autoscroll amount if newly inserted tab is the rightmost one, and this is because the tabs inserted during restoration are always at the rightmost position. BUG=679107 Review-Url: https://codereview.chromium.org/2628533002 Cr-Commit-Position: refs/heads/master@{#446042} Committed: https://chromium.googlesource.com/chromium/src/+/0a4e2e647f579fc30f46b88acd1a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0a4e2e647f579fc30f46b88acd1a... |
