Ryan Jones-Ward [:sciguyryan] Assignee
	Description • 18 years ago

According to Boris we should really use a protocol flag instead of an accepted protocol list as it allows extension authors to set their protocols with the flag also.

	Ryan Jones-Ward [:sciguyryan] Assignee
	Comment 1 • 18 years ago

Patch v1 - Like so?

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 18 years ago

Comment on attachment 263856 [details] [diff] [review]
Like so?

>Index: content/base/src/nsNoDataProtocolContentPolicy.cpp
>-    if (scheme.EqualsLiteral("http") ||
>-        scheme.EqualsLiteral("https") ||

I think you do want to leave this list, as an optimization...  But add a comment to that effect?

>     nsIIOService* ios = nsContentUtils::GetIOService();

Is this still used?  If not, why is it here?

>Index: netwerk/base/public/nsIProtocolHandler.idl
>+     * Channels from this protocol should not be loaded
>+     * without user interaction from content such as web pages.

Uh... no.  I think we want a comment more like "channels for this protocol never call OnDataAvailable on the listener passed to AsyncOpen" or something like that...  Check with biesi?

>+    const unsigned long URI_NO_RETURN_DATA = (1<<11);

I might prefer URI_DOES_NOT_RETURN_DATA, but again check with biesi.

The rest looks ok.

	Ryan Jones-Ward [:sciguyryan] Assignee
	Comment 3 • 18 years ago

Lets try this again; Patch v1.1

(In reply to comment #2)
> I think you do want to leave this list, as an optimization...  But add a
> comment to that effect?

Done.

> Is this still used?  If not, why is it here?

It seems it's no longer needed. Removed.

> Uh... no.  I think we want a comment more like "channels for this protocol
> never call OnDataAvailable on the listener passed to AsyncOpen" or something
> like that...  Check with biesi?
> 

Fixed.

> I might prefer URI_DOES_NOT_RETURN_DATA, but again check with biesi.
> 

Fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 18 years ago

Comment on attachment 263937 [details] [diff] [review]
Patch v1.1

>Index: content/base/src/nsNoDataProtocolContentPolicy.cpp

> +                                      nsIProtocolHandler::URI_NO_RETURN_DATA,

That's not going to compile now...

>+    if (NS_FAILED(rv)) {
>+      shouldBlock = PR_FALSE;
>     }
>+    if (shouldBlock) {
>       *aDecision = nsIContentPolicy::REJECT_REQUEST;
>     }

Just do

  if (NS_SUCCEEDED(rv) && shouldBlock) 

instead, ok?

With those fixed, r=bzbarsky

	Ryan Jones-Ward [:sciguyryan] Assignee
	Comment 5 • 18 years ago

Fixed the errors pointed to by bz above.

Could have sworn I changes |nsIProtocolHandler::URI_NO_RETURN_DATA| too |nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA| in nsNoDataProtocolContentPolicy.cpp

	Ryan Jones-Ward [:sciguyryan] Assignee
	Comment 6 • 18 years ago

Patch v1.3

Updated to latest trunk.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 7 • 17 years ago

Comment on attachment 268685 [details] [diff] [review]
Patch v1.3

What about irc:?

	Ryan Jones-Ward [:sciguyryan] Assignee
	Comment 8 • 17 years ago

Patch v1.4 (Now with IRC added)

Unless there any others I'm going to request checkin on this patch. Thanks for the review!

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 17 years ago

Comment on attachment 273785 [details] [diff] [review]
Patch v1.4

r=bzbarsky.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 17 years ago

Checked in.