Closed Bug 964220 Opened 7 years ago Closed 7 years ago

OpenBrowserWindow should return window instance when possible

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.27

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140123185438

Steps to reproduce:

from Bug 962065.

1. Run Mozmill test with SeaMonkey 2.23 on Mac OS X, with following command:
  mozmill -t someTest.js -b /Applications/SeaMonkey.app
2. call mozmill.newBrowserController() in someTest.js



Actual results:

OpenBrowserWindow in suite/common/tasksOverlay.js does not return window instance,
and undefined is passed as first argument for controller.MozMillController,
assertion "controller(): Window has been initialized." failed.



Expected results:

OpenBrowserWindow returns window instance.
There are 3 cases in OpenBrowserWindow,
in former 2 cases, window.openDialog is called, so we can return window instance,
in last case, nsICommandLineHandler.handle is called, which is not designed to return window instance:
> void handle(in nsICommandLine aCommandLine);
Modifying it will cause so much changes in many products,
but mozmill uses former 2 cases only,
so fixing former 2 cases is enough for now.
Attachment #8365890 - Flags: review?(iann_bugzilla)
Assignee: nobody → arai_a
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8365890 [details] [diff] [review]
return window instance in OpenBrowserWindow when possible

>+    return window.openDialog(getBrowserURL(), "_blank",
>+                             "chrome,all,dialog=no", null,
>+                             "charset=" + window.content.document.characterSet);
>   } else if (win) {

Strictly speaking you should eliminate the else after return i.e.

  return window.openDialog(getBrowserURL(), "_blank",
                           "chrome,all,dialog=no", null,
                           "charset=" + window.content.document.characterSet);
}

if (win) {
(In reply to neil@parkwaycc.co.uk from comment #2)
> Strictly speaking you should eliminate the else after return i.e.

Thank you for telling me.
Attachment #8365890 - Attachment is obsolete: true
Attachment #8365890 - Flags: review?(iann_bugzilla)
Attachment #8367985 - Flags: review?(iann_bugzilla)
Comment on attachment 8367985 [details] [diff] [review]
remove else after return

Looks like IanN is rather busy a the moment, so I'm stealing the review.
Attachment #8367985 - Flags: review?(iann_bugzilla) → review+
(In reply to Philip Chee from comment #4)
> Comment on attachment 8367985 [details] [diff] [review]
> remove else after return
> 
> Looks like IanN is rather busy a the moment, so I'm stealing the review.

Thank you for reviewing!
Could you submit the patch to try server?
Keywords: checkin-needed
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/d55033527ee3

(In reply to Tooru Fujisawa [:arai] from comment #5)
> Thank you for reviewing!
> Could you submit the patch to try server?
SeaMonkey doesn't have a try server (we could borrow the Thunderbird one I suppose).
Let's see if this fixes Bug 962065
Blocks: 962065
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.27
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 SeaMonkey/2.27a1
Build identifier: 20140209003001
Mozmill 2.0.3

Confirmed mozmill.newBrowserController() works correctly, without applying patch in Bug 962065.
No assertion failed and controller object is returned.

I think this bug is fixed.
Thank you!
> I think this bug is fixed.
> Thank you!
Thank you for the patch.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.