Remove dependency on //content from history DownloadDatabase

Remove dependency on //content from history DownloadDatabase

Move the mapping between content enumeration from download_row.h and
download_database.h into an embedder specific file and provide a
content implementation.

BUG=371836
Committed: https://crrev.com/71524662845133324432049a5e3829900451ed4e
Cr-Commit-Position: refs/heads/master@{#311057}

[sdefresne, 2015-01-05 17:40:04]

sdefresne@chromium.org changed reviewers:
+ avi@chromium.org, droger@chromium.org

[sdefresne, 2015-01-05 17:40:05]

droger: Please take a look.
sky: FYI as OWNERS of //chrome/browser/history and //components/history
avi: OWNERS for the new dependency on //content from
//components/history/content

[droger, 2015-01-05 18:02:58]

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
File components/history/content/browser/download_constants_utils.cc (right):

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
components/history/content/browser/download_constants_utils.cc:42: default:
If possible get rid of the default case. That way, if someone adds a new value,
a compile error will be generated here.

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
components/history/content/browser/download_constants_utils.cc:102: default:
If possible get rid of the default case. That way, if someone adds a new value,
a compile error will be generated here.

[Avi (use Gerrit), 2015-01-05 19:53:44]

On 2015/01/05 17:40:05, sdefresne wrote:
> avi: OWNERS for the new dependency on //content from
> //components/history/content

I guess so? I love when new stuff depends on content (yay content!) but am not
sure why you need my approval to make it happen. LGTM for what it's worth.

[sdefresne, 2015-01-05 20:03:19]

On 2015/01/05 at 19:53:44, avi wrote:
> On 2015/01/05 17:40:05, sdefresne wrote:
> > avi: OWNERS for the new dependency on //content from
> > //components/history/content
> 
> I guess so? I love when new stuff depends on content (yay content!) but am not
sure why you need my approval to make it happen. LGTM for what it's worth.

Approval from OWNERS are now required when adding a new DEPS. See:
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=975

[sdefresne, 2015-01-05 20:08:33]

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
File components/history/content/browser/download_constants_utils.cc (right):

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
components/history/content/browser/download_constants_utils.cc:42: default:
On 2015/01/05 at 18:02:58, droger wrote:
> If possible get rid of the default case. That way, if someone adds a new
value, a compile error will be generated here.

Done.

https://codereview.chromium.org/833033002/diff/20001/components/history/conte...
components/history/content/browser/download_constants_utils.cc:102: default:
On 2015/01/05 at 18:02:58, droger wrote:
> If possible get rid of the default case. That way, if someone adds a new
value, a compile error will be generated here.

Done.

[Avi (use Gerrit), 2015-01-05 20:17:07]

On 2015/01/05 20:03:19, sdefresne wrote:
> On 2015/01/05 at 19:53:44, avi wrote:
> > On 2015/01/05 17:40:05, sdefresne wrote:
> > > avi: OWNERS for the new dependency on //content from
> > > //components/history/content
> > 
> > I guess so? I love when new stuff depends on content (yay content!) but am
not
> sure why you need my approval to make it happen. LGTM for what it's worth.
> 
> Approval from OWNERS are now required when adding a new DEPS. See:
> https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=975

Oh! I did not know that.

[sdefresne, 2015-01-06 09:39:08]

droger: please take another look

[droger, 2015-01-06 10:22:18]

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
File chrome/browser/history/download_database.cc (left):

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
chrome/browser/history/download_database.cc:390:
UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
Where did this code go? I no longer see the histograms in particular.

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
File chrome/browser/history/history_database.cc (right):

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
chrome/browser/history/history_database.cc:38: :
DownloadDatabase(static_cast<DownloadInterruptReason>(
Can we use ToHistoryDownloadInterruptReason() here?

https://codereview.chromium.org/833033002/diff/100001/components/history/cont...
File components/history/content/browser/download_constants_utils.cc (right):

https://codereview.chromium.org/833033002/diff/100001/components/history/cont...
components/history/content/browser/download_constants_utils.cc:28: NOTREACHED();
Optional: remove this, as all the cases are handled in the switch. Same below
and in the other file.

[sdefresne, 2015-01-06 10:51:17]

PTAL

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
File chrome/browser/history/download_database.cc (left):

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
chrome/browser/history/download_database.cc:390:
UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
On 2015/01/06 at 10:22:18, droger wrote:
> Where did this code go? I no longer see the histograms in particular.

The file has been moved to components/history/core/browser/download_database.cc

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
File chrome/browser/history/history_database.cc (right):

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
chrome/browser/history/history_database.cc:38: :
DownloadDatabase(static_cast<DownloadInterruptReason>(
On 2015/01/06 at 10:22:18, droger wrote:
> Can we use ToHistoryDownloadInterruptReason() here?

Good catch.

https://codereview.chromium.org/833033002/diff/100001/components/history/cont...
File components/history/content/browser/download_constants_utils.cc (right):

https://codereview.chromium.org/833033002/diff/100001/components/history/cont...
components/history/content/browser/download_constants_utils.cc:28: NOTREACHED();
On 2015/01/06 at 10:22:18, droger wrote:
> Optional: remove this, as all the cases are handled in the switch. Same below
and in the other file.

I cannot remove this as otherwise I get compilation errors on Android
(apparently the compiler cannot see that this is un-reachable code):

../../components/history/content/browser/download_constants_utils.cc: In
function 'content::DownloadItem::DownloadState
history::ToContentDownloadState(history::DownloadState)':
../../components/history/content/browser/download_constants_utils.cc:28:1:
error: control reaches end of non-void function [-Werror=return-type]

Same for the other functions.

[droger, 2015-01-06 11:52:18]

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
File chrome/browser/history/download_database.cc (left):

https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
chrome/browser/history/download_database.cc:390:
UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
On 2015/01/06 10:51:17, sdefresne wrote:
> On 2015/01/06 at 10:22:18, droger wrote:
> > Where did this code go? I no longer see the histograms in particular.
> 
> The file has been moved to
components/history/core/browser/download_database.cc

I see, but 8 calls to UMA_HISTOGRAM_ are deleted here, and I don't see them
added in the component.

[sdefresne, 2015-01-06 12:30:16]

On 2015/01/06 at 11:52:18, droger wrote:
>
https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
> File chrome/browser/history/download_database.cc (left):
> 
>
https://codereview.chromium.org/833033002/diff/100001/chrome/browser/history/...
> chrome/browser/history/download_database.cc:390:
UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
> On 2015/01/06 10:51:17, sdefresne wrote:
> > On 2015/01/06 at 10:22:18, droger wrote:
> > > Where did this code go? I no longer see the histograms in particular.
> > 
> > The file has been moved to
components/history/core/browser/download_database.cc
> 
> I see, but 8 calls to UMA_HISTOGRAM_ are deleted here, and I don't see them
added in the component.

I did not remove them, and as far as I can tell, they are still there:

$ git grep UMA_HISTOGRAM_ components/history/core/browser/download_database.cc
components/history/core/browser/download_database.cc:     
UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
components/history/core/browser/download_database.cc:     
UMA_HISTOGRAM_ENUMERATION("Download.DatabaseRecordDropped",
components/history/core/browser/download_database.cc:   
UMA_HISTOGRAM_BOOLEAN("Download.DatabaseEmptyUrlChain", empty_url_chain);
components/history/core/browser/download_database.cc:     
UMA_HISTOGRAM_ENUMERATION("Download.DatabaseMainInsertError",
components/history/core/browser/download_database.cc:     
UMA_HISTOGRAM_BOOLEAN("Download.DatabaseCorruptUrls", corrupt_urls);
components/history/core/browser/download_database.cc:     
UMA_HISTOGRAM_ENUMERATION("Download.DatabaseURLChainInsertError",
components/history/core/browser/download_database.cc:   
UMA_HISTOGRAM_ENUMERATION("Download.DatabaseMainDeleteError",
components/history/core/browser/download_database.cc:   
UMA_HISTOGRAM_ENUMERATION("Download.DatabaseURLChainDeleteError",


Could it be an issue with how the patch is displayed? Note that the file is
considered moved, so most of the lines are folded when you look at
components/history/core/browser/download_database.cc.

[droger, 2015-01-06 12:32:40]

On 2015/01/06 12:30:16, sdefresne wrote:
> Could it be an issue with how the patch is displayed? Note that the file is
> considered moved, so most of the lines are folded when you look at
> components/history/core/browser/download_database.cc.

Yes indeed, I fail at reading diffs. Sorry.
LGTM.

[sdefresne, 2015-01-06 16:50:08]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-01-06 16:50:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833033002/160001

[commit-bot: I haz the power, 2015-01-06 16:58:01]

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

[commit-bot: I haz the power, 2015-01-06 16:58:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sdefresne, 2015-01-06 17:52:00]

sdefresne@chromium.org changed reviewers:
+ benjhayden@chromium.org, blundell@chromium.org, jhawkins@chromium.org

[sdefresne, 2015-01-06 17:52:01]

benjhayden: OWNERS of //chrome/browser/{download,extensions}
jhawkins: OWNERS of //chrome/browser/{BUILD.gn,safe_browsing,ui/webui}
blundell: OWNERS of //components/BUILD.gn

[James Hawkins, 2015-01-06 19:13:12]

https://codereview.chromium.org/833033002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right):

https://codereview.chromium.org/833033002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:15: #include
"components/history/core/browser/download_row.h"
This include appears to be unused.  Can you please verify that all current
includes of this file (in this CL) are used?

[blundell, 2015-01-07 10:08:39]

//components/BUILD.gn LGTM

[sdefresne, 2015-01-07 10:51:25]

I removed the obsolete #include, thank you for pointing them.

benjhayden, jhawkins: please take a look as OWNERS

[James Hawkins, 2015-01-07 17:31:59]

lgtm

[sdefresne, 2015-01-08 09:37:04]

ping

[sky, 2015-01-08 22:44:57]

Pro-tip, when you have multiple reviewers please indicate who you are
ping'ing (I assuming not me as you said FYI for me early on).

On Thu, Jan 8, 2015 at 1:37 AM,  <sdefresne@chromium.org> wrote:
> ping
>
> https://codereview.chromium.org/833033002/

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

[sdefresne, 2015-01-09 09:32:53]

sdefresne@chromium.org changed reviewers:
+ finnur@chromium.org, phajdan.jr@chromium.org
- benjhayden@chromium.org

[sdefresne, 2015-01-09 09:32:54]

Moving to OWNERS in upper directory in the hierarchy since benjhayden is not
responding.

phajdan.jr: can you review //chrome/browser/download
finnur: can you review //chrome/browser/extensions

[Finnur, 2015-01-12 10:23:25]

Easiest review evar. :)
c/b/e LGTM.

[Paweł Hajdan Jr., 2015-01-12 15:38:09]

chrome/browser/download LGTM

[sdefresne, 2015-01-12 15:41:32]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-01-12 15:42:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833033002/180001

[commit-bot: I haz the power, 2015-01-12 16:24:38]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-01-12 16:25:35]

Patchset 10 (id:??) landed as
https://crrev.com/71524662845133324432049a5e3829900451ed4e
Cr-Commit-Position: refs/heads/master@{#311057}