Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

As seen on bug 1315611 we currently do not check for already existent modal / tabmodal dialogs when initializing GeckoDriver in driver.js. As such tests are blocked because they cannot close this modal dialog.

So this is also required for bug 1279211.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 7 years ago

So I have a working solution locally. I'm only adding some unit tests now for this change.

A downside is that looking for existing tab modal dialogs on Fennec doesn't work that easily as for Firefox with getTabModalPromptBox(). Actually I didn't find a way to check for those. James, do you know if a method which could be utilized here? Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

The try build shows issues if e10s is disabled. I have to investigate that.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

Try build as reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a9291bc9d5&selectedJob=85751838

I can reproduce this problem locally with e10s disabled. By debugging I noticed that in case of a modal dialog the win.opener reference is null, while with e10s enabled it always points to the chrome window which opened the modal dialog.

Olli, do you think that this is a bug in Firefox, or is that intended? If it's the latter how can I find out which of the open browser windows has the modal dialog when iterating through windows retrieved via getEnumerator()?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 7 years ago

Since I don't know what GeckoDriver is doing, is there perhaps some difference whether privileged code is being run in e10s vs non-e10s?
http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/dom/base/nsGlobalWindow.cpp#5259-5279

Could you perhaps check whether nsGlobalWindow actually does have opener, but just doesn't return it in non-e10s case?
If there isn't opener even in C++ level, that does sound like a bug to me.
In that case some minimal testcase would be good.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 6 • 7 years ago

We don't really have anyone that knows that code anymore, but it looks to me like you need to do something here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/PromptService.js#162

So maybe adding something to nsIPromptService to query modal state would make sense? You could probably use that one call for both desktop and fennec.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #5)
> Since I don't know what GeckoDriver is doing, is there perhaps some
> difference whether privileged code is being run in e10s vs non-e10s?

The problem here is that this specific Marionette test is turning off the preference "prompts.tab_modal.enabled", which makes all tab modal dialog a modal dialog. As this works fine with e10s turned on, it fails with e10s disabled.

Given that this practice tests not-supported code, I will go ahead and refactor the modal dialog test to actually use real modal dialogs like a HTTP Basic Auth. As shown in a minimal test, this will work as expected.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #6)
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> PromptService.js#162
> 
> So maybe adding something to nsIPromptService to query modal state would
> make sense? You could probably use that one call for both desktop and fennec.

Thank you James! I will have a look at this.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Given that this practice tests not-supported code, I will go ahead and
> refactor the modal dialog test to actually use real modal dialogs like a
> HTTP Basic Auth. As shown in a minimal test, this will work as expected.

Looks like it's not necessary. When I'm using the getZOrderDOMWindowEnumerator I could simply pick the next window and check if it is the browser window we currently have selected.

> > So maybe adding something to nsIPromptService to query modal state would
> > make sense? You could probably use that one call for both desktop and fennec.
> 
> Thank you James! I will have a look at this.

So I had a look, and I don't actually want to fiddle around with Fennec code. So I will simply special-case the checks for Firefox only.

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review125154

::: commit-message-20123:3
(Diff revision 1)
> +To ensure that Marionette can also detect modal dialogs opened right after the application starts, and
> +before Marionette has been initialized, the modal dialog detection code has to be delayed until a new
> +session actually gets started. Then it's not enough to only register the observer notification, but it
> +should also be checked for open modal or tab modal dialogs.

Wrap at ~72 characters.

::: commit-message-20123:1
(Diff revision 2)
> +Bug 1348872 - Marionette has to check for existent modal dialogs in GeckoDriver initialization code.
> +
> +To ensure that Marionette can also detect modal dialogs opened right after the application starts, and
> +before Marionette has been initialized, the modal dialog detection code has to be delayed until a new
> +session actually gets started. Then it's not enough to only register the observer notification, but it
> +should also be checked for open modal or tab modal dialogs.

Reduce length of first line and wrap subsequent lines at ~72 characters.

