610661 - Addon object not passed to custom about dialogs.

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Matthew Tuck [:CodeMachine]]

extensions.js has:

        if (aboutURL)
          openDialog(aboutURL, "", "chrome,centerscreen,modal");
        else
          openDialog("chrome://mozapps/content/extensions/about.xul",
                     "", "chrome,centerscreen,modal", aAddon);

That is the addon object is passed to the default about dialog but not any custom ones.

This makes no sense as this object is just as useful to custom about dialogs, and without it, an asynchronous call is necessary which dramatically complicates things.

This should be an easy bugfix.

Comment 1

[Dave Townsend [:mossop]]

Would accept a tested patch

Comment 2

[Matthew Tuck [:CodeMachine]]

What constitutes "tested" here? Testing that it works for my extension, or some mozmill stuff?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #2)
> What constitutes "tested" here? Testing that it works for my extension, or some
> mozmill stuff?

Generally, "tested" means having some kind of automated tests in the tree. Browser-chrome tests would be best for this.

See: https://developer.mozilla.org/en/Browser_chrome_tests
And our existing browser-chrome tests are here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/

Comment 4

[Matthew Tuck [:CodeMachine]]

Not entirely sure what that would be in this context, but in any case, never mind, it'll just be another bug I need to work around.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #4)
> Not entirely sure what that would be in this context, but in any case, never
> mind, it'll just be another bug I need to work around.

It would be useful if you provide a patch, which should be pretty easy, even if you can't provide any tests. 

I don't see how adding that parameter would have a negative impact, and it's clearly useful for custom dialogs, as well as necessary for them to have parity with the default code.

Comment 6

[Matthew Tuck [:CodeMachine]]

Attachment: Pass addon object to custom about dialog.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 499310 [details] [diff] [review]
Pass addon object to custom about dialog.

Looks fine but this really does need a test before we should land it.

Comment 8

[:Paolo Amadini]

Attachment: Patch with test case [NOTE: has hg copies]

This includes Matthew's patch and the required test case, that is based on
the code for testing the preferences button and window (browser_bug562890.js).
Tested on Windows XP, running the local browser-chrome test suite.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 502079 [details] [diff] [review]
Patch with test case [NOTE: has hg copies]

Awesome, thanks

Comment 10

[:Paolo Amadini]

(In reply to comment #9)
> Awesome, thanks

Thanks to you for the quick review.

Comment 11

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/e30ee0ae39b6

Comment 12

[Dave Townsend [:mossop]]

Backed out due to browser_about.js timing out.

http://hg.mozilla.org/mozilla-central/rev/56bbebc9d205

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294769748.1294770950.13089.gz

Comment 13

[:Paolo Amadini]

(In reply to comment #12)
> Backed out due to browser_about.js timing out.

The test completes, and then the browser window fails to get focus. The funny
thing is that the test ends with the same sequence as browser_bug562890.js:

executeSoon -> close_manager -> finish

Any idea on what could be wrong? Works for me on Windows.

Comment 14

[:Paolo Amadini]

Attachment: Patch with fixed test case [NOTE: has hg copies]

Tried the suggestion Dave gave me on IRC:

https://bugzilla.mozilla.org/show_bug.cgi?id=579540#c490

The test now explicitly waits for focus both when the dialog is opened
and after it is closed, when focus should return to the browser window.
It still works on Windows XP, but might be worth a tryserver run to verify
if it now works on other platforms too.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 502958 [details] [diff] [review]
Patch with fixed test case [NOTE: has hg copies]

Requesting review so that this doesn't get lost.

Comment 16

[:Paolo Amadini]

(In reply to comment #15)
> Requesting review so that this doesn't get lost.

Thank you. This patch currently has "checkin-needed", on the condition
that the test case passes a full tryserver run.

Note that if the test still fails only on Fedora like it did before,
maybe the patch can be checked in with the test temporarily disabled
on that platform.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 502958 [details] [diff] [review]
Patch with fixed test case [NOTE: has hg copies]

I don't understand the status here. It has checkin-needed marked but you're saying you want a tryserver run first?

Comment 18

[:Paolo Amadini]

(In reply to comment #17)
> I don't understand the status here. It has checkin-needed marked but you're
> saying you want a tryserver run first?

Yes. Since I can't test the current patch on the platform where the test
failed with the previous patch, and both of them work for me on Windows,
I think it's worth running the patch through the tryserver before check-in,
to avoid potentially disrupting the main build again. I suppose there isn't
a tryserver-needed flag for this case :-)

Comment 19

[Dave Townsend [:mossop]]

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]

Comment 20

[Dave Townsend [:mossop]]

I needn't have pushed this to try, this is now failing on Linux and OSX even on my machine where it beachballs Firefox immediately after saying the first about dialog is closed although it still appears on screen.

Comment 21

[:Paolo Amadini]

Attachment: Patch with mixed test case [NOTE: has hg copies]

After applying the previous patch to the latest trunk I rebuilt yesterday, it
made my debug build crash during the tests where the Add-ons Manager is opened
in a separate window. Maybe a failing JS compartment check.

This patch solved the crash for me. It is a mix of the previous two: it uses
waitForFocus like the second patch, but also uses the window watcher service
instead of the window manager service, like the first patch did.

Dave, if it's not too much to ask you to try it locally on Mac or Linux,
that would be helpful to understand quickly if this is now the right magic
combination.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 505932 [details] [diff] [review]
Patch with mixed test case [NOTE: has hg copies]

This one looks good, passed on try server

Comment 23

[:Paolo Amadini]

(In reply to comment #22)
> This one looks good, passed on try server

I'm glad it works! Thank you very much for your help.

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c3ec6be6b1ca

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed based on check-in and no failing tests.

Comment 26

[:Paolo Amadini]

I did a quick search and here are the locations where I think this improvement
can be mentioned in the existing documentation:

https://developer.mozilla.org/en/Install_Manifests#aboutURL
https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon#Optional_attributes
[aboutURL]

Comment 27

[Eric Shepherd [:sheppy]]

This is updated in the two places mentioned.

Comment 28

[:Paolo Amadini]

(In reply to comment #27)
> This is updated in the two places mentioned.

Thank you!