Proposed download manager API changes for per-window private browsing

[Josh Matthews]

Per-window private browsing is coming
(https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
things in the platform require changing to support the new architecture.
Since the download manager is shared among other products, I've been
asked to start a conversation about my proposed changes so that
interested parties can provide input.

The existing API for the download manager
(http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl)
need to change to support concurrent private downloads and public
downloads. This means we'll need an in-memory table of private downloads
that gets cleared at certain points, and an on-disk persistent one for
the rest. This has implications about the unique identifier used
per-download. Here are the changes that are being discussed:

1) addDownload grows an extra optional parameter, nsILoadContext
context (NB. nsILoadContext contains a boolean attribute
usingPrivateBrowsing). If present, the usingPrivateBrowsing attribute on
this interface controls whether the download should reside in the
private, in-memory database or the disk-based non-private database. If
not present, the download is treated as public.

1b) Mardak has suggested passing an isPrivate boolean instead.

1c) Paolo suggests that we break the API for existing consumers (by
renaming or throwing an exception) to ensure that nobody unintentionally
uses the new API (defaulting to public) without considering whether the
download should be private.

2) Both private and non-private databases have a GUID column added to
ensure uniqueness between databases. getDownload, cancelDownload,
removeDownload, pauseDownload, resumeDownload, and retryDownload take
a GUID string instead of an integer ID.

2a) Mardak has suggested avoiding GUIDs, instead using negative IDs for
private downloads which would avoid the changes required in (2). Paolo
suggests that if the remaining changes to the API are nontrivial,
switching to GUIDs would still have benefits for various front-end
pieces of code.

3) activePrivateDownloads and activePrivateDownloadCount are added
which correspond with activeDownloads and activeDownloadCount.

removeDownloadsByTimeframe and cleanUp would continue to only operate on
public downloads. Private downloads are transient enough that I don't
think we need time-based APIs for removing them.

There have been mentions of either exposing a matching database
connection for private downloads, or removing the existing database
connection in the API and exposing any required API to provide needed
functionality. I haven't looked into this at all.

Thoughts? Concerns?

Cheers,
Josh

[Josh Matthews]

[Justin Wood (Callek)]

Josh Matthews wrote:
> Per-window private browsing is coming
> (https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
> things in the platform require changing to support the new architecture.
> Since the download manager is shared among other products, I've been
> asked to start a conversation about my proposed changes so that
> interested parties can provide input.

Just as a point of reference, Private Browsing is currently disableable,
(build config for core/shared components anyway) and SeaMonkey does not
currently ship with Private Browsing turned on at all.

> The existing API for the download manager
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl)
> need to change to support concurrent private downloads and public
> downloads. This means we'll need an in-memory table of private downloads
> that gets cleared at certain points, and an on-disk persistent one for
> the rest. This has implications about the unique identifier used
> per-download. Here are the changes that are being discussed:

SeaMonkey also uses a very customized UI for the Download Manager based
on the API so far exposed. So my comments take into account the above
two points.

> 1) addDownload grows an extra optional parameter, nsILoadContext
> context (NB. nsILoadContext contains a boolean attribute
> usingPrivateBrowsing). If present, the usingPrivateBrowsing attribute on
> this interface controls whether the download should reside in the
> private, in-memory database or the disk-based non-private database. If
> not present, the download is treated as public.
>
> 1b) Mardak has suggested passing an isPrivate boolean instead.
>

I support either of the above, preferably optional.

> 1c) Paolo suggests that we break the API for existing consumers (by
> renaming or throwing an exception) to ensure that nobody unintentionally
> uses the new API (defaulting to public) without considering whether the
> download should be private.

I strongly disagree with this, at least in the "core has private
browsing disabled" but if this happens when private browsing is on, and
not when off, then we also have API conflict and thats worse.

So *IFF* this is the decided on option, please do an assert here that we
pass in a decision to have public downloads rather than private (because
we don't have private browsing)

> 2) Both private and non-private databases have a GUID column added to
> ensure uniqueness between databases. getDownload, cancelDownload,
> removeDownload, pauseDownload, resumeDownload, and retryDownload take
> a GUID string instead of an integer ID.
>
> 2a) Mardak has suggested avoiding GUIDs, instead using negative IDs for
> private downloads which would avoid the changes required in (2). Paolo
> suggests that if the remaining changes to the API are nontrivial,
> switching to GUIDs would still have benefits for various front-end
> pieces of code.

I don't know API considerations in switching from ID to GUIDs, but I
would love to error on the side of less-invasive vs more invasive here.

> 3) activePrivateDownloads and activePrivateDownloadCount are added
> which correspond with activeDownloads and activeDownloadCount.

Please key these off the "Is private Browsing compiled" build-config
stuff, and add throwing(?) stub methods for them.

> removeDownloadsByTimeframe and cleanUp would continue to only operate on
> public downloads. Private downloads are transient enough that I don't
> think we need time-based APIs for removing them.

IMO both these should also work on private downloads, having the
separation just because one window/download is private temporarily is a
bad idea, imo.

> There have been mentions of either exposing a matching database
> connection for private downloads, or removing the existing database
> connection in the API and exposing any required API to provide needed
> functionality. I haven't looked into this at all.

