1412982 - Don't compile nsDownloadHistory when we are using Places

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Marco Castelluccio [:marco]]

From the code coverage report, it looks like this file is never executed during tests.
From a quick inspection, it looks like dead code to me.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Marco Bonardo [:mak]]

I think this is a mock implementation used when compiling without MOZ_PLACES, it's unused in firefox desktop because we always compile with MOZ_PLACES.

Comment 3

[Marco Castelluccio [:marco]]

Should we keep it around? Are we using it somewhere?

Comment 4

[Marco Castelluccio [:marco]]

I can make it compile conditionally if MOZ_PLACES is not defined, if we want to keep it around.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Marco Castelluccio [:marco] from comment #4)
> I can make it compile conditionally if MOZ_PLACES is not defined, if we want
> to keep it around.

That's likely the right thing for now.
This is what the docshell uses when nothing else (Places) implements nsIDownloadHistory.

Comment 6

[Marco Bonardo [:mak]]

To clarify, SM and Thunderbird both use Places, thus it wouldn't be a problem to remove this. What I'm not sure about is Android usage of the Docshell though.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Marco Bonardo [::mak] from comment #6)
> To clarify, SM and Thunderbird both use Places, thus it wouldn't be a
> problem to remove this. What I'm not sure about is Android usage of the
> Docshell though.

Android has its own separate history implementation IIRC.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Marco Castelluccio [:marco] from comment #8)
> Android has its own separate history implementation IIRC.

Yes, but it may still be a simple different implementation of nsIGlobalHistory (and thus nsIDownloadHistory). As I said, I don't know if it has its own separate docshell, it sounds unlikely.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8923772 [details] [diff] [review]
Patch

Review of attachment 8923772 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixed, and a try run.

::: docshell/base/moz.build
@@ +102,5 @@
>      'PendingGlobalHistoryEntry.cpp',
>      'SerializedLoadContext.cpp',
>  ]
>  
> +if CONFIG['MOZ_PLACES']:

shouldn't this be "if not CONFIG..."?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a05948f281b
Compile nsDownloadHistory only if Places is not used. r=mak

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+. It was green on try.

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5a05948f281b