Improve large tab strip by leveraging touch icons when present

Improve large tab strip by leveraging touch icons when present.

BUG=none
TEST=TemplateURLParserTest.TestDictionary, TestMSDN, TestWikipedia (as per sadrul)
Note: testing with mobile user-agent will also yield better results: --user-agent=iPhone or similar mobile

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

[Emmanuel Saint-loubert-Bié, 2011-06-03 02:39:01]

Hi Scott,

Before doing a detailed review, what do you think of this approach, based on
work by michaelbai which we discussed?

Thanks,

-- Emmanuel

[sky, 2011-06-03 15:48:30]

The touch icons are significantly larger then favicons 129x129, vs 16x16. At
that size we shouldn't be caching them in the navigation stack, instead folks
that want the richer icons should ask for them based on the current navigation
entry. This is the reason the code is like it.

  -Scott

[Peter Mayo, 2011-06-06 23:10:32]

Please re-enable the test for this, once it is working.
See http://codereview.chromium.org/7112013/

[Emmanuel Saint-loubert-Bié, 2011-06-08 22:41:21]

Hi Scott,

This is not finished, there are a few things I need to address (including the
refresh when getting the larger icomn). Is that in the ballpark of what you had
in mind?

-- Emmanuel

[sky, 2011-06-08 22:48:21]

Yes, this is along the lines of what I was suggesting.

  -Scott

[Emmanuel Saint-loubert-Bié, 2011-06-09 00:00:23]

Hi Scott,

OK I have done some more testing and it seems to be doing what we need. I think
it is ready for a formal review. Thanks!

-- Emmanuel

