Closed Bug 793644 Opened 7 years ago Closed 7 years ago

Browser API - Fire an event on mozbrowser iframes to confirm reload of a POST request

Categories

(Firefox OS Graveyard :: General, defect, P1)

defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: benfrancis, Assigned: kk1fff)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

As reported in https://github.com/mozilla-b2g/gaia/issues/4817 reload does not currently work for POST requests, presumably because the platform is waiting for confirmation from the user as in the desktop browser but no dialog is shown in the browser app UI.

This is a good catch by Tim but a very late landing requirement, nominating for blocking-basecamp so Product can make a call but I'm not expecting this for 9/28.

This is similar in nature to bug 775464 but at least this one doesn't cause a crash.
Justin, can you take this one?
Assignee: nobody → justin.lebar+bug
blocking-basecamp: ? → +
I'm totally overloaded at the moment, but I'll see if I can find someone else to do this.
Patrick, are you interested in investigating this one?  I don't know off the top of my head what's causing this problem.

Off the top of my head, the ideal fix might be to make BrowserElementChild.reload() always do a reload, but to add a function to BEC which lets you check whether the current page has POST data (i.e., whether we should show the POST-refresh warning).
I reproduced this bug with browser element's mochitest (e.g. not on device). I think the nsDocShell does send the repost warning in nsDocShell::confirmRepost() [1]. It calls confirmEx to show the message. The message can be printed by adding a log in BrowserElementPrompt.confirmEx(). I think getting the message from confirmEx() might also fix this bug.

[1] https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=488beb32bca5#11067
Okay, we can fix it the right way!  :)
Is this likely to be ready for today/tomorrow in time for feature complete?

If you have some idea what the API will look like I can start working on the Gaia end, if necessary.

Thanks
> Is this likely to be ready for today/tomorrow in time for feature complete?

No.  Even if someone coughed up a patch today, we can't even run a patch through tryserver in one day, much less review and land it.  We shouldn't use the deadline to cause us to chase abnormally-fast fixes like this; that's only a distraction from the high-quality work we need to continue doing.
Patrick, you seem to understand what's going on here; can you take this bug?
Assignee: justin.lebar+bug → nobody
Assignee: nobody → pwang
Add an nsIConfirmRepostDetail interface and its implementation. This object is used to transfer the reference of window and the decision to repost between nsDocShell and BEC.
Attachment #668386 - Flags: review?(justin.lebar+bug)
1. In nsDocShell::ConfirmRepost(), if nsDocShell found it is for a browser element, notify BEC through observer, and wait for the decision.

2. In BEC, handle the notification and show a sync prompt, with prompt type = "confirmrepost".
Attachment #668391 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — Splinter Review
Attachment #668393 - Flags: review?(justin.lebar+bug)
These patches are basically fine for what they're trying to do (modulo some English nits).  I'm not wild about the way we call from DocShell into BEC, but I don't have a better suggestion offhand, so it's probably OK.

But I'm concerned that we're working around this problem in the wrong way.

We'll have this same problem anywhere we use ConfirmEx, right?  If you search through the code, you'll see we use it in a number of places, for things such as

 * "Do you want to stay on this page?" triggered by an unload handler.
 * "This script is unresponsive; kill it?"
 * nsHttpChannelAuthProvider::ConfirmAuth, which I think happens when a page does something sketchy with http auth.

I suspect we have bugs similar to this one lurking around each of these calls to ConfirmEx.

So it seems to me that maybe the right thing to do is to pass an unlocalized identifier into ConfirmEx, modify all the ConfirmEx calls to include an identifier, and then handle the ConfirmEx call by passing that identifier to BEP and asking it to show an appropriate prompt.

I have a few questions about this approach:

1) Would we be able to modify ConfirmEx to add an extra parameter, or would we need to create a new method and move all Gecko callers over to the new method?

2) Should we put the message identifiers into nsIPrompt.idl?  It seems to me this would be a reasonable way of guaranteeing that there's a list somewhere of all the prompts we need to handle in BEC.

