RequestPolicy/tractest — Issue #191: Code Consolidation -- Allow/Deny Origin/Destination/Origin->Destination
Author: jsamuel
Created: 2011-12-22 02:45:57+00:00
State: open
Labels: extension

Reviewing the source for #190, found the following:

'''./components/requestpolicyService.js'''
function allowDestinationDelayStore(host) {
function allowDestination(host) {
function allowOriginDelayStore(host) {
function allowOrigin(host) {
function allowOriginToDestinationDelayStore(
function allowOriginToDestination(
function forbidDestinationDelayStore(host) {
function forbidDestination(host) {
function forbidOriginDelayStore(host) {
function forbidOrigin(host) {
function forbidOriginToDestinationDelayStore(
function forbidOriginToDestination(
function isAllowedDestination(host) {
function isAllowedOrigin(host) {
function isAllowedOriginToDestination(
function isTemporarilyAllowedDestination(
function isTemporarilyAllowedOrigin(host) {
function isTemporarilyAllowedOriginToDestination(
function registerAllowedRedirect(originUrl,
function temporarilyAllowDestination(host) {
function temporarilyAllowOrigin(host) {
function temporarilyAllowOriginToDestination(

It appears each of the 22 functions above is implemented separately. Seems code consolidation would be appropriate here, eg:

allow(contenttype, destination, origin, context, mimetype, extra);
deny(contenttype, destination, origin, context, mimetype, extra);
isallowed(contenttype, destination, origin, context, mimetype, extra);
isdenied(contenttype, destination, origin, context, mimetype, extra);

Above directly match [https://developer.mozilla.org/en/nsIContentPolicy nsIContentPolicy] shouldLoad() and shouldProcess(). Passing null for unused fields would ensure that we can add support for all shouldLoad enums later (without re-factoring), and still maintain current functionality. Yes, it is more fields than simply:

allow(origin, destination, temporary, asRedirect);
deny(origin, destination, temporary, asRedirect);
isallowed(origin, destination, temporary, asRedirect);
isdenied(origin, destination, temporary, asRedirect);

However, it seems more future-proof given there are already feature requests for allow/deny in different contexts that are already supported by shouldLoad() enums, eg asRedirect versus the shouldProcess() TYPE_REFRESH aContentType, and those appear to have been added as yet additional functions...

Alternately (and better yet), we can pass everything as an object (to eliminate the need for a lot of nulls), eg:

let content = {
'contenttype': contenttype,
'destination': destination,
'origin': origin,
'context': context,
'mimetype': mimetype,
'extra': extra,
'istemporarypolicy': istemporarypolicy
};

allow(content);
deny(content);
isallowed(content);
isdenied(content);

Still supports all fields of shouldLoad() and shouldProcess() and is quite a bit more transparent than a string of nulls...

Such consolidation should help to ensure that this basic functionality doesn't break when developing patches for, say, #8. Initially only found the basic 6 setters (origin, destination, origin->destination for temporary and permanent) until I grepped and found '''many''' more... Further, this would require significant code duplication as-is for enhancements, eg #8.

To further the case for this bug, it looks like I missed one even with grep. "registerAllowedRedirect" does not appear to have a corresponding "'''un'''register'''Previously'''{{{AllowedRedirect"}}} (that is denyRedirect) due to its not following the established naming scheme...

This code consolidation should also help with #159, as at least the code would be a bit more transparent, especially with the content object form...