Implements AeroPeek of Windows 7..

Implements AeroPeek of Windows 7.
This change integrates the custom AeroPeek implementation into Chromium, which shows the thumbnail list of all tabs and the preview image of the tab selected from the thumbnail list. It uses the AeroPeekManager object, which is a proxy between TabStripModel and Windows to translate events from TabStripModel for Windows, and vice versa. To listen events from TabStripModel without changing the existing part of Chromium, this AeroPeekManager class implements the TabStripModelObserver interface.
Even though this change doesn't include any automated tests for AeroPeek, I would like to send its automated UI test as a separate change. Nevertheless, it just creates/deletes a tab and see this AeroPeekManager can create its thumbnail window correctly.

BUG=6337
TEST=base_unittests.exe --gtest_filter=ScopedNativeLibrary.Basic

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41133

[Hironori Bono, 2010-01-15 11:13:18]

Sorry for sending such a big change without tests.
Even though this is not a perfect change, it implements most of AeroPeek
features. Is it possible to give me your comments?

Regards,

Hironori Bono

[Avi (use Gerrit), 2010-01-15 16:50:26]

I'm not qualified to review the Windows code, but it looked cool so here's a
drive-by.

http://codereview.chromium.org/303033/diff/6019/6020
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/303033/diff/6019/6020#newcode197
base/scoped_handle_win.h:197: class ScopedHICON  {
Why not have a specialization of ScopedGDIObject? (And an extra space before the
brace.)

http://codereview.chromium.org/303033/diff/6019/6024
File chrome/browser/aeropeek_manager.cc (right):

http://codereview.chromium.org/303033/diff/6019/6024#newcode39
chrome/browser/aeropeek_manager.cc:39: // TODO(hbono): Bug 16903: to be deleted
them when we use Windows SDK 7.0.
strike "them" from the sentence.

http://codereview.chromium.org/303033/diff/6019/6024#newcode157
chrome/browser/aeropeek_manager.cc:157: class ScopedCompatibleBitmap {
Why does this not go in the base/scoped_ref* with the other changes you made?

http://codereview.chromium.org/303033/diff/6019/6024#newcode209
chrome/browser/aeropeek_manager.cc:209: class ScopedNativeLibrary {
Surely we have something to handle this in base/ already?

http://codereview.chromium.org/303033/diff/6019/6024#newcode493
chrome/browser/aeropeek_manager.cc:493: //     +--------------------+
<3 comments like these. Awesome.

http://codereview.chromium.org/303033/diff/6019/6024#newcode860
chrome/browser/aeropeek_manager.cc:860: // SDK 6.1, we use their values instead.
Didn't you copy a good chunk of the SDK definitions to the top of this file? Can
we put these definitions there too and use the names here?

http://codereview.chromium.org/303033/diff/6019/6025
File chrome/browser/aeropeek_manager.h (right):

http://codereview.chromium.org/303033/diff/6019/6025#newcode23
chrome/browser/aeropeek_manager.h:23: // An AeroPek window receives the
following events:
Typo: AeroPeek

http://codereview.chromium.org/303033/diff/6019/6025#newcode32
chrome/browser/aeropeek_manager.h:32: // Also, it needs the following
information of the browser.
End sentence in :

http://codereview.chromium.org/303033/diff/6019/6025#newcode59
chrome/browser/aeropeek_manager.h:59: // following events sent from
TabStripModel and dispatch them to Windows.
:

http://codereview.chromium.org/303033/diff/6019/6025#newcode70
chrome/browser/aeropeek_manager.h:70: // TabStripModel.
:

http://codereview.chromium.org/303033/diff/6019/6025#newcode77
chrome/browser/aeropeek_manager.h:77: // 1. Create an instance of
TabThumbnailManager, and;
End sentence with . as you do all your other list items.

http://codereview.chromium.org/303033/diff/6019/6025#newcode155
chrome/browser/aeropeek_manager.h:155: // accoding tab (and TabContents) may be
deleted by Chrome. To prevent from
accoding? Perhaps "the corresponding tab (and TabContents) may have been
deleted..."

[Peter Kasting, 2010-01-15 18:08:29]

Drive-by: _Must_ we have --disable-aeropeek?  We usually try to avoid adding
options unless we really have to, and I'm not sure what justifies this one.

[Hironori Bono, 2010-01-21 10:34:17]

Thank you for your review and comment.

On 2010/01/15 18:08:29, Peter Kasting wrote:
> Drive-by: _Must_ we have --disable-aeropeek?  We usually try to avoid adding
> options unless we really have to, and I'm not sure what justifies this one.

Even though I added this option since some people wrote "it would be nice to
have..." in <http://crbug.com/6337>, I think this doesn't become a must-have
reason. (Firefox doesn't have such options, neither.) So, I deleted this option.
Is it possible to review the updated one?

Regards,

Hironori Bono

[Hironori Bono, 2010-01-21 10:35:42]

Peter and Avi,

Thank you for your review and comments.
I have updated this change to satisfy your comments as much as possible. Is it
possible to review the updated one?

Regards,

Hironori Bono

http://codereview.chromium.org/303033/diff/6019/6020
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/303033/diff/6019/6020#newcode197
base/scoped_handle_win.h:197: class ScopedHICON  {
On 2010/01/15 16:50:26, Avi wrote:
> Why not have a specialization of ScopedGDIObject? (And an extra space before
the
> brace.)

Done. Thank you for noticing this. I just forgot using template specialization
when I wrote this class. :)

http://codereview.chromium.org/303033/diff/6019/6024
File chrome/browser/aeropeek_manager.cc (right):

http://codereview.chromium.org/303033/diff/6019/6024#newcode39
chrome/browser/aeropeek_manager.cc:39: // TODO(hbono): Bug 16903: to be deleted
them when we use Windows SDK 7.0.
On 2010/01/15 16:50:26, Avi wrote:
> strike "them" from the sentence.

Done.

http://codereview.chromium.org/303033/diff/6019/6024#newcode157
chrome/browser/aeropeek_manager.cc:157: class ScopedCompatibleBitmap {
On 2010/01/15 16:50:26, Avi wrote:
> Why does this not go in the base/scoped_ref* with the other changes you made?

Done. Thank you for noticing this. Even though I wrote this class to share the
initialization code for a BITMAPINFO struct, I noticed it is the same as
gfx::CreateBitmapHelper(). So I removed this class and use this function.

http://codereview.chromium.org/303033/diff/6019/6024#newcode209
chrome/browser/aeropeek_manager.cc:209: class ScopedNativeLibrary {
On 2010/01/15 16:50:26, Avi wrote:
> Surely we have something to handle this in base/ already?

As far as I searched the Chromium code, it directly uses functions in
"base/native_library.h". So, I added "base/scoped_native_library.h" and its unit
test. (Please correct me if I'm wrong.)

http://codereview.chromium.org/303033/diff/6019/6024#newcode493
chrome/browser/aeropeek_manager.cc:493: //     +--------------------+
On 2010/01/15 16:50:26, Avi wrote:
> <3 comments like these. Awesome.

Thank you. I would love to use ASCII arts since I'm an ASCII-art lover. :)

http://codereview.chromium.org/303033/diff/6019/6024#newcode860
chrome/browser/aeropeek_manager.cc:860: // SDK 6.1, we use their values instead.
On 2010/01/15 16:50:26, Avi wrote:
> Didn't you copy a good chunk of the SDK definitions to the top of this file?
Can
> we put these definitions there too and use the names here?

Done. Thank you for noticing this.
I have copied whole this enum definitions here from Windows SDK 7.

http://codereview.chromium.org/303033/diff/6019/6025
File chrome/browser/aeropeek_manager.h (right):

http://codereview.chromium.org/303033/diff/6019/6025#newcode23
chrome/browser/aeropeek_manager.h:23: // An AeroPek window receives the
following events:
On 2010/01/15 16:50:26, Avi wrote:
> Typo: AeroPeek

Done.

http://codereview.chromium.org/303033/diff/6019/6025#newcode32
chrome/browser/aeropeek_manager.h:32: // Also, it needs the following
information of the browser.
On 2010/01/15 16:50:26, Avi wrote:
> End sentence in :

Done.

http://codereview.chromium.org/303033/diff/6019/6025#newcode59
chrome/browser/aeropeek_manager.h:59: // following events sent from
TabStripModel and dispatch them to Windows.
On 2010/01/15 16:50:26, Avi wrote:
> :

Done.

http://codereview.chromium.org/303033/diff/6019/6025#newcode70
chrome/browser/aeropeek_manager.h:70: // TabStripModel.
On 2010/01/15 16:50:26, Avi wrote:
> :

Done.

http://codereview.chromium.org/303033/diff/6019/6025#newcode77
chrome/browser/aeropeek_manager.h:77: // 1. Create an instance of
TabThumbnailManager, and;
On 2010/01/15 16:50:26, Avi wrote:
> End sentence with . as you do all your other list items.

Done.

http://codereview.chromium.org/303033/diff/6019/6025#newcode155
chrome/browser/aeropeek_manager.h:155: // accoding tab (and TabContents) may be
deleted by Chrome. To prevent from
On 2010/01/15 16:50:26, Avi wrote:
> accoding? Perhaps "the corresponding tab (and TabContents) may have been
> deleted..."

Done.

[Ben Goodger (Google), 2010-01-21 17:14:49]

Very sorry this dropped out of my inbox. I will review this today.

-Ben

On Fri, Jan 15, 2010 at 3:13 AM,  <hbono@chromium.org> wrote:
> Reviewers: Ben Goodger,
>
> Message:
> Sorry for sending such a big change without tests.
> Even though this is not a perfect change, it implements most of AeroPeek
> features. Is it possible to give me your comments?
>
> Regards,
>
> Hironori Bono
>
> Description:
> Implements AeroPeek of Windows 7.
> This change integrates the custom AeroPeek implementation into Chromium,
> which
> shows the thumbnail list of all tabs and the preview image of the tab
> selected
> from the thumbnail list. It uses the AeroPeekManager object, which is a
> proxy
> between TabStripModel and Windows to translate events from TabStripModel for
> Windows, and vice versa. To listen events from TabStripModel without
> changing
> the existing part of Chromium, this AeroPeekManager class implements the
> TabStripModelObserver interface. Also, this changes adds a new option
> "--disable-aeropeek" for those who don't like this behavior.
>
> Even though this change doesn't include any automated tests, I would like to
> send its automated UI test as a separate change. Nevertheless, it just
> creates/deletes a tab and see this AeroPeekManager can create its thumbnail
> window correctly.
>
> BUG=6337
> TEST=none
>
> Please review this at http://codereview.chromium.org/303033
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     base/scoped_handle_win.h
>  A     chrome/browser/aeropeek_manager.h
>  A     chrome/browser/aeropeek_manager.cc
>  M     chrome/browser/views/frame/browser_view.h
>  M     chrome/browser/views/frame/browser_view.cc
>  M     chrome/chrome_browser.gypi
>  M     chrome/common/chrome_switches.h
>  M     chrome/common/chrome_switches.cc
>
>
>

[Leith, 2010-01-30 11:40:53]

I tried this on my machine and it was very glitchy.

The toolbar height calculation was wrong, and GDI was screwing the alpha channel
from the whole window BitBlt.

I have made significant changes to get this working properly.

It now works the same way as IE8:
The bitmap only contains the tab contents (not the whole window/frame/toolbar).
DWM is told to position it on top of the foreground tab.
The rest of the window/frame comes from the original window that DWM keeps on
the screen.

This now works better than the horrible implementation in Firefox 3.7. They are
missing the window favicon, their frame has inactive flat window buttons (the
should be the active shiny ones), and you can see the original window though the
peek windows Aero Glass (as DWM keeps the original window visible in the
background.

I can't see how a non-author can change the patch set, so I am linking the diff
here:
https://sites.google.com/a/leithalweapon.geek.nz/www/fixed-aero-peek.diff?att...

[Leith, 2010-01-30 11:41:45]

Not sure if previous comment had the REviewrs/CC box filled out...

[Leith, 2010-01-31 08:29:30]

I have changed my diff file.

I have:
Fixed the calculation of the left, right, and bottom content insets when the
window is maximized.

Moved the code for updating the favicon and title of the taskbar tab to
TabChangedAt from DrawThumbnail, because Windows needs the icon and title
*before* it requests a thumbnail. It does it in this order because it shows the
popup with a rotating dummy thumbnail until it gets a thumbnail.

I have come across some more bugs/glitches/flaws:
The taskbar tab list is not correctly synchronized with Chrome's tab list.

Creating a new tab to the left of an existing tab (such as middle clicking a
link) - RegisterThumbnail assumes incorrectly that a new tab will always be at
the far left -> ITaskbarList3->SetTabOrder(window_, NULL).
That NULL should be the HWND of the tab window to insert *before* in the list of
taskbar tabs.

Moving the position of a tab doesn't result in the taskbar tab list getting
reorganized. TabMoved should call ITaskbarList3->SetTabOrder to notify Windows
that it needs to move the tab.

Dragging a tab into a new or extisting window screws the taskbar list bad as it
doesn't remove it from the old window's list so you end up with two tabs (the
old one points to nothing).
TabDetachedAt should call ITaskbarList3->UnregisterTab to remove it from the
list. Then it will be automatically added when the new window's TabInsertedAt's
handler.

Looking at the favicon code, what happens if contents->GetFavIcon().isNull() is
true?
Is this a possible situation, and if it is we should hand Windows Chrome's
default icon IDR_MAINFRAME otherwise we will get the ugly default Windows'
window icon.

The last major problem area is that background tabs to not get redrawn when the
thumbnail is requested for them. This is an important as we need to get it
working properly like IE8 does.

I suggest using the pre-exsting code in RenderView::CaptureThumbnail and
RenderView::CaptureSnapshot which appears to added for the extension API
(chrome.tabs.captureVisibleTab(integer windowId, function callback))

They both call PaintViewIntoCanvas which will render the tab fresh into a custom
canvas of any dimension completely bypassing the BackingStore and it's issues
(ie not updating when a tab is in the background).

This will result in a up-to-date tab render whenever Windows requests Chrome
generate a new thumbnail.

By specifying the canvas size to always be the same size as the "user perceived
content area" we will avoid all the issues resulting from the window being
resized after a tab has become background.

The other benefit is we can make the implementation even more IE8 compatible by
ensureing every thumbnail is generated from the same canvas dimensions as the
full size preview so that every thumbnail's aspect ratio will match regardless
of what info bars etc. are at the top.

When this is done we need to ensure the if statement in TabChangedAt is removed
so that we always invalidate the thumbnail even if the tab is in the background
and/or loading since Chrome will be able to always render on demand.

Just my two cents.

[Leith, 2010-01-31 08:30:21]

I have updated my comments and code after more testing.

[Hironori Bono, 2010-02-01 10:57:13]

Leith,

Thank you for your comments and diff.
I have merged some of your diff and fixed the detach bug.
Nevertheless, I'm wondering if we should move your code that updates the
thumbnail icon in TabChangedAt() because the function is called from the UI
thread, i.e. it blocks all UI events (including rendering events) of Chrome
while it creates an icon. Since we would like to support AeroPeek without losing
the performance of Chrome at all, this change avoids calling such heavy
functions from the UI thread. (This is the reason why this change creates an
icon in a task even though it causes problems written in your comment.)
Unfortunately, we probably need to make this change more complicated than the
current one to fix this problem without blocking the UI thread. As you know,
this change has been already very complicated so it takes long time for
reviewers to review it. So, I'm wondering if it is a good idea to make this
change more complicated and confuse reviewers now. (Personally, I would like to
create another change to fix reported problems.)

Regards,

Hironori Bono

[Ben Goodger (Google), 2010-02-03 04:17:20]

Bono-san,

Reading Leith's comments it sounds like you show the full window in the tab
preview? I think we only want to show the contents. Can you update the CL to
reflect this? Let me know when you have. Sorry for the lengthy delays :-(

-Ben

[Hironori Bono, 2010-02-03 10:39:11]

Ben,

Thank you for your comment.
On 2010/02/03 04:17:20, Ben Goodger wrote:
> Bono-san,
> 
> Reading Leith's comments it sounds like you show the full window in the tab
> preview? I think we only want to show the contents. Can you update the CL to
> reflect this? Let me know when you have.

I have applied his change that shows only the contents, which is a change to
SendLivePreviewTask::Run(), and fixed the detach/attach issue.
Would it be possible to review the updated one?

> Sorry for the lengthy delays :-(

they are not a problem at all :) I realize this is a very complicated change.
Sorry for sending such a complicated change.

Regards,

Hironori Bono

[Hironori Bono, 2010-02-16 09:39:39]

ping?

[Ben Goodger (Google), 2010-02-16 19:15:26]

Can you evaluate the page load performance impact of this change? Currently we
have no bots that do performance testing on Windows 7, so you'll have to do this
manually:

Make sure you haven't excluded the page cycler data in your .gclient file, sync
and then run:

page_cycler_tests.exe --gtest_filter=PageCycler*.MozFile

http://codereview.chromium.org/303033/diff/28001/29007
File chrome/browser/aeropeek_manager.cc (right):

http://codereview.chromium.org/303033/diff/28001/29007#newcode242
chrome/browser/aeropeek_manager.cc:242: class RegisterThumbnailTask : public
Task {
Can you explain your thread usage? It looks like you run all these tasks on the
IO thread, yet they make heavy use of objects that are not threadsafe and live
on the UI thread, like the delegate_ which touches TabContents, etc etc.

http://codereview.chromium.org/303033/diff/28001/29007#newcode292
chrome/browser/aeropeek_manager.cc:292: if (!active_)
to me this would be better as:

if (active_) {
...
}

rather than splitting the function with the early return.

http://codereview.chromium.org/303033/diff/28001/29007#newcode346
chrome/browser/aeropeek_manager.cc:346: delegate_->DrawTabThumbnail(tab_id_,
thumbnail_window_,
This task is run on the IO thread right? But your delegate lives on the UI
thread. You can't call directly from here, you need to post this back to the UI
thread.

But then what's the point of using the thread? What is it that's slow that
you're trying to parallelize?

http://codereview.chromium.org/303033/diff/28001/29007#newcode364
chrome/browser/aeropeek_manager.cc:364: // An event to notify this task
finishes.
to notify WHEN this task finishes.

http://codereview.chromium.org/303033/diff/28001/29007#newcode495
chrome/browser/aeropeek_manager.cc:495: // This window uses this information not
only for high-lighting the selected
highlighting

[Hironori Bono, 2010-02-17 10:46:42]

Thank you for your review and comments.
To run PageCyclerTests on my Win7 box with our without this change (as of
r39180), their performances are almost the same. Nevertheless, it is better to
avoid accessing TabContents from an I/O thread. I will send you another review
request when I finish rewriting this change.

* PageCyclerTest.MozFile
  - With this change: 25433 ms, 25598 ms, 25491 ms
  - Without this change: 25892 ms, 25639 ms, 25573 ms
* PageCyclerReferenceTest.MozFile
  - With this change: 25706 ms, 25431 ms, 22633 ms
  - Without this change: 25586 ms, 25983 ms, 25967 ms
* PageCyclerExtensionTest.MozFile
  - With this change: 25726 ms, 25319 ms, 25698 ms
  - Without this change: 25292 ms, 25665 ms, 25518 ms

Regards,

Hironori Bono

http://codereview.chromium.org/303033/diff/28001/29007
File chrome/browser/aeropeek_manager.cc (right):

http://codereview.chromium.org/303033/diff/28001/29007#newcode242
chrome/browser/aeropeek_manager.cc:242: class RegisterThumbnailTask : public
Task {
On 2010/02/16 19:15:27, Ben Goodger wrote:
> Can you explain your thread usage? It looks like you run all these tasks on
the
> IO thread, yet they make heavy use of objects that are not threadsafe and live
> on the UI thread, like the delegate_ which touches TabContents, etc etc.

In honest, I also concerned this and needed advices.
I have created these tasks just because it is slow to create a DIB (used by
AeroPeek), resize a tab thumbnail, copy the resized thumbnail to the DIB, and
send the DIB to Windows. Maybe it is better to create tasks just does the above
work from a copy of a tab thumbnail (SkBitmap)? (I'm not sure how long it takes
to create a copy of a SkBitmap, though.)

http://codereview.chromium.org/303033/diff/28001/29007#newcode292
chrome/browser/aeropeek_manager.cc:292: if (!active_)
On 2010/02/16 19:15:27, Ben Goodger wrote:
> to me this would be better as:
> 
> if (active_) {
> ...
> }
> 
> rather than splitting the function with the early return.

Done.

http://codereview.chromium.org/303033/diff/28001/29007#newcode364
chrome/browser/aeropeek_manager.cc:364: // An event to notify this task
finishes.
On 2010/02/16 19:15:27, Ben Goodger wrote:
> to notify WHEN this task finishes.

Done.

http://codereview.chromium.org/303033/diff/28001/29007#newcode495
chrome/browser/aeropeek_manager.cc:495: // This window uses this information not
only for high-lighting the selected
On 2010/02/16 19:15:27, Ben Goodger wrote:
> highlighting

Done.

[Ben Goodger (Google), 2010-02-17 20:39:16]

On Wed, Feb 17, 2010 at 2:46 AM,  <hbono@chromium.org> wrote:
> Thank you for your review and comments.
> To run PageCyclerTests on my Win7 box with our without this change (as of
> r39180), their performances are almost the same. Nevertheless, it is better
> to
> avoid accessing TabContents from an I/O thread. I will send you another
> review
> request when I finish rewriting this change.
>
> * PageCyclerTest.MozFile
>  - With this change: 25433 ms, 25598 ms, 25491 ms
>  - Without this change: 25892 ms, 25639 ms, 25573 ms
> * PageCyclerReferenceTest.MozFile
>  - With this change: 25706 ms, 25431 ms, 22633 ms
>  - Without this change: 25586 ms, 25983 ms, 25967 ms
> * PageCyclerExtensionTest.MozFile
>  - With this change: 25726 ms, 25319 ms, 25698 ms
>  - Without this change: 25292 ms, 25665 ms, 25518 ms

Thanks. Please re-run these when you get done with the other changes
in case they have an impact.

If you can extract the data somehow in a way that doesn't involve
depending on objects that live in the UI thread and do the
manipulation on the other thread that'd be best.

-Ben

[Hironori Bono, 2010-03-01 09:14:31]

Thank you for your comments and sorry for my slow updates.
To avoid accessing TabContents from an I/O thread, I have updated this change to
use SkBitmaps when creating tasks in OnDwmSendIconicThumbnail() and
OnDwmSendIconicLivePreviewBitmap(). Also, to avoid hurting the rendering
performance, I have also updated this change to create bitmaps and icons (used
by AeroPeek) only when we hover the taskbar icon.
As for page_cycler_test results, I ran page_cycler_test on my Win7 box with or
without this change again. Even though it somehow ran faster when we ran
page_cycler_test with this change by 4%, I think it is just an error.

* Without this change (as of r40249)
 - PageCyclerTest.MozFile
   (25233 ms + 25288 ms + 25276 ms) / 3 = 25265.7
 - PageCyclerReferenceTest.MozFile
   (25294 ms + 25466 ms + 25249 ms) / 3 = 25336.3
 - PageCyclerExtensionTest.MozFile
   (25447 ms + 25244 ms + 25337 ms) / 3 = 25342.7
* With this change (as of r40249)
 - PageCyclerTest.MozFile
   (22082 ms + 25298 ms + 25209 ms) / 3 = 24196.3 (-4.233%)
 - PageCyclerReferenceTest.MozFile
   (25260 ms + 22049 ms + 25196 ms) / 3 = 24168.3 (-4.610%)
 - PageCyclerExtensionTest.MozFile
   (25301 ms + 25424 ms + 25399 ms) / 3 = 25374.7 (+0.001%)

Would it be possible to review the updated change and give me your comments?

Regards,

Hironori Bono

[Hironori Bono, 2010-03-08 11:28:56]

I have updated the test results to use r40887.

* Without this change (r40887)
 - PageCyclerTest.MozFile
   (25159 ms + 25226 ms + 22112 ms) / 3 = 24165.67
 - PageCyclerReferenceTest.MozFile
   (25286 ms + 25150 ms + 25200 ms) / 3 = 25212.00
 - PageCyclerExtensionTest.MozFile
   (26452 ms + 25166 ms + 25119 ms) / 3 = 25579.00

* With this change (r40887)
 - PageCyclerTest.MozFile
   (25330 ms + 25442 ms + 25158 ms) / 3 = 25310.00 (+4.74%)
 - PageCyclerReferenceTest.MozFile
   (22111 ms + 25671 ms + 22251 ms) / 3 = 23344.33 (-7.41%)
 - PageCyclerExtensionTest.MozFile
   (25170 ms + 25281 ms + 25120 ms) / 3 = 25190.33 (-1.52%)

Regards,

Hironori Bono

[Ben Goodger (Google), 2010-03-08 22:03:22]

OK LGTM. Sorry for the latency.

http://codereview.chromium.org/303033/diff/44001/44008
File chrome/browser/aeropeek_manager.cc (right):

http://codereview.chromium.org/303033/diff/44001/44008#newcode160
chrome/browser/aeropeek_manager.cc:160: // * DwmSetIconicThumbnail()
Why not just place these comments next to the relevant function?