Jan de Mooij [:jandem] Assignee
	Description • 6 years ago

Bug 1514210 exposed a few things here.

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 6 years ago

The code currently accesses it using getProperty("backgroundInterval"), but it
really is just a property like the others so we should define and use it like
that. This is needed for the next patch.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 6 years ago

If we QI nsIPropertyBag on an XPCWrappedNative wrapping an XPCWrappedJS, calling
the getProperty method might incorrectly end up calling .getProperty on the
XPCWrappedJS itself, because it also implements nsIPropertyBag.

This became a problem with same-compartment chrome globals because if we no longer
cross a compartment boundary, we don't create a new WrappedNative and can now end up
seeing the nsIPropertyBag if it got queried before nsIWritablePropertyBag.

Depends on D15076

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 6 years ago

I'm happy to remove the backgroundInterval property completely, but do you mind explaining why we can do this now? Then I can add that to the commit message :)

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 6 years ago

Also what should this code look like? Do we always want to use DOWNLOAD_FOREGROUND_INTERVAL or should we keep the ternary somehow?

https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/nsUpdateService.js#3556-3557

	Robert Strong (they/them - no direct email)
	Comment 5 • 6 years ago

(In reply to Jan de Mooij [:jandem] from comment #4)
> Also what should this code look like? Do we always want to use
> DOWNLOAD_FOREGROUND_INTERVAL or should we keep the ternary somehow?
> 
> https://searchfox.org/mozilla-central/rev/
> 9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/
> nsUpdateService.js#3556-3557
Remove both
const DOWNLOAD_BACKGROUND_INTERVAL  = 600; // seconds
const DOWNLOAD_FOREGROUND_INTERVAL  = 0;

and for the code here
https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/nsUpdateService.js#3556-3557

Just set interval to 0 with a comment that "The interval is 0 since there is no need to throttle downloads." or something similar.

	Robert Strong (they/them - no direct email)
	Comment 6 • 6 years ago

(In reply to Jan de Mooij [:jandem] from comment #3)
> I'm happy to remove the backgroundInterval property completely, but do you
> mind explaining why we can do this now? Then I can add that to the commit
> message :)
For the commit message something like
Removed backgroundInterval. It was added in case there were CDN issues with downloading unthrottled and since there haven't been any issues it is no longer needed.

backgroundInterval was originally added because we overloaded the download mirrors (provided mainly by universities) we had way back when and at some point we changed to using the CDN we have today. We wanted to move to an interval of 0 to improve update rates but there were some concerns that this might have issues as well so we went with the ability to configure it via the update advertisement. This is why it was added to the update xml provided by the update server. Since there haven't been any issues we got the ok to remove it and go with a value of 0. The plan was to remove it when the code was rewritten to use standard networking code but that isn't resourced as of yet.

	Robert Strong (they/them - no direct email)
	Comment 7 • 6 years ago

If you want to just remove it from nsIUpdate. Then we'll remove it entirely when the networking code is changed to use standard networking code instead of nsIIncrementalDownload

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 6 years ago

Ok thanks. I had already written the patch to remove it earlier today so I just submitted that with some tweaks based on your suggestions.

	Pulsebot
	Comment 9 • 6 years ago

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dab6389dca55
part 1 - Remove backgroundInterval from nsIUpdate. r=rstrong
https://hg.mozilla.org/integration/autoland/rev/74bbbdf69467
part 2 - QI nsIWritablePropertyBag instead of nsIPropertyBag on nsIUpdate. r=rstrong

	Arthur Iakab [arthur_iakab]
	Comment 10 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dab6389dca55
https://hg.mozilla.org/mozilla-central/rev/74bbbdf69467