Do we feel we need to make private downloads VERY hard to get at for
extensions? Or is it better to assume an extension can already get this
data and thus no needto split the database connection api here. [I
really don't know, just posing the thought]

--
~Justin Wood (Callek)

[Josh Matthews]

On 12-03-13 5:47 AM, Justin Wood (Callek) wrote:
> Josh Matthews wrote:
>> Per-window private browsing is coming
>> (https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
>> things in the platform require changing to support the new architecture.
>> Since the download manager is shared among other products, I've been
>> asked to start a conversation about my proposed changes so that
>> interested parties can provide input.
>
> Just as a point of reference, Private Browsing is currently disableable,
> (build config for core/shared components anyway) and SeaMonkey does not
> currently ship with Private Browsing turned on at all.

This might be changing. The actual PB-per-window infrastructure is
moving into core docshell code now, and I haven't bothered to include
any kind of compile-time disabling so far. This is perhaps something
that should be discussed, I guess?

I don't think I understand what you're asking for. I don't think we want
to condition the API-breakage based on whether PB is enabled at compile
time or not.

>> 2) Both private and non-private databases have a GUID column added to
>> ensure uniqueness between databases. getDownload, cancelDownload,
>> removeDownload, pauseDownload, resumeDownload, and retryDownload take
>> a GUID string instead of an integer ID.
>>
>> 2a) Mardak has suggested avoiding GUIDs, instead using negative IDs for
>> private downloads which would avoid the changes required in (2). Paolo
>> suggests that if the remaining changes to the API are nontrivial,
>> switching to GUIDs would still have benefits for various front-end
>> pieces of code.
>
> I don't know API considerations in switching from ID to GUIDs, but I
> would love to error on the side of less-invasive vs more invasive here.
>
>> 3) activePrivateDownloads and activePrivateDownloadCount are added
>> which correspond with activeDownloads and activeDownloadCount.
>
> Please key these off the "Is private Browsing compiled" build-config
> stuff, and add throwing(?) stub methods for them.
>
>> removeDownloadsByTimeframe and cleanUp would continue to only operate on
>> public downloads. Private downloads are transient enough that I don't
>> think we need time-based APIs for removing them.
>
> IMO both these should also work on private downloads, having the
> separation just because one window/download is private temporarily is a
> bad idea, imo.

The problem here is that the existing modal Firefox download manager
window really can only show public downloads. Making the clean up button
inside of it also clean up private downloads sounds surprising and not
user-friendly.

>> There have been mentions of either exposing a matching database
>> connection for private downloads, or removing the existing database
>> connection in the API and exposing any required API to provide needed
>> functionality. I haven't looked into this at all.
>
> Do we feel we need to make private downloads VERY hard to get at for
> extensions? Or is it better to assume an extension can already get this
> data and thus no needto split the database connection api here. [I
> really don't know, just posing the thought]

It's not a matter of splitting the database connection. The downloads
are stored in separate databases with separate connections.

Thanks for your comments!

Cheers,
Josh

[Robert Kaiser]

Josh Matthews schrieb:

> This might be changing. The actual PB-per-window infrastructure is
> moving into core docshell code now, and I haven't bothered to include
> any kind of compile-time disabling so far. This is perhaps something
> that should be discussed, I guess?

Actually, making PB more generic is probably great for SeaMonkey as that
team will have an easier time if it wants to implement PB as well!

As the original author of the SeaMonkey download manager UI (and the
author of the in-tab "Jökulsárlón Download Manager" add-on for Firefox
and SeaMonkey which is based on that SeaMonkey code), I think most of
your proposal sound fine, though we'll of course need to audit the code
for this. I'm not sure yet how a common downloads tab/window would
represent the different types of downloads, but that's orthogonal, as
long as it can, I think.
One request though: Please make it possible to in some way run the same
(JS/XUL) add-on with versions both before and after the changes (even if
it means minor cases of version detection at certain places). ;-)

Robert Kaiser

[Paolo Amadini]

