Allowing multiple SaveItems to have same URLs.

chrome/browser/download/save_page_browsertest.cc

 // Copyright (c) 2012 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 <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 1091 matching lines @@
 
   std::string arr[] = {
       "frames-runtime-changes.htm: 4388232f-8d45-4d2e-9807-721b381be153",
       "subframe1: 21595339-61fc-4854-b6df-0668328ea263",
       "subframe2: adf55719-15e7-45be-9eda-d12fe782a1bd",
       "subframe3: 50e294bf-3a5b-499d-8772-651ead26952f",
       "subframe4: e0ea9289-7467-4d32-ba5c-c604e8d84cb7",
   };
   std::vector<std::string> expected_substrings(std::begin(arr), std::end(arr));
 
-  // TODO(lukasza): crbug.com/106364: Fix complete-html mode as well.
-  if (save_page_type == content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML)
-    return;
+  // TODO(lukasza): crbug.com/106364: Fix complete-html.

[Randy Smith (Not in Mondays), 2015/12/03 20:58:28]

Hmmm.  Is this comment still up to date?  This CL at least partially fixes that
issue; should this comment mention expanding the test?  If so, could you add a
clause gesturing at how the test needs to be expanded?

[Łukasz Anforowicz, 2015/12/04 21:16:45]

The comment is up to date.  The test can be enabled in the CL under review (i.e.
right now the test verifies no more crashes even though we no longer deduplicate
based on URLs), but will need to be expanded in the future (i.e. in the future
mhtml and complete-html should pass the same test verification).

I've added more explanation / context to the comment.

[Randy Smith (Not in Mondays), 2015/12/07 19:57:14]

Willing to change "Fix complete-html" to "Expand complete-html test beyond just
being a crash test"?  It's a nit (and if you don't want to, I'm ok with that)
but I think of it as being important for TODOs to mean something to people other
than the person who wrote the TODO, just in case.  

[Łukasz Anforowicz, 2015/12/07 20:32:31]

Done.

+  if (save_page_type == content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML) {
+    std::string complete_html_arr[] = {
+        "frames-runtime-changes.htm: 4388232f-8d45-4d2e-9807-721b381be153",
+    };
+    expected_substrings = std::vector<std::string>(
+        std::begin(complete_html_arr), std::end(complete_html_arr));
+  }
 
   GURL url(embedded_test_server()->GetURL(
       "a.com", "/save_page/frames-runtime-changes.htm?do_runtime_changes=1"));
 
   TestMultiFramePage(save_page_type, url, 5, expected_substrings);
 }
 
 INSTANTIATE_TEST_CASE_P(
     SaveType,
     SavePageMultiFrameBrowserTest,
     ::testing::Values(content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML,
                       content::SAVE_PAGE_TYPE_AS_MHTML));
 
 }  // namespace