Refactor the code for loading icons into the PageInfoModel.

Refactor the code for loading icons into the PageInfoModel.

This code is basically duplicated twice on each platform. This consolidates all
that into PageInfoModel::GetIconImage() and renames the SectionInfo.state to icon_id.

BUG=none
TEST=Page info windows and bubbles work as they did before across all platforms.

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

[Robert Sesek, 2010-10-01 16:29:25]

+finnur for Window and the model
+mattm for Gtk
+tvl for Mac

[TVL, 2010-10-01 17:17:16]

http://codereview.chromium.org/3560004/diff/12001/13006
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3560004/diff/12001/13006#newcode21
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:21: void
AddSection(SectionStateIcon state,
change the var name too?

http://codereview.chromium.org/3560004/diff/12001/13009
File chrome/browser/cocoa/page_info_window_mac_unittest.mm (right):

http://codereview.chromium.org/3560004/diff/12001/13009#newcode22
chrome/browser/cocoa/page_info_window_mac_unittest.mm:22: void
AddSection(SectionStateIcon state,
change the var?

http://codereview.chromium.org/3560004/diff/12001/13012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/12001/13012#newcode44
chrome/browser/page_info_model.cc:44:
icons_.push_back(GetBitmapNamed(IDR_PAGEINFO_INFO));
doesn't this mean we always load/decode the images that we might not need?

http://codereview.chromium.org/3560004/diff/12001/13013
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3560004/diff/12001/13013#newcode17
chrome/browser/page_info_model.h:17: class NSImage;
#ifdef __OBJC__
@class NSImage;
#else
class NSImage;
#endif

[Finnur, 2010-10-01 17:46:02]

I like where this is headed...

A few comments.

http://codereview.chromium.org/3560004/diff/12001/13012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/12001/13012#newcode292
chrome/browser/page_info_model.cc:292: DCHECK(static_cast<size_t>(icon_id) <
icons_.size());
Adding DCHECKs to prevent a crash on the next line just adds cruft to the debug
build. It is generally speaking not useful.

http://codereview.chromium.org/3560004/diff/12001/13012#newcode329
chrome/browser/page_info_model.cc:329: ICON_STATE_INFO,
Were these two changes requested by Ian/Jeff or someone?

http://codereview.chromium.org/3560004/diff/12001/13013
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3560004/diff/12001/13013#newcode49
chrome/browser/page_info_model.h:49: #endif
ech... Does this need to be within this class? Can it be on top with the rest of
the if def cruft?

http://codereview.chromium.org/3560004/diff/12001/13013#newcode121
chrome/browser/page_info_model.h:121: // Performs the call to the ResourceBundle
to get a native resource of the
nit: Wording is a bit weird... "Performs the call"

http://codereview.chromium.org/3560004/diff/12001/13013#newcode129
chrome/browser/page_info_model.h:129: // Vector of all possible icons.
That part is obvious. Add to the comment what icons these are. :)

http://codereview.chromium.org/3560004/diff/12001/13015
File chrome/browser/views/page_info_bubble_view.h (right):

http://codereview.chromium.org/3560004/diff/12001/13015#newcode51
chrome/browser/views/page_info_bubble_view.h:51: PageInfoModel* model() { return
&model_; }
Add a TODO for "Remove this once we throw away page_info_window_view" or
something.

http://codereview.chromium.org/3560004/diff/12001/13016
File chrome/browser/views/page_info_window_view.cc (left):

http://codereview.chromium.org/3560004/diff/12001/13016#oldcode350
chrome/browser/views/page_info_window_view.cc:350: }
Have you verified all the icons appear correctly in both the info bubble and the
old-style dialog? Specifically, make sure you still get the orange exclamation
point on http sites for the old dialog...

[Robert Sesek, 2010-10-01 20:37:27]

Thanks for the feedback. Addressed all comments.