On 13/03/2012 2.54, Josh Matthews wrote:
> Per-window private browsing is coming
> (https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
> things in the platform require changing to support the new architecture.
> Since the download manager is shared among other products, I've been
> asked to start a conversation about my proposed changes so that
> interested parties can provide input.

Thanks for starting the conversation, and for the clear overview of
one of the possible API changes we could implement.

We know that we need to change the API to accommodate per-window private
browsing, and I'd also add that we're planning to improve the Download
Manager back-end performance by making it asynchronous. This probably
requires some API changes as well, so this is a good time to discuss.

What we should create in this phase is a sort of "compatibility plan"
for the evolution of the Download Manager back-end. So the first phase
is understanding the consumers of the current API and their needs.

So far, I've collected this list of consumers on bug 722859:
- The Downloads window allows viewing and handling downloads globally.
- The same does the Firefox Downloads Panel (coming soon).
- addDownload is called in a few places (e.g. uriloader/exthandler).
- Probably, some add-ons add new downloads, some can manipulate them.

Callek added two interesting elements:
- SeaMonkey uses a very customized UI for the Download Manager based

on the API so far exposed.

- Private Browsing is currently disableable and SeaMonkey does not

currently ship with Private Browsing turned on at all.

Thus, we'll certainly end up with at least three different interactions
with downloads in different products, and as Robert mentioned, some
add-ons might want to support all of them at the same time (fully or
partially):
- SeaMonkey current: no private browsing.
- Firefox current: global private browsing.
- Firefox future: per-window private browsing.

Current consumers handle the first two cases with the same API. We
should try and make it simple to continue to support those two cases.

But we must make sure that, when per-window private browsing is added,
existing consumers (either core or third-party, default-to-compatible
add-ons) don't cause private-mode downloads to leak into
non-private-mode areas. In particular:

*Adding new user-visible downloads*

This can currently be done using either the addDownload method or
indirectly through nsITransfer (used by Save As). This is the
easiest use case and I guess many add-ons do just this (as opposed
to doing full download management).

To check that the consumer is aware of whether the window from which
the download is initiated is in private mode or not, we should break
the ability to call the current form of addDownload, at least in cases
where per-window PBM is enabled. I've not thought about nsITransfer
but we should probably do the same there.

We can choose to keep the current API available as-is for the
"no private browsing" or "global private browsing" cases.

*Manipulating downloads*

The API for managing downloads currently includes nsIDownloadManager,
nsIDownload, and a set of related, global observer notifications.
nsIDownloadManager provides access to the underlying SQLite database,
which is essential for getting the list of downloads.

We could keep compatibility with the current API, also when per-window
PBM is enabled. But, unless we conclude that we can mix private-mode and
non-private-mode downloads in the same list (thoughts on this?), we
should ensure that existing consumers have access to non-private-mode
downloads only. This _might_ mean that the global observer
notifications should be sent for public downloads only.

Consumers that are aware of per-window private browsing, on the other
hand, should be able to access the two download lists concurrently.

There are also a few more thoughts to add:
- We might want to add a fully-asynchronous API that doesn't require
the user interface to query the database directly (as Josh already
mentioned).
- The Firefox Downloads Panel doesn't need to persist the full
download history across sessions anymore. Only active downloads
are kept. A SQLite database is overkill for that. We could store
the data in the session restore file, or other places, to improve
the performance of starting and managing a download.
- With the need to display two different download views concurrently,
we might consider to use an interface that is not a singleton.
- We are considering the benefits of de-XPCOMifying the management
interface and the notifications. The Downloads Panel already uses
a set of JavaScript model objects that represent downloads.

With all of this in mind, let's discuss what we think are the most
important requirements, and brainstorm about possible designs for
a revised Download Manager back-end. The changes Josh outlined
probably match most of the expectations above, though we've not
talked about performance improvements and other changes yet.

Thoughts?

Cheers,
Paolo

[Ehsan Akhgari]

On Tue, Mar 13, 2012 at 12:38 PM, Josh Matthews <jo...@joshmatthews.net>wrote:

> On 12-03-13 5:47 AM, Justin Wood (Callek) wrote:
>
>> Josh Matthews wrote:
>>
>>> Per-window private browsing is coming

>>> (https://wiki.mozilla.org/Per-**window_Private_Browsing<https://wiki.mozilla.org/Per-window_Private_Browsing>),

>>> and certain
>>> things in the platform require changing to support the new architecture.
>>> Since the download manager is shared among other products, I've been
>>> asked to start a conversation about my proposed changes so that
>>> interested parties can provide input.
>>>
>>
>> Just as a point of reference, Private Browsing is currently disableable,
>> (build config for core/shared components anyway) and SeaMonkey does not
>> currently ship with Private Browsing turned on at all.
>>
>
> This might be changing. The actual PB-per-window infrastructure is moving
> into core docshell code now, and I haven't bothered to include any kind of
> compile-time disabling so far. This is perhaps something that should be
> discussed, I guess?
>

This is actually not true today. There is no build time option to disable
support for PB. The reason why SM does not support PB today is that the PB
service currently lives in browser/, and so does all of the UI, and
everywhere else in the code, we check for the existence of the PB service
and will assume the non-PB case if it does not exist (which is the case for
any Gecko based app which is not Firefox.)

I don't have any interest in converting this sort of check into a build
time option at this time, so I think this issue is moot.

(Also note that I've heard some SM folks expressing interest in supporting
PB mode, IIRC the reason that has not happened was that nobody had the time
to work on it.)

> The existing API for the download manager

>>> (http://mxr.mozilla.org/**mozilla-central/source/**
>>> toolkit/components/downloads/**nsIDownloadManager.idl<http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl>

>>> )
>>> need to change to support concurrent private downloads and public
>>> downloads. This means we'll need an in-memory table of private downloads
>>> that gets cleared at certain points, and an on-disk persistent one for
>>> the rest. This has implications about the unique identifier used
>>> per-download. Here are the changes that are being discussed:
>>>
>>
>> SeaMonkey also uses a very customized UI for the Download Manager based
>> on the API so far exposed. So my comments take into account the above
>> two points.
>>
>> 1) addDownload grows an extra optional parameter, nsILoadContext
>>> context (NB. nsILoadContext contains a boolean attribute
>>> usingPrivateBrowsing). If present, the usingPrivateBrowsing attribute on
>>> this interface controls whether the download should reside in the
>>> private, in-memory database or the disk-based non-private database. If
>>> not present, the download is treated as public.
>>>
>>> 1b) Mardak has suggested passing an isPrivate boolean instead.
>>>
>>>
>> I support either of the above, preferably optional.
>>
>> 1c) Paolo suggests that we break the API for existing consumers (by
>>> renaming or throwing an exception) to ensure that nobody unintentionally
>>> uses the new API (defaulting to public) without considering whether the
>>> download should be private.
>>>
>>
>> I strongly disagree with this, at least in the "core has private
>> browsing disabled" but if this happens when private browsing is on, and
>> not when off, then we also have API conflict and thats worse.
>>
>

I don't think this is a good idea. Add-ons which are not aware of PB mode
can already screw things up in so many ways. Changing the signature of the
function in order to break existing code so that the add-on authors realize
that they need to pay attention to the PB mode is not worth the hassle IMO.

>
>> So *IFF* this is the decided on option, please do an assert here that we
>> pass in a decision to have public downloads rather than private (because
>> we don't have private browsing)
>>
>
> I don't think I understand what you're asking for. I don't think we want
> to condition the API-breakage based on whether PB is enabled at compile
> time or not.
>

Like I said, this issue is moot. :-)

>
> 2) Both private and non-private databases have a GUID column added to
>>> ensure uniqueness between databases. getDownload, cancelDownload,
>>> removeDownload, pauseDownload, resumeDownload, and retryDownload take
>>> a GUID string instead of an integer ID.
>>>
>>> 2a) Mardak has suggested avoiding GUIDs, instead using negative IDs for
>>> private downloads which would avoid the changes required in (2). Paolo
>>> suggests that if the remaining changes to the API are nontrivial,
>>> switching to GUIDs would still have benefits for various front-end
>>> pieces of code.
>>>
>>
>> I don't know API considerations in switching from ID to GUIDs, but I
>> would love to error on the side of less-invasive vs more invasive here.
>>
> >
>

I think using negativeness to mean "private" is a bad decision.

How about storing a GUID on the download object internally, exposing IDL
accessors for it, and then in the accessor for the download ID, throwing
an exception if the download object is private? That way, the only people
who would need to update their code are the people who are writing a
Download Manager UI for an app which supports PB mode (which will be
Firefox for now) and the other consumers can still use the ID accesors
without any problem (based on the premise that no download object that they
need to deal with will ever be private.)

> 3) activePrivateDownloads and activePrivateDownloadCount are added
>>> which correspond with activeDownloads and activeDownloadCount.
>>>
>>
>> Please key these off the "Is private Browsing compiled" build-config
>> stuff, and add throwing(?) stub methods for them.
>>
>

There is no build config, and I don't think we want different IDLs in
different build configurations anyway. I think these should exist on
nsIDownloadManager, and just do the right thing where the PB service does
not exist (i.e., pretend there is no private downloads). This way I could
write a DM UI extension which would work in both Firefox and SeaMonkey for
example.

>
>> removeDownloadsByTimeframe and cleanUp would continue to only operate on
>>> public downloads. Private downloads are transient enough that I don't
>>> think we need time-based APIs for removing them.
>>>
>>
>> IMO both these should also work on private downloads, having the
>> separation just because one window/download is private temporarily is a
>> bad idea, imo.
>>
>
> The problem here is that the existing modal Firefox download manager
> window really can only show public downloads. Making the clean up button
> inside of it also clean up private downloads sounds surprising and not
> user-friendly.
>

Hmm, I thought that we're going to block this work until Paolo's per-window
DM UI lands?

Regardless, I think removeDownloadsByTimeframe should work on both download
types, as it is designed for use cases such as the Clear Recent History
dialog. cleanUp however should only work on non-private downloads, and we
should add an alternate method for cleaning up private downloads.

Also, Callek, please note that nothing makes private windows temporary,
really. A user may keep both types of windows open for the entire duration
of the browsing session.

>
> There have been mentions of either exposing a matching database
>>> connection for private downloads, or removing the existing database
>>> connection in the API and exposing any required API to provide needed
>>> functionality. I haven't looked into this at all.
>>>
>>
>> Do we feel we need to make private downloads VERY hard to get at for
>> extensions? Or is it better to assume an extension can already get this
>> data and thus no needto split the database connection api here. [I
>> really don't know, just posing the thought]
>>
>

We should assume that the extension should be allowed to access the private
downloads just as easy as non-private ones, but we should make the existing
APIs do what makes sense depending on the API in question (see the
removeDownloadsByTimeframe/cleanUp case above for example.)

Please let me know if you disagree, or if I have missed something.

Thanks!
--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Tue, Mar 13, 2012 at 1:57 PM, Paolo Amadini <paolo....@amadzone.org>wrote:

> On 13/03/2012 2.54, Josh Matthews wrote:
> > Per-window private browsing is coming
> > (https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
> > things in the platform require changing to support the new architecture.
> > Since the download manager is shared among other products, I've been
> > asked to start a conversation about my proposed changes so that
> > interested parties can provide input.
>
> Thanks for starting the conversation, and for the clear overview of
> one of the possible API changes we could implement.
>
> We know that we need to change the API to accommodate per-window private
> browsing, and I'd also add that we're planning to improve the Download
> Manager back-end performance by making it asynchronous. This probably
> requires some API changes as well, so this is a good time to discuss.
>

I think these are separate discussions, but anyways. ;-)

> What we should create in this phase is a sort of "compatibility plan"
> for the evolution of the Download Manager back-end. So the first phase
> is understanding the consumers of the current API and their needs.
>
> So far, I've collected this list of consumers on bug 722859:
> - The Downloads window allows viewing and handling downloads globally.
> - The same does the Firefox Downloads Panel (coming soon).
> - addDownload is called in a few places (e.g. uriloader/exthandler).
>

Please note that the in-tree consumers don't really need backwards
compatibility. We can change them all (including the comm-central ones.)
But we should make sure that we don't do a dumb thing which can make the
lives of Firefox/SM developers hard in the future...

> - Probably, some add-ons add new downloads, some can manipulate them.
>
> Callek added two interesting elements:
> - SeaMonkey uses a very customized UI for the Download Manager based
> on the API so far exposed.
>

See above.

> - Private Browsing is currently disableable and SeaMonkey does not
> currently ship with Private Browsing turned on at all.
>

In the interest of making sure that the thread continues with the correct
assumptions, PB is not disableable at build time, and the way it is
"disabled" at runtime is by nothing making a download (or other things)
private.

> Thus, we'll certainly end up with at least three different interactions
> with downloads in different products, and as Robert mentioned, some
> add-ons might want to support all of them at the same time (fully or
> partially):
> - SeaMonkey current: no private browsing.
> - Firefox current: global private browsing.
> - Firefox future: per-window private browsing.
>

No these are actually only two cases:

A window being private
A window being non-private

It just so happens that all SM windows are non-private. There is nothing
else special there. Also note that currently in Firefox, if you're in the
global PB mode, all windows are marked as private and vice versa, therefore
the above categorization still holds.

>
> Current consumers handle the first two cases with the same API. We
> should try and make it simple to continue to support those two cases.
>
> But we must make sure that, when per-window private browsing is added,
> existing consumers (either core or third-party, default-to-compatible
> add-ons) don't cause private-mode downloads to leak into
> non-private-mode areas. In particular:
>
> *Adding new user-visible downloads*
>
> This can currently be done using either the addDownload method or
> indirectly through nsITransfer (used by Save As). This is the
> easiest use case and I guess many add-ons do just this (as opposed
> to doing full download management).
>
> To check that the consumer is aware of whether the window from which
> the download is initiated is in private mode or not, we should break
> the ability to call the current form of addDownload, at least in cases
> where per-window PBM is enabled. I've not thought about nsITransfer
> but we should probably do the same there.
>
> We can choose to keep the current API available as-is for the
> "no private browsing" or "global private browsing" cases.
>

Well, as I said before, it takes more than just this to make sure the
add-ons support the PB mode. :/

Josh, is there any way to get our hands on the docshell associated with the
caller of the API?

> *Manipulating downloads*
>
> The API for managing downloads currently includes nsIDownloadManager,
> nsIDownload, and a set of related, global observer notifications.
> nsIDownloadManager provides access to the underlying SQLite database,
> which is essential for getting the list of downloads.
>
> We could keep compatibility with the current API, also when per-window
> PBM is enabled. But, unless we conclude that we can mix private-mode and
> non-private-mode downloads in the same list (thoughts on this?),

We should not do that at all, ever! :-)

> we
> should ensure that existing consumers have access to non-private-mode
> downloads only. This _might_ mean that the global observer
> notifications should be sent for public downloads only.
>
> Consumers that are aware of per-window private browsing, on the other
> hand, should be able to access the two download lists concurrently.
>

This makes perfect sense to me.

> There are also a few more thoughts to add:
> - We might want to add a fully-asynchronous API that doesn't require
> the user interface to query the database directly (as Josh already
> mentioned).
>

I think we should do that, but it may be orthogonal to the discussion here.

> - The Firefox Downloads Panel doesn't need to persist the full
> download history across sessions anymore. Only active downloads
> are kept. A SQLite database is overkill for that. We could store
> the data in the session restore file, or other places, to improve
> the performance of starting and managing a download.
>

This is also not directly related to this discussion, as long as we make
sure that such data is not written to disk anywhere for private windows.

> - With the need to display two different download views concurrently,
> we might consider to use an interface that is not a singleton.
>

Or an interface which is a singleton but has two sets of APIs for listing
private and non-private downloads?

> - We are considering the benefits of de-XPCOMifying the management
> interface and the notifications. The Downloads Panel already uses
> a set of JavaScript model objects that represent downloads.
>

I don't know enough about this to see whether per-window PB is going to
affect things here or not.


Cheers,
--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Tue, Mar 13, 2012 at 5:47 AM, Justin Wood (Callek) <Cal...@gmail.com>wrote:

> > 1) addDownload grows an extra optional parameter, nsILoadContext
> > context (NB. nsILoadContext contains a boolean attribute
> > usingPrivateBrowsing). If present, the usingPrivateBrowsing attribute on
> > this interface controls whether the download should reside in the
> > private, in-memory database or the disk-based non-private database. If
> > not present, the download is treated as public.
> >
> > 1b) Mardak has suggested passing an isPrivate boolean instead.
> >
>
> I support either of the above, preferably optional.
>

Actually, now that I think of it, if we want to present different lists on
different private windows, this needs to be a load context.

--
Ehsan
<http://ehsanakhgari.org/>

[Josh Matthews]

No, hence the need for the extra nsILoadContext parameter that is
provided by the caller.

[Justin Dolske]

On 3/13/12 2:47 AM, Justin Wood (Callek) wrote:

> Just as a point of reference, Private Browsing is currently disableable,
> (build config for core/shared components anyway) and SeaMonkey does not
> currently ship with Private Browsing turned on at all.

I think we should change this -- it's already caused enough headaches
for code that has to determine both if PB is supported, and if it's enabled.

It's a big enough feature that we should just require it in all Gecko
apps, even if it's a simple stub service that just always says "nope,
not in PB mode!".

>> 1c) Paolo suggests that we break the API for existing consumers (by
>> renaming or throwing an exception) to ensure that nobody unintentionally
>> uses the new API (defaulting to public) without considering whether the
>> download should be private.
>
> I strongly disagree with this, at least in the "core has private
> browsing disabled" but if this happens when private browsing is on, and
> not when off, then we also have API conflict and thats worse.

As we've discovered over time, freezing APIs because of potential (or
even known!) consumers is _extremely_ limiting. I have no particular
opinion about this actual API, but unless changing it creates some kind
of extreme burden for other apps and addons it should just be done.

No free lunch and all that.

Justin

[Neil]

Ehsan Akhgari wrote:

>the PB service currently lives in browser/, and so does all of the UI, and everywhere else in the code, we check for the existence of the PB service and will assume the non-PB case if it does not exist
>

Although annoyingly in certain conditions the one in the Error console
can itself trigger an error when the console is being closed.
(Fortunately the console service has recursive error detection.)

--
Warning: May contain traces of nuts.

[Robert Kaiser]

Justin Dolske schrieb:

> It's a big enough feature that we should just require it in all Gecko
> apps, even if it's a simple stub service that just always says "nope,
> not in PB mode!".

I think that's fine if the implementation actually lives in core code
(not the UI, but all the backend support). I'm not sure this is the case
right now, but it seems to be a planned part of those changes now
anyhow. Will certainly make a whole lot of things easier!

Robert Kaiser

[Justin Wood (Callek)]

Josh Matthews wrote:
> On 12-03-13 5:47 AM, Justin Wood (Callek) wrote:
>> Josh Matthews wrote:
>>> Per-window private browsing is coming
>>> (https://wiki.mozilla.org/Per-window_Private_Browsing), and certain
>>> things in the platform require changing to support the new architecture.
>>> Since the download manager is shared among other products, I've been
>>> asked to start a conversation about my proposed changes so that
>>> interested parties can provide input.
>>
>> Just as a point of reference, Private Browsing is currently disableable,
>> (build config for core/shared components anyway) and SeaMonkey does not
>> currently ship with Private Browsing turned on at all.
>
> This might be changing. The actual PB-per-window infrastructure is
> moving into core docshell code now, and I haven't bothered to include
> any kind of compile-time disabling so far. This is perhaps something
> that should be discussed, I guess?

Actually sounds good. If the PB infra moves to core code, and everything
assumes non-PB at least when other apps don't initialize any PB stuff
directly, we'll be great. And with it in core SeaMonkey can implement it
MUCH easier.

>>> 1) addDownload grows an extra optional parameter, nsILoadContext
>>> context (NB. nsILoadContext contains a boolean attribute
>>> usingPrivateBrowsing). If present, the usingPrivateBrowsing attribute on
>>> this interface controls whether the download should reside in the
>>> private, in-memory database or the disk-based non-private database. If
>>> not present, the download is treated as public.
>>>
>>> 1b) Mardak has suggested passing an isPrivate boolean instead.
>>>
>>
>> I support either of the above, preferably optional.
>>
>>> 1c) Paolo suggests that we break the API for existing consumers (by
>>> renaming or throwing an exception) to ensure that nobody unintentionally
>>> uses the new API (defaulting to public) without considering whether the
>>> download should be private.
>>
>> I strongly disagree with this, at least in the "core has private
>> browsing disabled" but if this happens when private browsing is on, and
>> not when off, then we also have API conflict and thats worse.
>>
>> So *IFF* this is the decided on option, please do an assert here that we
>> pass in a decision to have public downloads rather than private (because
>> we don't have private browsing)
>
> I don't think I understand what you're asking for. I don't think we want
> to condition the API-breakage based on whether PB is enabled at compile
> time or not.

Basically I agree that we don't want API breakage conditioned on if PB
is enabled or not, I just was hoping to error on the side of no API
breakage where reasonable, or break but make it easy to maintain current
API useage designs (esp in the case of no PB service). If that makes
sense...

Fair, I was mostly trying to say, "If the download window shows a
download, the cleanup button should clean it" Which covers the case of
allowing a per-window PB-Aware-download manager clean up all the public
downloads in it, and any private downloads it can be aware of, but
*another* windows private downloads not cleaned. [hope that is more clear]

>>> There have been mentions of either exposing a matching database
>>> connection for private downloads, or removing the existing database
>>> connection in the API and exposing any required API to provide needed
>>> functionality. I haven't looked into this at all.
>>
>> Do we feel we need to make private downloads VERY hard to get at for
>> extensions? Or is it better to assume an extension can already get this
>> data and thus no needto split the database connection api here. [I
>> really don't know, just posing the thought]
>
> It's not a matter of splitting the database connection. The downloads
> are stored in separate databases with separate connections.

Ahh ok, agreed so if they are stored separate, separate connections are
fine. I just desire that this connection/data split is handled purely by
the download manager backend, and download items returned by it come for
free without needing to manually join the lists in the manager. But I
guess that approaches even further into impl detail that is separate
from where this discussion needs to be

> Thanks for your comments!

No Problem

--
~Justin Wood (Callek)

[Justin Wood (Callek)]

Ehsan Akhgari wrote:
> On Tue, Mar 13, 2012 at 12:38 PM, Josh Matthews <jo...@joshmatthews.net>wrote:
>
>> On 12-03-13 5:47 AM, Justin Wood (Callek) wrote:
>>
>>> Josh Matthews wrote:
>>>
>>>> Per-window private browsing is coming
>>>> (https://wiki.mozilla.org/Per-**window_Private_Browsing<https://wiki.mozilla.org/Per-window_Private_Browsing>),
>>>> and certain
>>>> things in the platform require changing to support the new architecture.
>>>> Since the download manager is shared among other products, I've been
>>>> asked to start a conversation about my proposed changes so that
>>>> interested parties can provide input.
>>>>
>>>
>>> Just as a point of reference, Private Browsing is currently disableable,
>>> (build config for core/shared components anyway) and SeaMonkey does not
>>> currently ship with Private Browsing turned on at all.
>>>
>>
>> This might be changing. The actual PB-per-window infrastructure is moving
>> into core docshell code now, and I haven't bothered to include any kind of
>> compile-time disabling so far. This is perhaps something that should be
>> discussed, I guess?
>>
>
> This is actually not true today. There is no build time option to disable
> support for PB.

I stand corrected, and I blame incorrect memory. I did know for sure
that there was core code that properly dealt with Private Browsing not
being implemented/ported to SeaMonkey.

> I don't have any interest in converting this sort of check into a build
> time option at this time, so I think this issue is moot.

Fair enough, as long as the status-quo of "we don't do anything special
to support PB mode" is maintained (at least short-term)

> (Also note that I've heard some SM folks expressing interest in supporting
> PB mode, IIRC the reason that has not happened was that nobody had the time
> to work on it.)

And *of course* we want PB mode support, it just has not been a large
priority yet, and that due to our relative time available to commit to
things.

>> 3) activePrivateDownloads and activePrivateDownloadCount are added
>>>> which correspond with activeDownloads and activeDownloadCount.
>>>>
>>>
>>> Please key these off the "Is private Browsing compiled" build-config
>>> stuff, and add throwing(?) stub methods for them.
>>>
>>
> There is no build config, and I don't think we want different IDLs in
> different build configurations anyway. I think these should exist on
> nsIDownloadManager, and just do the right thing where the PB service does
> not exist (i.e., pretend there is no private downloads). This way I could
> write a DM UI extension which would work in both Firefox and SeaMonkey for
> example.

That would work fine for my case as well.

>>> removeDownloadsByTimeframe and cleanUp would continue to only operate on
>>>> public downloads. Private downloads are transient enough that I don't
>>>> think we need time-based APIs for removing them.
>>>>
>>>
>>> IMO both these should also work on private downloads, having the
>>> separation just because one window/download is private temporarily is a
>>> bad idea, imo.
>>>
>>
>> The problem here is that the existing modal Firefox download manager
>> window really can only show public downloads. Making the clean up button
>> inside of it also clean up private downloads sounds surprising and not
>> user-friendly.
>>
>
> Hmm, I thought that we're going to block this work until Paolo's per-window
> DM UI lands?
>
> Regardless, I think removeDownloadsByTimeframe should work on both download
> types, as it is designed for use cases such as the Clear Recent History
> dialog. cleanUp however should only work on non-private downloads, and we
> should add an alternate method for cleaning up private downloads.

As I said, if a download is visible in a window, I do feel cleanUp
button should affect all "visible" downloads, private or otherwise. And
not make private ones extra-special. (But I agree if the download is NOT
visible, cleanUp killing those off is unexpected)

> Also, Callek, please note that nothing makes private windows temporary,
> really. A user may keep both types of windows open for the entire duration
> of the browsing session.

assumption confirmed.

--
~Justin Wood (Callek)

[Ehsan Akhgari]

A large part of the implementation already lives in core code. I think
that this time around, we will do the right thing and will put everything
related to the service itself which does not involve UI stuff in toolkit/
and will put the rest in browser/. This way, SeaMonkey, Fennec or other
apps which are interested in this will only need to implement the UI parts
if they choose to support private browsing mode.

--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Wed, Mar 14, 2012 at 1:24 AM, Justin Dolske <dol...@mozilla.com> wrote:

> On 3/13/12 2:47 AM, Justin Wood (Callek) wrote:
>
> Just as a point of reference, Private Browsing is currently disableable,
>> (build config for core/shared components anyway) and SeaMonkey does not
>> currently ship with Private Browsing turned on at all.
>>
>
> I think we should change this -- it's already caused enough headaches for
> code that has to determine both if PB is supported, and if it's enabled.
>

Like I said before, this has never been the case. There is no way to
disable PB at build time. The core code just tries to grab the
nsIPrivateBrowsingService, and if that fails, it will assume non-private.

[Robert Kaiser]

Ehsan Akhgari schrieb:

> A large part of the implementation already lives in core code. I think
> that this time around, we will do the right thing and will put everything
> related to the service itself which does not involve UI stuff in toolkit/
> and will put the rest in browser/. This way, SeaMonkey, Fennec or other
> apps which are interested in this will only need to implement the UI parts
> if they choose to support private browsing mode.

Awesome.

Robert Kaiser

[Justin Wood (Callek)]

Ehsan Akhgari wrote:
>> We could keep compatibility with the current API, also when per-window
>> > PBM is enabled. But, unless we conclude that we can mix private-mode and
>> > non-private-mode downloads in the same list (thoughts on this?),
>
> We should not do that at all, ever! :-)

IMO a user wanting to view "downloaded files" in a "PB Mode Enabled
Window" Might expect to see downloads from other parts of his session,
[at least where the download is considered "public"].

Even if this is anti-climactic to what Firefox proper wishes to expose
in the UI, I would certainly expect an addon to be able to do this sort
of UI without too much jumping-through-hoops.

Please consider that potential in the API design/constructs. (and hence
why I suggested about the cleanUp stuff).

--
~Justin Wood (Callek)

[Paolo Amadini]

On 13/03/2012 22.18, Ehsan Akhgari wrote:
>> We know that we need to change the API to accommodate per-window private
>> browsing, and I'd also add that we're planning to improve the Download
>> Manager back-end performance by making it asynchronous. This probably
>> requires some API changes as well, so this is a good time to discuss.
>>
> I think these are separate discussions, but anyways. ;-)

The relationship is about compatibility, i.e. how much similarity we
should have between the new API and the current one. It is unnecessary
to do additional work to make a perfectly compatible API for per-window
PBM, if we need a substantially different API when we go asynchronous.

> Please note that the in-tree consumers don't really need backwards
> compatibility. We can change them all (including the comm-central ones.)
> But we should make sure that we don't do a dumb thing which can make the
> lives of Firefox/SM developers hard in the future...

True!

> In the interest of making sure that the thread continues with the correct
> assumptions, PB is not disableable at build time

Thanks.

>> To check that the consumer is aware of whether the window from which
>> the download is initiated is in private mode or not, we should break
>> the ability to call the current form of addDownload, at least in cases
>> where per-window PBM is enabled. I've not thought about nsITransfer
>> but we should probably do the same there.
>>
>> We can choose to keep the current API available as-is for the
>> "no private browsing" or "global private browsing" cases.
>
> Well, as I said before, it takes more than just this to make sure the
> add-ons support the PB mode. :/

Sure, that's not under discussion. But there are simple pitfalls that we
can avoid with little effort. Take the case of a simple add-on that only
adds a "quick download" toolbar button calling addDownload, and
currently supports PBM perfectly. With per-window PBM, the same add-on,
unchanged, can cause a data leak if not updated.

But in this case it's easy to do the right thing, to prevent that add-on
authors, through inaction, allow users to come to harm. We can simply
block the current form of addDownload when we don't know whether the
operation could have been initiated by a private-mode window, unless
the caller provides an nsILoadContext, a boolean PBM flag, or similar.

Cheers,
Paolo

[Ehsan Akhgari]

Yeah, for sure. I don't see why add-ons can't do this.


--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Thu, Mar 15, 2012 at 5:06 AM, Paolo Amadini <paolo....@amadzone.org>wrote:

> On 13/03/2012 22.18, Ehsan Akhgari wrote:
> >> We know that we need to change the API to accommodate per-window private
> >> browsing, and I'd also add that we're planning to improve the Download
> >> Manager back-end performance by making it asynchronous. This probably
> >> requires some API changes as well, so this is a good time to discuss.
> >>
> > I think these are separate discussions, but anyways. ;-)
>
> The relationship is about compatibility, i.e. how much similarity we
> should have between the new API and the current one. It is unnecessary
> to do additional work to make a perfectly compatible API for per-window
> PBM, if we need a substantially different API when we go asynchronous.
>

OK.

Hmm, I see what you mean now. Whether or not we should break APIs like
addDownload is really something for the module owner to decide... :/