Refactor dangerous download warning text generation into DownloadItemModel.

Refactor dangerous download warning text generation into DownloadItemModel.

Previously, we had multiple copies of the logic for generating warning text for a dangerous or malicious download. Move all of these into DownloadModel. As an additional benefit, we no longer require UI code to interpret DownloadItem::GetDangerType().

BUG=116551
TEST=Start a dangerous, malicious, and extension download. Check whether the warning text is correct, and any embedded filename is elided if necessary.


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

[asanka, 2012-03-01 16:22:20]

Randy: Can you comment on the use of DownloadItemModel? I'll add other reviewers
for UI specific code afterwards.

[Randy Smith (Not in Mondays), 2012-03-01 21:34:45]

I really like this; in general, I'd like to see as much UI logic as possible
folded into non-GUI specific classes.  I'm not a good person to judge if the
interface matches the GUI use case well; I'll leave that to other folks.  But
from my perspective, with the extra interface specification requested below,
LGTM.

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:41: virtual string16
GetWarningText(const gfx::Font& font, int base_width) = 0;
When is this valid to call, in terms of IsDangerous() and IsMalicious() values? 
If it's valid when they are not true, what does it return?

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:58: class DownloadItemModel :
public BaseDownloadItemModel {
Not really part of this CL, but remind me of the inheritance model here?  This
looks as if the impl of BaseDownloadItemModel for views is being declared in the
non-GUI type specific class, which seems wrong.

[asanka, 2012-03-01 21:55:33]

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:41: virtual string16
GetWarningText(const gfx::Font& font, int base_width) = 0;
On 2012/03/01 21:34:45, rdsmith wrote:
> When is this valid to call, in terms of IsDangerous() and IsMalicious()
values? 
> If it's valid when they are not true, what does it return?

It's only valid to call when IsDangerous() is true. I added a comment to this
effect.

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:58: class DownloadItemModel :
public BaseDownloadItemModel {
On 2012/03/01 21:34:45, rdsmith wrote:
> Not really part of this CL, but remind me of the inheritance model here?  This
> looks as if the impl of BaseDownloadItemModel for views is being declared in
the
> non-GUI type specific class, which seems wrong.

BaseDownloadItemModel and DownloadItemModel are both independent of UI toolkit.
Historically, we used to have a SavePageModel that also implemented
BaseDownloadItemModel. BaseDownloadItemModel was used so that the UI didn't need
to know whether it was talking to a DownloadItem or a SavePackage.

SavePageModel is now no more. Perhaps we should get rid of BaseDownloadItemModel
interface and just use DownloadItemModel.

[Randy Smith (Not in Mondays), 2012-03-01 22:02:32]

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:58: class DownloadItemModel :
public BaseDownloadItemModel {
On 2012/03/01 21:55:33, asanka wrote:
> On 2012/03/01 21:34:45, rdsmith wrote:
> > Not really part of this CL, but remind me of the inheritance model here? 
This
> > looks as if the impl of BaseDownloadItemModel for views is being declared in
> the
> > non-GUI type specific class, which seems wrong.
> 
> BaseDownloadItemModel and DownloadItemModel are both independent of UI
toolkit.
> Historically, we used to have a SavePageModel that also implemented
> BaseDownloadItemModel. BaseDownloadItemModel was used so that the UI didn't
need
> to know whether it was talking to a DownloadItem or a SavePackage.
> 
> SavePageModel is now no more. Perhaps we should get rid of
BaseDownloadItemModel
> interface and just use DownloadItemModel.

That would make sense to me; at a minimum we should clarify the above comment. 
No need for this CL, though.

[asanka, 2012-03-02 17:45:16]

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9569011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:58: class DownloadItemModel :
public BaseDownloadItemModel {
On 2012/03/01 22:02:32, rdsmith wrote:
> On 2012/03/01 21:55:33, asanka wrote:
> > On 2012/03/01 21:34:45, rdsmith wrote:
> > > Not really part of this CL, but remind me of the inheritance model here? 
> This
> > > looks as if the impl of BaseDownloadItemModel for views is being declared
in
> > the
> > > non-GUI type specific class, which seems wrong.
> > 
> > BaseDownloadItemModel and DownloadItemModel are both independent of UI
> toolkit.
> > Historically, we used to have a SavePageModel that also implemented
> > BaseDownloadItemModel. BaseDownloadItemModel was used so that the UI didn't
> need
> > to know whether it was talking to a DownloadItem or a SavePackage.
> > 
> > SavePageModel is now no more. Perhaps we should get rid of
> BaseDownloadItemModel
> > interface and just use DownloadItemModel.
> 
> That would make sense to me; at a minimum we should clarify the above comment.

> No need for this CL, though.

Done.

[asanka, 2012-03-02 17:57:05]

For review. The overall goal is to reduce code duplication and relieve the UI
code from having to interpret the danger types.

dmazzoni: for .../views/
estade: for .../gtk/
thakis: for .../cocoa/

[Nico, 2012-03-02 18:11:39]

Nice!

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (left):

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
chrome/browser/ui/cocoa/download/download_item_controller.mm:220: &rootname);
It looks like this previously didn't have to create a gfx::Font() (which needs
to compute font metrics, which can be slow).

Is there a way to prevent that? It looks like gtk and mac both used
ElideString() and only win used the variant with a font.

[asanka, 2012-03-02 19:14:10]

Thanks for looking!

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (left):

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
chrome/browser/ui/cocoa/download/download_item_controller.mm:220: &rootname);
On 2012/03/02 18:11:39, Nico wrote:
> It looks like this previously didn't have to create a gfx::Font() (which needs
> to compute font metrics, which can be slow).
> 
> Is there a way to prevent that? It looks like gtk and mac both used
> ElideString() and only win used the variant with a font.

Looking at PlatformFontMac, I now see how this can be a problem on Mac.

We could persist the font. Or we could instead use the ElideString() based logic
in DownloadItemModel.

Regarding ElideString() vs. ElideFilename(): I opted to use ElideFilename()
because it already dealt with eliding the filenames and seemed to do a better
job of not clobbering UTF-16 surrogate pairs.

[dmazzoni, 2012-03-02 20:33:59]

lgtm for .../views/

[Evan Stade, 2012-03-03 00:00:29]

cool, lgtm

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:236:
UTF16ToUTF8(download_model->GetWarningConfirmButtonText()).c_str());
why is it this one is the parameter whereas on L196 you use the member var?

[asanka, 2012-03-05 17:49:25]

Thanks for the reviews!

thakis: Can you take another look?

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (left):

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/cocoa/down...
chrome/browser/ui/cocoa/download/download_item_controller.mm:220: &rootname);
Added a persistent font_ member that is used for the warning text as well as the
tooltip.

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9569011/diff/7001/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:236:
UTF16ToUTF8(download_model->GetWarningConfirmButtonText()).c_str());
On 2012/03/03 00:00:29, Evan Stade wrote:
> why is it this one is the parameter whereas on L196 you use the member var?

Oops. Fixed.

[Nico, 2012-03-05 22:16:37]

LGTM if it still looks fine on mac

[asanka, 2012-03-05 22:51:44]

[+sky for chrome/browser/ui/OWNERS]

Thanks! I re-ran tests for Mac.

[sky, 2012-03-06 00:37:59]

LGTM

[commit-bot: I haz the power, 2012-03-06 00:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/9569011/24001

[commit-bot: I haz the power, 2012-03-06 02:49:14]

Change committed as 125082