http://codereview.chromium.org/3560004/diff/12001/13006
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3560004/diff/12001/13006#newcode21
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:21: void
AddSection(SectionStateIcon state,
On 2010/10/01 17:17:16, TVL wrote:
> change the var name too?

Done.

http://codereview.chromium.org/3560004/diff/12001/13009
File chrome/browser/cocoa/page_info_window_mac_unittest.mm (right):

http://codereview.chromium.org/3560004/diff/12001/13009#newcode22
chrome/browser/cocoa/page_info_window_mac_unittest.mm:22: void
AddSection(SectionStateIcon state,
On 2010/10/01 17:17:16, TVL wrote:
> change the var?

Done.

http://codereview.chromium.org/3560004/diff/12001/13012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/12001/13012#newcode44
chrome/browser/page_info_model.cc:44:
icons_.push_back(GetBitmapNamed(IDR_PAGEINFO_INFO));
On 2010/10/01 17:17:16, TVL wrote:
> doesn't this mean we always load/decode the images that we might not need?

Yes, but I think that's a minimal cost.

http://codereview.chromium.org/3560004/diff/12001/13012#newcode292
chrome/browser/page_info_model.cc:292: DCHECK(static_cast<size_t>(icon_id) <
icons_.size());
On 2010/10/01 17:46:02, Finnur wrote:
> Adding DCHECKs to prevent a crash on the next line just adds cruft to the
debug
> build. It is generally speaking not useful.

Done.

http://codereview.chromium.org/3560004/diff/12001/13012#newcode329
chrome/browser/page_info_model.cc:329: ICON_STATE_INFO,
On 2010/10/01 17:46:02, Finnur wrote:
> Were these two changes requested by Ian/Jeff or someone?

If you look at the code from the individual platforms impls of the icon loader,
this was the actual effect.

http://codereview.chromium.org/3560004/diff/12001/13013
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3560004/diff/12001/13013#newcode17
chrome/browser/page_info_model.h:17: class NSImage;
On 2010/10/01 17:17:16, TVL wrote:
> #ifdef __OBJC__
> @class NSImage;
> #else
> class NSImage;
> #endif

Done.

http://codereview.chromium.org/3560004/diff/12001/13013#newcode49
chrome/browser/page_info_model.h:49: #endif
On 2010/10/01 17:46:02, Finnur wrote:
> ech... Does this need to be within this class? Can it be on top with the rest
of
> the if def cruft? 

I didn't want to introduce a global type, so I put it in the class. I intend to
actually move this out to be a gfx::NativeImage type and add a
ResourceBundle::GetNativeImage(), but that's outside of the scope of this CL.

http://codereview.chromium.org/3560004/diff/12001/13013#newcode121
chrome/browser/page_info_model.h:121: // Performs the call to the ResourceBundle
to get a native resource of the
On 2010/10/01 17:46:02, Finnur wrote:
> nit: Wording is a bit weird... "Performs the call" 

Done.

http://codereview.chromium.org/3560004/diff/12001/13013#newcode129
chrome/browser/page_info_model.h:129: // Vector of all possible icons.
On 2010/10/01 17:46:02, Finnur wrote:
> That part is obvious. Add to the comment what icons these are. :)

Done.

http://codereview.chromium.org/3560004/diff/12001/13015
File chrome/browser/views/page_info_bubble_view.h (right):

http://codereview.chromium.org/3560004/diff/12001/13015#newcode51
chrome/browser/views/page_info_bubble_view.h:51: PageInfoModel* model() { return
&model_; }
On 2010/10/01 17:46:02, Finnur wrote:
> Add a TODO for "Remove this once we throw away page_info_window_view" or
> something.

Removed and just pass the icon to the Section ctor.

http://codereview.chromium.org/3560004/diff/12001/13016
File chrome/browser/views/page_info_window_view.cc (left):

http://codereview.chromium.org/3560004/diff/12001/13016#oldcode350
chrome/browser/views/page_info_window_view.cc:350: }
On 2010/10/01 17:46:02, Finnur wrote:
> Have you verified all the icons appear correctly in both the info bubble and
the
> old-style dialog? Specifically, make sure you still get the orange exclamation
> point on http sites for the old dialog... 

I hadn't, but I've corrected it now. I only tested on Mac, because that's all I
have access to (not yet a Googler).

[mattm, 2010-10-01 20:47:11]

gtk bits LGTM

[TVL, 2010-10-01 20:50:58]

lgtm

http://codereview.chromium.org/3560004/diff/12001/13012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/12001/13012#newcode44
chrome/browser/page_info_model.cc:44:
icons_.push_back(GetBitmapNamed(IDR_PAGEINFO_INFO));
On 2010/10/01 20:37:27, rsesek wrote:
> On 2010/10/01 17:17:16, TVL wrote:
> > doesn't this mean we always load/decode the images that we might not need?
> 
> Yes, but I think that's a minimal cost.

