If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: sciguyryan, Assigned: sciguyryan)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 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.
(Assignee)

Comment 1

11 years ago
Created attachment 263856 [details] [diff] [review]
Like so?

Patch v1 - Like so?
Attachment #263856 - Flags: review?(bzbarsky)
Blocks: 379804
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.
Attachment #263856 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 3

11 years ago
Created attachment 263937 [details] [diff] [review]
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.
Attachment #263856 - Attachment is obsolete: true
Attachment #263937 - Flags: superreview?(cbiesinger)
Attachment #263937 - Flags: review?(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
Attachment #263937 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 263943 [details] [diff] [review]
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
Attachment #263937 - Attachment is obsolete: true
Attachment #263943 - Flags: superreview?(cbiesinger)
Attachment #263937 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 6

10 years ago
Created attachment 268685 [details] [diff] [review]
Patch v1.3

Patch v1.3

Updated to latest trunk.
Attachment #263943 - Attachment is obsolete: true
Attachment #268685 - Flags: superreview?(cbiesinger)
Attachment #263943 - Flags: superreview?(cbiesinger)
Comment on attachment 268685 [details] [diff] [review]
Patch v1.3

What about irc:?
Attachment #268685 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 8

10 years ago
Created attachment 273785 [details] [diff] [review]
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!
Attachment #268685 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #273785 - Flags: review?(bzbarsky)
Comment on attachment 273785 [details] [diff] [review]
Patch v1.4

r=bzbarsky.
Attachment #273785 - Flags: review?(bzbarsky) → review+
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 167475
You need to log in before you can comment on or make changes to this bug.