259084 - review comments on bug 173010 (protocol whitelist)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

spun off from bug 173010 comment 30.

 ------- Additional Comment #30 From Christian Biesinger (:bi)  2004-09-12 13:01
PDT  [reply] -------

(From update of attachment 158476 [details] [diff] [review])
Index: mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp

+    PRBool PromptForScheme(nsIURI *aURI, nsACString& aScheme);

since aScheme seems to be an in parameter it should be const. but the scheme
could be gotten off the URI too, why pass it here?

+  NS_ENSURE_ARG_POINTER(aNotificationCallbacks);

this should not be a runtime check, since people who pass NULL to a get
function totally deserve what they get. also, scripts can't do that.

+  return NS_OK;       // don't fail when trying to set this

the comment seems to be pointless now that this function actually does
something

+static void *PR_CALLBACK handleExtProtoEvent(PLEvent *event)
+{
+  nsExtProtocolChannel *channel =
+      NS_STATIC_CAST(nsExtProtocolChannel*, PL_GetEventOwner(event));
+
+  if (channel)

when can this be null? same in destroyExtProtoEvent


Two more issues:
1. the code asserts that it, being a channel, has
notificationCallbacks set, and that it can get an nsIPrompt off
them...  but necko does not require to have an nsIPrompt available on the
notificationCallbacks, this should be a NS_WARNING.

2. channels are supposed to try to get interfaces off the
loadgroup's notificationCallbacks if they can't find them on the channel

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160789 [details] [diff] [review]
patch

>Index: uriloader/exthandler/nsExternalProtocolHandler.cpp
>   mCallbacks->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));

Make this type-safe, like so:

  CallGetInterface(mCallbacks, getter_AddRefs(prompt));

r=bzbarsky with that.

I guess the part about getting nsIPrompt off the loadgroup is covered by
darin's patch in that other bug?

Comment 3

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

yes, it is.

Comment 4

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

(In reply to comment #2)
>   CallGetInterface(mCallbacks, getter_AddRefs(prompt));

/home/chb/mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp:182: error: no
   matching function for call to `CallGetInterface(
   nsCOMPtr<nsIInterfaceRequestor>&, nsGetterAddRefs<nsIPrompt>)'

a .get() doesn't really help much.

bah, this works:
   nsCOMPtr<nsIPrompt> prompt = do_GetInterface(mCallbacks);

Comment 5

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

Checking in uriloader/exthandler/nsExternalProtocolHandler.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp,v  <-- 
nsExternalProtocolHandler.cpp
new revision: 1.25; previous revision: 1.24
done