671528 - consider adding aReason argument to nsIHelperAppLauncher.js so extensions can make use of it

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[scorpion]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Comment 1

[scorpion]

Attachment: patch

Comment 2

[:Paolo Amadini]

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

Review of attachment 545882 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch! I've set the r+ flag, meaning that overall it looks OK to me.

I'd recommend two changes though, please look at the review below. You should attach a new patch for final check-in. Also, since I'm not a peer of this module, you'll need to set the "review" flag on the new patch to "?", and indicate Shawn (:sdwilsh) as requestee, for final approval.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +57,5 @@
>  function nsUnknownContentTypeDialog() {
>      // Initialize data properties.
>      this.mLauncher = null;
>      this.mContext  = null;
> +    this.mReason   = 0;

I'd set this.mReason to null here to indicate that the field hasn't been initialized. Zero indicates Ci.nsIHelperAppLauncherDialog.REASON_CANTHANDLE instead.

@@ +90,5 @@
>      //       one of those is via that route).
>      show: function(aLauncher, aContext, aReason)  {
>        this.mLauncher = aLauncher;
>        this.mContext  = aContext;
> +      this.mReason   = aReason;

I'd add a comment saying that we set this field for use by add-ons. This ensures that it's not accidentally removed in the future (however, please remember that it could be intentionally removed in case we refactor the code to work differently; this is not an external API).

Our prevailing comment style here is leading uppercase letter and trailing dot.

Comment 3

[kernp25]

Attachment: 671528.patch

Comment 4

[kernp25]

yes
I think, this can be useful for extensions, that are integrated into the download dialog.
So, they can find out why the dialog is shown.
This is really useful information.

Comment 5

[kernp25]

(In reply to :Paolo Amadini from comment #2)
> Comment on attachment 545882 [details] [diff] [review]
> patch
> 
> Review of attachment 545882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch! I've set the r+ flag, meaning that overall it looks
> OK to me.
> 
> I'd recommend two changes though, please look at the review below. You
> should attach a new patch for final check-in. Also, since I'm not a peer of
> this module, you'll need to set the "review" flag on the new patch to "?",
> and indicate Shawn (:sdwilsh) as requestee, for final approval.
> 
> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +57,5 @@
> >  function nsUnknownContentTypeDialog() {
> >      // Initialize data properties.
> >      this.mLauncher = null;
> >      this.mContext  = null;
> > +    this.mReason   = 0;
> 
> I'd set this.mReason to null here to indicate that the field hasn't been
> initialized. Zero indicates Ci.nsIHelperAppLauncherDialog.REASON_CANTHANDLE
> instead.
>
Or maybe to -1? this.mReason   = -1;?
What you think?
 
> @@ +90,5 @@
> >      //       one of those is via that route).
> >      show: function(aLauncher, aContext, aReason)  {
> >        this.mLauncher = aLauncher;
> >        this.mContext  = aContext;
> > +      this.mReason   = aReason;
> 
> I'd add a comment saying that we set this field for use by add-ons. This
> ensures that it's not accidentally removed in the future (however, please
> remember that it could be intentionally removed in case we refactor the code
> to work differently; this is not an external API).
> 
Is this comment here needed?
Can we just add this tiny mReason param to http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js?
And why it can be removed?
I don't think that this tiny reason param here hurts someone.

Comment 6

[:Paolo Amadini]

Comment on attachment 8441343 [details] [diff] [review]
671528.patch

Thank you for reviving this old patch!

Just one note, I've noticed a difference in an empty line, this may indicate a mismatch in line endings (CRLF versus LF). Please check that you're using Unix line endings in your text editor, and attach a fixed patch if this is the case.

Comment 7

[kernp25]

Shawn Wilsher is still on bugzilla?

Comment 8

[:Paolo Amadini]

(In reply to kernp25 from comment #5)
> Or maybe to -1? this.mReason   = -1;?
> What you think?

I think null is fine, you can use the === to check, but generally extension code will be invoked after the value has been set anyways.

> > I'd add a comment saying that we set this field for use by add-ons. This
> > ensures that it's not accidentally removed in the future (however, please
> > remember that it could be intentionally removed in case we refactor the code
> > to work differently; this is not an external API).
> > 
> Is this comment here needed?

It can be useful for the reasons explained, but is not mandatory.

> And why it can be removed?
> I don't think that this tiny reason param here hurts someone.

I agree, at present the reason parameter is useful. But since there is no documented interface to extend this dialog, nor automated tests for it, this is more likely to change in the future (not only the parameter, but the dialog in general).

Comment 9

[:Paolo Amadini]

(In reply to kernp25 from comment #7)
> Shawn Wilsher is still on bugzilla?

I don't think he's been active recently, but I'm now a peer of the module so my review is enough.

Comment 10

[kernp25]

Thank you for your time!!!
I will create another patch.

Comment 11

[kernp25]

Attachment: 671528.patch

Comment 12

[:Paolo Amadini]

Comment on attachment 8441357 [details] [diff] [review]
671528.patch

Looks good to me!

Comment 13

[kernp25]

When this will be added to http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js?

When it will be available?

Comment 14

[kernp25]

Is this bug now fixed?

Comment 15

[kernp25]

But i really think we should add this useful information to http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js

Comment 16

[:Paolo Amadini]

Attachment: Patch for tryserver

(In reply to kernp25 from comment #14)
> Is this bug now fixed?

I've requested automated tests to be run on your patch, you can see the results here:

https://tbpl.mozilla.org/?tree=Try&rev=47bd5cdc000d

Initially the letters on the right side are gray, then they become green when the tests pass. We must wait until all the letters are green (note that we can ignore some orange letters), then we can set the checkin-needed keyword on the bug:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 17

[kernp25]

> Initially the letters on the right side are gray, then they become green when the tests pass. We must > wait until all the letters are green (note that we can ignore some orange letters), then we can set
> the checkin-needed keyword on the bug:
How long does this take?
But, i can see many green letters :)
Did the test passed?
But again, thank you for taking your time to fix this and also thank you for creating a new patch for the testing.

Comment 18

[kernp25]

But what are these red letters?
It says it's burning?
> Windows XP Opt B
> Windows XP Debug B
> Windows 2012 x64 Opt B
> Windows 2012 x64 Debug B

Comment 19

[kernp25]

Can we also ignore this?

Comment 20

[:Paolo Amadini]

(In reply to kernp25 from comment #19)
> Can we also ignore this?

At the current time, I don't see any red letters, and I see only one orange (OS X 10.6 Debug bc1) that is a known intermittent failure, thus it can be ignored.

I think you can go ahead and set the "checkin-needed" keyword on the bug.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb625761ecb

Comment 22

[kernp25]

I have found this:
https://bugzilla.mozilla.org/show_bug.cgi?id=285517

and this one:
https://bugzilla.mozilla.org/show_bug.cgi?id=285976
It looks like the same bug

Paolo what you think?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4eb625761ecb

Comment 24

[:Paolo Amadini]

(In reply to kernp25 from comment #22)
> I have found this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=285517
> 
> and this one:
> https://bugzilla.mozilla.org/show_bug.cgi?id=285976
> It looks like the same bug
> 
> Paolo what you think?

I don't think those are the same bug as this one, but thank you a lot for searching for duplicates! It's very helpful.