Last Comment Bug 380556 - nsIContentPolicy.ShouldProcess is useless
: nsIContentPolicy.ShouldProcess is useless
Status: NEW
:
Product: Core
Classification: Components
Component: Image Blocking (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 714699 (view as bug list)
Depends on:
Blocks: abp
  Show dependency treegraph
 
Reported: 2007-05-13 04:43 PDT by Wladimir Palant
Modified: 2014-06-29 18:34 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Wladimir Palant 2007-05-13 04:43:10 PDT
I was asked today what nsIContentPolicy.ShouldLoad 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 ShouldLoad 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 ShouldLoad to justify calling it everywhere. Please comment.
Comment 1 Wladimir Palant 2007-05-13 04:45:14 PDT
I forgot the third possible solution: Remove TYPE_REFRESH and fix the documentation to tell what this feature is really good for.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2007-05-13 09:33:12 PDT
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?
Comment 3 Wladimir Palant 2007-05-13 10:10:39 PDT
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).
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2007-05-13 11:38:13 PDT
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)...
Comment 5 shawn.sumin 2011-03-31 16:06:16 PDT
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
Comment 6 John Schoenick [:johns] 2012-08-10 17:21:55 PDT
*** Bug 714699 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.