Last Comment Bug 610661 - Addon object not passed to custom about dialogs.
: Addon object not passed to custom about dialogs.
Status: VERIFIED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla2.0b11
Assigned To: :Paolo Amadini
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-09 06:19 PST by Matthew Tuck [:CodeMachine]
Modified: 2011-02-18 04:07 PST (History)
8 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Pass addon object to custom about dialog. (633 bytes, patch)
2010-12-22 09:20 PST, Matthew Tuck [:CodeMachine]
dtownsend: review+
Details | Diff | Splinter Review
Patch with test case [NOTE: has hg copies] (5.60 KB, patch)
2011-01-07 13:14 PST, :Paolo Amadini
dtownsend: review+
dtownsend: approval2.0+
Details | Diff | Splinter Review
Patch with fixed test case [NOTE: has hg copies] (5.57 KB, patch)
2011-01-11 15:09 PST, :Paolo Amadini
dtownsend: review+
Details | Diff | Splinter Review
Patch with mixed test case [NOTE: has hg copies] (5.57 KB, patch)
2011-01-21 13:22 PST, :Paolo Amadini
dtownsend: review+
dtownsend: approval2.0+
Details | Diff | Splinter Review

Description Matthew Tuck [:CodeMachine] 2010-11-09 06:19:56 PST
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] 2010-11-09 09:49:16 PST
Would accept a tested patch
Comment 2 Matthew Tuck [:CodeMachine] 2010-12-15 19:54:22 PST
What constitutes "tested" here? Testing that it works for my extension, or some mozmill stuff?
Comment 3 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-12-15 20:41:28 PST
(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] 2010-12-15 21:10:40 PST
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] 2010-12-17 14:50:07 PST
(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] 2010-12-22 09:20:37 PST
Created attachment 499310 [details] [diff] [review]
Pass addon object to custom about dialog.
Comment 7 Dave Townsend [:mossop] 2010-12-22 09:58:53 PST
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 2011-01-07 13:14:41 PST
Created attachment 502079 [details] [diff] [review]
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] 2011-01-07 13:57:07 PST
Comment on attachment 502079 [details] [diff] [review]
Patch with test case [NOTE: has hg copies]

Awesome, thanks
Comment 10 :Paolo Amadini 2011-01-07 14:19:48 PST
(In reply to comment #9)
> Awesome, thanks

Thanks to you for the quick review.
Comment 11 Dave Townsend [:mossop] 2011-01-11 09:59:35 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/e30ee0ae39b6
Comment 13 :Paolo Amadini 2011-01-11 13:59:40 PST
(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 2011-01-11 15:09:25 PST
Created attachment 502958 [details] [diff] [review]
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] 2011-01-18 16:11:07 PST
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 2011-01-18 16:29:31 PST
(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] 2011-01-19 10:20:14 PST
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 2011-01-19 14:11:52 PST
(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] 2011-01-19 14:42:10 PST
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=dtownsend@mozilla.com
Comment 20 Dave Townsend [:mossop] 2011-01-20 10:57:46 PST
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 2011-01-21 13:22:30 PST
Created attachment 505932 [details] [diff] [review]
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] 2011-01-24 15:32:21 PST
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 2011-01-25 10:24:01 PST
(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 25 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-01-27 14:18:14 PST
Marking as verified fixed based on check-in and no failing tests.
Comment 26 :Paolo Amadini 2011-01-27 14:39:31 PST
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] 2011-02-17 05:56:09 PST
This is updated in the two places mentioned.
Comment 28 :Paolo Amadini 2011-02-18 04:07:34 PST
(In reply to comment #27)
> This is updated in the two places mentioned.

Thank you!

Note You need to log in before you can comment on or make changes to this bug.