Chromium Code Reviews
Help | Chromium Project | Sign in
(143)

Issue 3163003: Mac tabpose: Add thumbnails (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by Nico (ooo until Aug 1)
Modified:
5 years, 2 months ago
CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Paweł Hajdan (OOO back Aug 1), brettw-cc_chromium.org
Visibility:
Public.

Description

Mac tabpose: Add thumbnails Most things actually work. Missing from this CL: * Reloading thumbnails for tabs that change. This is required to show non-white thumbnails for thumbnails that are still waiting on the net when tabpose is opened. * Showing infobars / bookmark bar in the thumbnail * Showing accelerated surfaces (youtube videos on 10.6, compositor on 10.6) BUG=50307 TEST=Enable tabpose. Should see thumbnails for all tabs (some loaded after a delay). Thumbnails should animate in correctly even if a tab has info bars, a detached NTP, or docked devtools. Tabs that haven't been frontmost since the window was last resized should look good. Opening many tabs and then immediately jumping into expose shouldn't crash. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57901

Patch Set 1 #

Patch Set 2 : bg #

Patch Set 3 : add notes #

Patch Set 4 : rebase #

Patch Set 5 : rebase #

Patch Set 6 : add code! #

Patch Set 7 : cancel ipcs #

Patch Set 8 : update comments #

Patch Set 9 : '' #

Patch Set 10 : observer stuff #

Patch Set 11 : rebase #

Patch Set 12 : comments and stuff #

Patch Set 13 : layout experiments #

Patch Set 14 : getting there. missing: clean-ups, tabstrip model observing #

Patch Set 15 : '' #

Patch Set 16 : crashing tests! #

Patch Set 17 : first test passes #

Patch Set 18 : tests pass #

Patch Set 19 : ... #

Patch Set 20 : stuff works! "only" cleanup missing #

Patch Set 21 : tests done #

Patch Set 22 : cleaning up... #

Patch Set 23 : '' #

Patch Set 24 : 'done?' #

Total comments: 72

Patch Set 25 : comments #

Patch Set 26 : comments #

Patch Set 27 : more comments #

Total comments: 5

Patch Set 28 : test #

Patch Set 29 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+733 lines, -114 lines) Patch
M base/scoped_vector.h View 1 chunk +24 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/tab_strip_controller.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/cocoa/tabpose_window.h View 2 3 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/tabpose_window.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 19 chunks +618 lines, -102 lines 0 comments Download
M chrome/browser/cocoa/tabpose_window_unittest.mm View 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +77 lines, -11 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 12 (0 generated)
Nico (ooo until Aug 1)
For some reason, this was super-annoying to write. Not sure why. Seems like a good ...
5 years, 11 months ago (2010-08-30 08:18:52 UTC) #1
viettrungluu
Some preliminary comments/questions/nits.... http://codereview.chromium.org/3163003/diff/34004/47004 File chrome/browser/cocoa/tabpose_window.mm (right): http://codereview.chromium.org/3163003/diff/34004/47004#newcode76 chrome/browser/cocoa/tabpose_window.mm:76: // renderer process, it' stored in ...
5 years, 11 months ago (2010-08-30 15:12:51 UTC) #2
Nico (ooo until Aug 1)
Answered all the non-nits comments. Will address the rest soon. http://codereview.chromium.org/3163003/diff/34004/47004 File chrome/browser/cocoa/tabpose_window.mm (right): http://codereview.chromium.org/3163003/diff/34004/47004#newcode134 ...
5 years, 11 months ago (2010-08-30 15:35:25 UTC) #3
viettrungluu
Mostly looks good, with some nits. Still have to review the unit test. http://codereview.chromium.org/3163003/diff/34004/47004 File ...
5 years, 11 months ago (2010-08-30 18:23:27 UTC) #4
Nico (ooo until Aug 1)
Let me put you my thanks. http://codereview.chromium.org/3163003/diff/34004/47004 File chrome/browser/cocoa/tabpose_window.mm (right): http://codereview.chromium.org/3163003/diff/34004/47004#newcode76 chrome/browser/cocoa/tabpose_window.mm:76: // renderer process, ...
5 years, 11 months ago (2010-08-30 18:31:03 UTC) #5
Nico (ooo until Aug 1)
http://codereview.chromium.org/3163003/diff/34004/47004 File chrome/browser/cocoa/tabpose_window.mm (right): http://codereview.chromium.org/3163003/diff/34004/47004#newcode154 chrome/browser/cocoa/tabpose_window.mm:154: - (id)initWithTabContents:(TabContents*)contents fullSize:(NSSize)fullSize { On 2010/08/30 18:23:28, viettrungluu wrote: ...
5 years, 11 months ago (2010-08-30 18:46:30 UTC) #6
viettrungluu
LA(lmost)GTLM. http://codereview.chromium.org/3163003/diff/34004/47004 File chrome/browser/cocoa/tabpose_window.mm (right): http://codereview.chromium.org/3163003/diff/34004/47004#newcode154 chrome/browser/cocoa/tabpose_window.mm:154: - (id)initWithTabContents:(TabContents*)contents fullSize:(NSSize)fullSize { On 2010/08/30 18:46:31, Nico ...
5 years, 11 months ago (2010-08-30 19:52:41 UTC) #7
viettrungluu
Let me try that again: LA(lmost)GE(nough)TM.
5 years, 11 months ago (2010-08-30 19:53:07 UTC) #8
Nico (ooo until Aug 1)
On 2010/08/30 19:53:07, viettrungluu wrote: > LAGETM. That sounds a bit like an airport.
5 years, 11 months ago (2010-08-30 20:38:16 UTC) #9
viettrungluu
LGETM.
5 years, 11 months ago (2010-08-30 20:44:05 UTC) #10
Nico (ooo until Aug 1)
right, "reply" doesn't send comments http://codereview.chromium.org/3163003/diff/53001/40012 File chrome/browser/cocoa/tabpose_window_unittest.mm (right): http://codereview.chromium.org/3163003/diff/53001/40012#newcode77 chrome/browser/cocoa/tabpose_window_unittest.mm:77: model->MoveTabContentsAt(0, 1, /*select_after_move=*/false); On ...
5 years, 11 months ago (2010-08-30 20:44:16 UTC) #11
viettrungluu
5 years, 11 months ago (2010-08-31 02:50:17 UTC) #12
I'm happy to see you working on your enthusiasm.

On 2010/08/30 20:44:16, Nico wrote:
> right, "reply" doesn't send comments
> 
> http://codereview.chromium.org/3163003/diff/53001/40012
> File chrome/browser/cocoa/tabpose_window_unittest.mm (right):
> 
> http://codereview.chromium.org/3163003/diff/53001/40012#newcode77
> chrome/browser/cocoa/tabpose_window_unittest.mm:77:
model->MoveTabContentsAt(0,
> 1, /*select_after_move=*/false);
> On 2010/08/30 19:52:41, viettrungluu wrote:
> > Could you exercise a bunch of different move tab scenarios (to < from, to >
> > from, to = from)? Also with different selected tabs....
> 
> That's a _great_ idea! I'm very excited about having the opportunity to work
on
> this.
> 
> Also, done.
> 
> http://codereview.chromium.org/3163003/diff/53001/40012#newcode86
> chrome/browser/cocoa/tabpose_window_unittest.mm:86:
model->CloseTabContentsAt(0,
> TabStripModel::CLOSE_NONE);
> On 2010/08/30 19:52:41, viettrungluu wrote:
> > Ditto.
> 
> Done as well. (The excitement wore off a little.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 0973731