An alternative approach would be to implement ConfirmEx in the browser-element code by passing the arguments straight through.  That is, the browser element parent tells the embedder "do a prompt with these parameters", where the params come straight from ConfirmEx (maybe we modify them a bit to make a better interface, but they are basically unchanged).  That probably wouldn't be too hard.  My main concern with that approach are that:

a) The embedder can't tell which prompt we're trying to show, because it only gets a localized message.  So if, for example, the embedder wants to take a default action for a certain prompt, it can't.  We could solve this by including a unique identifier with each prompt, as described above.

b) We'd be passing localized params to the embedder, which we don't do elsewhere in the browser-element code -- that would make for a bad web-api.

I don't know who owns the prompt code -- nsIPrompt.idl hasn't been touched for a while.  bz, do you have any thoughts, or know who we should loop in here?
Comment on attachment 668386 [details] [diff] [review]
Part 1: Add nsIConfirmRepostDetail to carry the message between nsDocShell and BEC

Cancelling these reviews until we figure out how we want to proceed here.
Attachment #668386 - Flags: review?(justin.lebar+bug)
Attachment #668391 - Flags: review?(justin.lebar+bug)
Attachment #668393 - Flags: review?(justin.lebar+bug)
Priority: -- → P1
bz, we could use some assistance here (comment 12), or a redirect to the right person.
Sorry, I had missed comment 12.

bsmedberg may be a better person to ask about prompts....
<bsmedberg> jlebar: it looks to me like we shouldn't try to solve the "good API for prompts" problem for v1, so ISTM we should just pass the prompt args through (with whatever minimal security checking is necessary)

That's the approach in comment 12 that starts with "An alternative approach ...".

This is still a fair bit of work (particularly on the front-end), but at least at the end we'd  have a correct implementation of confirmEx, which is better than we have now.
Attachment #668386 - Attachment is obsolete: true
Attachment #668391 - Attachment is obsolete: true
Attachment #673171 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — Splinter Review
Attachment #668393 - Attachment is obsolete: true
Attachment #673172 - Flags: review?(justin.lebar+bug)
Comment on attachment 673172 [details] [diff] [review]
Test case

These tests are hard to write in a way that's readable, but I have some
suggestions below.

>+// Bug 793644, fire an event when attempting to reloads browser element after
>+// POST respest.

s/reloads/reload/
s/POST repest/a POST request/

