Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(510)

Issue 10937028: Do not discard selected tab (Closed)

Created:
8 years, 3 months ago by simonhong_
Modified:
8 years, 3 months ago
Reviewers:
James Cook, sky
CC:
chromium-reviews
Base URL:
http://git.chromium.org/chromium/src.git@new
Visibility:
Public.

Description

When chrome is in low memory condition, it also discards selected tab. Selected tab should not be discarded. BUG=151446 TEST=OomPriorityManagerTest.OomPriorityManagerBasics Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158333

Patch Set 1 #

Total comments: 4

Patch Set 2 : Modify and Add Unit Tests #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : Disable discard selected tab and its unit test #

Total comments: 2

Patch Set 5 : handle active tab #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -14 lines) Patch
M chrome/browser/chromeos/oom_priority_manager.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/oom_priority_manager_browsertest.cc View 1 2 3 4 3 chunks +13 lines, -8 lines 0 comments Download
M chrome/browser/ui/tabs/tab_strip_model.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/tabs/tab_strip_model_unittest.cc View 1 2 3 4 1 chunk +3 lines, -4 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
simonhong_
Dear sky, Please review this patch. Thank you.
8 years, 3 months ago (2012-09-19 14:57:23 UTC) #1
sky
https://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc#newcode221 chrome/browser/ui/tabs/tab_strip_model.cc:221: // Only mark tabs that are not selected. When ...
8 years, 3 months ago (2012-09-19 17:06:59 UTC) #2
simonhong_
http://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): http://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc#newcode221 chrome/browser/ui/tabs/tab_strip_model.cc:221: // Only mark tabs that are not selected. When ...
8 years, 3 months ago (2012-09-20 10:26:28 UTC) #3
sky
http://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): http://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc#newcode221 chrome/browser/ui/tabs/tab_strip_model.cc:221: // Only mark tabs that are not selected. When ...
8 years, 3 months ago (2012-09-20 15:24:08 UTC) #4
simonhong_
https://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/10937028/diff/1/chrome/browser/ui/tabs/tab_strip_model.cc#newcode221 chrome/browser/ui/tabs/tab_strip_model.cc:221: // Only mark tabs that are not selected. When ...
8 years, 3 months ago (2012-09-20 16:07:59 UTC) #5
sky
https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc#newcode224 chrome/browser/ui/tabs/tab_strip_model.cc:224: if (!IsTabSelected(index)) { I don't understand this. What is ...
8 years, 3 months ago (2012-09-20 19:31:09 UTC) #6
simonhong_
https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc#newcode224 chrome/browser/ui/tabs/tab_strip_model.cc:224: if (!IsTabSelected(index)) { I'll explain problematic scenario according to ...
8 years, 3 months ago (2012-09-21 00:16:37 UTC) #7
simonhong_
On 2012/09/21 00:16:37, Simon Hong wrote: > https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc > File chrome/browser/ui/tabs/tab_strip_model.cc (right): > > https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc#newcode224 ...
8 years, 3 months ago (2012-09-21 12:57:45 UTC) #8
sky
On Thu, Sep 20, 2012 at 5:16 PM, <simon.hong81@gmail.com> wrote: > > https://codereview.chromium.org/10937028/diff/4002/chrome/browser/ui/tabs/tab_strip_model.cc > File ...
8 years, 3 months ago (2012-09-21 16:18:54 UTC) #9
simonhong_
2012. 9. 22. 오전 1:18에 "Scott Violet" <sky@chromium.org>님이 작성: > > On Thu, Sep 20, ...
8 years, 3 months ago (2012-09-21 23:01:41 UTC) #10
sky
On Fri, Sep 21, 2012 at 4:01 PM, Simon Hong <simon.hong81@gmail.com> wrote: > > 2012. ...
8 years, 3 months ago (2012-09-21 23:12:53 UTC) #11
simonhong_
On Sat, Sep 22, 2012 at 8:12 AM, Scott Violet <sky@chromium.org> wrote: > On Fri, ...
8 years, 3 months ago (2012-09-21 23:22:17 UTC) #12
sky
The right fix is either not to do that, or ignore requests to discard the ...
8 years, 3 months ago (2012-09-21 23:33:42 UTC) #13
simonhong_
I also think that disabling discarding the active tab is better. I'll make a patch ...
8 years, 3 months ago (2012-09-21 23:37:01 UTC) #14
simonhong_
Dear Scott, New patch and unit test are uploaded. Please review this patch. Thank you.
8 years, 3 months ago (2012-09-22 05:59:56 UTC) #15
sky
https://codereview.chromium.org/10937028/diff/10003/chrome/browser/chromeos/oom_priority_manager.cc File chrome/browser/chromeos/oom_priority_manager.cc (right): https://codereview.chromium.org/10937028/diff/10003/chrome/browser/chromeos/oom_priority_manager.cc#newcode234 chrome/browser/chromeos/oom_priority_manager.cc:234: if (model->IsTabDiscarded(idx) || model->IsTabSelected(idx)) Is it the selected tab, ...
8 years, 3 months ago (2012-09-24 13:56:11 UTC) #16
simonhong_
https://codereview.chromium.org/10937028/diff/10003/chrome/browser/chromeos/oom_priority_manager.cc File chrome/browser/chromeos/oom_priority_manager.cc (right): https://codereview.chromium.org/10937028/diff/10003/chrome/browser/chromeos/oom_priority_manager.cc#newcode234 chrome/browser/chromeos/oom_priority_manager.cc:234: if (model->IsTabDiscarded(idx) || model->IsTabSelected(idx)) On 2012/09/24 13:56:11, sky wrote: ...
8 years, 3 months ago (2012-09-24 15:15:46 UTC) #17
sky
LGTM
8 years, 3 months ago (2012-09-24 16:46:10 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/10937028/15001
8 years, 3 months ago (2012-09-24 16:48:02 UTC) #19
commit-bot: I haz the power
8 years, 3 months ago (2012-09-24 18:47:41 UTC) #20
Change committed as 158333

Powered by Google App Engine
This is Rietveld 408576698