388701 - nsExternalHelperAppService::GetProtocolHandlerInfo should get OS Specific implementation

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Shawn Wilsher :sdwilsh]

Right now nsExternalHelperAppService::GetProtocolHandlerInfo returns an nsMIMEInfoImpl and not the OS specific version of the MIMEInfo.  We should add a GetProtocolInfoFromOS method to nsExternalHelperAppService that gets each OS's version.

Ideally, it would also fill in the default application description if it existed as well.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This was a bit easier than expected...

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273004 [details] [diff] [review]
v1.0

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
> [...]
>-  } // if we have a node in the graph for this content type
>-  // If we had success, but entry doesn't exist, we don't want to return success
>-  else if (NS_SUCCEEDED(rv)) {
>-    rv = NS_ERROR_NOT_AVAILABLE;
>+  }
>+  else if (NS_SUCCEEDED(rv) && !exists)
>+  {
>+      // We don't know it, so we always ask
>+      (void)aHandlerInfo->SetPreferredAction(nsIHandlerInfo::alwaysAsk);
>+      (void)aHandlerInfo->SetAlwaysAskBeforeHandling(PR_TRUE);
>   }

The resulting code flow ends up being difficult to read.  I think the conditionals want to be nested:

if (NS_SUCCEEDED(rv)) {
  if (exists) {
    ...
  } else { 
    ...
  }
}

Syntactic comments aside, this seems like sane behavior for the case where the OS doesn't already set the preferredAction.  However, for the MIME type case (at least on Windows), the OS does set such an action, so this will change behavior.  So I'd prefer it if you'd hoist the setter calls up out of this function into some of the protocol-specific, and add comments explaining this hoisting.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

sr=dmose
Also added comment as per discussion with him.

Comment 4

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

Comment on attachment 273182 [details] [diff] [review]
v1.1

+    (void)(*aHandlerInfo)->SetPreferredAction(nsIHandlerInfo::alwaysAsk);
+    (void)(*aHandlerInfo)->SetAlwaysAskBeforeHandling(PR_TRUE);

please don't cast to void.

(also in the other files)

I don't think we really use an alwaysAsk prefered action, just use useSystemDefault instead

beos/nsOSHelperAppService.cpp:
+  NS_ENSURE_SUCCESS(rv, handlerInfo);

it seems clearer to use nsnull here rather than handlerInfo

then you can also move the handlerInfo decl to where you use it

+  NS_ENSURE_TRUE(handlerInfo, handlerInfo);

here too

uriloader/exthandler/beos/nsOSHelperAppService.h
+  already_AddRefed<nsIHandlerInfo> GetProtocolInfoFromOS(const nsACString &aScheme);

wrong indentation

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

For checkin

