Hamed Zaghaghi Reporter
	Description • 13 years ago

User Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.18 Safari/534.10

Steps to reproduce:

We created a patch that introduce a new interface, imgIContentPolicy, so that it's implementations can evaluate every image after decode was done.
With this feature addon developers can produce adult content filtering addons. they can clear image surface if it contains adult contents.

	Josh Matthews [:jdm]
	Comment 1 • 13 years ago

Joe, I suspect this ball's in your court.

	Daniel Holbert [:dholbert]
	Comment 4 • 13 years ago

Comment on attachment 570546 [details] [diff] [review]
diff file for this patch

Drive-by nit (mostly stylistic / spacing):

>+++ b/modules/libpr0n/public/imgIContentPolicy.idl

modules/libpr0n/ no longer exists in trunk -- it's been moved to /image.

>+interface imgIContentPolicy : nsISupports
>+{
>+	const short ACCEPT = 1;
>+	const short REJECT = -1;
>+	short shouldShow( in imgIContainer image );
>+  boolean shouldDisableProgressiveShow();
>+	[noscript] void filter( in imgFrame frame );

The Indentation is inconsistent here.  Other imgIContainer IDLs use 2-space indent, so it looks like the 'boolean' line here is correctly-indented & the other lines should be aligned with it.

(While you're at it, might as well nix the spaces adjacent to the parenthesis, to fit in with mozilla coding style.)

>+++ b/modules/libpr0n/src/RasterImage.cpp
>@@ -197,17 +199,18 @@ RasterImage::RasterImage(imgStatusTracke
[...]
>   mWorkerPending(PR_FALSE),
>   mInDecoder(PR_FALSE),
>-  mAnimationFinished(PR_FALSE)
>+  mAnimationFinished(PR_FALSE),
>+  mImageContentPolicyComponent(nsnull)

There was a recent s/PR_FALSE/false/ tree-wide conversion -- this chunk will need bitrot-fixing for that in order to apply correctly.

>@@ -290,16 +293,23 @@ RasterImage::Init(imgIDecoderObserver *a
>+  // Instantiate the content policy component if exists
>+  if (!mMultipart && mURIString.Find("chrome", true)!=0 && mURIString.Find("resource", true)!=0) {

Add spaces around "!=" (elsewhere too)

>+    nsresult rv;
>+    mImageContentPolicyComponent = do_CreateInstance(IMGCONTENTPOLICY_ContractID, &rv);
>+    if(NS_FAILED(rv))

Add space after "if" (elsewhere too)

>+  if (hasContentPolicyComponent()) {
>+    PRInt16 show = false;

PRInt16 initialized to a boolean value (and then later checked against an enum value)? this looks incorrect.

>+    mImageContentPolicyComponent->ShouldShow(this, &show);
>+    if(show == imgIContentPolicy::REJECT) {
>+      imgFrame* frame = GetImgFrame(0);
>+      frame->LockImageData();
>+      mImageContentPolicyComponent->Filter(frame);
>+      frame->UnlockImageData();
>+	  }

That } is misindented

>@@ -2682,17 +2702,22 @@ imgDecodeWorker::Run()
>+  if(image->hasContentPolicyComponent() && !image->isSourceDataCompleted()){

Add space after "if" and before "{"

>+    PRBool rv;

rv is a semi-reserved variable-name, used only for 'nsresult'. Call this "success" or something.
Also, this should be "bool" now instead of PRBool. (recent treewide conversion)

>+    image->mImageContentPolicyComponent->ShouldDisableProgressiveShow(&rv);
>+    if(rv==PR_TRUE)

Add space after "if", and drop the "PR_TRUE" check -- you can just directly check "if (myBool)" instead of "if (myBool == true)"

>diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h
>+  bool hasContentPolicyComponent() { return !mAnim && mImageContentPolicyComponent!=nsnull; }
>+  bool isSourceDataCompleted() { return mHasSourceData; }

Mozilla style uses capitalized function-names.

>@@ -524,16 +527,18 @@ private: // data
>   // Decoding
>   nsresult WantDecodedFrames();
>   nsresult SyncDecode();
>   nsresult InitDecoder(bool aDoSizeDecode);
>   nsresult WriteToDecoder(const char *aBuffer, PRUint32 aCount);
>   nsresult DecodeSomeData(PRUint32 aMaxBytes);
>   PRBool   IsDecodeFinished();
> 
>+  // content-policy for image data
>+  nsCOMPtr<imgIContentPolicy>	mImageContentPolicyComponent;
>   // Decoder shutdown
>   enum eShutdownIntent {

Looks like this chunk wants a newline after it

	Daniel Holbert [:dholbert]
	Comment 5 • 13 years ago

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Mozilla style uses capitalized function-names.

(in C++, that is)

	Hamed Zaghaghi Reporter
	Comment 6 • 13 years ago

I made the patch agaist mozilla-central, Should I do that against mozilla-aurora?

	Dave Garrett
	Comment 7 • 13 years ago

(In reply to Hamed Zaghaghi from comment #6)
> I made the patch agaist mozilla-central, Should I do that against
> mozilla-aurora?

New functionality goes to mozilla-central then gets to Aurora with the next merge (which is only in 9 days anyway).

	Hamed Zaghaghi Reporter
	Comment 9 • 13 years ago

In previous one I forgot to update Makefile.in.

	Hamed Zaghaghi Reporter
	Comment 10 • 13 years ago

Sorry for multiple uploads.