As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 774660 - openPreferences should consistently not return a window object
: openPreferences should consistently not return a window object
Status: RESOLVED FIXED
[good first bug][mentor=dao][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Saurabh Anand [:sawrubh]
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 07:45 PDT by :Ehsan Akhgari
Modified: 2012-07-19 07:35 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.33 KB, patch)
2012-07-18 07:17 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v2 (1.36 KB, patch)
2012-07-18 08:07 PDT, Saurabh Anand [:sawrubh]
dao+bmo: feedback+
Details | Diff | Splinter Review
Patch v3 (1.42 KB, patch)
2012-07-18 08:29 PDT, Saurabh Anand [:sawrubh]
dao+bmo: review+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2012-07-17 07:45:36 PDT
See <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#477>.  This will be a problem when we use the return value in the test case for bug 722978.
Comment 1 User image Dão Gottwald [:dao] 2012-07-17 08:07:42 PDT
The current return value appears to be unused and returning a window is non-trivial in the in-content case, so we should instead remove it. The test in bug 722978 should use observers or something.
Comment 2 User image :Ehsan Akhgari 2012-07-17 19:46:27 PDT
(In reply to comment #1)
> The current return value appears to be unused and returning a window is
> non-trivial in the in-content case, so we should instead remove it. The test in
> bug 722978 should use observers or something.

How would using observers get us to the right window object?
Comment 3 User image Dão Gottwald [:dao] 2012-07-18 02:36:41 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> (In reply to comment #1)
> > The current return value appears to be unused and returning a window is
> > non-trivial in the in-content case, so we should instead remove it. The test in
> > bug 722978 should use observers or something.
> 
> How would using observers get us to the right window object?

Via the subject argument, or the test would just look for the window itself using window watcher and tabbrowser APIs.
Comment 4 User image Saurabh Anand [:sawrubh] 2012-07-18 04:15:46 PDT
@Dao, @Ehsan, will this bug block bug 722978 ?

BTW, I think I can work on this :)
Comment 5 User image Dão Gottwald [:dao] 2012-07-18 06:12:53 PDT
(In reply to Saurabh Anand [:sawrubh] from comment #4)
> @Dao, @Ehsan, will this bug block bug 722978 ?

No, I morphed it. This bug is now about making openPreferences never return a window.
Comment 6 User image Saurabh Anand [:sawrubh] 2012-07-18 07:17:10 PDT
Created attachment 643365 [details] [diff] [review]
Patch v1
Comment 7 User image :Ehsan Akhgari 2012-07-18 07:32:46 PDT
(In reply to comment #4)
> @Dao, @Ehsan, will this bug block bug 722978 ?

Hmm, I think so, yes, since now you cannot use the openPreferences() function in a straightforward manner there.

> BTW, I think I can work on this :)

Great!
Comment 8 User image Saurabh Anand [:sawrubh] 2012-07-18 08:07:01 PDT
Created attachment 643378 [details] [diff] [review]
Patch v2
Comment 9 User image Dão Gottwald [:dao] 2012-07-18 08:13:44 PDT
Comment on attachment 643378 [details] [diff] [review]
Patch v2

>+    openDialog("chrome://browser/content/preferences/preferences.xul",
>                       "Preferences", features, paneID, extraArgs);

Looks good except for the indentation in the second line here.
Comment 10 User image Saurabh Anand [:sawrubh] 2012-07-18 08:29:57 PDT
Created attachment 643391 [details] [diff] [review]
Patch v3

Fixed indentation.
Comment 12 User image Ed Morley [:emorley] 2012-07-19 07:35:18 PDT
https://hg.mozilla.org/mozilla-central/rev/61b822ad93a1

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