nsExternalHelperAppService::GetProtocolHandlerInfo should get OS Specific implementation

RESOLVED FIXED in mozilla1.9alpha8

Status

RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9alpha8
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 273004 [details] [diff] [review]
v1.0

This was a bit easier than expected...
Assignee: dmose → sdwilsh
Attachment #273004 - Flags: superreview?(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.
Attachment #273004 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 3

12 years ago
Created attachment 273182 [details] [diff] [review]
v1.1

sr=dmose
Also added comment as per discussion with him.
Attachment #273004 - Attachment is obsolete: true
Attachment #273182 - Flags: review?(cbiesinger)
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
Attachment #273182 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 273319 [details] [diff] [review]
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 :(
Attachment #273182 - Attachment is obsolete: true
(Assignee)

Comment 6

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Comment 7

12 years ago
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
(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?

(Assignee)

Comment 9

12 years ago
(In reply to comment #8)
> You mean the front ends actually ignore always ask?
I think that is the case.  Scary, isn't it?
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.
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)
(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

12 years ago
Created attachment 273477 [details] [diff] [review]
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+.
(Assignee)

Comment 14

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

You certainly want it - sorry for missing you in the first place...
Attachment #273477 - Flags: superreview?(dmose)
Attachment #273477 - Flags: review?(cbiesinger)
Comment on attachment 273477 [details] [diff] [review]
the patch for OS/2

sr=dmose
Attachment #273477 - Flags: superreview?(dmose) → superreview+
Attachment #273477 - Flags: review?(cbiesinger) → review+
Depends on: 389632

Comment 16

12 years ago
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

12 years ago
Created attachment 274392 [details] [diff] [review]
[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+.
Attachment #273477 - Attachment is obsolete: true
Attachment #274392 - Flags: review+

Comment 18

12 years ago
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

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

Checked into trunk.
Attachment #274392 - Attachment description: unbitrot os/2 patch → [checked in] unbitrot os/2 patch

Updated

11 years ago
Duplicate of this bug: 426358
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.