Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
102683004:
Add mime type information to the download database (Closed)
|
Created:
7 years ago by Tiger Modified:
6 years, 6 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, benjhayden+dwatch_chromium.org, browser-components-watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd mime type information to the download database
BUG=328382
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review fixes #Patch Set 3 : Review fixes (missed one) #
Total comments: 8
Patch Set 4 : Readded space before namespace comment #Patch Set 5 : Fix MigrateDownloadsReasonPathsAndDangerType migration #Patch Set 6 : Move kSchema and kUrlChainSchema to InitDownloadTable #
Total comments: 3
Patch Set 7 : #Patch Set 8 : Review fixes #
Total comments: 1
Patch Set 9 : Review fixes #
Total comments: 3
Patch Set 10 : Review fixes #
Total comments: 1
Messages
Total messages: 37 (0 generated)
jochen@chromium.org: Please review changes in ben@chromium.org: Please review changes in sky@chromium.org: Please review changes in darin@chromium.org: Please review changes in
you'll want rdsmith@ or somebody else from downloads to review the entire CL
On 2013/12/16 15:50:56, jochen wrote: > you'll want rdsmith@ or somebody else from downloads to review the entire CL Thanks, updated review. Added rdsmith and asanka.
On Mon, Dec 16, 2013 at 7:48 AM, <tiger@opera.com> wrote: > Reviewers: jochen, Ben Goodger (Google), sky, darin, > > Message: > jochen@chromium.org: Please review changes in > > ben@chromium.org: Please review changes in > > sky@chromium.org: Please review changes in > > darin@chromium.org: Please review changes in Don't keep us hanging! -Scott > > > > Description: > Add mime type information to the download database > > BUG=328382 > > Please review this at https://codereview.chromium.org/102683004/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+111, -9 lines): > M chrome/browser/download/download_history.cc > M chrome/browser/download/download_history_unittest.cc > M chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc > M chrome/browser/history/download_database.cc > M chrome/browser/history/download_row.h > M chrome/browser/history/download_row.cc > M chrome/browser/history/history_unittest.cc > M chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc > M content/browser/download/download_item_factory.h > M content/browser/download/download_item_impl.h > M content/browser/download/download_item_impl.cc > M content/browser/download/download_manager_impl.h > M content/browser/download/download_manager_impl.cc > M content/browser/download/download_manager_impl_unittest.cc > M content/public/browser/download_manager.h > M content/public/test/mock_download_manager.h > M content/public/test/mock_download_manager.cc > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I really hope you don't regret opening this particular can of worms :-J. Any chance to the database requires bumping the database version number, including upgrade code that converts from one version to the next, and including test code for that upgrade code. See for example chrome/browser/history/download_database.cc:DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType(), the code that calls that from history_database.cc (migration from DB v23 to v24), and the code that tests that in history_unittest.cc. The CL that put that change in was https://codereview.chromium.org/11363222/, and I still occasionally have nightmares about it. On the bright side I believe you can ignore the chrome/test/data/profiles/* section of that CL, as that's all been junked in favor of creating the profiles and then restarting chrome all within a test (so we aren't trying to keep binary copies of profiles up to date with the DB version). Let me know if you have any questions. https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... chrome/browser/download/download_history.cc:137: // id, db_handle, which don't change. Question for Asanka: Might mime_type/original_mime_type change on download resumption?
On 2013/12/16 21:23:18, rdsmith wrote: > I really hope you don't regret opening this particular can of worms :-J. Any > chance to the database requires bumping the database version number, including Sorry, "change". > upgrade code that converts from one version to the next, and including test code > for that upgrade code. See for example > chrome/browser/history/download_database.cc:DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType(), > the code that calls that from history_database.cc (migration from DB v23 to > v24), and the code that tests that in history_unittest.cc. > > The CL that put that change in was https://codereview.chromium.org/11363222/, > and I still occasionally have nightmares about it. On the bright side I believe > you can ignore the chrome/test/data/profiles/* section of that CL, as that's all > been junked in favor of creating the profiles and then restarting chrome all > within a test (so we aren't trying to keep binary copies of profiles up to date > with the DB version). > > Let me know if you have any questions. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > File chrome/browser/download/download_history.cc (right): > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > chrome/browser/download/download_history.cc:137: // id, db_handle, which don't > change. > Question for Asanka: Might mime_type/original_mime_type change on download > resumption?
I'll also second Randy's question regarding whether we need a DB version bump. IMHO it's good to have a deterministic mapping between DB version and DB schema. https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... chrome/browser/download/download_history.cc:137: // id, db_handle, which don't change. On 2013/12/16 21:23:19, rdsmith wrote: > Question for Asanka: Might mime_type/original_mime_type change on download > resumption? Nothing prevents a subsequent response from changing the MIME type. But, I think it's okay to go with the first MIME type seen the URL. https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... chrome/browser/download/download_history_unittest.cc:6: #include <string> nit: not necessary. Neither is <set>, it seems. https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... chrome/browser/download/download_history_unittest.cc:371: const std::string mime_type, Nit: const std::string&. Here and elsewhere. https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... chrome/browser/history/download_database.cc:46: "mime_type VARCHAR(255) NOT NULL," // Mime type. Why VARCHAR(255) ? I don't think length constraints have any effect in SQLite. https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... chrome/browser/history/download_database.cc:294: " DEFAULT ''") && Nit: \"\" for consistency. Also as Randy asked, this may need to be moved into a migration function. https://codereview.chromium.org/102683004/diff/1/content/browser/download/dow... File content/browser/download/download_manager_impl_unittest.cc (right): https://codereview.chromium.org/102683004/diff/1/content/browser/download/dow... content/browser/download/download_manager_impl_unittest.cc:249: const net::BoundNetLog& bound_net_log) OVERRIDE; 19 arguments! I'll make a note to pull this into a structure. Hopefully we can consolidate that with DownloadRow and CreateDownloadItemAdapter. Not for this CL. https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... File content/public/test/mock_download_manager.cc (right): https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... content/public/test/mock_download_manager.cc:7: #include <string> nit: not necessary in the .cc file since the declaration already depends on std::string being a complete type. https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... File content/public/test/mock_download_manager.h (right): https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... content/public/test/mock_download_manager.h:54: const std::string &mime_type, Nit: "const std::string& mime_type". I.e. Keep & with the type, not the identifier. Here and elsewhere.
On 2013/12/16 21:51:48, asanka wrote: > I'll also second Randy's question regarding whether we need a DB version bump. > IMHO it's good to have a deterministic mapping between DB version and DB schema. Just to be clear, I feel pretty strongly about this. We've played a bit fast and loose in the past, and been in pain because of it. If we're changing the schema, we need a version bump with the usual trimmings. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > File chrome/browser/download/download_history.cc (right): > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > chrome/browser/download/download_history.cc:137: // id, db_handle, which don't > change. > On 2013/12/16 21:23:19, rdsmith wrote: > > Question for Asanka: Might mime_type/original_mime_type change on download > > resumption? > > Nothing prevents a subsequent response from changing the MIME type. But, I think > it's okay to go with the first MIME type seen the URL. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > File chrome/browser/download/download_history_unittest.cc (right): > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > chrome/browser/download/download_history_unittest.cc:6: #include <string> > nit: not necessary. Neither is <set>, it seems. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/download/down... > chrome/browser/download/download_history_unittest.cc:371: const std::string > mime_type, > Nit: const std::string&. Here and elsewhere. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... > File chrome/browser/history/download_database.cc (right): > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... > chrome/browser/history/download_database.cc:46: "mime_type VARCHAR(255) NOT > NULL," // Mime type. > Why VARCHAR(255) ? I don't think length constraints have any effect in SQLite. > > https://codereview.chromium.org/102683004/diff/1/chrome/browser/history/downl... > chrome/browser/history/download_database.cc:294: " DEFAULT ''") && > Nit: \"\" for consistency. Also as Randy asked, this may need to be moved into a > migration function. > > https://codereview.chromium.org/102683004/diff/1/content/browser/download/dow... > File content/browser/download/download_manager_impl_unittest.cc (right): > > https://codereview.chromium.org/102683004/diff/1/content/browser/download/dow... > content/browser/download/download_manager_impl_unittest.cc:249: const > net::BoundNetLog& bound_net_log) OVERRIDE; > 19 arguments! I'll make a note to pull this into a structure. Hopefully we can > consolidate that with DownloadRow and CreateDownloadItemAdapter. Not for this > CL. > > https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... > File content/public/test/mock_download_manager.cc (right): > > https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... > content/public/test/mock_download_manager.cc:7: #include <string> > nit: not necessary in the .cc file since the declaration already depends on > std::string being a complete type. > > https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... > File content/public/test/mock_download_manager.h (right): > > https://codereview.chromium.org/102683004/diff/1/content/public/test/mock_dow... > content/public/test/mock_download_manager.h:54: const std::string &mime_type, > Nit: "const std::string& mime_type". I.e. Keep & with the type, not the > identifier. Here and elsewhere.
I've bumped the db version, added an upgrade test, moved the database upgrade to a separate function, and fixed most review issues here. I'm leaving the field type as VARCHAR(255) for now since it is at least an indicator of the maximum length of this particular field. It is true that this just gets converted to TEXT in SQLite, but I am also seeing both VARCHAR and LONGVARCHAR as field types here. I don't feel strongly about this so let me know again if you want this as just TEXT or VARCHAR.
From my perspective this is looking good modulo the issues below. I think sky@ should look at least at chrome/browser/history, and I'd also like his opinion on the issue I bring up in download_database.cc below. Sky? https://codereview.chromium.org/102683004/diff/40001/chrome/browser/download/... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/102683004/diff/40001/chrome/browser/download/... chrome/browser/download/download_history_unittest.cc:476: } // anonymous namespace very nit: I'm not completely clear why, but I believe two spaces is the coding style for terminal namespace comments. I can't point you to a definitive statement on the subject, but see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces "Terminate namespaces with comments as shown in the given examples." all of which have two spaces. https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:252: " original_mime_type ) " This brings up a philosophical question, which may point out a bug in the existing (i.e. my) code. I had conceptualized each of these migrations as strictly moving the DB scheme from one version number to the next, *not* moving the DB to the most recent scheme. But the use of the "Execute(kSchema)" above means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move from version N to version N+1, but instead from version N to most recent (but possibly with aspects of the DB not being updated). But doing it the other way would require this file to keep records of all previous schemas. This code won't cause any problems because there's nothing in MigrateMimeType() that depends on whether or not the mime_type and the original_mime_type were already in the DB. But I'm not sure whether it's the right pattern, and if it's not we should fix it now. Scott? https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:301: nit: Why the extra line? https://codereview.chromium.org/102683004/diff/40001/chrome/test/data/History... File chrome/test/data/History/history.28.sql (right): https://codereview.chromium.org/102683004/diff/40001/chrome/test/data/History... chrome/test/data/History/history.28.sql:26: nit: No need for ending blank line.
Could you update the DownloadTest.DownloadTest_History test in chrome/browser/download/download_browsertest.cc to also check for persisted MIME types? This is so that we have at least one end-to-end test that makes sure the Content-Type from the server makes it to the history DB. Bonus points if you add a test file that gets sniffed as a different MIME type than what the Content-Type header states. That way you can test for persistence of both mime_type and original_mime_type. :) https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:47: "original_mime_type VARCHAR(255) NOT NULL," // Original mime type. Can you just use VARCHAR? I understand that CHAR would be enough, but use VARCHAR for consistency. Let's not introduce another variation here. Also where does this 255 character limit come from? Where is it enforced? I hope no code depends on this being a hard limit. https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:222: " DEFAULT \"\""); Same here. https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:546: "start_time, " Also micro nit: One extra space in the first column. :-)
https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:252: " original_mime_type ) " On 2013/12/17 18:48:58, rdsmith wrote: > This brings up a philosophical question, which may point out a bug in the > existing (i.e. my) code. I had conceptualized each of these migrations as > strictly moving the DB scheme from one version number to the next, *not* moving > the DB to the most recent scheme. But the use of the "Execute(kSchema)" above > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move from > version N to version N+1, but instead from version N to most recent (but > possibly with aspects of the DB not being updated). But doing it the other way > would require this file to keep records of all previous schemas. > > This code won't cause any problems because there's nothing in MigrateMimeType() > that depends on whether or not the mime_type and the original_mime_type were > already in the DB. But I'm not sure whether it's the right pattern, and if it's > not we should fix it now. > > Scott? You are correct. Migrations needs to move from version N to N+1. Otherwise the schema for future migrations is not well known and likely to fail in bad ways. 240 looks extremely error prone here.
On 2013/12/17 19:22:46, sky wrote: > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > File chrome/browser/history/download_database.cc (right): > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > chrome/browser/history/download_database.cc:252: " original_mime_type ) " > On 2013/12/17 18:48:58, rdsmith wrote: > > This brings up a philosophical question, which may point out a bug in the > > existing (i.e. my) code. I had conceptualized each of these migrations as > > strictly moving the DB scheme from one version number to the next, *not* > moving > > the DB to the most recent scheme. But the use of the "Execute(kSchema)" above > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move from > > version N to version N+1, but instead from version N to most recent (but > > possibly with aspects of the DB not being updated). But doing it the other > way > > would require this file to keep records of all previous schemas. > > > > This code won't cause any problems because there's nothing in > MigrateMimeType() > > that depends on whether or not the mime_type and the original_mime_type were > > already in the DB. But I'm not sure whether it's the right pattern, and if > it's > > not we should fix it now. > > > > Scott? > > You are correct. Migrations needs to move from version N to N+1. Otherwise the > schema for future migrations is not well known and likely to fail in bad ways. > 240 looks extremely error prone here. @tiger: Are you willing to fix this? If it gets complicated, I'm happy to do it, but I think it's simple enough that I don't feel too guilty asking you to fix my mistake in your CL :-}. I think it's as simple as making a copy of the kSchema before your change, then rename it kReasonPathDangerSchema (or something like that--I don't think it should have the version number in it as that's managed in history_database.cc), and use that in MigrateDownloadsReasonPathsAndDangerType()?
On 2013/12/17 20:55:05, rdsmith wrote: > On 2013/12/17 19:22:46, sky wrote: > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > File chrome/browser/history/download_database.cc (right): > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > chrome/browser/history/download_database.cc:252: " original_mime_type ) " > > On 2013/12/17 18:48:58, rdsmith wrote: > > > This brings up a philosophical question, which may point out a bug in the > > > existing (i.e. my) code. I had conceptualized each of these migrations as > > > strictly moving the DB scheme from one version number to the next, *not* > > moving > > > the DB to the most recent scheme. But the use of the "Execute(kSchema)" > above > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move from > > > version N to version N+1, but instead from version N to most recent (but > > > possibly with aspects of the DB not being updated). But doing it the other > > way > > > would require this file to keep records of all previous schemas. > > > > > > This code won't cause any problems because there's nothing in > > MigrateMimeType() > > > that depends on whether or not the mime_type and the original_mime_type were > > > already in the DB. But I'm not sure whether it's the right pattern, and if > > it's > > > not we should fix it now. > > > > > > Scott? > > > > You are correct. Migrations needs to move from version N to N+1. Otherwise the > > schema for future migrations is not well known and likely to fail in bad ways. > > 240 looks extremely error prone here. > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy to do > it, but I think it's simple enough that I don't feel too guilty asking you to > fix my mistake in your CL :-}. I think it's as simple as making a copy of the > kSchema before your change, then rename it kReasonPathDangerSchema (or something > like that--I don't think it should have the version number in it as that's > managed in history_database.cc), and use that in > MigrateDownloadsReasonPathsAndDangerType()? I can fix it, no problem. I thought this was a bit odd. Note that this was a problem even before my review in: MigrateDownloadedByExtension when bumping to version 27 and MigrateDownloadValidators when bumping to version 28. So to do this correctly the kReasonPathDangerSchema should reflect how it looks in version 23 and not in version 28 (which is the current one, before my patch).
On 2013/12/17 21:20:19, Tiger wrote: > On 2013/12/17 20:55:05, rdsmith wrote: > > On 2013/12/17 19:22:46, sky wrote: > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > File chrome/browser/history/download_database.cc (right): > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > chrome/browser/history/download_database.cc:252: " original_mime_type ) " > > > On 2013/12/17 18:48:58, rdsmith wrote: > > > > This brings up a philosophical question, which may point out a bug in the > > > > existing (i.e. my) code. I had conceptualized each of these migrations as > > > > strictly moving the DB scheme from one version number to the next, *not* > > > moving > > > > the DB to the most recent scheme. But the use of the "Execute(kSchema)" > > above > > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move > from > > > > version N to version N+1, but instead from version N to most recent (but > > > > possibly with aspects of the DB not being updated). But doing it the > other > > > way > > > > would require this file to keep records of all previous schemas. > > > > > > > > This code won't cause any problems because there's nothing in > > > MigrateMimeType() > > > > that depends on whether or not the mime_type and the original_mime_type > were > > > > already in the DB. But I'm not sure whether it's the right pattern, and > if > > > it's > > > > not we should fix it now. > > > > > > > > Scott? > > > > > > You are correct. Migrations needs to move from version N to N+1. Otherwise > the > > > schema for future migrations is not well known and likely to fail in bad > ways. > > > 240 looks extremely error prone here. > > > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy to do > > it, but I think it's simple enough that I don't feel too guilty asking you to > > fix my mistake in your CL :-}. I think it's as simple as making a copy of the > > kSchema before your change, then rename it kReasonPathDangerSchema (or > something > > like that--I don't think it should have the version number in it as that's > > managed in history_database.cc), and use that in > > MigrateDownloadsReasonPathsAndDangerType()? > > I can fix it, no problem. I thought this was a bit odd. > > Note that this was a problem even before my review in: > MigrateDownloadedByExtension when bumping to version 27 and > MigrateDownloadValidators when bumping to version 28. So to do this correctly > the kReasonPathDangerSchema should reflect how it looks in version 23 and not in > version 28 (which is the current one, before my patch). Sorry, it should be version 24 of course.
There we go. How is this looking?
On 2013/12/17 21:20:19, Tiger wrote: > On 2013/12/17 20:55:05, rdsmith wrote: > > On 2013/12/17 19:22:46, sky wrote: > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > File chrome/browser/history/download_database.cc (right): > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > chrome/browser/history/download_database.cc:252: " original_mime_type ) " > > > On 2013/12/17 18:48:58, rdsmith wrote: > > > > This brings up a philosophical question, which may point out a bug in the > > > > existing (i.e. my) code. I had conceptualized each of these migrations as > > > > strictly moving the DB scheme from one version number to the next, *not* > > > moving > > > > the DB to the most recent scheme. But the use of the "Execute(kSchema)" > > above > > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move > from > > > > version N to version N+1, but instead from version N to most recent (but > > > > possibly with aspects of the DB not being updated). But doing it the > other > > > way > > > > would require this file to keep records of all previous schemas. > > > > > > > > This code won't cause any problems because there's nothing in > > > MigrateMimeType() > > > > that depends on whether or not the mime_type and the original_mime_type > were > > > > already in the DB. But I'm not sure whether it's the right pattern, and > if > > > it's > > > > not we should fix it now. > > > > > > > > Scott? > > > > > > You are correct. Migrations needs to move from version N to N+1. Otherwise > the > > > schema for future migrations is not well known and likely to fail in bad > ways. > > > 240 looks extremely error prone here. > > > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy to do > > it, but I think it's simple enough that I don't feel too guilty asking you to > > fix my mistake in your CL :-}. I think it's as simple as making a copy of the > > kSchema before your change, then rename it kReasonPathDangerSchema (or > something > > like that--I don't think it should have the version number in it as that's > > managed in history_database.cc), and use that in > > MigrateDownloadsReasonPathsAndDangerType()? > > I can fix it, no problem. I thought this was a bit odd. Thanks! > Note that this was a problem even before my review in: > MigrateDownloadedByExtension when bumping to version 27 and > MigrateDownloadValidators when bumping to version 28. So to do this correctly > the kReasonPathDangerSchema should reflect how it looks in version 23 and not in > version 28 (which is the current one, before my patch). I don't follow (and as a result, I still think kReasonPathDangerSchema should match v28 not v23). The only places that use kSchema are MigrateDownloadReasonPathsAndDangetType (wrong) and InitDownloadTable() (used for creating a DB directly at the current version when we have nothing; not a migration function (*)). The other migration functions each will result in an N->N+1 transition. They don't *require* N, but they also don't jump up to the last version. So I think I don't understand the point you're making? (*) Actually it does have migration logic, which is what sourced my "we've not done the version number upgrade in the past and had pain for it :-J". But pay no attention to the man behind the curtain.
On 2013/12/18 00:47:05, rdsmith wrote: > On 2013/12/17 21:20:19, Tiger wrote: > > On 2013/12/17 20:55:05, rdsmith wrote: > > > On 2013/12/17 19:22:46, sky wrote: > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > File chrome/browser/history/download_database.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > chrome/browser/history/download_database.cc:252: " original_mime_type ) " > > > > On 2013/12/17 18:48:58, rdsmith wrote: > > > > > This brings up a philosophical question, which may point out a bug in > the > > > > > existing (i.e. my) code. I had conceptualized each of these migrations > as > > > > > strictly moving the DB scheme from one version number to the next, *not* > > > > moving > > > > > the DB to the most recent scheme. But the use of the "Execute(kSchema)" > > > above > > > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move > > from > > > > > version N to version N+1, but instead from version N to most recent (but > > > > > possibly with aspects of the DB not being updated). But doing it the > > other > > > > way > > > > > would require this file to keep records of all previous schemas. > > > > > > > > > > This code won't cause any problems because there's nothing in > > > > MigrateMimeType() > > > > > that depends on whether or not the mime_type and the original_mime_type > > were > > > > > already in the DB. But I'm not sure whether it's the right pattern, and > > if > > > > it's > > > > > not we should fix it now. > > > > > > > > > > Scott? > > > > > > > > You are correct. Migrations needs to move from version N to N+1. Otherwise > > the > > > > schema for future migrations is not well known and likely to fail in bad > > ways. > > > > 240 looks extremely error prone here. > > > > > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy to > do > > > it, but I think it's simple enough that I don't feel too guilty asking you > to > > > fix my mistake in your CL :-}. I think it's as simple as making a copy of > the > > > kSchema before your change, then rename it kReasonPathDangerSchema (or > > something > > > like that--I don't think it should have the version number in it as that's > > > managed in history_database.cc), and use that in > > > MigrateDownloadsReasonPathsAndDangerType()? > > > > I can fix it, no problem. I thought this was a bit odd. > > Thanks! > > > Note that this was a problem even before my review in: > > MigrateDownloadedByExtension when bumping to version 27 and > > MigrateDownloadValidators when bumping to version 28. So to do this correctly > > the kReasonPathDangerSchema should reflect how it looks in version 23 and not > in > > version 28 (which is the current one, before my patch). > > I don't follow (and as a result, I still think kReasonPathDangerSchema should > match v28 not v23). The only places that use kSchema are > MigrateDownloadReasonPathsAndDangetType (wrong) and InitDownloadTable() (used > for creating a DB directly at the current version when we have nothing; not a > migration function (*)). The other migration functions each will result in an > N->N+1 transition. They don't *require* N, but they also don't jump up to the > last version. So I think I don't understand the point you're making? > > > (*) Actually it does have migration logic, which is what sourced my "we've not > done the version number upgrade in the past and had pain for it :-J". But pay > no attention to the man behind the curtain. Since the MigrateDownloadReasonPathsAndDangetType function are meant to upgrade the database from version 23 to version 24 it should actually create a version 24 table at this point if we are going strictly by the N -> N+1 transitions. Since then three commits have modified the MigrateDownloadReasonPathsAndDangetType function to actually do larger upgrade steps than N -> N+1. These commits are: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... So, we could of course stop this just at my commit here and use version 28, and otherwise keep MigrateDownloadReasonPathsAndDangetType as it was (doing upgrading from 23 -> 28) or we could go back to the original version of MigrateDownloadReasonPathsAndDangetType, before the three commits listed above was merged and let it only do upgrades 23 -> 24. I suggested the later solution here to keep the strictness of N -> N+1 transitions.
On 2013/12/18 08:33:53, Tiger wrote: > On 2013/12/18 00:47:05, rdsmith wrote: > > On 2013/12/17 21:20:19, Tiger wrote: > > > On 2013/12/17 20:55:05, rdsmith wrote: > > > > On 2013/12/17 19:22:46, sky wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > > File chrome/browser/history/download_database.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > > chrome/browser/history/download_database.cc:252: " original_mime_type ) > " > > > > > On 2013/12/17 18:48:58, rdsmith wrote: > > > > > > This brings up a philosophical question, which may point out a bug in > > the > > > > > > existing (i.e. my) code. I had conceptualized each of these > migrations > > as > > > > > > strictly moving the DB scheme from one version number to the next, > *not* > > > > > moving > > > > > > the DB to the most recent scheme. But the use of the > "Execute(kSchema)" > > > > above > > > > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just > move > > > from > > > > > > version N to version N+1, but instead from version N to most recent > (but > > > > > > possibly with aspects of the DB not being updated). But doing it the > > > other > > > > > way > > > > > > would require this file to keep records of all previous schemas. > > > > > > > > > > > > This code won't cause any problems because there's nothing in > > > > > MigrateMimeType() > > > > > > that depends on whether or not the mime_type and the > original_mime_type > > > were > > > > > > already in the DB. But I'm not sure whether it's the right pattern, > and > > > if > > > > > it's > > > > > > not we should fix it now. > > > > > > > > > > > > Scott? > > > > > > > > > > You are correct. Migrations needs to move from version N to N+1. > Otherwise > > > the > > > > > schema for future migrations is not well known and likely to fail in bad > > > ways. > > > > > 240 looks extremely error prone here. > > > > > > > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy to > > do > > > > it, but I think it's simple enough that I don't feel too guilty asking you > > to > > > > fix my mistake in your CL :-}. I think it's as simple as making a copy of > > the > > > > kSchema before your change, then rename it kReasonPathDangerSchema (or > > > something > > > > like that--I don't think it should have the version number in it as that's > > > > managed in history_database.cc), and use that in > > > > MigrateDownloadsReasonPathsAndDangerType()? > > > > > > I can fix it, no problem. I thought this was a bit odd. > > > > Thanks! > > > > > Note that this was a problem even before my review in: > > > MigrateDownloadedByExtension when bumping to version 27 and > > > MigrateDownloadValidators when bumping to version 28. So to do this > correctly > > > the kReasonPathDangerSchema should reflect how it looks in version 23 and > not > > in > > > version 28 (which is the current one, before my patch). > > > > I don't follow (and as a result, I still think kReasonPathDangerSchema should > > match v28 not v23). The only places that use kSchema are > > MigrateDownloadReasonPathsAndDangetType (wrong) and InitDownloadTable() (used > > for creating a DB directly at the current version when we have nothing; not a > > migration function (*)). The other migration functions each will result in an > > N->N+1 transition. They don't *require* N, but they also don't jump up to the > > last version. So I think I don't understand the point you're making? > > > > > > (*) Actually it does have migration logic, which is what sourced my "we've not > > done the version number upgrade in the past and had pain for it :-J". But pay > > no attention to the man behind the curtain. > > Since the MigrateDownloadReasonPathsAndDangetType function are meant to upgrade > the database from version 23 to version 24 it should actually create a version > 24 table at this point if we are going strictly by the N -> N+1 transitions. > Since then three commits have modified the > MigrateDownloadReasonPathsAndDangetType function to actually do larger upgrade > steps than N -> N+1. These commits are: > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > > So, we could of course stop this just at my commit here and use version 28, and > otherwise keep MigrateDownloadReasonPathsAndDangetType as it was (doing > upgrading from 23 -> 28) or we could go back to the original version of > MigrateDownloadReasonPathsAndDangetType, before the three commits listed above > was merged and let it only do upgrades 23 -> 24. I suggested the later solution > here to keep the strictness of N -> N+1 transitions. Ah! Gotcha. Sorry I had missed that. Yes, I agree, but I'm nervous that there will be potential gotchas in the upgrade path that I don't see in going back and fixing things correctly. So I'd like Scott's opinion again, just in case he sees anything I don't. Scott?
On 2013/12/18 19:55:08, rdsmith wrote: > On 2013/12/18 08:33:53, Tiger wrote: > > On 2013/12/18 00:47:05, rdsmith wrote: > > > On 2013/12/17 21:20:19, Tiger wrote: > > > > On 2013/12/17 20:55:05, rdsmith wrote: > > > > > On 2013/12/17 19:22:46, sky wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > > > File chrome/browser/history/download_database.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/102683004/diff/40001/chrome/browser/history/d... > > > > > > chrome/browser/history/download_database.cc:252: " original_mime_type > ) > > " > > > > > > On 2013/12/17 18:48:58, rdsmith wrote: > > > > > > > This brings up a philosophical question, which may point out a bug > in > > > the > > > > > > > existing (i.e. my) code. I had conceptualized each of these > > migrations > > > as > > > > > > > strictly moving the DB scheme from one version number to the next, > > *not* > > > > > > moving > > > > > > > the DB to the most recent scheme. But the use of the > > "Execute(kSchema)" > > > > > above > > > > > > > means that MigrateDownloadsReasonPathsAndDangerType() doesn't just > > move > > > > from > > > > > > > version N to version N+1, but instead from version N to most recent > > (but > > > > > > > possibly with aspects of the DB not being updated). But doing it > the > > > > other > > > > > > way > > > > > > > would require this file to keep records of all previous schemas. > > > > > > > > > > > > > > This code won't cause any problems because there's nothing in > > > > > > MigrateMimeType() > > > > > > > that depends on whether or not the mime_type and the > > original_mime_type > > > > were > > > > > > > already in the DB. But I'm not sure whether it's the right pattern, > > and > > > > if > > > > > > it's > > > > > > > not we should fix it now. > > > > > > > > > > > > > > Scott? > > > > > > > > > > > > You are correct. Migrations needs to move from version N to N+1. > > Otherwise > > > > the > > > > > > schema for future migrations is not well known and likely to fail in > bad > > > > ways. > > > > > > 240 looks extremely error prone here. > > > > > > > > > > @tiger: Are you willing to fix this? If it gets complicated, I'm happy > to > > > do > > > > > it, but I think it's simple enough that I don't feel too guilty asking > you > > > to > > > > > fix my mistake in your CL :-}. I think it's as simple as making a copy > of > > > the > > > > > kSchema before your change, then rename it kReasonPathDangerSchema (or > > > > something > > > > > like that--I don't think it should have the version number in it as > that's > > > > > managed in history_database.cc), and use that in > > > > > MigrateDownloadsReasonPathsAndDangerType()? > > > > > > > > I can fix it, no problem. I thought this was a bit odd. > > > > > > Thanks! > > > > > > > Note that this was a problem even before my review in: > > > > MigrateDownloadedByExtension when bumping to version 27 and > > > > MigrateDownloadValidators when bumping to version 28. So to do this > > correctly > > > > the kReasonPathDangerSchema should reflect how it looks in version 23 and > > not > > > in > > > > version 28 (which is the current one, before my patch). > > > > > > I don't follow (and as a result, I still think kReasonPathDangerSchema > should > > > match v28 not v23). The only places that use kSchema are > > > MigrateDownloadReasonPathsAndDangetType (wrong) and InitDownloadTable() > (used > > > for creating a DB directly at the current version when we have nothing; not > a > > > migration function (*)). The other migration functions each will result in > an > > > N->N+1 transition. They don't *require* N, but they also don't jump up to > the > > > last version. So I think I don't understand the point you're making? > > > > > > > > > (*) Actually it does have migration logic, which is what sourced my "we've > not > > > done the version number upgrade in the past and had pain for it :-J". But > pay > > > no attention to the man behind the curtain. > > > > Since the MigrateDownloadReasonPathsAndDangetType function are meant to > upgrade > > the database from version 23 to version 24 it should actually create a version > > 24 table at this point if we are going strictly by the N -> N+1 transitions. > > Since then three commits have modified the > > MigrateDownloadReasonPathsAndDangetType function to actually do larger upgrade > > steps than N -> N+1. These commits are: > > > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/downlo... > > > > So, we could of course stop this just at my commit here and use version 28, > and > > otherwise keep MigrateDownloadReasonPathsAndDangetType as it was (doing > > upgrading from 23 -> 28) or we could go back to the original version of > > MigrateDownloadReasonPathsAndDangetType, before the three commits listed above > > was merged and let it only do upgrades 23 -> 24. I suggested the later > solution > > here to keep the strictness of N -> N+1 transitions. > > Ah! Gotcha. Sorry I had missed that. Yes, I agree, but I'm nervous that there > will be potential gotchas in the upgrade path that I don't see in going back and > fixing things correctly. So I'd like Scott's opinion again, just in case he > sees anything I don't. Scott? I like keeping the N->N+1 strictness. Especially since in this case it doesn't look like using a newer schema with an old migration would cause problems. But we should nuke kSchema, it's just too easy to want to use it in migration code and run into potential future problems. Instead move the schema to the only place that should use it, InitDownloadTable.
> I like keeping the N->N+1 strictness. Especially since in this case it doesn't > look like using a newer schema with an old migration would cause problems. > > But we should nuke kSchema, it's just too easy to want to use it in migration > code and run into potential future problems. Instead move the schema to the only > place that should use it, InitDownloadTable. I agree with this. It doesn't look like fixing this will affect the old migration. I've moved kSchema and the kUrlChainSchema to the InitDownloadTable function as well.
https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... chrome/browser/history/download_database.cc:208: const char kReasonPathDangerSchema[] = This is not static where as the rest are. I tend to think none of them should be static. https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... File chrome/browser/history/download_row.h (right): https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... chrome/browser/history/download_row.h:64: std::string mime_type; If this is based on heuristics, how come it needs to be cached? That is, can't we calculated it as needed? https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... chrome/browser/history/download_row.h:67: std::string original_mime_type; How about coverage that mime_type and original_mime_type are persisted/fetched appropriately from db?
On 2013/12/18 23:24:05, sky wrote: > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > File chrome/browser/history/download_database.cc (right): > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > chrome/browser/history/download_database.cc:208: const char > kReasonPathDangerSchema[] = > This is not static where as the rest are. I tend to think none of them should be > static. > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > File chrome/browser/history/download_row.h (right): > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > chrome/browser/history/download_row.h:64: std::string mime_type; > If this is based on heuristics, how come it needs to be cached? That is, can't > we calculated it as needed? > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > chrome/browser/history/download_row.h:67: std::string original_mime_type; > How about coverage that mime_type and original_mime_type are persisted/fetched > appropriately from db? I don't actually know how the heuristics look, but yes it might be possible to recalculate this if the file are still intact. Anyone know if this is feasible and where this calculation is done? Unit tests that covers mime_type and original_mime_type fetching and storing are added in download_history_unittest.cc.
On 2013/12/18 23:47:49, Tiger wrote: > On 2013/12/18 23:24:05, sky wrote: > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > File chrome/browser/history/download_database.cc (right): > > > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > chrome/browser/history/download_database.cc:208: const char > > kReasonPathDangerSchema[] = > > This is not static where as the rest are. I tend to think none of them should > be > > static. > > > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > File chrome/browser/history/download_row.h (right): > > > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > chrome/browser/history/download_row.h:64: std::string mime_type; > > If this is based on heuristics, how come it needs to be cached? That is, can't > > we calculated it as needed? > > > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > chrome/browser/history/download_row.h:67: std::string original_mime_type; > > How about coverage that mime_type and original_mime_type are persisted/fetched > > appropriately from db? > > I don't actually know how the heuristics look, but yes it might be possible to > recalculate this if the file are still intact. Anyone know if this is feasible > and where this calculation is done? > > Unit tests that covers mime_type and original_mime_type fetching and storing are > added in download_history_unittest.cc. I couldn't readily tell if those actually write using download_db.
On 2013/12/18 23:47:49, Tiger wrote: > On 2013/12/18 23:24:05, sky wrote: > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > File chrome/browser/history/download_row.h (right): > > > > > https://codereview.chromium.org/102683004/diff/90001/chrome/browser/history/d... > > chrome/browser/history/download_row.h:64: std::string mime_type; > > If this is based on heuristics, how come it needs to be cached? That is, can't > > we calculated it as needed? > > I don't actually know how the heuristics look, but yes it might be possible to > recalculate this if the file are still intact. Anyone know if this is feasible > and where this calculation is done? I looked into the heuristics and it is done with net/base/mime_sniffer.h and the SniffMimeType function. Using that after a restart could work, but would ignore the special handling that are done when the download are created -- like the X-Content-Type-Options=nosniff header, so I would actually advice against redoing this as it could give a different result since not all information is available to recreate the mime_type information in the correct way.
> > Unit tests that covers mime_type and original_mime_type fetching and storing > are > > added in download_history_unittest.cc. > > I couldn't readily tell if those actually write using download_db. Ah yeah, sorry, I think I misunderstood here. I've added testing of reading and writing those fields to the ClearBrowsingData_Downloads test in HistoryBackendDBTest. I also fixed a comment (not changed from a copy-paste).
DB looks fine (modulo the nit below). Your other reviewers will have to comment on your question about recreating mime_type. https://codereview.chromium.org/102683004/diff/130001/chrome/browser/history/... File chrome/browser/history/history_unittest.cc (right): https://codereview.chromium.org/102683004/diff/130001/chrome/browser/history/... chrome/browser/history/history_unittest.cc:174: "application/octet-stream", nit: can you use a different strings here? That way if say there were a bug where we always returned mime_type the test would fail.
On 2013/12/19 16:48:37, sky wrote: > DB looks fine (modulo the nit below). Your other reviewers will have to comment > on your question about recreating mime_type. > > https://codereview.chromium.org/102683004/diff/130001/chrome/browser/history/... > File chrome/browser/history/history_unittest.cc (right): > > https://codereview.chromium.org/102683004/diff/130001/chrome/browser/history/... > chrome/browser/history/history_unittest.cc:174: "application/octet-stream", > nit: can you use a different strings here? That way if say there were a bug > where we always returned mime_type the test would fail. I've fixed this as well. Thanks for reviewing!
LGTM from me, but I'm deferring to Asanka around the mime-type re-creation issue; please make sure he's ok with this choice. https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... chrome/browser/history/download_database.cc:289: nit: Style guide advocates for minimizing vertical whitespace, and specifically not starting a function wiht a blank line. https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... chrome/browser/history/download_database.cc:325: nit, suggestion: My take would be that this blank line doesn't add anything either. https://codereview.chromium.org/102683004/diff/150001/chrome/test/data/Histor... File chrome/test/data/History/history.28.sql (right): https://codereview.chromium.org/102683004/diff/150001/chrome/test/data/Histor... chrome/test/data/History/history.28.sql:26: nit: Unneeded blank line.
On 2013/12/19 23:12:07, rdsmith wrote: > LGTM from me, but I'm deferring to Asanka around the mime-type re-creation > issue; please make sure he's ok with this choice. > > https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... > File chrome/browser/history/download_database.cc (right): > > https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... > chrome/browser/history/download_database.cc:289: > nit: Style guide advocates for minimizing vertical whitespace, and specifically > not starting a function wiht a blank line. > > https://codereview.chromium.org/102683004/diff/150001/chrome/browser/history/... > chrome/browser/history/download_database.cc:325: > nit, suggestion: My take would be that this blank line doesn't add anything > either. > > https://codereview.chromium.org/102683004/diff/150001/chrome/test/data/Histor... > File chrome/test/data/History/history.28.sql (right): > > https://codereview.chromium.org/102683004/diff/150001/chrome/test/data/Histor... > chrome/test/data/History/history.28.sql:26: > nit: Unneeded blank line. Thanks, I'll fix those issues as well. @asanka, how about the mime type re-ceation?
Why store original_mimetype at all? Also could you respond to the comments from https://codereview.chromium.org/102683004/#msg11 ? https://codereview.chromium.org/102683004/diff/170001/chrome/browser/history/... File chrome/browser/history/download_database.h (right): https://codereview.chromium.org/102683004/diff/170001/chrome/browser/history/... chrome/browser/history/download_database.h:56: bool MigrateMimeType(); Nit: HistoryDatabase ends up inheriting from DownloadDatabase, URLDatabase, VisitDatabase etc It'll be clearer if this was called MigrateDownloadMimeTypes() or somesuch.
On 2013/12/20 17:58:16, asanka wrote: > Why store original_mimetype at all? Regarding re-sniffing the MIME type: I'd prefer that we don't because of changes that can happen between the time the file is downloaded and the time the file is opened. The MIME type is used for filename determination. As a result, it indirectly affects the security assumptions and decisions we make about the file. If the MIME type is also used to guide opening behavior (in the future?), then I'd strongly prefer that both opening and downloading use the exact same MIME type. This can't be guaranteed if we are re-sniffing the MIME type because the sniffing logic may (and has in the past) change in the meantime. > Also could you respond to the comments from > https://codereview.chromium.org/102683004/#msg11 ? > > https://codereview.chromium.org/102683004/diff/170001/chrome/browser/history/... > File chrome/browser/history/download_database.h (right): > > https://codereview.chromium.org/102683004/diff/170001/chrome/browser/history/... > chrome/browser/history/download_database.h:56: bool MigrateMimeType(); > Nit: HistoryDatabase ends up inheriting from DownloadDatabase, URLDatabase, > VisitDatabase etc It'll be clearer if this was called MigrateDownloadMimeTypes() > or somesuch.
Ping?
Ok, LGTM
lgtm
Thanks everyone. LGTM from me as well. tiger@opera: Ping?
On 2014/02/06 18:08:08, asanka wrote: > Thanks everyone. LGTM from me as well. > > tiger@opera: Ping? Turns out this review got lost in the shuffle when Tiger moved on to a new employer. I've refreshed the patch resolving a reject due to an spelling fix in a comment and reposted it to https://codereview.chromium.org/319703002/ as I wasn't allowed to update this review. Please head over there and rubber stamp it! :) |
