Implement additional UI changes for dangerous download warnings.

chrome/browser/ui/views/download/download_item_view.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/download/download_item_view.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 73 matching lines @@
 
 DownloadItemView::DownloadItemView(DownloadItem* download,
     DownloadShelfView* parent,
     BaseDownloadItemModel* model)
   : warning_icon_(NULL),
     download_(download),
     parent_(parent),
     status_text_(l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_STARTING)),
     body_state_(NORMAL),
     drop_down_state_(NORMAL),
+    mode_(NORMAL_MODE),
     progress_angle_(download_util::kStartAngleDegrees),
     drop_down_pressed_(false),
     dragging_(false),
     starting_drag_(false),
     model_(model),
     save_button_(NULL),
     discard_button_(NULL),
     dangerous_download_label_(NULL),
     dangerous_download_label_sized_(false),
     disabled_while_opening_(false),
     creation_time_(base::Time::Now()),
-    ALLOW_THIS_IN_INITIALIZER_LIST(reenable_method_factory_(this)) {
+    ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   DCHECK(download_);
   download_->AddObserver(this);
 
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
 
   BodyImageSet normal_body_image_set = {
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_TOP),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_MIDDLE),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_BOTTOM),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_TOP),
@@ skipping 58 matching lines @@
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_BOTTOM),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_TOP),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_MIDDLE),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_BOTTOM),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_TOP_NO_DD),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_MIDDLE_NO_DD),
     rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_BOTTOM_NO_DD)
   };
   dangerous_mode_body_image_set_ = dangerous_mode_body_image_set;
 
+  BodyImageSet malicious_mode_body_image_set = {
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_TOP),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_MIDDLE),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_LEFT_BOTTOM),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_TOP),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_MIDDLE),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_CENTER_BOTTOM),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_TOP),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_MIDDLE),
+    rb.GetBitmapNamed(IDR_DOWNLOAD_BUTTON_RIGHT_BOTTOM)
+  };
+  malicious_mode_body_image_set_ = malicious_mode_body_image_set;

[dmazzoni, 2011/12/01 19:49:22]

Can you just say malicious_mode_body_image_set_ = dangerous_mode_body_image_set?
They seem to be the same. If not, could you add a Clone() or a copy constructor
so this can be easier?

[asanka, 2011/12/02 00:05:04]

Done.  I think you meant normal_body_image_set.  Currently the body background
looks the same in malicious and normal modes.

+
   LoadIcon();
   tooltip_text_ = download_->GetFileNameToReportUser().LossyDisplayName();
 
   font_ = ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont);
   box_height_ = std::max<int>(2 * kVerticalPadding + font_.GetHeight() +
                                   kVerticalTextPadding + font_.GetHeight(),
                               2 * kVerticalPadding +
                                   normal_body_image_set_.top_left->height() +
                                   normal_body_image_set_.bottom_left->height());
 
   if (download_util::kSmallProgressIconSize > box_height_)
     box_y_ = (download_util::kSmallProgressIconSize - box_height_) / 2;
   else
     box_y_ = kVerticalPadding;
 
-  gfx::Size size = GetPreferredSize();
-  if (base::i18n::IsRTL()) {
-    // Drop down button is glued to the left of the download shelf.
-    drop_down_x_left_ = 0;
-    drop_down_x_right_ = normal_drop_down_image_set_.top->width();
-  } else {
-    // Drop down button is glued to the right of the download shelf.
-    drop_down_x_left_ =
-        size.width() - normal_drop_down_image_set_.top->width();
-    drop_down_x_right_ = size.width();
-  }
-
   body_hover_animation_.reset(new ui::SlideAnimation(this));
   drop_hover_animation_.reset(new ui::SlideAnimation(this));
 
+  UpdateDropDownButtonPosition();
+
   if (download->GetSafetyState() == DownloadItem::DANGEROUS)
