Addon object not passed to custom about dialogs.

VERIFIED FIXED in mozilla2.0b11

Status

()

Toolkit
Add-ons Manager
--
enhancement
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: CodeMachine, Assigned: Paolo)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b11
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Would accept a tested patch
Severity: normal → enhancement
blocking2.0: --- → -
(Reporter)

Comment 2

7 years ago
What constitutes "tested" here? Testing that it works for my extension, or some mozmill stuff?
(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/
(Reporter)

Comment 4

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

Comment 6

7 years ago
Created attachment 499310 [details] [diff] [review]
Pass addon object to custom about dialog.
Attachment #499310 - Flags: review?(dtownsend)
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.
Attachment #499310 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 8

7 years ago
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.
Assignee: nobody → paolo.02.prg
Status: NEW → ASSIGNED
Attachment #502079 - Flags: review?(dtownsend)
Comment on attachment 502079 [details] [diff] [review]
Patch with test case [NOTE: has hg copies]

Awesome, thanks
Attachment #502079 - Flags: review?(dtownsend)
Attachment #502079 - Flags: review+
Attachment #502079 - Flags: approval2.0+
(Assignee)

Comment 10

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

Thanks to you for the quick review.
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Attachment #499310 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/e30ee0ae39b6
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

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

Comment 14

7 years ago
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.
Attachment #502079 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [needs tryserver run before checkin]
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.
Attachment #502958 - Flags: review?(dtownsend)
(Assignee)

Comment 16

6 years ago
(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 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?
Attachment #502958 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 18

6 years ago
(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 :-)
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=dtownsend@mozilla.com
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.
Keywords: checkin-needed
Whiteboard: [needs tryserver run before checkin]
(Assignee)

Comment 21

6 years ago
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.
Attachment #502958 - Attachment is obsolete: true
Attachment #505932 - Flags: review?(dtownsend)
Comment on attachment 505932 [details] [diff] [review]
Patch with mixed test case [NOTE: has hg copies]

This one looks good, passed on try server
Attachment #505932 - Flags: review?(dtownsend)
Attachment #505932 - Flags: review+
Attachment #505932 - Flags: approval2.0+
(Assignee)

Comment 23

6 years ago
(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.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c3ec6be6b1ca
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla2.0b10 → mozilla2.0b11
Marking as verified fixed based on check-in and no failing tests.
Status: RESOLVED → VERIFIED
Keywords: dev-doc-needed
Version: unspecified → Trunk
(Assignee)

Comment 26

6 years ago
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]
This is updated in the two places mentioned.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 28

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

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