Closed Bug 380556 Opened 17 years ago Closed 2 months ago

nsIContentPolicy.ShouldProcess is useless

Categories

(Core :: Graphics: Image Blocking, defect)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1447256

People

(Reporter: jwkbugzilla, Unassigned)

References

Details

I was asked today what nsIContentPolicy.ShouldProcess does and I found it really difficult to answer. The comment in the interface says:

   ShouldProcess will be called once all the information passed to it has
   been determined about the resource, typically after part of the resource
   has been loaded.

In my understanding that would mean that any ShouldLoad call should have a corresponding ShouldProcess call once the load started and more information is available. So much about theory. However, from what I see there is exactly one caller for ShouldProcess in the code base (nsImageDocument) and zero consumers (all consumers I could find either always accept or delegate the call to ShouldLoad). Not even the documented ShouldProcess use case (TYPE_REFRESH) works, there are no content policy calls for META Refresh.

I see two possible courses of action:

1. Fix up TYPE_REFRESH (should it be fixed?), fix bug 305699 and bug 309524, add a few dozen ShouldProcess calls in other places as well to make this feature somewhat consistent/useful.

2. Remove TYPE_REFRESH and ShouldProcess altogether and stop pretending to have a feature that is plainly broken.

I tend to choose the second solution since there doesn't seem to be enough use cases for ShouldProcess to justify calling it everywhere. Please comment.
I forgot the third possible solution: Remove TYPE_REFRESH and fix the documentation to tell what this feature is really good for.
Did you mean "ShouldProcess" most of the times you typed "ShouldLoad" in comment 0 and the summary?

The right solution is whatever the consumers want, really.  I'd go with #1, myself, but if that wouldn't be useful we could consider removing it instead...

Part of the issue for me is that content policy calls are really quite expensive (in that the policies do things that take a _lot_ of time during pageload).  Doubling that stuff by also adding ShouldProcess calls is not that appealing in some ways; I would be happier if we had a faster solution somehow.  :(

Perhaps if more of the built-in content policies made their ShouldProcess impls no-ops instead of synonyms for ShouldLoad?
Ouch, how did that happen? Yes, you are right, I somehow mistyped this several times and didn't notice it through several rounds of proof-reading :-(

Yes, performance is an issue. It would help if consumers could subscribe to ShouldLoad and ShouldProcess separately, that would save quite some time calling through XPConnect when dealing with JavaScript implementations.

But how would ShouldProcess be useful? According to bug 309524 we can correct the mime type guess once the download started. That's useful when dealing with objects and documents/frames, for almost everything else the mime type is irrelevant however. TYPE_REFRESH and bug 305699 are for things where ShouldLoad doesn't get called because nothing gets loaded - that's an entirely different concept already (one that makes sense but would require changes in documentation).
Summary: nsIContentPolicy.ShouldLoad is useless → nsIContentPolicy.ShouldProcess is useless
It would mostly be useful for cases where ShouldLoad is never called (inline stylesheets, inline scripts, meta refresh) and for cases where ShouldLoad doesn't know what you're loading (iframes and objects)...
Blocks: abp
Flags: blocking1.9.0.19?
Flags: blocking1.9.0.19?
The Tor Project / Electronic Frontier Foundation is paying to have this bug
fixed.

"If you know C++ and/or Firefox internals, we should be able to pay you for
your time to address these issues and shepherd the relevant patches through
Mozilla's review process."

Source:
https://blog.torproject.org/blog/web-developers-and-firefox-hackers-help-us-firefox-4
Severity: normal → S3
Depends on: 1873474
Status: NEW → RESOLVED
Closed: 2 months ago
No longer depends on: 1873474
Duplicate of bug: 1447256
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.