964220 - OpenBrowserWindow should return window instance when possible

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: return window instance in OpenBrowserWindow when possible

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.

Comment 2

[[e-mail]]

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) {

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: remove else after return

(In reply to [e-mail] from comment #2)
> Strictly speaking you should eliminate the else after return i.e.

Thank you for telling me.

Comment 4

[Philip Chee]

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.

Comment 5

[Tooru Fujisawa [:arai]]

(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?

Comment 6

[Philip Chee]

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

Comment 7

[Tooru Fujisawa [:arai]]

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!

Comment 8

[Philip Chee]

> I think this bug is fixed.
> Thank you!
Thank you for the patch.