Closed Bug 388701 Opened 14 years ago Closed 14 years ago

nsExternalHelperAppService::GetProtocolHandlerInfo should get OS Specific implementation

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch v1.0 (obsolete) — Splinter Review
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+
Attached patch v1.1 (obsolete) — Splinter Review
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+
Attached patch v1.2Splinter Review
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
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
Closed: 14 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
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?

(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.
Attached patch the patch for OS/2 (obsolete) — Splinter Review
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 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+
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...
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+
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 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
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.