Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1141423002:
[iOS] Fixed tab snapshot cache refresh issues on iPad. (Closed)
|
Created:
5 years, 7 months ago by sdefresne Modified:
5 years, 7 months ago Reviewers:
noyau (Ping after 24h) CC:
chromium-reviews, jbbegue Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[iOS] Fixed tab snapshot cache refresh issues on iPad.
BUG=463851
Committed: https://crrev.com/9b48f66b525f8e9d7e4d64a502f3b6ed4dafe715
Cr-Commit-Position: refs/heads/master@{#330731}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #Messages
Total messages: 11 (2 generated)
sdefresne@chromium.org changed reviewers: + noyau@chromium.org
Please take a look. jbbegue: FYI
https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... File ios/chrome/browser/snapshots/snapshot_cache.mm (right): https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:146: [[NSMutableDictionary alloc] initWithCapacity:kCacheInitialCapacity]); No need to allocate this on tablets. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:220: callback(image); Why would we retrieve the image if the callback is nil? On phone, it may be used to prefill the cache, but on iPad this is a very bad idea. At least in this case don't call the PostTask. Ideally can you add a DCHECK(!IsIPadIdiom()||callback) to the method (making sure nothing calls it that way)? https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:361: imageDictionary_.reset(dictionary); Same here, no need to even run anything here on ipad. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:372: [self retrieveImageForSessionID:sessionID callback:nil]; Not on ipad…
On 2015/05/19 21:36:35, noyau wrote: > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > File ios/chrome/browser/snapshots/snapshot_cache.mm (right): > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > ios/chrome/browser/snapshots/snapshot_cache.mm:146: [[NSMutableDictionary alloc] > initWithCapacity:kCacheInitialCapacity]); > No need to allocate this on tablets. > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > ios/chrome/browser/snapshots/snapshot_cache.mm:220: callback(image); > Why would we retrieve the image if the callback is nil? On phone, it may be used > to prefill the cache, but on iPad this is a very bad idea. At least in this case > don't call the PostTask. > > Ideally can you add a DCHECK(!IsIPadIdiom()||callback) to the method (making > sure nothing calls it that way)? > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > ios/chrome/browser/snapshots/snapshot_cache.mm:361: > imageDictionary_.reset(dictionary); > Same here, no need to even run anything here on ipad. > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > ios/chrome/browser/snapshots/snapshot_cache.mm:372: [self > retrieveImageForSessionID:sessionID callback:nil]; > Not on ipad… This is just upstreaming change incorrectly made downstream in https://chromereviews.googleplex.com/189677013. I'll forward the comment to the downstream developer so that he address them in a followup CL.
On 2015/05/20 08:13:54, sdefresne wrote: > On 2015/05/19 21:36:35, noyau wrote: > > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > > File ios/chrome/browser/snapshots/snapshot_cache.mm (right): > > > > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > > ios/chrome/browser/snapshots/snapshot_cache.mm:146: [[NSMutableDictionary > alloc] > > initWithCapacity:kCacheInitialCapacity]); > > No need to allocate this on tablets. > > > > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > > ios/chrome/browser/snapshots/snapshot_cache.mm:220: callback(image); > > Why would we retrieve the image if the callback is nil? On phone, it may be > used > > to prefill the cache, but on iPad this is a very bad idea. At least in this > case > > don't call the PostTask. > > > > Ideally can you add a DCHECK(!IsIPadIdiom()||callback) to the method (making > > sure nothing calls it that way)? > > > > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > > ios/chrome/browser/snapshots/snapshot_cache.mm:361: > > imageDictionary_.reset(dictionary); > > Same here, no need to even run anything here on ipad. > > > > > https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... > > ios/chrome/browser/snapshots/snapshot_cache.mm:372: [self > > retrieveImageForSessionID:sessionID callback:nil]; > > Not on ipad… > > This is just upstreaming change incorrectly made downstream in > https://chromereviews.googleplex.com/189677013. I'll forward the comment to the > downstream developer so that he address them in a followup CL. Sorry, rereading my statement I see that it can be misunderstood. I wanted to say that the change was done downstream (in https://chromereviews.googleplex.com/189677013) and was never upstreamed. The current issue is just upstreaming the changes that makes the snapshot_cache works on iPad. It is currently being redesigned and it is estimated that investing more effort in dealing with iPad/iPhone differences is not worth it.
Please take another look. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... File ios/chrome/browser/snapshots/snapshot_cache.mm (right): https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:146: [[NSMutableDictionary alloc] initWithCapacity:kCacheInitialCapacity]); On 2015/05/19 21:36:35, noyau wrote: > No need to allocate this on tablets. Done. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:220: callback(image); On 2015/05/19 21:36:34, noyau wrote: > Why would we retrieve the image if the callback is nil? On phone, it may be used > to prefill the cache, but on iPad this is a very bad idea. At least in this case > don't call the PostTask. > > Ideally can you add a DCHECK(!IsIPadIdiom()||callback) to the method (making > sure nothing calls it that way)? Changed the method to be a no-op on iPad if there is no callback. I didn't use DCHECK(!IsIPadIdiom()) as that may break downstream as the method is part of public API. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:361: imageDictionary_.reset(dictionary); On 2015/05/19 21:36:35, noyau wrote: > Same here, no need to even run anything here on ipad. Done. https://codereview.chromium.org/1141423002/diff/1/ios/chrome/browser/snapshot... ios/chrome/browser/snapshots/snapshot_cache.mm:372: [self retrieveImageForSessionID:sessionID callback:nil]; On 2015/05/19 21:36:35, noyau wrote: > Not on ipad… Done.
lgtm
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141423002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9b48f66b525f8e9d7e4d64a502f3b6ed4dafe715 Cr-Commit-Position: refs/heads/master@{#330731} |