::: testing/marionette/driver.js:217
(Diff revision 2)
> +// Global listener for modal dialogs
> +GeckoDriver.prototype.globalModalDialogHandler = function (subject, topic) {

The comment is pretty much repeated in the function name.  Remove.

::: testing/marionette/driver.js:219
(Diff revision 2)
> +  let winr;
> +  if (topic === modal.COMMON_DIALOG_LOADED) {
> +    // Always keep a weak reference to the current dialog
> +    winr = Cu.getWeakReference(subject);
> +  }
> +  this.dialog = new modal.Dialog(() => this.curBrowser, winr);

I know this was already in driver.js, but it doesn’t belong here.

::: testing/marionette/driver.js:234
(Diff revision 2)
> +GeckoDriver.prototype.initModalDialogs = function () {
> +  // Setup global listener for modal dialogs
> +  modal.addHandler(this.globalModalDialogHandler.bind(this));

It doesn’t feel right to add the handler and look for existing dialogues in the same call.  I would suggest making them two separate invocations.

::: testing/marionette/driver.js:238
(Diff revision 2)
> +  // Check if there is already an open modal dialog present for the current
> +  // browser window.
> +  let winEn = Services.wm.getZOrderDOMWindowEnumerator(null, true);
> +  while (winEn.hasMoreElements()) {
> +    let win = winEn.getNext();
> +
> +    if (win.document.documentURI === "chrome://global/content/commonDialog.xul") {
> +      // Most of the modal dialogs have a reference to the chrome window which opened
> +      // it. But some (eg. clear recent history) don't have. So simply pick the
> +      // next chrome window which basically should have opened the modal dialog.
> +      let opener = win.opener || winEn.getNext();
> +
> +      if (opener && opener === this.curBrowser.window) {
> +        return new modal.Dialog(() => this.curBrowser, Cu.getWeakReference(win));
> +      }
> +    }
> +  }

It feels like this check should live in testing/marionette/modal.js.

::: testing/marionette/driver.js:256
(Diff revision 2)
> +  // If no modal dialog has been found, also check if there is an open tab modal
> +  // dialog present for the current tab.
> +  // TODO: Find an adequate implementation for Fennec.
> +  if (this.curBrowser.tab && this.curBrowser.tabBrowser.getTabModalPromptBox) {
> +    let contentBrowser = browser.getBrowserForTab(this.curBrowser.tab);
> +    let promptManager = this.curBrowser.tabBrowser.getTabModalPromptBox(contentBrowser);
> +    let prompts = promptManager.listPrompts();
> +
> +    if (prompts.length) {
> +      return new modal.Dialog(() => this.curBrowser, null);
> +    }
> +  }

Likewise for this check.  It feels like the abstractions aren’t right here.

::: testing/marionette/harness/marionette_harness/runner/mixins/window_manager.py:24
(Diff revision 2)
> -        if len(self.marionette.chrome_window_handles) != len(self.start_windows):
> +        if len(self.marionette.chrome_window_handles) > len(self.start_windows):
>              raise Exception("Not all windows as opened by the test have been closed")
>  
> -        if len(self.marionette.window_handles) != len(self.start_tabs):
> +        if len(self.marionette.window_handles) > len(self.start_tabs):
>              raise Exception("Not all tabs as opened by the test have been closed")

This change makes the exception message not hold true any more.  What is the reason for this change?  I don’t understand why we expect there to be leftover windows after the test.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review125154

> I know this was already in driver.js, but it doesn’t belong here.

I tried to minimize the code changes before the large upcoming refactoring on bug 1263661.

Also the handler is only used in driver.js, and no-where else. So why should it be in modal.js? It's just a driver/browser specific requirement.

> This change makes the exception message not hold true any more.  What is the reason for this change?  I don’t understand why we expect there to be leftover windows after the test.

It still does. But keep in mind that there might have been a modal dialog open at the beginning of the test, which then gets closed. That means at the end of the test you will have a `window_count - 1`. The above still passes for an equal window count, or if it's lower than at the beginning. That's all what we want.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago

Hm, I got notified about bug 156333 which covers the fact that `getZOrderDOMWindowEnumerator` is still somewhat broken on Linux. But that should generally only affect our unit test, which changes the preference `prompts.tab_modal.enabled` to an unsupported value.

So we would have to update the test to use real modal dialogs, or which is easier now and also unblocks us from getting the patch on bug 1315611 landed on beta, I can add the usage of `WindowManagerMixin` to the `test_modal_dialogs.py` file to ensure we have only a single window left open after each test. I definitely prefer the latter one.

Unhappily there are lots of failures in the try run, which I cannot reproduce locally because they only happen for Linus and Windows:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3efe96f7e5a&selectedJob=86061957

I think that I will pick a one click loaner to figure that out.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #12)
> So we would have to update the test to use real modal dialogs, or which is
> easier now and also unblocks us from getting the patch on bug 1315611 landed
> on beta, I can add the usage of `WindowManagerMixin` to the
> `test_modal_dialogs.py` file to ensure we have only a single window left
> open after each test. I definitely prefer the latter one.

Looking more on the try build I think that I was wrong here. In case a modal dialog is already open, we get two windows by the enumerator. If the browser window comes first, we would not assign the modal because there is no next window to check for the opener. 

I think that we have to update the test to make use of a real modal dialog.

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review126250

::: testing/marionette/driver.js:671
(Diff revision 3)
> +  // Setup global listener for modal dialogs, and check if there is already
> +  // one open for the currently selected browser window.
> +  modal.addHandler(this.globalModalDialogHandler.bind(this));
> +  this.dialog = modal.findModalDialogs(this.curBrowser);

Thanks.  This looks like a more sane delegation to me.

::: testing/marionette/driver.js:2387
(Diff revision 3)
>        Services.obs.removeObserver(this.observing[topic], topic);
>      }
>      this.observing = null;
>    }
>  
> +  modal.removeHandler(this.globalModalDialogHandler.bind(this));

