downloads: add the ability to undo download removal.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index f681486cdcbe4ee660ad38d5378f7a584db242c2..2d94513f659d3757d160d8d47239514f1180d5ed 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -379,6 +379,7 @@ void DownloadItemImpl::Resume() {
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
+    case REMOVED_INTERNAL:

[benjhayden, 2014/11/13 18:01:59]

Out of order?

Also, how do you ensure that this isn't reached?

       NOTREACHED();
   }
 }
@@ -433,6 +434,18 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   TransitionTo(CANCELLED_INTERNAL, UPDATE_OBSERVERS);
 }
 
+void DownloadItemImpl::MarkRemoved() {
+  TransitionTo(REMOVED_INTERNAL, UPDATE_OBSERVERS);
+}
+
+void DownloadItemImpl::UndoRemove() {
+  if (state_ != REMOVED_INTERNAL) {

[arv (Not doing code reviews), 2014/11/13 19:02:30]

Why not DCHECK if this stat cannot happen?

+    NOTREACHED();
+    return;
+  }
+  TransitionTo(prev_state_, UPDATE_OBSERVERS);
+}
+
 void DownloadItemImpl::Remove() {
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -531,6 +544,7 @@ bool DownloadItemImpl::IsDone() const {
     case RESUMING_INTERNAL:
       return false;
 
+    case REMOVED_INTERNAL:

[benjhayden, 2014/11/13 18:01:59]

How do you ensure that this isn't reached?

     case MAX_DOWNLOAD_INTERNAL_STATE:
       break;
   }
@@ -1564,7 +1578,7 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
   if (state_ == new_state)
     return;
 
-  DownloadInternalState old_state = state_;
+  prev_state_ = state_;

[benjhayden, 2014/11/13 18:01:59]

I might want to be a bit more conservative here. I'd like to at least discourage
future use of prev_state_ for anything other than UndoRemove(). Maybe rename it
"removed_state_" and only set it when transitioning to REMOVED?

   state_ = new_state;
 
   switch (state_) {
@@ -1585,7 +1599,7 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
                      received_bytes_, &hash_state_));
       break;
     case IN_PROGRESS_INTERNAL:
-      if (old_state == INTERRUPTED_INTERNAL) {
+      if (prev_state_ == INTERRUPTED_INTERNAL) {
         bound_net_log_.AddEvent(
             net::NetLog::TYPE_DOWNLOAD_ITEM_RESUMED,
             base::Bind(&ItemResumingNetLogCallback,
@@ -1603,13 +1617,13 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
   }
 
   VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true)
-    << " " << InternalToExternalState(old_state)
+    << " " << InternalToExternalState(prev_state_)
     << " " << InternalToExternalState(state_);
 
   bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
                   state_ != COMPLETING_INTERNAL);
-  bool was_done = (old_state != IN_PROGRESS_INTERNAL &&
-                   old_state != COMPLETING_INTERNAL);
+  bool was_done = (prev_state_ != IN_PROGRESS_INTERNAL &&
+                   prev_state_ != COMPLETING_INTERNAL);
   // Termination
   if (is_done && !was_done)
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
@@ -1744,6 +1758,8 @@ DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
       return INTERRUPTED;
     case RESUMING_INTERNAL:
       return INTERRUPTED;
+    case REMOVED_INTERNAL:
+      return REMOVED;
     case MAX_DOWNLOAD_INTERNAL_STATE:
       break;
   }
@@ -1764,6 +1780,8 @@ DownloadItemImpl::ExternalToInternalState(
       return CANCELLED_INTERNAL;
     case INTERRUPTED:
       return INTERRUPTED_INTERNAL;
+    case REMOVED:
+      return REMOVED_INTERNAL;
     default:
       NOTREACHED();
   }
@@ -1785,6 +1803,8 @@ const char* DownloadItemImpl::DebugDownloadStateString(
       return "INTERRUPTED";
     case RESUMING_INTERNAL:
       return "RESUMING";
+    case REMOVED_INTERNAL:
+      return "REMOVED";
     case MAX_DOWNLOAD_INTERNAL_STATE:
       break;
   };