-    EnterDangerousMode();
+    ShowWarningDialog();
 
   UpdateAccessibleName();
   set_accessibility_focusable(true);
 
   // Set up our animation.
   StartDownloadProgress();
 }
 
 DownloadItemView::~DownloadItemView() {
-  if (context_menu_.get())
-    context_menu_->Stop();

[dmazzoni, 2011/12/01 19:49:22]

Why are you deleting this? Was it wrong or is it no longer needed?

[asanka, 2011/12/02 00:05:04]

It was used as a signal that DownloadItemView (and everything down the
hierarchy) was deleted.  DownloadShelfContextMenu would set its download_item_
member to NULL in response.

In DownloadItemView::ShowContextMenu(), we were calling context_menu_->Run() and
afterwards checking (context_menu_->download_item()) to see if it was safe to
access 'this' and if so change the pushed state of the drop down icon.

I thought this was wrong because:

  - DownloadItemView::context_menu_ is a scoped_ptr<DownloadShelfContextMenu>,
so the DownloadShelfContextMenu is deleted after Stop() is called.

  - The context_menu_->download() check accesses the potentially deleted *this,
and the potentially deleted DownloadShelfContextMenu.

The addition of the "Discard" option to the drop down menu made it so that we
were reliably hitting this case.  We now post a non-nestable task with a WeakPtr
to the DownloadItemView to un-press the drop down button, which simply doesn't
run if the DownloadItemView was deleted as a result of running the drop down
menu.

   icon_consumer_.CancelAllRequests();
   StopDownloadProgress();
   download_->RemoveObserver(this);
 }
 
 // Progress animation handlers.
 
 void DownloadItemView::UpdateDownloadProgress() {
   progress_angle_ = (progress_angle_ +
                      download_util::kUnknownIncrementDegrees) %
@@ skipping 19 matching lines @@
     parent()->SchedulePaint();
 }
 
 // DownloadObserver interface.
 
 // Update the progress graphic on the icon and our text status label
 // to reflect our current bytes downloaded, time remaining.
 void DownloadItemView::OnDownloadUpdated(DownloadItem* download) {
   DCHECK(download == download_);
 
-  if (body_state_ == DANGEROUS &&
+  if (IsShowingWarningDialog() &&
       download->GetSafetyState() == DownloadItem::DANGEROUS_BUT_VALIDATED) {
     // We have been approved.
-    ClearDangerousMode();
-  } else if (body_state_ != DANGEROUS &&
-      download->GetSafetyState() == DownloadItem::DANGEROUS) {
-    EnterDangerousMode();
+    ClearWarningDialog();
+  } else if (!IsShowingWarningDialog() &&
+             download->GetSafetyState() == DownloadItem::DANGEROUS) {
+    ShowWarningDialog();
     // Force the shelf to layout again as our size has changed.
     parent_->Layout();
     SchedulePaint();
   } else {
     string16 status_text = model_->GetStatusText();
     switch (download_->GetState()) {
       case DownloadItem::IN_PROGRESS:
         download_->IsPaused() ?
             StopDownloadProgress() : StartDownloadProgress();
         LoadIconIfItemPathChanged();
@@ skipping 39 matching lines @@
   // for painting.
   parent()->SchedulePaint();
 }
 
 void DownloadItemView::OnDownloadOpened(DownloadItem* download) {
   disabled_while_opening_ = true;
   SetEnabled(false);
   MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&DownloadItemView::Reenable,
-                 reenable_method_factory_.GetWeakPtr()),
+                 weak_ptr_factory_.GetWeakPtr()),
       kDisabledOnOpenDuration);
 
   // Notify our parent.
   parent_->OpenedDownload(this);
 }
 
 // View overrides
 
 // In dangerous mode we have to layout our buttons.
 void DownloadItemView::Layout() {
-  if (IsDangerousMode()) {
+  if (IsShowingWarningDialog()) {
+    BodyImageSet* body_image_set =
+        (mode_ == DANGEROUS_MODE) ? &dangerous_mode_body_image_set_ :
+            &malicious_mode_body_image_set_;
     dangerous_download_label_->SetEnabledColor(
       GetThemeProvider()->GetColor(ThemeService::COLOR_BOOKMARK_TEXT));
 
-    int x = kLeftPadding + dangerous_mode_body_image_set_.top_left->width() +
-      warning_icon_->width() + kLabelPadding;
+    int x = kLeftPadding + body_image_set->top_left->width() +
+        warning_icon_->width() + kLabelPadding;
     int y = (height() - dangerous_download_label_->height()) / 2;
     dangerous_download_label_->SetBounds(x, y,
                                          dangerous_download_label_->width(),
                                          dangerous_download_label_->height());
     gfx::Size button_size = GetButtonSize();
     x += dangerous_download_label_->width() + kLabelPadding;
     y = (height() - button_size.height()) / 2;
-    save_button_->SetBounds(x, y, button_size.width(), button_size.height());
-    x += button_size.width() + kButtonPadding;
+    if (save_button_) {
+      save_button_->SetBounds(x, y, button_size.width(), button_size.height());
+      x += button_size.width() + kButtonPadding;
+    }
     discard_button_->SetBounds(x, y, button_size.width(), button_size.height());
   }
 }
 
 gfx::Size DownloadItemView::GetPreferredSize() {
   int width, height;
 
   // First, we set the height to the height of two rows or text plus margins.
   height = 2 * kVerticalPadding + 2 * font_.GetHeight() + kVerticalTextPadding;
   // Then we increase the size if the progress icon doesn't fit.
   height = std::max<int>(height, download_util::kSmallProgressIconSize);
 
-  if (IsDangerousMode()) {
-    width = kLeftPadding + dangerous_mode_body_image_set_.top_left->width();
+  if (IsShowingWarningDialog()) {
+    BodyImageSet* body_image_set =
+        (mode_ == DANGEROUS_MODE) ? &dangerous_mode_body_image_set_ :
+            &malicious_mode_body_image_set_;
+    width = kLeftPadding + body_image_set->top_left->width();
     width += warning_icon_->width() + kLabelPadding;
     width += dangerous_download_label_->width() + kLabelPadding;
     gfx::Size button_size = GetButtonSize();
     // Make sure the button fits.
     height = std::max<int>(height, 2 * kVerticalPadding + button_size.height());
     // Then we make sure the warning icon fits.
     height = std::max<int>(height, 2 * kVerticalPadding +
                                    warning_icon_->height());
-    width += button_size.width() * 2 + kButtonPadding;
-    width += dangerous_mode_body_image_set_.top_right->width();
+    if (mode_ == DANGEROUS_MODE)

[dmazzoni, 2011/12/01 19:49:22]

How about making this based on whether save_button is showing or not, like:

if (save_button_)
  width += button_size.width() + kButtonPadding;

...since that's the real difference, right?

[asanka, 2011/12/02 00:05:04]

Done.

+      width += button_size.width() + kButtonPadding;
+    width += button_size.width();
+    width += body_image_set->top_right->width();
+    if (mode_ == MALICIOUS_MODE)
+      width += normal_drop_down_image_set_.top->width();
   } else {
     width = kLeftPadding + normal_body_image_set_.top_left->width();
     width += download_util::kSmallProgressIconSize;
     width += kTextWidth;
     width += normal_body_image_set_.top_right->width();
     width += normal_drop_down_image_set_.top->width();
   }
   return gfx::Size(width, height);
 }
 
 // Handle a mouse click and open the context menu if the mouse is
 // over the drop-down region.
 bool DownloadItemView::OnMousePressed(const views::MouseEvent& event) {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (mode_ == DANGEROUS_MODE)
     return true;
 
   // Stop any completion animation.
   if (complete_animation_.get() && complete_animation_->is_animating())
     complete_animation_->End();
 
   if (event.IsOnlyLeftMouseButton()) {
     if (InDropDownButtonXCoordinateRange(event.x())) {
       drop_down_pressed_ = true;
       SetState(NORMAL, PUSHED);
       // We are setting is_mouse_gesture to false when calling ShowContextMenu
       // so that the positioning of the context menu will be similar to a
       // keyboard invocation.  I.e. we want the menu to always be positioned
       // next to the drop down button instead of the next to the pointer.
       ShowContextMenu(event.location(), false);
-    } else {
+    } else if (!IsShowingWarningDialog()) {
       SetState(PUSHED, NORMAL);
     }
   }
   return true;
 }
 
 // Handle drag (file copy) operations.
 bool DownloadItemView::OnMouseDragged(const views::MouseEvent& event) {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (IsShowingWarningDialog())
     return true;
 
   if (!starting_drag_) {
     starting_drag_ = true;
     drag_start_point_ = event.location();
   }
   if (dragging_) {
     if (download_->IsComplete()) {
       IconManager* im = g_browser_process->icon_manager();
       gfx::Image* icon = im->LookupIcon(download_->GetUserVerifiedFilePath(),
                                         IconLoader::SMALL);
       if (icon) {
         views::Widget* widget = GetWidget();
         download_util::DragDownload(download_, icon,
                                     widget ? widget->GetNativeView() : NULL);
       }
     }
   } else if (ExceededDragThreshold(
                  event.location().x() - drag_start_point_.x(),
                  event.location().y() - drag_start_point_.y())) {
     dragging_ = true;
   }
   return true;
 }
 
 void DownloadItemView::OnMouseReleased(const views::MouseEvent& event) {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (mode_ == DANGEROUS_MODE)
     return;
 
   if (event.IsOnlyLeftMouseButton() &&
-      !InDropDownButtonXCoordinateRange(event.x())) {
+      !InDropDownButtonXCoordinateRange(event.x()) &&
+      !IsShowingWarningDialog()) {
     OpenDownload();
   }
 
   SetState(NORMAL, NORMAL);
 }
 
 void DownloadItemView::OnMouseCaptureLost() {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (mode_ == DANGEROUS_MODE)
     return;
 
   if (dragging_) {
     // Starting a drag results in a MouseCaptureLost.
     dragging_ = false;
     starting_drag_ = false;
   } else {
     SetState(NORMAL, NORMAL);
   }
 }
 
 void DownloadItemView::OnMouseMoved(const views::MouseEvent& event) {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (mode_ == DANGEROUS_MODE)
     return;
 
   bool on_body = !InDropDownButtonXCoordinateRange(event.x());
   SetState(on_body ? HOT : NORMAL, on_body ? NORMAL : HOT);
   if (on_body) {
-    body_hover_animation_->Show();
+    if (!IsShowingWarningDialog())

[Randy Smith (Not in Mondays), 2011/12/01 19:50:46]

How can we be in dangerous mode and not showing a warning dialog?

[asanka, 2011/12/02 00:05:04]

We get here when we aren't showing a warning dialog as well.  We exit early only
if we are in DANGEROUS_MODE because no part of the view responds to mouse hover
in that mode.

+      body_hover_animation_->Show();
     drop_hover_animation_->Hide();
   } else {
-    body_hover_animation_->Hide();
+    if (!IsShowingWarningDialog())
+      body_hover_animation_->Hide();
     drop_hover_animation_->Show();
   }
 }
 
 void DownloadItemView::OnMouseExited(const views::MouseEvent& event) {
   // Mouse should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (mode_ == DANGEROUS_MODE)
     return;
 
   SetState(NORMAL, drop_down_pressed_ ? PUSHED : NORMAL);
-  body_hover_animation_->Hide();
+  if (!IsShowingWarningDialog())
+    body_hover_animation_->Hide();
   drop_hover_animation_->Hide();
 }
 
 bool DownloadItemView::OnKeyPressed(const views::KeyEvent& event) {
   // Key press should not activate us in dangerous mode.
-  if (IsDangerousMode())
+  if (IsShowingWarningDialog())
     return true;
 
   if (event.key_code() == ui::VKEY_SPACE ||
       event.key_code() == ui::VKEY_RETURN) {
     OpenDownload();
     return true;
   }
   return false;
 }
 
@@ skipping 23 matching lines @@
       SetMouseHandler(NULL);
 
   // If |is_mouse_gesture| is false, |p| is ignored. The menu is shown aligned
   // to drop down arrow button.
   if (!is_mouse_gesture) {
     drop_down_pressed_ = true;
     SetState(NORMAL, PUSHED);
     point.SetPoint(drop_down_x_left_, box_y_);
     size.SetSize(drop_down_x_right_ - drop_down_x_left_, box_height_);
   }
-
+  // Post a task to release the button.  When we call the Run method on the menu
+  // below, it runs an inner message loop that might cause us to be deleted.
+  // Posting a task with a WeakPtr lets us safely handle the button release.
+  MessageLoop::current()->PostNonNestableTask(
+      FROM_HERE,
+      base::Bind(&DownloadItemView::ReleaseDropDown,
+                 weak_ptr_factory_.GetWeakPtr()));
   views::View::ConvertPointToScreen(this, &point);
 
   if (!context_menu_.get())
     context_menu_.reset(new DownloadShelfContextMenuView(model_.get()));
-  // When we call the Run method on the menu, it runs an inner message loop
-  // that might causes us to be deleted.
-  if (context_menu_->Run(GetWidget()->GetTopLevelWidget(),
-                         gfx::Rect(point, size)))
-    return;  // We have been deleted! Don't access 'this'.
-
-  // If the menu action was to remove the download, this view will also be
-  // invalid so we must not access 'this' in this case.
-  if (context_menu_->download_item()) {
-    drop_down_pressed_ = false;
-    // Showing the menu blocks. Here we revert the state.
-    SetState(NORMAL, NORMAL);
-  }
+  context_menu_->Run(GetWidget()->GetTopLevelWidget(),
+                     gfx::Rect(point, size));
+  // We could be deleted now.
 }
 
 void DownloadItemView::GetAccessibleState(ui::AccessibleViewState* state) {
   state->name = accessible_name_;
   state->role = ui::AccessibilityTypes::ROLE_PUSHBUTTON;
   if (download_->GetSafetyState() == DownloadItem::DANGEROUS) {
     state->state = ui::AccessibilityTypes::STATE_UNAVAILABLE;
   } else {
     state->state = ui::AccessibilityTypes::STATE_HASPOPUP;
   }
 }
 
 void DownloadItemView::ButtonPressed(
     views::Button* sender, const views::Event& event) {
   if (sender == discard_button_) {
     UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download",
                              base::Time::Now() - creation_time_);
     if (download_->IsPartialDownload())
       download_->Cancel(true);
     download_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
     // WARNING: we are deleted at this point.  Don't access 'this'.
-  } else if (sender == save_button_) {
+  } else if (save_button_ && sender == save_button_) {
     // The user has confirmed a dangerous download.  We'd record how quickly the
     // user did this to detect whether we're being clickjacked.
     UMA_HISTOGRAM_LONG_TIMES("clickjacking.save_download",
                              base::Time::Now() - creation_time_);
     // This will change the state and notify us.
     download_->DangerousDownloadValidated();
   }
 }
 
 void DownloadItemView::AnimationProgressed(const ui::Animation* animation) {
   // We don't care if what animation (body button/drop button/complete),
   // is calling back, as they all have to go through the same paint call.
   SchedulePaint();
 }
 
 void DownloadItemView::OnPaint(gfx::Canvas* canvas) {

[dmazzoni, 2011/12/01 19:49:22]

This logic is getting a little more confusing to follow, please add a couple of
very brief comments that help clarify:

* Why do dangerous and malicious have a different body set, and why don't they
distinguish between pushed or not?
* Note that there's a drop-down in nromal and malicious mode, but in dangerous
mode there's no drop-down

[asanka, 2011/12/02 00:05:04]

I added a comment before OnPaint() that gives an overview of what we are going
to paint, and also added a few comments within OnPaint().  Let me know if that
is sufficient.

   BodyImageSet* body_image_set = NULL;
-  switch (body_state_) {
-    case NORMAL:
-    case HOT:
-      body_image_set = &normal_body_image_set_;
+  switch (mode_) {
+    case NORMAL_MODE:
+      if (body_state_ == PUSHED)
+        body_image_set = &pushed_body_image_set_;
+      else                      // NORMAL or HOT
+        body_image_set = &normal_body_image_set_;
       break;
-    case PUSHED:
-      body_image_set = &pushed_body_image_set_;
-      break;
-    case DANGEROUS:
+    case DANGEROUS_MODE:
       body_image_set = &dangerous_mode_body_image_set_;
       break;
-    default:
-      NOTREACHED();
-  }
-  DropDownImageSet* drop_down_image_set = NULL;
-  switch (drop_down_state_) {
-    case NORMAL:
-    case HOT:
-      drop_down_image_set = &normal_drop_down_image_set_;
-      break;
-    case PUSHED:
-      drop_down_image_set = &pushed_drop_down_image_set_;
-      break;
-    case DANGEROUS:
-      drop_down_image_set = NULL;  // No drop-down in dangerous mode.

[dmazzoni, 2011/12/01 19:49:22]

Please preserve this comment somewhere.

[asanka, 2011/12/02 00:05:04]

Done.

+    case MALICIOUS_MODE:
+      body_image_set = &malicious_mode_body_image_set_;
       break;
     default:
       NOTREACHED();
   }
 
+  DropDownImageSet* drop_down_image_set = NULL;
+  if (mode_ == NORMAL_MODE || mode_ == MALICIOUS_MODE) {
+    switch (drop_down_state_) {
+      case NORMAL:
+      case HOT:
+        drop_down_image_set = &normal_drop_down_image_set_;
+        break;
+      case PUSHED:
+        drop_down_image_set = &pushed_drop_down_image_set_;
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
+
   int center_width = width() - kLeftPadding -
                      body_image_set->left->width() -
                      body_image_set->right->width() -
                      (drop_down_image_set ?
                         normal_drop_down_image_set_.center->width() :
                         0);
 
   // May be caused by animation.
   if (center_width <= 0)
     return;
 
   // Draw status before button image to effectively lighten text.
-  if (!IsDangerousMode()) {
+  if (!IsShowingWarningDialog()) {
     if (!status_text_.empty()) {
       int mirrored_x = GetMirroredXWithWidthInView(
           download_util::kSmallProgressIconSize, kTextWidth);
       // Add font_.height() to compensate for title, which is drawn later.
       int y = box_y_ + kVerticalPadding + font_.GetHeight() +
               kVerticalTextPadding;
       SkColor file_name_color = GetThemeProvider()->GetColor(
           ThemeService::COLOR_BOOKMARK_TEXT);
       // If text is light-on-dark, lightening it alone will do nothing.
       // Therefore we mute luminance a wee bit before drawing in this case.
@@ skipping 32 matching lines @@
                body_image_set->top, body_image_set->center,
                body_image_set->bottom,
                x, box_y_, box_height_, center_width);
   x += center_width;
   PaintBitmaps(canvas,
                body_image_set->top_right, body_image_set->right,
                body_image_set->bottom_right,
                x, box_y_, box_height_, body_image_set->top_right->width());
 
   // Overlay our body hot state.
-  if (body_hover_animation_->GetCurrentValue() > 0) {
+  if (!IsShowingWarningDialog() &&
+      body_hover_animation_->GetCurrentValue() > 0) {
     canvas->SaveLayerAlpha(
         static_cast<int>(body_hover_animation_->GetCurrentValue() * 255));
     canvas->GetSkCanvas()->drawARGB(0, 255, 255, 255, SkXfermode::kClear_Mode);
 
     int x = kLeftPadding;
     PaintBitmaps(canvas,
                  hot_body_image_set_.top_left, hot_body_image_set_.left,
                  hot_body_image_set_.bottom_left,
                  x, box_y_, box_height_, hot_body_image_set_.top_left->width());
     x += body_image_set->top_left->width();
@@ skipping 36 matching lines @@
   }
 
   // Restore the canvas to avoid file name etc. text are drawn flipped.
   // Consequently, the x-axis of following canvas->DrawXXX() method should be
   // mirrored so the text and images are down in the right positions.
   canvas->Restore();
 
   // Print the text, left aligned and always print the file extension.
   // Last value of x was the end of the right image, just before the button.
   // Note that in dangerous mode we use a label (as the text is multi-line).
-  if (!IsDangerousMode()) {
+  if (!IsShowingWarningDialog()) {
     string16 filename;
     if (!disabled_while_opening_) {
       filename = ui::ElideFilename(download_->GetFileNameToReportUser(),
                                    font_, kTextWidth);
     } else {
       // First, Calculate the download status opening string width.
       string16 status_string =
           l10n_util::GetStringFUTF16(IDS_DOWNLOAD_STATUS_OPENING, string16());
       int status_string_width = font_.GetStringWidth(status_string);
       // Then, elide the file name.
@@ skipping 18 matching lines @@
                           IsEnabled() ? file_name_color :
                                         kFileNameDisabledColor,
                           mirrored_x, y, kTextWidth, font_.GetHeight());
   }
 
   // Load the icon.
   IconManager* im = g_browser_process->icon_manager();
   gfx::Image* image = im->LookupIcon(download_->GetUserVerifiedFilePath(),
                                      IconLoader::SMALL);
   const SkBitmap* icon = NULL;
-  if (IsDangerousMode())
+  if (IsShowingWarningDialog())
     icon = warning_icon_;
   else if (image)
     icon = *image;
 
   // We count on the fact that the icon manager will cache the icons and if one
   // is available, it will be cached here. We *don't* want to request the icon
   // to be loaded here, since this will also get called if the icon can't be
   // loaded, in which case LookupIcon will always be NULL. The loading will be
   // triggered only when we think the status might change.
   if (icon) {
-    if (!IsDangerousMode()) {
+    if (!IsShowingWarningDialog()) {
       if (download_->IsInProgress()) {
         download_util::PaintDownloadProgress(canvas, this, 0, 0,
                                              progress_angle_,
                                              download_->PercentComplete(),
                                              download_util::SMALL);
       } else if (download_->IsComplete() &&
                  complete_animation_.get() &&
                  complete_animation_->is_animating()) {
         if (download_->IsInterrupted()) {
           download_util::PaintDownloadInterrupted(canvas, this, 0, 0,
               complete_animation_->GetCurrentValue(),
               download_util::SMALL);
         } else {
           download_util::PaintDownloadComplete(canvas, this, 0, 0,
               complete_animation_->GetCurrentValue(),
               download_util::SMALL);
         }
       }
     }
 
     // Draw the icon image.
     int icon_x, icon_y;
 
-    if (IsDangerousMode()) {
+    if (IsShowingWarningDialog()) {
       icon_x = kLeftPadding + body_image_set->top_left->width();
       icon_y = (height() - icon->height()) / 2;
     } else {
       icon_x = download_util::kSmallProgressIconOffset;
       icon_y = download_util::kSmallProgressIconOffset;
     }
     icon_x = GetMirroredXWithWidthInView(icon_x, icon->width());
     if (IsEnabled()) {
       canvas->DrawBitmapInt(*icon, icon_x, icon_y);
     } else {
       // Use an alpha to make the image look disabled.
       SkPaint paint;
       paint.setAlpha(120);
       canvas->DrawBitmapInt(*icon, icon_x, icon_y, paint);
     }
   }
 }
 
 void DownloadItemView::OpenDownload() {
+  DCHECK(!IsShowingWarningDialog());
   // We're interested in how long it takes users to open downloads.  If they
   // open downloads super quickly, we should be concerned about clickjacking.
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.open_download",
                            base::Time::Now() - creation_time_);
   download_->OpenDownload();
   UpdateAccessibleName();
 }
 
 void DownloadItemView::LoadIcon() {
   IconManager* im = g_browser_process->icon_manager();
@@ skipping 30 matching lines @@
                         0, 0, center_bitmap->width(), center_bitmap->height(),
                         x, y, width, middle_height, false);
   y += middle_height;
   // Draw the bottom.
   canvas->DrawBitmapInt(*bottom_bitmap,
                         0, 0, bottom_bitmap->width(), bottom_bitmap->height(),
                         x, y, width, bottom_bitmap->height(), false);
 }
 
 void DownloadItemView::SetState(State body_state, State drop_down_state) {
-  if (body_state_ == body_state && drop_down_state_ == drop_down_state)
+  if ((IsShowingWarningDialog() || body_state_ == body_state) &&

[dmazzoni, 2011/12/01 19:49:22]

I don't understand why IsShowingWarningDialog() is grouped with (body_state_ ==
body_state), can you add a comment? Could you split it into two separate if
statements so the logic is more clear?

[asanka, 2011/12/02 00:05:04]

If we are showing the warning dialog, we don't change the body state. So it's
equivalent to the case where the new and the current body states are the same. 
Reorganized code to make this clearer.

+      drop_down_state_ == drop_down_state)
     return;
+  // We shouldn't be calling SetState if we are in DANGEROUS_MODE.
+  DCHECK(mode_ != DANGEROUS_MODE);
 
-  body_state_ = body_state;
+  // If we are showing a warning dialog, we don't change body state, unless
+  // through ClearWarningDialog().
+  if (!IsShowingWarningDialog()) {
+    body_state_ = body_state;
+  } else {
+    DCHECK(body_state_ == NORMAL);
+  }
   drop_down_state_ = drop_down_state;
   SchedulePaint();
 }
 
-void DownloadItemView::ClearDangerousMode() {
+void DownloadItemView::ClearWarningDialog() {
   DCHECK(download_->GetSafetyState() == DownloadItem::DANGEROUS_BUT_VALIDATED &&
-         body_state_ == DANGEROUS && drop_down_state_ == DANGEROUS);
+         (mode_ == DANGEROUS_MODE || mode_ == MALICIOUS_MODE));
 
+  mode_ = NORMAL_MODE;
   body_state_ = NORMAL;
   drop_down_state_ = NORMAL;
 
-  // Remove the views used by the dangerous mode.
-  RemoveChildView(save_button_);
-  delete save_button_;
-  save_button_ = NULL;
+  // Remove the views used by the warning dialog.
+  if (save_button_) {
+    RemoveChildView(save_button_);
+    delete save_button_;
+    save_button_ = NULL;
+  }
   RemoveChildView(discard_button_);
   delete discard_button_;
   discard_button_ = NULL;
   RemoveChildView(dangerous_download_label_);
   delete dangerous_download_label_;
   dangerous_download_label_ = NULL;
   dangerous_download_label_sized_ = false;
+  cached_button_size_.SetSize(0,0);
 
   // Set the accessible name back to the status and filename instead of the
   // download warning.
   UpdateAccessibleName();
+  UpdateDropDownButtonPosition();
 
   // We need to load the icon now that the download_ has the real path.
   LoadIcon();
   tooltip_text_ = download_->GetFileNameToReportUser().LossyDisplayName();
 
   // Force the shelf to layout again as our size has changed.
   parent_->Layout();
   parent_->SchedulePaint();
 }
 
-void DownloadItemView::EnterDangerousMode() {
-  DCHECK(body_state_ != DANGEROUS && drop_down_state_ != DANGEROUS);
+void DownloadItemView::ShowWarningDialog() {
+  DCHECK(mode_ != DANGEROUS_MODE && mode_ != MALICIOUS_MODE);
+  if (download_->GetDangerType() == DownloadStateInfo::DANGEROUS_URL ||
+      download_->GetDangerType() == DownloadStateInfo::DANGEROUS_CONTENT) {
+    mode_ = MALICIOUS_MODE;
+  } else {
+    DCHECK(download_->GetDangerType() == DownloadStateInfo::DANGEROUS_FILE);
+    mode_ = DANGEROUS_MODE;
+  }
+  body_state_ = NORMAL;
+  drop_down_state_ = NORMAL;
   tooltip_text_.clear();
-  body_state_ = DANGEROUS;
-  drop_down_state_ = DANGEROUS;
-  save_button_ = new views::NativeTextButton(this,
-      l10n_util::GetStringUTF16(
-           ChromeDownloadManagerDelegate::IsExtensionDownload(download_) ?
-              IDS_CONTINUE_EXTENSION_DOWNLOAD : IDS_CONFIRM_DOWNLOAD));
-  save_button_->set_ignore_minimum_size(true);
+  if (mode_ == DANGEROUS_MODE) {
+    save_button_ = new views::NativeTextButton(this,
+        l10n_util::GetStringUTF16(
+            ChromeDownloadManagerDelegate::IsExtensionDownload(download_) ?
+            IDS_CONTINUE_EXTENSION_DOWNLOAD : IDS_CONFIRM_DOWNLOAD));
+    save_button_->set_ignore_minimum_size(true);
+    AddChildView(save_button_);
+  }
   discard_button_ = new views::NativeTextButton(
       this, l10n_util::GetStringUTF16(IDS_DISCARD_DOWNLOAD));
   discard_button_->set_ignore_minimum_size(true);
-  AddChildView(save_button_);
   AddChildView(discard_button_);
 
   // Ensure the file name is not too long.
 
   // Extract the file extension (if any).
   FilePath filename(download_->GetTargetName());
 #if defined(OS_POSIX)
   string16 extension = WideToUTF16(base::SysNativeMBToWide(
       filename.Extension()));
 #else
@@ skipping 11 matching lines @@
 #endif
 
   // Elide giant extensions (this shouldn't currently be hit, but might
   // in future, should we ever notice unsafe giant extensions).
   if (extension.length() > kFileNameMaxLength / 2)
     ui::ElideString(extension, kFileNameMaxLength / 2, &extension);
 
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   // The dangerous download label text and icon are different
   // under different cases.
-  if (download_->GetDangerType() == DownloadStateInfo::DANGEROUS_URL ||
-      download_->GetDangerType() == DownloadStateInfo::DANGEROUS_CONTENT) {
+  if (mode_ == MALICIOUS_MODE) {
     warning_icon_ = rb.GetBitmapNamed(IDR_SAFEBROWSING_WARNING);
   } else {
     DCHECK(download_->GetDangerType() == DownloadStateInfo::DANGEROUS_FILE);
+    // The download file has dangerous file type (e.g.: an executable).
     warning_icon_ = rb.GetBitmapNamed(IDR_WARNING);
   }
   string16 dangerous_label;
   if (download_->GetDangerType() == DownloadStateInfo::DANGEROUS_URL) {
     // Safebrowsing shows the download URL or content leads to malicious file.
     dangerous_label = l10n_util::GetStringUTF16(
         IDS_PROMPT_MALICIOUS_DOWNLOAD_URL);
   } else if (download_->GetDangerType() == DownloadStateInfo::DANGEROUS_FILE &&
              ChromeDownloadManagerDelegate::IsExtensionDownload(download_)) {
     dangerous_label =
@@ skipping 15 matching lines @@
         filename);
   }
 
   dangerous_download_label_ = new views::Label(dangerous_label);
   dangerous_download_label_->SetMultiLine(true);
   dangerous_download_label_->SetHorizontalAlignment(
       views::Label::ALIGN_LEFT);
   dangerous_download_label_->SetAutoColorReadabilityEnabled(false);
   AddChildView(dangerous_download_label_);
   SizeLabelToMinWidth();
+  UpdateDropDownButtonPosition();
 }
 
 gfx::Size DownloadItemView::GetButtonSize() {
-  DCHECK(save_button_ && discard_button_);
+  DCHECK(discard_button_ && (mode_ == MALICIOUS_MODE || save_button_));
   gfx::Size size;
 
   // We cache the size when successfully retrieved, not for performance reasons
   // but because if this DownloadItemView is being animated while the tab is
   // not showing, the native buttons are not parented and their preferred size
   // is 0, messing-up the layout.
   if (cached_button_size_.width() != 0)
     return cached_button_size_;
 
-  size = save_button_->GetMinimumSize();
+  if (save_button_)
+    size = save_button_->GetMinimumSize();
   gfx::Size discard_size = discard_button_->GetMinimumSize();
 
   size.SetSize(std::max(size.width(), discard_size.width()),
                std::max(size.height(), discard_size.height()));
 
   if (size.width() != 0)
     cached_button_size_ = size;
 
   return size;
 }
@@ skipping 62 matching lines @@
 
   dangerous_download_label_->SetBounds(0, 0, size.width(), size.height());
   dangerous_download_label_sized_ = true;
 }
 
 void DownloadItemView::Reenable() {
   disabled_while_opening_ = false;
   SetEnabled(true);  // Triggers a repaint.
 }
 
+void DownloadItemView::ReleaseDropDown() {
+  drop_down_pressed_ = false;
+  SetState(NORMAL, NORMAL);
+}
+
 bool DownloadItemView::InDropDownButtonXCoordinateRange(int x) {
   if (x > drop_down_x_left_ && x < drop_down_x_right_)
     return true;
   return false;
 }
 
 void DownloadItemView::UpdateAccessibleName() {
   string16 new_name;
-  if (download_->GetSafetyState() == DownloadItem::DANGEROUS) {
+  if (IsShowingWarningDialog()) {
     new_name = dangerous_download_label_->GetText();

[dmazzoni, 2011/12/01 19:49:22]

Does this need to be updated for dangerous vs malicious?

[asanka, 2011/12/02 00:05:04]

For both cases, the accessible text is going to be the text that was set in the
dangerous_download_label_.

   } else {
     new_name = status_text_ + char16(' ') +
         download_->GetFileNameToReportUser().LossyDisplayName();
   }
 
   // If the name has changed, notify assistive technology that the name
   // has changed so they can announce it immediately.
   if (new_name != accessible_name_) {
     accessible_name_ = new_name;
     if (GetWidget()) {
       GetWidget()->NotifyAccessibilityEvent(
           this, ui::AccessibilityTypes::EVENT_NAME_CHANGED, true);
     }
   }
 }
+
+void DownloadItemView::UpdateDropDownButtonPosition() {
+  gfx::Size size = GetPreferredSize();
+  if (base::i18n::IsRTL()) {
+    // Drop down button is glued to the left of the download shelf.
+    drop_down_x_left_ = 0;
+    drop_down_x_right_ = normal_drop_down_image_set_.top->width();
+  } else {
+    // Drop down button is glued to the right of the download shelf.
+    drop_down_x_left_ =
+      size.width() - normal_drop_down_image_set_.top->width();
+    drop_down_x_right_ = size.width();
+  }
+}