Add mime type information to the download database

Add mime type information to the download database

BUG=328382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276775

[Fredrik Öhrn, 2014-06-05 14:23:05]

This is a rebased version of the already approved
https://codereview.chromium.org/102683004/

Original author is tiger@opera.com but he moved on to new adventures.

[sky, 2014-06-05 16:02:02]

LGTM

https://codereview.chromium.org/319703002/diff/1/chrome/browser/history/downl...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/319703002/diff/1/chrome/browser/history/downl...
chrome/browser/history/download_database.cc:294: "mime_type VARCHAR(255) NOT
NULL,"    // Mime type.
Add new types to the end so it's easier to see what's changed over time. (and
easier to compare with block on 208).

[Fredrik Öhrn, 2014-06-05 16:28:25]

https://codereview.chromium.org/319703002/diff/1/chrome/browser/history/downl...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/319703002/diff/1/chrome/browser/history/downl...
chrome/browser/history/download_database.cc:294: "mime_type VARCHAR(255) NOT
NULL,"    // Mime type.
On 2014/06/05 16:02:03, sky wrote:
> Add new types to the end so it's easier to see what's changed over time. (and
> easier to compare with block on 208).

Moved in patch #2.

[sky, 2014-06-05 18:10:11]

Thanks, LGTM

[Randy Smith (Not in Mondays), 2014-06-05 19:00:17]

I'll LGTM download_manager.h to unblock the CL.

[Randy Smith (Not in Mondays), 2014-06-05 19:01:27]

Huh, look at that, I'm not an owner for that file anymore.  Sorry, no help here.

[asanka, 2014-06-05 20:32:22]

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
chrome/browser/history/download_database.cc:308: "last_modified VARCHAR NOT
NULL,";    // Last-Modified header
There's an extra semicolon here. ^^ and one is missing below.

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
chrome/browser/history/download_database.cc:565: "start_time, "
Nit: add an extra space at the start of these strings.

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
File chrome/browser/history/download_row.h (right):

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
chrome/browser/history/download_row.h:66: // The originial mime type of the
download.
Nit: original.

Nit: MIME type

[jochen (gone - plz use gerrit), 2014-06-06 07:06:54]

lgtm

[Fredrik Öhrn, 2014-06-09 11:11:16]

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/319703002/diff/20001/chrome/browser/history/d...
chrome/browser/history/download_database.cc:308: "last_modified VARCHAR NOT
NULL,";    // Last-Modified header
On 2014/06/05 20:32:22, asanka wrote:
> There's an extra semicolon here. ^^ and one is missing below.

Doh! That should teach me to test compile even the most trivial changes.

All your issues should be addressed in patch #4.

[asanka, 2014-06-09 20:40:00]

LGTM

[Fredrik Öhrn, 2014-06-10 09:52:41]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-06-10 09:54:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ohrn@opera.com/319703002/50001

[commit-bot: I haz the power, 2014-06-10 21:59:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-10 22:06:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 22:06:05]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Fredrik Öhrn, 2014-06-11 07:09:47]

On 2014/06/10 22:06:05, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
>   android_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
>   android_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
>   linux_chromium_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
>   linux_chromium_gn_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
>   linux_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

I'm a bit stumped. If i understand the above logs correctly the patch failed to
apply due to conflicts?

But in that case, why didn't it fail on all bots? In my naiveté I assume all
bots build identical source trees...

[sky, 2014-06-11 15:59:49]

Timing perhaps? The bots don't all necessarily run at the same time. I
would merge to tip of tree and try again.


On Wed, Jun 11, 2014 at 12:09 AM, <ohrn@opera.com> wrote:

> On 2014/06/10 22:06:05, I haz the power (commit-bot) wrote:
>
>> Try jobs failed on following builders:
>>    android_chromium_gn_compile_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> android_chromium_gn_compile_rel/builds/12440)
>
>>    android_clang_dbg on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> android_clang_dbg/builds/150661)
>
>>    android_dbg on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> android_dbg/builds/194006)
>
>>    chromium_presubmit on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> chromium_presubmit/builds/72796)
>
>>    linux_chromium_chromeos_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_chromeos_rel/builds/36230)
>
>>    linux_chromium_clang_dbg on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_clang_dbg/builds/33061)
>
>>    linux_chromium_gn_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_gn_rel/builds/15270)
>
>>    linux_chromium_rel on tryserver.chromium
>>
>
> (http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_rel/builds/40362)
>
> I'm a bit stumped. If i understand the above logs correctly the patch
> failed to
> apply due to conflicts?
>
> But in that case, why didn't it fail on all bots? In my naiveté I assume
> all
> bots build identical source trees...
>
> https://codereview.chromium.org/319703002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fredrik Öhrn, 2014-06-12 08:55:18]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-06-12 08:59:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ohrn@opera.com/319703002/70001

[commit-bot: I haz the power, 2014-06-12 13:02:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Fredrik Öhrn, 2014-06-12 13:32:24]

The CQ bit was unchecked by ohrn@opera.com

[Fredrik Öhrn, 2014-06-12 13:35:12]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-06-12 13:36:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ohrn@opera.com/319703002/90001

[commit-bot: I haz the power, 2014-06-12 17:16:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-12 19:39:52]

Change committed as 276775