When you rebind this.globalModalDialogHandler again here, is it guaranteed to be the same object that you passed to modal.addHandler on :673?

This aspect of JS has always confused me, and I just want to make sure we are actually removing the handler successfully.

::: testing/marionette/modal.js:57
(Diff revision 3)
> + * Check for already existing modal or tab modal dialogs
> + *
> + * @param {browser.Context} context
> + *     Reference to the browser context to check for existent dialogs.
> + *
> + * @return {Dialog}

s/Dialog/modal.Dialog/

	Andreas Tolfsen ❲:ato❳
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8851518 [details]
Bug 1348872 - Add contentBrowser property to browser's Context class.

https://reviewboard.mozilla.org/r/123846/#review126254

::: testing/marionette/browser.js:128
(Diff revision 1)
>      this._browserWasRemote = null;
>      this._hasRemotenessChange = false;
>    }
>  
>    /**
> +   * Returns the content browser for the currently selected tab.

Also explain null behaviour.

::: testing/marionette/browser.js:148
(Diff revision 1)
>    get curFrameId() {
>      let rv = null;
>      if (this.driver.appName == "B2G") {
>        rv = this._curFrameId;
>      } else if (this.tab) {
> -      rv = this.getIdForBrowser(browser.getBrowserForTab(this.tab));
> +      rv = this.getIdForBrowser(this.contentBrowser);

I like this change.

::: testing/marionette/browser.js:314
(Diff revision 1)
> -        if (target == browser.getBrowserForTab(this.tab)) {
> -          this.updateIdForBrowser(browser.getBrowserForTab(this.tab), uid);
> +        if (target === this.contentBrowser) {
> +          this.updateIdForBrowser(this.contentBrowser, uid);

This simplifies the code a lot.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review125154

> I tried to minimize the code changes before the large upcoming refactoring on bug 1263661.
> 
> Also the handler is only used in driver.js, and no-where else. So why should it be in modal.js? It's just a driver/browser specific requirement.

The latest revision looks better as more code is moved to testing/marionette/modal.js.  The reason I don’t feel this belongs in testing/marionette/driver.js is that it leaks implementation details about how dialogues must be stored as weak refs &c.

But since your patch isn’t making this any worse than the status quo, I’m resolving this issue.

> It still does. But keep in mind that there might have been a modal dialog open at the beginning of the test, which then gets closed. That means at the end of the test you will have a `window_count - 1`. The above still passes for an equal window count, or if it's lower than at the beginning. That's all what we want.

OK.  I still don’t think the English prose here is being entirely truthful, though.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #15)

> (In reply to Henrik Skupin (:whimboo) from comment #12)
>
> > So we would have to update the test to use real modal dialogs,
> > or which is easier now and also unblocks us from getting the
> > patch on bug 1315611 landed on beta, I can add the usage of
> > `WindowManagerMixin` to the `test_modal_dialogs.py` file to ensure
> > we have only a single window left open after each test. I definitely
> > prefer the latter one.
>
> Looking more on the try build I think that I was wrong here. In case a
> modal dialog is already open, we get two windows by the enumerator. If
> the browser window comes first, we would not assign the modal because
> there is no next window to check for the opener.
>
> I think that we have to update the test to make use of a real modal
> dialog.

Can you explain what you mean by “real modal dialog” here?  It
seems somewhat likely that we could drop support for global modal
dialogues and just support tab modal dialogues if that would make the
implementation easier.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 20 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #19)
> I think that we have to update the test to make use of a real modal
> > dialog.
> 
> Can you explain what you mean by “real modal dialog” here?  It
> seems somewhat likely that we could drop support for global modal
> dialogues and just support tab modal dialogues if that would make the
> implementation easier.

Dialogs like HTTP Basic authentication, clear recent history (under history menu), or even the printer dialog (but not sure about this one). We should not fake those dialogs.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 21 • 7 years ago
mozreview-review-reply

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review126250

> When you rebind this.globalModalDialogHandler again here, is it guaranteed to be the same object that you passed to modal.addHandler on :673?
> 
> This aspect of JS has always confused me, and I just want to make sure we are actually removing the handler successfully.

Good call! This is actually not the case. Calling `bind()` on the method again, will actually create a different copy/ref. So calling removeHandler will not remove it, and we add additional handlers each time we create a new session. This clearly needs a fix.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8851518 [details]
Bug 1348872 - Add contentBrowser property to browser's Context class.

https://reviewboard.mozilla.org/r/123846/#review126672

::: testing/marionette/driver.js:1106
(Diff revision 2)
>    if (!this.curBrowser.tab) {
>      // Navigation does not work for non-browser windows
>      return;
>    }
>  
> -  let contentBrowser = browser.getBrowserForTab(this.curBrowser.tab)
> +  if (!this.curBrowser.contentBrowser.webNavigation.canGoBack) {

This copy&paste mistake slipped through the review. I will fix it with the next update of this patch.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 28 • 7 years ago

Andreas, please also check the remaining unreviewed latest changes for the other two commits. Thanks.

	Andreas Tolfsen ❲:ato❳
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8851518 [details]
Bug 1348872 - Add contentBrowser property to browser's Context class.

https://reviewboard.mozilla.org/r/123846/#review126810

	Andreas Tolfsen ❲:ato❳
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8851979 [details]
Bug 1348872 - Marionette unit tests should test real modal dialogs.

https://reviewboard.mozilla.org/r/124240/#review126812

::: commit-message-82d83:3
(Diff revision 1)
> +Until now the unit tests only covered modal dialogs as shown when
> +disabling the preference `prompts.tab_modal.enabled`. This actually
> +tests an unsupported feature. Replacing it with a real modal dialog
> +like HTTP authentication is what we really want here.

Good discovery!

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:39
(Diff revision 1)
> +                        "Tab modal alerts should be enabled by default."))
> +
> +        self.marionette.navigate(self.marionette.absolute_url('test_tab_modal_dialogs.html'))
> +
> +    def tearDown(self):
> +        self.marionette.execute_script("window.onbeforeunload = null;")

If we navigate in setUp, is this necessary?

	Andreas Tolfsen ❲:ato❳
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8850190 [details]
Bug 1348872 - Check for existent modal dialogs for new Marionette sessions.

https://reviewboard.mozilla.org/r/122866/#review126816

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:98
(Diff revision 5)
>          alert = self.marionette.switch_to_alert()
>          alert.dismiss()
>          self.wait_for_condition(lambda mn: mn.find_element(By.ID, 'prompt-result').text == 'null')
>  
> +    def test_alert_opened_before_session_starts(self):
> +        self.marionette.find_element(By.ID, 'tab-modal-alert').click()

Double quotes, please.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:242
(Diff revision 5)
>  
>          status = self.marionette.find_element(By.ID, "status")
>          self.assertEqual(status.text, "restricted")
> +
> +    def test_alert_opened_before_session_starts(self):
> +        self.marionette.navigate(self.marionette.absolute_url('http_auth'))

Double quotes, please.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:52
(Diff revision 5)
> +        import time
>          # Helper method to run simple back and forward testcases.
>          for index, page in enumerate(test_pages):
>              if "error" in page:
>                  with self.assertRaises(page["error"]):
>                      self.marionette.navigate(page["url"])
>              else:
>                  self.marionette.navigate(page["url"])
> +            time.sleep(1)
>              self.assertEqual(page["url"], self.marionette.get_url())
>              self.assertEqual(self.history_length, index + 1)
>  
>          for page in test_pages[-2::-1]:
>              if "error" in page:
>                  with self.assertRaises(page["error"]):
>                      self.marionette.go_back()
>              else:
>                  self.marionette.go_back()
> +                time.sleep(1)
>              self.assertEqual(page["url"], self.marionette.get_url())
>  
>          for page in test_pages[1::]:
>              if "error" in page:
>                  with self.assertRaises(page["error"]):
>                      self.marionette.go_forward()
>              else:
>                  self.marionette.go_forward()
> +            time.sleep(1)
>              self.assertEqual(page["url"], self.marionette.get_url())

Debugging statements?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:82
(Diff revision 5)
> -    def test_no_history_items(self):
> +    def tst_no_history_items(self):
>          # Both methods should not raise a failure if no navigation is possible
>          self.marionette.go_back()
>          self.marionette.go_forward()
>  
>      def test_data_urls(self):
>          test_pages = [
>              {"url": inline("<p>foobar</p>")},
>              {"url": self.test_page},
>              {"url": inline("<p>foobar</p>")},
>          ]
>          self.run_test(test_pages)
>  
> -    def test_same_document_hash_change(self):
> +    def tst_same_document_hash_change(self):
>          test_pages = [
>              {"url": "{}#23".format(self.test_page)},
>              {"url": self.test_page},
>              {"url": "{}#42".format(self.test_page)},
>          ]
>          self.run_test(test_pages)
>  
>      @skip("Causes crashes for JS GC (bug 1344863) and a11y (bug 1344868)")
> -    def test_frameset(self):
> +    def tst_frameset(self):
>          test_pages = [
>              {"url": self.marionette.absolute_url("frameset.html")},
>              {"url": self.test_page},
>              {"url": self.marionette.absolute_url("frameset.html")},
>          ]
>          self.run_test(test_pages)
>  
> -    def test_frameset_after_navigating_in_frame(self):
> +    def tst_frameset_after_navigating_in_frame(self):

s/tst/test/g

	Andreas Tolfsen ❲:ato❳
	Comment 32 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Andreas, please also check the remaining unreviewed latest changes for the
> other two commits. Thanks.

Checked.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 33 • 7 years ago

Latest try still shows failures mainly for Windows builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab77dd64c61

> TEST-UNEXPECTED-ERROR | test_modal_dialogs.py TestModalAlerts.test_http_auth_dismiss | NoSuchElementException: Unable to locate element: status

When I was talking with Andreas over IRC you raised a good point that this could be a timing issue because we are trying to access an element after a page load. So to be safe we should extend the timeout for the necessary checks to the given page load timeout.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 34 • 7 years ago
mozreview-review-reply

Comment on attachment 8851979 [details]
Bug 1348872 - Marionette unit tests should test real modal dialogs.

https://reviewboard.mozilla.org/r/124240/#review126812

> If we navigate in setUp, is this necessary?

This is the fallback for tests which set the onbeforeunload handler. We really have to reset it here to ensure that we do not leak it, or the next test is affected by a formerly set handler.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 38 • 7 years ago

Andreas, please have a look for the following changes:
https://reviewboard.mozilla.org/r/122864/diff/5-6/

Thanks

	Andreas Tolfsen ❲:ato❳
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8851518 [details]
Bug 1348872 - Add contentBrowser property to browser's Context class.

https://reviewboard.mozilla.org/r/122864/#review127002

r+

	Andreas Tolfsen ❲:ato❳
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8852193 [details]
Bug 1348872 - Wait() should accept None as timeout and interval.

https://reviewboard.mozilla.org/r/124426/#review127004

	Andreas Tolfsen ❲:ato❳
	Comment 41 • 7 years ago
mozreview-review-reply

Comment on attachment 8851979 [details]
Bug 1348872 - Marionette unit tests should test real modal dialogs.

https://reviewboard.mozilla.org/r/124240/#review126812

> This is the fallback for tests which set the onbeforeunload handler. We really have to reset it here to ensure that we do not leak it, or the next test is affected by a formerly set handler.

Makes sense.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 42 • 7 years ago

All tests are passing, and no remaining open issues in the reviews. So we are good for getting it landed.

	Pulsebot
	Comment 43 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da0f2767b8d0
Add contentBrowser property to browser's Context class. r=ato
https://hg.mozilla.org/integration/autoland/rev/c5355e013dcf
Wait() should accept None as timeout and interval. r=ato
https://hg.mozilla.org/integration/autoland/rev/dac3987cbdc6
Marionette unit tests should test real modal dialogs. r=ato
https://hg.mozilla.org/integration/autoland/rev/7df43a5dca0d
Check for existent modal dialogs for new Marionette sessions. r=ato

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/da0f2767b8d0
https://hg.mozilla.org/mozilla-central/rev/c5355e013dcf
https://hg.mozilla.org/mozilla-central/rev/dac3987cbdc6
https://hg.mozilla.org/mozilla-central/rev/7df43a5dca0d

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 45 • 7 years ago

There are no obvious regressions from landing this. So I would suggest we get this test-only patch uplifted to aurora and beta. If all plays well with the GFX patch on bug 1315611, we should also uplift to esr52 and if still possible to the 52 release branch.

I won't be around the next days, so it would be great if someone could cover possible backport changes to the patch.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 46 • 7 years ago

Ryan, why did you cancel the uplift request? It's not clear to me, even without giving any comment when removing the whiteboard entries. Thanks.

I was hoping to have this already on beta when coming back from PTO. :/

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #46)
> Ryan, why did you cancel the uplift request? It's not clear to me, even
> without giving any comment when removing the whiteboard entries. Thanks.

I had commented in another one of the bugs this depends on at the same time. Sorry if that wasn't clear enough. Andreas also said on IRC at the time that he'd be working on the rebase.

> I was hoping to have this already on beta when coming back from PTO. :/

Then make sure they apply cleanly before you go. Sorry if I disappointed you...

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 48 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #47)
> I had commented in another one of the bugs this depends on at the same time.
> Sorry if that wasn't clear enough. Andreas also said on IRC at the time that
> he'd be working on the rebase.

Sorry, but I do not see any comment on another bug I'm working on. Can you please give me a link?

> > I was hoping to have this already on beta when coming back from PTO. :/
> 
> Then make sure they apply cleanly before you go. Sorry if I disappointed
> you...

If we cannot land a patch on a branch we should make sure to add the comment on the bug it is related to. 

Anyway, I will have a look why it cannot be uplifted.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 49 • 7 years ago

I tested everything and each individual commit applies cleanly for the aurora branch. So not sure what wasn't working for you.

	Ryan VanderMeulen [:RyanVM]
	Comment 50 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/7362d3d66d54
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8935d652686
https://hg.mozilla.org/releases/mozilla-aurora/rev/43bc1b59535b
https://hg.mozilla.org/releases/mozilla-aurora/rev/22f422b9ff0a

Doesn't graft cleanly to Beta.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 51 • 7 years ago

It's a kinda complicated uplift due to all the dependencies which are nearly circular.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 52 • 7 years ago

Given the complexity of the uplift, the right order and results are all layed out on bug 1322383.

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/5084200c9256
https://hg.mozilla.org/releases/mozilla-beta/rev/205219485c9e
https://hg.mozilla.org/releases/mozilla-beta/rev/df5268d3497b
https://hg.mozilla.org/releases/mozilla-beta/rev/42024440f945

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 54 • 7 years ago

Today we decided to no longer uplift feature additions for Marionette to esr52. Only fixes for regressions or other critical issues will be uplifted.

Also as long as bug 1315611 (GFX window fix) is not present in the code we would also not hit this issue.