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

Issue 3163003: Mac tabpose: Add thumbnails (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by Nico
Modified:
5 years, 3 months ago
CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr., 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
For some reason, this was super-annoying to write. Not sure why. Seems like a good ...
6 years 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 ...
6 years ago (2010-08-30 15:12:51 UTC) #2
Nico
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 ...
6 years 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 ...
6 years ago (2010-08-30 18:23:27 UTC) #4
Nico
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, ...
6 years ago (2010-08-30 18:31:03 UTC) #5
Nico
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: ...
6 years 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 ...
6 years ago (2010-08-30 19:52:41 UTC) #7
viettrungluu
Let me try that again: LA(lmost)GE(nough)TM.
6 years ago (2010-08-30 19:53:07 UTC) #8
Nico
On 2010/08/30 19:53:07, viettrungluu wrote: > LAGETM. That sounds a bit like an airport.
6 years ago (2010-08-30 20:38:16 UTC) #9
viettrungluu
LGETM.
6 years ago (2010-08-30 20:44:05 UTC) #10
Nico
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 ...
6 years ago (2010-08-30 20:44:16 UTC) #11
viettrungluu
6 years 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 e15fb99