>+"use strict";
>+SimpleTest.waitForExplicitFinish();
>+
>+var iframe;
>+var gotConfirmRepost = false;
>+var doRepost = true;
>+var timer;
>+
>+function getExpectedString() {
>+  let result = {};
>+  let bundleService = SpecialPowers.Cc['@mozilla.org/intl/stringbundle;1'].
>+    getService(SpecialPowers.Ci.nsIStringBundleService);
>+  let appBundle = bundleService.createBundle("chrome://global/locale/appstrings.properties");
>+  let brandBundle = bundleService.createBundle("chrome://branding/locale/brand.properties");
>+  try {
>+    let brandName = brandBundle.GetStringFromName("brandShortName");
>+    result.message = appBundle.formatStringFromName("confirmRepostPrompt",
>+                                                    [brandName], 1);
>+  } catch (e) {
>+    // for the case that we don't have brandShortName
>+    result.message = appBundle.GetStringFromName("confirmRepostPrompt");
>+  }
>+  result.resend = appBundle.GetStringFromName("resendButton.label");
>+
>+  return result;

This function returns an object {message: "...", resend: "..."}, so it
shouldn't be called "GetExpectedString".  At least "GetExpectedStrings".

>+function checkResult() {
>+  ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload");
>+  if (doRepost) {
>+    doRepost = false;
>+    // Run again, with repost disallowed.
>+    iframe.src = 'file_post_request.html';
>+    iframe.addEventListener('mozbrowserloadend', postPageLoadDone);
>+  } else {
>+    SimpleTest.finish();
>+  }
>+}

You're not really "checking a result" here; you're checking that we got a
confirm-repost dialog and then running the next test if appropriate.

I think it would be clearer if you inlined this function into its callers; both
callers already branch on doRepost, so I don't think you'd end up repeating any
code, and it would make the control-flow explicit.

>+function reloadDone() {
>+  if (!doRepost) {
>+    // Browser element reloaded, we got a failure.
>+    clearTimeout(timer);
>+    timer = null;
>+    ok(false, "Browser element reloaded when we don't allow it to reload.");
>+  }
>+  iframe.removeEventListener('mozbrowserloadend', reloadDone);
>+  checkResult();
>+}
>+
>+function postRequestDone() {
>+  gotConfirmRepost = false;
>+  iframe.removeEventListener('mozbrowserloadend', postRequestDone);
>+  iframe.addEventListener('mozbrowserloadend', reloadDone);
>+  if (!doRepost) {
>+    // We don't expect browserelement really reload, use a timer to make sure
>+    // it is not reloaded.

s/really/to/
s/,/;/

>+    timer = window.setTimeout(checkResult, 1000);
>+  }
>+  iframe.reload();
>+}
>+
>+function postPageLoadDone() {
>+  iframe.removeEventListener('mozbrowserloadend', postPageLoadDone);
>+  iframe.addEventListener('mozbrowserloadend', postRequestDone);
>+}

Rather than swapping out event listeners like this, I think it would be more
clear if you had one event listener and you used a variable to determine its
behavior.  This is kind of hard to follow as-is.

>+function runTest() {
>+  browserElementTestHelpers.setEnabledPref(true);
>+  browserElementTestHelpers.addPermission();
>+
>+  iframe = document.createElement('iframe');
>+  iframe.mozbrowser = true;
>+
>+  iframe.src = 'file_post_request.html';
>+  document.body.appendChild(iframe);
>+
>+  iframe.addEventListener('mozbrowserloadend', postPageLoadDone);
>+
>+  let expectedMessage = getExpectedString();
>+  iframe.addEventListener('mozbrowsershowmodalprompt', function(e) {
>+    if (e.detail.promptType == 'confirmEx') {
>+      gotConfirmRepost = true;
>+      e.preventDefault();
>+      e.detail.returnValue = {
>+        button: doRepost ? 0 : 1,
>+      };
>+      is(e.detail.button0.titleType, 'string');
>+      is(e.detail.button0.titleStr, expectedMessage.resend);
>+      is(e.detail.message, expectedMessage.message);
>+      is(e.detail.button1.titleType, 'no');
>+      e.detail.unblock();

Can we check that button2 is not specified?

Also, I don't think titleStr should be defined if titleType is not 'string', so
we should check that, too.
Attachment #673172 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 673171 [details] [diff] [review]
Patch: Implement confirmEx function

This is totally the right idea, but I'd prefer if we tweaked the interface we
exposed to callers.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -209,17 +209,18 @@ BrowserElementChild.prototype = {
> 
>     args.windowID = { outer: utils.outerWindowID,
>                       inner: this._tryGetInnerWindowID(win) };
>     sendAsyncMsg('showmodalprompt', args);
> 
>     let returnValue = this._waitForResult(win);
> 
>     if (args.promptType == 'prompt' ||
>-        args.promptType == 'confirm') {
>+        args.promptType == 'confirm' ||
>+        args.promptType == 'confirmEx') {

Can we call this "custom-prompt" or something?

>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>     return this._browserElementChild.showModalPrompt(
>       this._win, {promptType: "confirm", title: title, message: text, returnValue: undefined});
>   },
> 
>   confirmCheck: function(title, text, checkMsg, checkState) {
>     return this.confirm(title, text);
>   },
> 
>+  // For each button, there is an object to descript its look

       Each button is described by an object with the following schema:

("descript" is not used in modern English; you mean "to describe".)

>+  // {
>+  //   boolean show, // True if this button should be shown.
>+  //   string titleType, // 'ok', 'cancel', 'yes', 'no', 'save', 'dontsave', 'revert' or 'string'
>+  //   string titleStr,  // Title in string, valid when titleType == 'string'.
>+  // }
>+  // Expected result from embedder:
>+  // {
>+  //   int button, // Number of the button that user pressed: 0, 1, 2
>+  //   boolean checked, // True if a check box is checked.
>+  // }

Can we modify the API sent to the embedder to make it a bit nicer?  Instead of
button0, button1, and button2, could we have a list of buttons?

  buttons: [button]
  button:  {messageType: 'builtin', message: 'ok'/'cancel'/etc} and
  button:  {messageType: 'custom',  message: 'whatever string is passed'}

I'd prefer if we also explicitly said whether we wanted to show the checkbox, as:

  showCheckbox:
  checkboxMessage:
  checkboxCheckedByDefault: true/false

>   confirmEx: function(title, text, buttonFlags, button0Title, button1Title, button2Title, checkMsg, checkState) {
>-    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>+    let buttons = this._buildConfirmExButtonProperty(buttonFlags, button0Title, button1Title, button2Title);
>+    let ret = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType: "confirmEx",
>+        title: title,
>+        message: text,
>+        defaultButton: buttons.defaultButton,
>+        button0: buttons.button0,
>+        button1: buttons.button1,
>+        button2: buttons.button2,
>+        checkMsg: checkMsg,
>+        checkState: checkState.value,
>+        returnValue: undefined
>+      }
>+    );
>+    checkMsg = ret.checked;

Do you mean |checkState.value = ret.checked|?

>+    return ret.button;
>   },

>+  _buildConfirmExButtonProperty: function(buttonFlags, button0Title, button1Title, button2Title) {
>+    let r = {};

>+    // Find the default button.
>+    r.defaultButton = (buttonFlags & (3<<24)) >> 24;

These magic numbers make me worried, mainly because we might change them in the
nsIPrompt interface.  I know it would be more verbose, but could you write this
while assuming as little about the constants in the interface as possible?  For
example

  if (buttonFlags & Ci.nsIPrompt.BUTTON_POS_0_DEFAULT) {...}
  else if (...) {...}
  else if (...) {...}
  else { // throw an exception, or let button 0 be the default? }

>+    // Properties of each button.
>+    let titleTypeString = ['ok', 'cancel', 'yes', 'no', 'save', 'dontsave', 'revert', 'string'];
>+    function buildButton(buttonFlags, buttonTitle, buttonNumber) {
>+      let ret = {};
>+      let bitOffset = (buttonNumber * 8);

I'm OK if you want to leave the * 8 here, because this is annoying to work
around, but if you wanted to work around it, you could by doing something like
iterating over the list [Ci.nsIPrompt.BUTTON_POS_0, Ci.nsIPrompt_BUTTON_POS_1,
...].

>+      let mask = Ci.nsIPrompt.BUTTON_TITLE_IS_STRING << bitOffset;
>+      let titleType = (buttonFlags & mask) >> bitOffset;
>+      if (titleType == 0) {
>+        ret.show = false;
>+      } else if (titleType < 127) {
>+        ret.show = true;
>+        ret.titleType = titleTypeString[titleType + 1];
>+      } else { // titleType == 127
>+        ret.show = true;
>+        ret.titleType = 'string';
>+        ret.titleStr = buttonTitle;
>+      }

Please assert one way or another that your assumption that titleType <= 127
means that titleType == 127 is correct.
Attachment #673171 - Flags: review?(justin.lebar+bug)
Summarize of this update:
1. Change name of prompt to 'custom-prompt'.
2. Use a list of buttons to pass the text on buttons to the embedder.
3. Use symbol in nsIPrompt instead of use the number directly.
4. Fix problems according to comment 20.
Attachment #673171 - Attachment is obsolete: true
Attachment #674590 - Flags: review?(justin.lebar+bug)
Attached patch Test case v2 (obsolete) — Splinter Review
1. update test case with the change of patch.
2. According to comment 19, change some call flow and try to make it easier to read.

Hi Justin,
Because some of the flow in the test case is changed, would you help to review again?

Thanks
Attachment #673172 - Attachment is obsolete: true
Attachment #674599 - Flags: review?(justin.lebar+bug)
Comment on attachment 674590 [details] [diff] [review]
Patch: Implement confirmEx function v2

>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>--- a/dom/browser-element/BrowserElementPromptService.jsm
>+++ b/dom/browser-element/BrowserElementPromptService.jsm
>@@ -46,18 +46,53 @@ BrowserElementPrompt.prototype = {
>     return this._browserElementChild.showModalPrompt(
>       this._win, {promptType: "confirm", title: title, message: text, returnValue: undefined});
>   },
> 
>   confirmCheck: function(title, text, checkMsg, checkState) {
>     return this.confirm(title, text);
>   },
> 
>-  confirmEx: function(title, text, buttonFlags, button0Title, button1Title, button2Title, checkMsg, checkState) {
>-    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>+  // Each button is described by an object with the following schema
>+  // {
>+  //   string messageType,  // 'builtin' or 'custom'
>+  //   string message, // 'ok', 'cancel', 'yes', 'no', 'save', 'dontsave', 
>+  //                   // 'revert' or a string from caller if messageType was 'custom'.
>+  // }
>+  //
>+  // Expected result from embedder:
>+  // {
>+  //   int button, // Index of the button that user pressed.
>+  //   boolean checked, // True if a check box is checked.
>+  // }

s/a check box/the checkbox/ -- the API only lets us have one checkbox.

>+  confirmEx: function(title, text, buttonFlags, button0Title, button1Title,
>+                      button2Title, checkMsg, checkState) {
>+    let buttons = this._buildConfirmExButtonProperty(buttonFlags,
>+                                                     button0Title,
>+                                                     button1Title,
>+                                                     button2Title);

It's kind of misleading that we have a variable called "buttons" that isn't
actually a list of buttons.

Could you call this buttonProperties, maybe?  Or alternatively, we could make
use of a fancy language feature to do

  [buttons, defaultButton, indexToButtonNumberMap] = this._buildConfirmExButtonProperty()

if _buildConfirmExButtonProperty() returned an array instead of an object.

>+    let ret = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType: "custom-prompt",
>+        title: title,
>+        message: text,
>+        defaultButton: buttons.defaultButton,
>+        buttons: buttons.buttons,
>+        showCheckbox: !!checkMsg,
>+        checkboxMessage: checkMsg,
>+        checkboxCheckedByDefault: !!checkState.value,
>+        returnValue: {
>+          button: buttons.defaultButton,

Call this selectedButton?

>+          checked: checkState.value
>+        }
>+      }
>+    );
>+    checkState.value = ret.checked;
>+    return buttons.mapIndexToButtonNumber[ret.button];
>   },

>+  _buildConfirmExButtonProperty: function(buttonFlags, button0Title,
>+                                          button1Title, button2Title) {

_buildConfirmExButtonProperties might be a better name.

>+      // If this is the default button, full the index of this button in
>+      // array into r.defaultButton. This value is going to be exposed to
>+      // embedder.
>+      if (defaultButton === buttonNumber) {
>+        r.defaultButton = r.buttons.length;
>+      }

"full" isn't a verb.  "If this is the default button, set r.defaultButton to
the index of this button in the array." perhaps?

Also, this needs more articles: "in /the/ array", "to /the/ embedder".

r=me with these nits fixed.
Attachment #674590 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 674599 [details] [diff] [review]
Test case v2

This is a bit easier to follow; thanks.

>+function pageLoadDone() {
>+  if (!isPostRequestSubmitted) {
>+    // This loadend is done by setting url in address bar, so we don't need to
>+    // do anything. The test page will submit a POST request.
>+    isPostRequestSubmitted = true;
>+    return;
>+  }
>+
>+  gotConfirmRepost = false;
>+  iframe.removeEventListener('mozbrowserloadend', pageLoadDone);
>+  if (doRepost) {
>+    iframe.addEventListener('mozbrowserloadend', reloadDone);
>+  } else {
>+    // We don't expect browserelement to reload; use a timer to make sure
>+    // it is not reloaded.
>+    iframe.addEventListener('mozbrowserloadend', failBecauseReloaded);
>+    timer = window.setTimeout(function() {
>+      iframe.removeEventListener('mozbrowserloadend', failBecauseReloaded);
>+      ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload");
>+      SimpleTest.finish();
>+    }, 1000);

This is asking for trouble (random oranges).

In the last patch, as I understood it (and I may have misunderstood!), we were
doing a SetTimeout() to make sure that something /didn't/ happen after one
second.  That's not ideal, but it's OK.  But here, you're checking that the
repost happens after one second, and that's not OK.

>+  }
>+  iframe.reload();
>+}
>+
>+function runTest() {
>+  browserElementTestHelpers.setEnabledPref(true);
>+  browserElementTestHelpers.addPermission();
>+
>+  iframe = document.createElement('iframe');
>+  iframe.mozbrowser = true;
>+
>+  isPostRequestSubmitted = false;
>+  iframe.src = 'file_post_request.html';
>+  document.body.appendChild(iframe);
>+
>+  iframe.addEventListener('mozbrowserloadend', pageLoadDone);
>+
>+  let expectedMessage = getExpectedStrings();
>+  iframe.addEventListener('mozbrowsershowmodalprompt', function(e) {
>+    if (e.detail.promptType == 'custom-prompt') {
>+      gotConfirmRepost = true;
>+      e.preventDefault();
>+      e.detail.returnValue = {
>+        button: doRepost ? 0 : 1,
>+      };

Hm.  Optimally, the e.detail.returnValue that we pass in here wouldn't have the
|checked| property unless !!checkMsg.  But that's not a big deal; up to you if
you want to fix it.

>+      is(e.detail.buttons[0].messageType, 'custom');
>+      is(e.detail.buttons[0].message, expectedMessage.resend);
>+      is(e.detail.buttons[1].messageType, 'builtin');
>+      is(e.detail.buttons[1].message, 'cancel');

Please check e.detail.buttons.length.

>+      is(e.detail.message, expectedMessage.message);
>+      is(e.detail.showCheckbox, false);
>+      is(e.detail.checkMessage, null);
>+      e.detail.unblock();
>+    }
>+  });
>+}

r- because we need to figure out the setTimeout issue.
Attachment #674599 - Flags: review?(justin.lebar+bug) → review-
(In reply to Justin Lebar [:jlebar] from comment #24)
> >+    // We don't expect browserelement to reload; use a timer to make sure
> >+    // it is not reloaded.
> >+    iframe.addEventListener('mozbrowserloadend', failBecauseReloaded);
> >+    timer = window.setTimeout(function() {
> >+      iframe.removeEventListener('mozbrowserloadend', failBecauseReloaded);
> >+      ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload");
> >+      SimpleTest.finish();
> >+    }, 1000);
> 
> This is asking for trouble (random oranges).
> 
> In the last patch, as I understood it (and I may have misunderstood!), we
> were
> doing a SetTimeout() to make sure that something /didn't/ happen after one
> second.  That's not ideal, but it's OK.  But here, you're checking that the
> repost happens after one second, and that's not OK.

I use setTimeout() here for the same reason as setTimeout() in previous
version: make sure 'mozbrowserloadend' isn't fired in a second. If
'mozbrowserloadend' is called before time out, the test will fail. But if 
'mozbrowserloadend' isn't called before time out, we might still need to
check if 'mozbrowsershowmodalprompt' has been fired, so I put a test in
the time out handler.

I think I should move the 'setTimeout' into the handler of
'mozbrowsershowmodalprompt', otherwise, if that event isn't fired in a
second, the test will fail.
> I think I should move the 'setTimeout' into the handler of
> 'mozbrowsershowmodalprompt', otherwise, if that event isn't fired in a
> second, the test will fail.

I think I understand, and what you're saying sounds right.
Fix previous patch according to comment 23 and comment 24.
Attachment #674590 - Attachment is obsolete: true
Attachment #675630 - Flags: review+
Attached patch Test case v3Splinter Review
1. Fix according to comment 24.
2. Move setTimeout() to handler of mozbrowsershowmodalprompt.
Attachment #674599 - Attachment is obsolete: true
Attachment #675631 - Flags: review?(justin.lebar+bug)
Comment on attachment 675631 [details] [diff] [review]
Test case v3

Yes, this looks good to me.
Attachment #675631 - Flags: review?(justin.lebar+bug) → review+
Let me know when you're ready to check this in.
You need to log in before you can comment on or make changes to this bug.