379819 - Use protocol flags as a replacement for the accepted protocol list in bug 181860

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ryan Jones-Ward [:sciguyryan]]

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.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Like so?

Patch v1 - Like so?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.2

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

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.3

Patch v1.3

Updated to latest trunk.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

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

What about irc:?

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.4

Patch v1.4 (Now with IRC added)

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in.