Lazy load in the code where you fetch them?

[Finnur, 2010-10-01 21:07:17]

LGTM with nits

http://codereview.chromium.org/3560004/diff/12001/13013
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3560004/diff/12001/13013#newcode49
chrome/browser/page_info_model.h:49: #endif
Can you add a TODO then?

On 2010/10/01 20:37:27, rsesek wrote:
> On 2010/10/01 17:46:02, Finnur wrote:
> > ech... Does this need to be within this class? Can it be on top with the
rest
> of
> > the if def cruft? 
> 
> I didn't want to introduce a global type, so I put it in the class. I intend
to
> actually move this out to be a gfx::NativeImage type and add a
> ResourceBundle::GetNativeImage(), but that's outside of the scope of this CL.

http://codereview.chromium.org/3560004/diff/12001/13016
File chrome/browser/views/page_info_window_view.cc (left):

http://codereview.chromium.org/3560004/diff/12001/13016#oldcode350
chrome/browser/views/page_info_window_view.cc:350: }
Well, since this is now common to all platforms, testing one platform should be
enough. Good to know you've gone through the scenarios though (even if only on a
Mac). :)

On 2010/10/01 20:37:27, rsesek wrote:
> On 2010/10/01 17:46:02, Finnur wrote:
> > Have you verified all the icons appear correctly in both the info bubble and
> the
> > old-style dialog? Specifically, make sure you still get the orange
exclamation
> > point on http sites for the old dialog... 
> 
> I hadn't, but I've corrected it now. I only tested on Mac, because that's all
I
> have access to (not yet a Googler).

http://codereview.chromium.org/3560004/diff/9002/22012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/9002/22012#newcode290
chrome/browser/page_info_model.cc:290: icons_[ICON_STATE_WARNING_MAJOR];
This return clause is really confusing... 

Why not:
if (!command_line->HasSwitch(switches::kEnableNewPageInfoBubble) &&
    icon_id != ICON_STATE_OK)) {
  return icons_[ICON_STATE_WARNING_MAJOR];
}

http://codereview.chromium.org/3560004/diff/9002/22012#newcode331
chrome/browser/page_info_model.cc:331: (as_bubble) ? ICON_STATE_INFO :
ICON_STATE_OK,
nit: why (as_bubble) as opposed to as_bubble?

[Robert Sesek, 2010-10-01 21:50:13]

http://codereview.chromium.org/3560004/diff/12001/13012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/12001/13012#newcode44
chrome/browser/page_info_model.cc:44:
icons_.push_back(GetBitmapNamed(IDR_PAGEINFO_INFO));
On 2010/10/01 20:50:58, TVL wrote:
> On 2010/10/01 20:37:27, rsesek wrote:
> > On 2010/10/01 17:17:16, TVL wrote:
> > > doesn't this mean we always load/decode the images that we might not need?
> > 
> > Yes, but I think that's a minimal cost.
> 
> Lazy load in the code where you fetch them?

Lazily loading significantly increases the complexity of this code. If you think
the cost/benefit is worth it, I'll do it, though.

http://codereview.chromium.org/3560004/diff/12001/13013
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3560004/diff/12001/13013#newcode49
chrome/browser/page_info_model.h:49: #endif
On 2010/10/01 21:07:17, Finnur wrote:
> Can you add a TODO then?

Done.

http://codereview.chromium.org/3560004/diff/9002/22012
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3560004/diff/9002/22012#newcode290
chrome/browser/page_info_model.cc:290: icons_[ICON_STATE_WARNING_MAJOR];
On 2010/10/01 21:07:17, Finnur wrote:
> This return clause is really confusing... 
> 
> Why not:
> if (!command_line->HasSwitch(switches::kEnableNewPageInfoBubble) &&
>     icon_id != ICON_STATE_OK)) {
>   return icons_[ICON_STATE_WARNING_MAJOR];
> }

Nice.

http://codereview.chromium.org/3560004/diff/9002/22012#newcode331
chrome/browser/page_info_model.cc:331: (as_bubble) ? ICON_STATE_INFO :
ICON_STATE_OK,
On 2010/10/01 21:07:17, Finnur wrote:
> nit: why (as_bubble) as opposed to as_bubble?

Personal style habit. Done.