Comment 78 for bug 294712

(From update of attachment 384802)
>+++ b/browser/base/content/test/image_Reload.html

You have Windows newlines here. Fix that, please.

>+++ b/extensions/permissions/nsContentBlocker.cpp

>+struct OverridePermissionEntry {
>+ OverridePermissionEntry(nsCString aHost, PRUint32 aContentType, PRInt16 aPolicy) :
>+ mContentType(aContentType),
>+ mPolicy(aPolicy)
>+ {
>+ mHost.Assign(aHost);

Why not just mHost(aHost) in the initializers?

>+ OverridePermissionEntry(const OverridePermissionEntry &entry) :
>+ mContentType(entry.mContentType),
>+ mPolicy(entry.mPolicy)
>+ {
>+ mHost.Assign(entry.mHost);

And mHost(entry.mHost) here?

Also, s/entry/aOther/ or some such?

>+ OverridePermissionEntry() {};

>+static nsTArray<OverridePermissionEntry> mOverridePermissionEntries;

This will show up as a leak. Why exactly do you need this as a static global instead of a member on nsContentBlocker?

>+static int FindPermission(nsIURI *aURI, PRUint32 aContentType)
....
>+ NS_ENSURE_SUCCESS(rv, -1);
...
>+ return mOverridePermissionEntries.IndexOf(temp);

IndexOf on nsTArray doesn't return |int|. It returns the relevant index_type. Make this function return nsTArray<OverridePermissionEntry>::index_type (and return NoIndex in the case when GetHost fails).

>+static int iprint = 0;

I have no idea what this is left over from, but presumably it should go away.

>+nsContentBlocker::IgnorePolicy(nsIURI *aURI,
>+ NS_ENSURE_ARG(aContentType);

I'm not sure why you're doing that, since you're not actually making sure it's a "valid" content type.

>+ if (!aNewDecision)
>+ aNewDecision = nsIContentPolicy::ACCEPT;

I don't like this part, but more on that below.

>+ nsCString host;
>+ nsresult rv = aURI->GetHost(host);
>+ NS_ENSURE_SUCCESS(rv, rv);

Did you actually test this? It looks like with your patch calling reloadImage on a data: image, say, will throw an exception on the ignorePolicy() call and that you don't bother catching that. I don't know whether you want to change this code or the caller, but one or the other needs changing. I should note that your IDL doesn't talk about the fact that this method will throw on all sorts of URIs...

>+ if (aReturn && *aReturn == aNewDecision)
>+ return NS_OK; // xxx: Should we override future policies?

Why exactly are we null-testing aReturn here? It can't be null if the caller is following the contract.

>+ const int loc = FindPermission(aURI, aContentType);
>+ if (loc == -1) {

Again, index_type. You might want to typedef the right type (e.g. the relevant nsTArray specialization type) for readability.

>+nsContentBlocker::RestorePolicy(nsIURI *aURI,
>+ NS_ENSURE_ARG(aContentType);

Again, not sure what that would even mean.

>+ const int loc = FindPermission(aURI, aContentType);
>+ if (loc != -1)
>+ mOverridePermissionEntries.RemoveElementAt(loc);

This looks bad to me. In particular if I ignorePolicy, then someone else does ignorePolicy, then they restorePolicy they'll restore both ignores. If that's not desirable, then we should either throw on nested ignore or properly have an ignore/restore stack or something.

I'm not sure why ignorePolicy and restorePolicy are returning "the current policy", given the above.

>+nsContentBlocker::IsPolicyOverriden(nsIURI *aURI,
>+ NS_ENSURE_ARG(aContentType);

Again, not sure what this means.

>+ if (aReturn)
>+ *aReturn = FindPermission(aURI, aContentType) != -1;

aReturn can't be null here And again, use the right types and NoIndex.

>@@ -264,6 +358,12 @@ nsContentBlocker::TestPermission(nsIURI

>+ const int loc = FindPermission(aCurrentURI, aContentType);
>+ if (loc != -1) {

Again, the right types and NoIndex.

>+ *aPermission = mOverridePermissionEntries[loc].mPolicy == BEHAVIOR_ACCEPT ? PR_TRUE :
>+ PR_FALSE;

No need for the whole "? PR_TRUE : PR_FALSE" part.

Also, shouldn't this compare to nsIContentPolicy::ACCEPT, since that's what the callers will be passing in?

It looks like overriding with REJECT_* is just ignored. Is that actually on purpose? If so, the API should reflect that, I would think.

>+++ b/extensions/permissions/nsIContentBlocker.idl

Nix the Windows newlines throughout this file.

>+ * @param aContentLocation The location for which to override content policies.
>+ * The policy is ignored for the domain of the site.

This should document what happens if the URI passed in doesn't have a meaningful "domain" (whatever that is; are these policies really per-host and not at least per hostport?)

>+ * @param aContentType The type of content for which to override the policy.

That's completely useless documentation. The right way to document something like this is to at least mention that the values are precisely the TYPE_* values of nsIContentPolicy or precisely the types one would pass to nsIContentPolicy::shouldLoad, or something.

>+ * @param aNewDecision What the new policy should be. If null, the policy
>+ * will be ACCEPT.

Similar here; document what the values here can actually be, preferably by reference to the canonical place where they're defined. @see is your friend.

>+ * @returns The current nsContentBlocker policy that is being overriden.

Probably clearer as "@returns the value content blocker policy had for this location and type before this ignorePolicy call". That is, don't mention the concrete implementation and make it clear when the value is measured.

Similar comments apply to the other functions on this interface, where applicable. And discuss the behavior if calls to ignore/restorePolicy are interleaved in various ways, ok?

>+ * @returns The current nsContentBlocker policy for this content/domain.

Here "current" means "after this call completes", right?