[sky, 2011-06-09 16:53:00]

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab.cc (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:32: void
calc_touch_icon_target_size(int* width, int* height) {
This should really be static.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:209: ? kFaviconSize :
data().favicon.width());
Shouldn't this be kTouchTargetIconSize always?

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:212: int favicon_x = x;
I don't think 212-214 are needed.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:225: y, kTouchTargetIconSize,
kTouchTargetIconSize, false);
Is the throbber size 32x32 so that it lines up nicely with the  scaled image?

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:241: SkBitmap scaled_image =
skia::ImageOperations::Resize(data().favicon,
Seems painful to resize the image each time through. Can't you resize once and
cache that?

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:243:
canvas->DrawBitmapInt(scaled_image, 0, 0, scaled_image.width(),
If the favicon width is not the same as the height, then won't this result in
stretching? Shouldn't you use the width/height you calculate for where to draw
it?

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:17: TabStripModel*
model) :
nit: ':' on next line, and each parameter on its own line.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:27: int model_index,
TabChangeType change_type) {
nit: each param on its own line. See
http://dev.chromium.org/developers/coding-style (search for 'for function
declarations and definitions').

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:29: if (profile_ !=
NULL && change_type == ALL) {
You care about INVALIDATE_URL or INVALIDATE_TAB.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:32: if
(favicon_service && contents->tab_contents()) {
contents->tab_contents() is always non-null.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:23: virtual void
TabChangedAt(TabContentsWrapper* contents, int model_index,
nit: if you have to wrap, then put each param on its own line and align with the
openning '('.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:26: void
OnTouchIconAvailable(FaviconService::Handle h,
make this private.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:34: 
nit: no newline here.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:36: Profile* profile_;
Remove this and make profile() in BrowserTabStripController protected.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:39: typedef
std::map<int, GURL> ModelIndexMap;
If you cache by model index it means you have to track moves and deletes.
Additionally the Tab already has the url and icon. You should do the following:
. Make SetTabRendererDataFromModel take a boolean indicating if the tab is being
newly inserted.
. In TouchTabStripController if the tab isn't being inserted you know you can
get the TabRendererData from the existing tab to see if you can reuse the
existing bitmap, or if you need to do a new load.

Then you need to figure out how to resolve what tab an icon being returned from
OnTouchIconAvailable corresponds to. Maybe you could add an ever increasing
integer to TouchTab for that.

When doing url comparison you should ignore everything after the fragment.

[Emmanuel Saint-loubert-Bié, 2011-06-10 02:21:45]

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab.cc (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:32: void
calc_touch_icon_target_size(int* width, int* height) {
Yes of course, sorry for this cut and paste error

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:209: ? kFaviconSize :
data().favicon.width());
Yes good catch, and it will matter even more now that we returned kFaviconSize
to 16

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:212: int favicon_x = x;
Indeed, seems like old cruft

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:225: y, kTouchTargetIconSize,
kTouchTargetIconSize, false);
Yes it has been 32x32 for a while. No change needed here.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:231: SkBitmap
crashed_favicon(*rb.GetBitmapNamed(IDR_SAD_FAVICON));
On the other hand the IDR_SAD_FAVICON is not 32x32, so I will get that fixed.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:241: SkBitmap scaled_image =
skia::ImageOperations::Resize(data().favicon,
That is a great point. We did not cache before, but now that we do I will move
that code.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab.cc:243:
canvas->DrawBitmapInt(scaled_image, 0, 0, scaled_image.width(),
On 2011/06/09 16:53:00, sky wrote:
> If the favicon width is not the same as the height, then won't this result in
> stretching? Shouldn't you use the width/height you calculate for where to draw
> it?

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:17: TabStripModel*
model) :
On 2011/06/09 16:53:00, sky wrote:
> nit: ':' on next line, and each parameter on its own line.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:27: int model_index,
TabChangeType change_type) {
On 2011/06/09 16:53:00, sky wrote:
> nit: each param on its own line. See
> http://dev.chromium.org/developers/coding-style (search for 'for function
> declarations and definitions').

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:29: if (profile_ !=
NULL && change_type == ALL) {
On 2011/06/09 16:53:00, sky wrote:
> You care about INVALIDATE_URL or INVALIDATE_TAB.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:32: if
(favicon_service && contents->tab_contents()) {
On 2011/06/09 16:53:00, sky wrote:
> contents->tab_contents() is always non-null.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:23: virtual void
TabChangedAt(TabContentsWrapper* contents, int model_index,
On 2011/06/09 16:53:00, sky wrote:
> nit: if you have to wrap, then put each param on its own line and align with
the
> openning '('.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:26: void
OnTouchIconAvailable(FaviconService::Handle h,
On 2011/06/09 16:53:00, sky wrote:
> make this private.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:34: 
On 2011/06/09 16:53:00, sky wrote:
> nit: no newline here.

Done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:36: Profile* profile_;
Cool and done.

http://codereview.chromium.org/7065052/diff/7009/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:39: typedef
std::map<int, GURL> ModelIndexMap;
You are right of course using the model that way is not ideal. I have changed
the code to a vector of requests with the index of the request passed to the
callback. That way I know for sure which GURL need to receive the bitmap in the 
callback. I keep the model_index around just to be used on the tab refresh. If
we are unlucky then we might not refresh the right tab, but that is probably not
a big deal?

Finally I now use GURL::GetWithEmptyPath() for the comparison this should ignore
the fragment from what I understand, but let me know your thoughts.

[Emmanuel Saint-loubert-Bié, 2011-06-10 02:22:26]

> Finally I now use GURL::GetWithEmptyPath() for the comparison this should
ignore
> the fragment from what I understand, but let me know your thoughts.

Actually that did not work the way I expected, so I went a different way.

[sky, 2011-06-10 15:38:17]

The down side of the approach you've gone with is you never remove icons from
the map. I think you're better served by the approach I outlined last time
around.

  -Scott

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:18: // static
Remove the comment.

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:36: typedef
std::pair<GURL, int> RequestInfo;
typedefs should be first in a section
(http://www.corp.google.com/eng/doc/cppguide.xml#Declaration_Order)

[Emmanuel Saint-loubert-Bié, 2011-06-10 16:09:09]

Hi Scott,

Yes you are right this is a definite downside to the current approach. The
interaction with all the tabs is fairly complex so I have been taking the lazzy
route.

In your last notes you wrote:
> Additionally the Tab already has the url and icon. You should
> do the following:
> . Make SetTabRendererDataFromModel take a boolean indicating
> if the tab is being newly inserted.

I assume you mean changing the base class in BrowserTabStripController then

> . In TouchTabStripController if the tab isn't being inserted you know
> you can get the TabRendererData from the existing tab to see if you
>  can reuse the existing bitmap, or if you need to do a new load.

How persistent is the TabRendererData? I have tried to stay away from data
structures which lifecycle I do not fully comprehend.

Finally, I do realize that you would have written all this code faster than it
would take you to got through all these steps with me, so I am very grateful for
your help.

-- Emmanuel

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:18: // static
On 2011/06/10 15:38:18, sky wrote:
> Remove the comment.

Done.

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/8014/chrome/browser/ui/touch/tabs...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:36: typedef
std::pair<GURL, int> RequestInfo;
sorry I did cut and paste from another similar declaration

[Emmanuel Saint-loubert-Bié, 2011-06-15 23:49:30]

Hi Scott,

This new implementation no longer cache the touch icons as before. Instead those
icons are added to the TouchTab, which is I believe consistent with your
suggestions. 
Hopefully I am getting closer. Thanks for your help!

-- Emmanuel

[sky, 2011-06-16 15:43:41]

Yes, this is getting much closer.

  -Scott

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.cc:218: && data().favicon.height() ==
kFaviconSize) {
&& should be on the previous line.
Also, you should change SetTabRendererDataFromModel to scale the image there.
That way, you only scale once no matter how many times you might end up
painting.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.h:41: const SkBitmap& get_touch_icon() {
For getters like this the name should match the field name, eg:

touch_icon().

Also, make it const.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:19: // static
Only use '// static' for class member definitions.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:36: {
{ on 35.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:58: BaseTab* tab =
tabstrip_->GetBaseTabAtModelIndex(model_index);
You do this cast a couple of times. You should add a GetTouchTabAtModelIndex
method to TouchTabStrip that does this for you.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:60: if (touch_tab &&
!touch_tab->get_touch_icon().isNull()) {
You shouldn't have to check touch_tab. If you feel you want a check, use a
DCHECK.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:80: data, new_tab);
indent to align with ( on 79.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:95: GURL page_url =
gurl_without_fragment(wrapper->tab_contents()->GetURL());
Remove wrapper and change this to:
gurl_wihout_fragment(contents->GetURL());

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:119: TouchTab*
touch_tab = consumer_.GetClientData(favicon_service, h);
117-119 can be replaced with consumer_.GetClientDataForCurrentRequest();

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:127:
favicon.image_data->size(),
nit: align 127,128 with ( on 126.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:144:
browser_->GetTabContentsAt(model_index)->NotifyNavigationStateChanged(
I believe instead of this you want:
touch_tab->SchedulePaint();

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:24: virtual void
TabChangedAt(TabContentsWrapper* contents,
You'll also want to override TabDetachedAt and remove any requests that are
associated with the tab.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:332: true);
align with ( on 331.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:362: false);
nit: align with ( on 361

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:410: false);
nit: align with ( on 409.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:102: virtual void
SetTabRendererDataFromModel(TabContents* contents,
Position of definition should match that of declaration. So, since you moved
this you also need to move the definition in the .cc.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:105: bool new_tab);
Use an enum instead of a boolean.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:109: TabStripModel*
model_;
No protected members. Add protected accessors instead.

http://codereview.chromium.org/7065052/diff/14002/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

http://codereview.chromium.org/7065052/diff/14002/ui/gfx/favicon_size.h#newco...
ui/gfx/favicon_size.h:15: const int kTouchTargetIconSize = 32;
This is only used in the tab strip. Keep it there until we decide other places
want the 32x32 size.

[tfarina, 2011-06-16 15:52:44]

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:20: static void
calc_touch_icon_target_size(int* width, int* height) {
Also wrap these static/private functions into an unnamed namespace, i.e:

namespace {

void CalcTouchIconTargetSize(...) {
}

...

}  // namespace

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:35: static GURL
gurl_without_fragment(const GURL& gurl)
the function name should be CamelCase.

[Emmanuel Saint-loubert-Bié, 2011-06-17 00:01:44]

Hi Scott,

I applied all the review comments. Thanks,

-- Emmanuel

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.cc:218: && data().favicon.height() ==
kFaviconSize) {
On 2011/06/16 15:43:41, sky wrote:
> && should be on the previous line.
> Also, you should change SetTabRendererDataFromModel to scale the image there.
> That way, you only scale once no matter how many times you might end up
> painting.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.h:41: const SkBitmap& get_touch_icon() {
On 2011/06/16 15:43:41, sky wrote:
> For getters like this the name should match the field name, eg:
> 
> touch_icon().
> 
> Also, make it const.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:19: // static
Done. My bad this was a cut and paste from ui/gfx/favicon_size.h

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:20: static void
calc_touch_icon_target_size(int* width, int* height) {
Thanks, also a cut and paste from ui/gfx/favicon_size.h

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:35: static GURL
gurl_without_fragment(const GURL& gurl)
On 2011/06/16 15:52:44, tfarina wrote:
> the function name should be CamelCase.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:36: {
On 2011/06/16 15:43:41, sky wrote:
> { on 35.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:58: BaseTab* tab =
tabstrip_->GetBaseTabAtModelIndex(model_index);
On 2011/06/16 15:43:41, sky wrote:
> You do this cast a couple of times. You should add a GetTouchTabAtModelIndex
> method to TouchTabStrip that does this for you.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:60: if (touch_tab &&
!touch_tab->get_touch_icon().isNull()) {
On 2011/06/16 15:43:41, sky wrote:
> You shouldn't have to check touch_tab. If you feel you want a check, use a
> DCHECK.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:80: data, new_tab);
On 2011/06/16 15:43:41, sky wrote:
> indent to align with ( on 79.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:95: GURL page_url =
gurl_without_fragment(wrapper->tab_contents()->GetURL());
On 2011/06/16 15:43:41, sky wrote:
> Remove wrapper and change this to:
> gurl_wihout_fragment(contents->GetURL());

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:119: TouchTab*
touch_tab = consumer_.GetClientData(favicon_service, h);
On 2011/06/16 15:43:41, sky wrote:
> 117-119 can be replaced with consumer_.GetClientDataForCurrentRequest();

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:127:
favicon.image_data->size(),
On 2011/06/16 15:43:41, sky wrote:
> nit: align 127,128 with ( on 126.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:144:
browser_->GetTabContentsAt(model_index)->NotifyNavigationStateChanged(
Actually that does not cause the refresh, so I left the code as is since it is
working.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:24: virtual void
TabChangedAt(TabContentsWrapper* contents,
On 2011/06/16 15:43:41, sky wrote:
> You'll also want to override TabDetachedAt and remove any requests that are
> associated with the tab.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:332: true);
OK I agree this is probably more readable. Here is what our coding guidelines
say, and I find this a bit confusing:

When calling a function, however, feel free to collapse things to take less
space.  Do whatever seems most readable.
     ThisIsARatherLongFunctionCall(
    first_lengthy_argument, second_argument, yet_another,
    more_arguments_here, okay_thats_enough);

ThisIsAnotherLongFunctionCall(first_lengthy_argument,
                              second_argument, yet_another);

AndAnotherLongFunctionCall(first_lengthy_argument,
    second_argument, yet_another);

AnotherLongFunctionCall(
    first_lengthy_argument,
    second_argument, yet_another);

bool result =
    YetAnotherLongFunctionCall(first_argument, second_argument);

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:362: false);
On 2011/06/16 15:43:41, sky wrote:
> nit: align with ( on 361

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:102: virtual void
SetTabRendererDataFromModel(TabContents* contents,
On 2011/06/16 15:43:41, sky wrote:
> Position of definition should match that of declaration. So, since you moved
> this you also need to move the definition in the .cc.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:105: bool new_tab);
On 2011/06/16 15:43:41, sky wrote:
> Use an enum instead of a boolean.

Done.

http://codereview.chromium.org/7065052/diff/14002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:109: TabStripModel*
model_;
On 2011/06/16 15:43:41, sky wrote:
> No protected members. Add protected accessors instead.

Done.

http://codereview.chromium.org/7065052/diff/14002/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

http://codereview.chromium.org/7065052/diff/14002/ui/gfx/favicon_size.h#newco...
ui/gfx/favicon_size.h:15: const int kTouchTargetIconSize = 32;
On 2011/06/16 15:43:41, sky wrote:
> This is only used in the tab strip. Keep it there until we decide other places
> want the 32x32 size.

Done.

[tfarina, 2011-06-17 00:06:40]

http://codereview.chromium.org/7065052/diff/20002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/20002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:35: GURL
GurlWithoutFragment(const GURL& gurl) {
would you rename this to GetURLWithoutFragment?

http://codereview.chromium.org/7065052/diff/20002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:169: 
remove this blank line.

[sky, 2011-06-17 15:34:55]

The tabstrip changes LGTM. You'll want Brett to review the changes to
cancelablerequest.

  -Scott

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.h (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.h:16: const int kTouchTargetIconSize =
32;
Make this is a static field of TouchTab, just like TouchTab::kViewClassName.

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:143: if (model_index
== -1 || !IsValidIndex(model_index))
IsValidIndex checks for model_index >= 0, so you can remove the model_index ==
-1 conditional.

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:9: #include <map>
Remove this include.

[Emmanuel Saint-loubert-Bié, 2011-06-17 16:11:23]

Hi Brett,

I got an LGTM from Scott foe everything except for
content/browser/cancelable_request.h which we would like you to review.

Thanks,

-- Emmanuel

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab.h (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab.h:16: const int kTouchTargetIconSize =
32;
On 2011/06/17 15:34:55, sky wrote:
> Make this is a static field of TouchTab, just like TouchTab::kViewClassName.

Done.

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:143: if (model_index
== -1 || !IsValidIndex(model_index))
On 2011/06/17 15:34:55, sky wrote:
> IsValidIndex checks for model_index >= 0, so you can remove the model_index ==
> -1 conditional.

Done.

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/23003/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:9: #include <map>
On 2011/06/17 15:34:55, sky wrote:
> Remove this include.

Done.

[brettw, 2011-06-17 16:20:49]

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:45: } // namespace
Two spaces before end-of-line comments.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:72: // reset touch
icon if the url are different
Comments should have a capital and a period.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:105:
data->favicon.height() == kFaviconSize)
Need {} for this

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:167: // Refresh UI
since favicon changed
Period.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:16: // Subclassing
BrowserTabStripController to provide touch specific behavior.
This comment seems like it just says the same thing the actual declaration does.
Can you write something a little more useful?

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:17: class
TouchTabStripController: public BrowserTabStripController {
Space before colon.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:41: // Our consumer
for touch icon requests
Make sure your comments end with periods.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:415: for (int i =
0; i < tabstrip_->tab_count(); ++i) {
No {} here.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:250: // Cancels all requests outstanding
matching the client data
Needs period.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:347: bool
CancelableRequestConsumerTSimple<T>::HasPendingRequestsForClientData(
Do we really need this function? In your single call site that uses it, it would
be better to remove. You're doing a brute-force search to find if there are any
requests, and then another brute-force search to remove them all. Why not just
skip the first one?

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:351: if (i->second == client_data) {
No {} for single-line conditionals.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:383: i != copied_requests.end(); ++i)
You should use {} for this for loop since it contains multiple lines
(Chrome-team style).

[Emmanuel Saint-loubert-Bié, 2011-06-17 16:52:44]

Applied all comments.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:45: } // namespace
On 2011/06/17 16:20:49, brettw wrote:
> Two spaces before end-of-line comments.

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:72: // reset touch
icon if the url are different
On 2011/06/17 16:20:49, brettw wrote:
> Comments should have a capital and a period.

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.cc:105:
data->favicon.height() == kFaviconSize)
On 2011/06/17 16:20:49, brettw wrote:
> Need {} for this

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
File chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:16: // Subclassing
BrowserTabStripController to provide touch specific behavior.
On 2011/06/17 16:20:49, brettw wrote:
> This comment seems like it just says the same thing the actual declaration
does.
> Can you write something a little more useful?

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:17: class
TouchTabStripController: public BrowserTabStripController {
On 2011/06/17 16:20:49, brettw wrote:
> Space before colon.

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/touch/tab...
chrome/browser/ui/touch/tabs/touch_tab_strip_controller.h:41: // Our consumer
for touch icon requests
On 2011/06/17 16:20:49, brettw wrote:
> Make sure your comments end with periods.

Done.

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/7065052/diff/28002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:415: for (int i =
0; i < tabstrip_->tab_count(); ++i) {
I did not write that code. I just moved it because I noticed it was in the wrong
place in the file, but ok I will fix :-)

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:250: // Cancels all requests outstanding
matching the client data
On 2011/06/17 16:20:49, brettw wrote:
> Needs period.

Done.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:347: bool
CancelableRequestConsumerTSimple<T>::HasPendingRequestsForClientData(
On 2011/06/17 16:20:49, brettw wrote:
> Do we really need this function? In your single call site that uses it, it
would
> be better to remove. You're doing a brute-force search to find if there are
any
> requests, and then another brute-force search to remove them all. Why not just
> skip the first one?

Done.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:351: if (i->second == client_data) {
On 2011/06/17 16:20:49, brettw wrote:
> No {} for single-line conditionals.

Done.

http://codereview.chromium.org/7065052/diff/28002/content/browser/cancelable_...
content/browser/cancelable_request.h:383: i != copied_requests.end(); ++i)
OK but this is a cut-and-paste from above :-) Which I will fix.