(In reply to comment #4)
> uriloader/exthandler/beos/nsOSHelperAppService.h
> +  already_AddRefed<nsIHandlerInfo> GetProtocolInfoFromOS(const nsACString
> &aScheme);
> 
> wrong indentation
bah - they used tabs :(

Comment 6

[Shawn Wilsher :sdwilsh]

on trunk

Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.316; previous revision: 1.315
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
new revision: 1.82; previous revision: 1.81
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
new revision: 1.18; previous revision: 1.17
Checking in uriloader/exthandler/beos/nsOSHelperAppService.h;
new revision: 1.12; previous revision: 1.11
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
new revision: 1.48; previous revision: 1.47
Checking in uriloader/exthandler/mac/nsOSHelperAppService.h;
new revision: 1.17; previous revision: 1.16
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
new revision: 1.63; previous revision: 1.62
Checking in uriloader/exthandler/unix/nsOSHelperAppService.h;
new revision: 1.21; previous revision: 1.20
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
new revision: 1.68; previous revision: 1.67
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
new revision: 1.28; previous revision: 1.27

Comment 7

[Shawn Wilsher :sdwilsh]

on branch

Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.312.2.8; previous revision: 1.312.2.7
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
new revision: 1.80.4.2; previous revision: 1.80.4.1
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
new revision: 1.17.4.1; previous revision: 1.17
Checking in uriloader/exthandler/beos/nsOSHelperAppService.h;
new revision: 1.11.4.1; previous revision: 1.11
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
new revision: 1.46.4.2; previous revision: 1.46.4.1
Checking in uriloader/exthandler/mac/nsOSHelperAppService.h;
new revision: 1.16.4.1; previous revision: 1.16
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
new revision: 1.62.4.1; previous revision: 1.62
Checking in uriloader/exthandler/unix/nsOSHelperAppService.h;
new revision: 1.20.4.1; previous revision: 1.20
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
new revision: 1.67.4.1; previous revision: 1.67
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
new revision: 1.27.4.1; previous revision: 1.27

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> (From update of attachment 273182 [details] [diff] [review])
> +    (void)(*aHandlerInfo)->SetPreferredAction(nsIHandlerInfo::alwaysAsk);
> +    (void)(*aHandlerInfo)->SetAlwaysAskBeforeHandling(PR_TRUE);
> 
> please don't cast to void.

Why not?  Casting to void makes it clear to the reader that the return value is being ignored intentionally.

> I don't think we really use an alwaysAsk prefered action, just use
> useSystemDefault instead

You mean the front ends actually ignore always ask?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> You mean the front ends actually ignore always ask?
I think that is the case.  Scary, isn't it?

Comment 10

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

That's not what I meant (although I think it is the case; the backend is meant to do that). I just meant that it's the alwaysAsk attribute that's looked at, not the alwaysAsk preferredAction.

Comment 11

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

As for casting to void... it looks ugly, and it doesn't tell you why ignoring the return value is safe. If it is obvious why it's safe, there's no point in using this extra syntax; if it's not obvious, then IMO you should just write a comment.

(Also, existing code in our tree uses that _very_ rarely)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #10)
> That's not what I meant (although I think it is the case; the backend is 
> meant to do that). I just meant that it's the alwaysAsk attribute that's
> looked at, not the alwaysAsk preferredAction.

Right.  The nsIHandlerInfo::alwaysAsk constant, one of the five values to which nsIHandlerInfo::preferredAction can presumably be set, is actually never used by the code, i.e. preferredAction is never set to that value by nsExternalHelperAppService::FillContentHandlerProperties.

Instead, the code sets the separate attribute nsIHandlerInfo::alwaysAskBeforeHandling, which makes sense, since that way a user can orthogonally specify both a default action and that the application should always confirm the action with the user.

Presumably alwaysAsk was a valid value for preferredAction in an earlier version of the code and then someone neglected to remove it once it stopped being used.  In any case, we should at least mark it as deprecated and unused in the IDL file.

Comment 13

[Walter Meinl]

Attachment: the patch for OS/2

Peter, if we need the respective patch also for OS/2 here it is (dunno if this would interfere with bug 305061) If we want/need it we should ask probably biesi for r+.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 273477 [details] [diff] [review]
the patch for OS/2

You certainly want it - sorry for missing you in the first place...

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273477 [details] [diff] [review]
the patch for OS/2

sr=dmose

Comment 16

[Peter Weilbacher]

Comment on attachment 273477 [details] [diff] [review]
the patch for OS/2

Walter, thanks for the patch. Normally this kind of stuff now falls under "OS/2 build break" and gets checked in without reviews. But because I was offline and we now have reviews, even better. Of course now trunk is closed, another milestone missed on OS/2...

What "branch" did the other platform changes go to (comment 7)? Without approval?!? I guess I have to do that for the OS/2 fix, too...

Comment 17

[Walter Meinl]

Attachment: [checked in] unbitrot os/2 patch

Peter, actually during your offline status many patches were checked-in to get the final freeze. However, some patches have to be overhauled subsequently. One line in nsOSHelperAppService.h was removed, therefore my former patch doesn't apply anymore. Hopefully, this one will stay until the tree opens again. It's for M7 (but since its a regression, should be no problem). Since the changes in this patch only affect surrounding lines, I feel free to take over r+ and sr+.

Comment 18

[Peter Weilbacher]

Walter, don't worry about such small bitrotting. That I can take care of during when I apply them here before checkin. Other things are more difficult to solve...

Comment 19

[Peter Weilbacher]

Comment on attachment 274392 [details] [diff] [review]
[checked in] unbitrot os/2 patch

Checked into trunk.