1071566 - Add new method in the the tabBrowser class to handle the TabSelect event

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Daniel Gherasim]

We handle the TabOpen event in _waitForTabOpened but we also need to correctly handle selecting already opened tabs. That would be the "TabSelect" event. (https://developer.mozilla.org/en-US/docs/Web/Events/TabSelect).

We would like to make use of the method in the tabBrowser.selectedIndex setter and also when trying to open different in-content pages (preferences, about accounts page etc.) which, if already open, will just select the tab.

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

Comment 2

[Andrei Eftimie]

Comment on attachment 8511964 [details] [diff] [review]
v1.patch

Review of attachment 8511964 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good enhancement.

See inline for some comments.

::: firefox/lib/tabs.js
@@ +875,5 @@
> +    if (typeof aSpec.index === "number") {
> +      tab = this.getTab(aSpec.index);
> +    }
> +    else {
> +      assert.ok(aSpec.tab, "Either index or tab should be defined");

We should have this assert before the if clause and it should test whether we have one or the other defined.

@@ +883,5 @@
> +    if (tab.getNode().selected) {
> +      return;
> +    }
> +
> +    assert.waitFor(function() {

Make this a fat arrow function

@@ +894,5 @@
> +          // Issue a mousemove event to allow the tab activation click event to propagate
> +          // Tab activation is disabled if the mouse is hovering over the close button
> +          // See: http://hg.mozilla.org/mozilla-central/file/e5b09585215f/browser/base/content/tabbrowser.xml#l4802
> +          tab.mouseEvent(null, null, {type: "mousemove"});
> +          this._controller.click(tab);

tab.click()

@@ +952,5 @@
> +   *
> +   * @param {function} aCallback
> +   *        Callback that selects the tab
> +   */
> +  _waitForTabSelect: function tabBrowser_waitForTabSelect(aCallback) {

Do we get notified if this fails for any reason?

Comment 3

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #2)
> Comment on attachment 8511964 [details] [diff] [review]
> v1.patch
> 
> Review of attachment 8511964 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +952,5 @@
> > +   *
> > +   * @param {function} aCallback
> > +   *        Callback that selects the tab
> > +   */
> > +  _waitForTabSelect: function tabBrowser_waitForTabSelect(aCallback) {
> 
> Do we get notified if this fails for any reason?

Do you mean something else then waiting for the TabSelect event to be triggered?

Comment 4

[Daniel Gherasim]

Attachment: v2.patch

Comment 5

[Andrei Eftimie]

Comment on attachment 8512467 [details] [diff] [review]
v2.patch

Review of attachment 8512467 [details] [diff] [review]:
-----------------------------------------------------------------

With the 2 nits fixed please ask a review from Henrik.

::: firefox/lib/tabs.js
@@ +873,5 @@
> +
> +    assert.ok((aSpec.tab || typeof aSpec.index === "number"),
> +              "Either index or tab should be defined");
> +    var tab = (typeof aSpec.index === "number") ? this.getTab(aSpec.index) : aSpec.tab;
> +    if (tab.getNode().selected) {

Leave an empty newline and add a small comment: // Request tab is already selected

@@ +955,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => tabSelected, "Tab has been selected");
> +    }
> +    catch (ex) { assert.fail(ex); }

Please let the assert live on a newline

Comment 6

[Daniel Gherasim]

Attachment: v3.patch

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8513368 [details] [diff] [review]
v3.patch

Review of attachment 8513368 [details] [diff] [review]:
-----------------------------------------------------------------

Some parts have to be updated and made more clear to understand. Mind adding also a small test for it?

::: firefox/lib/tabs.js
@@ +862,5 @@
> +   * @param {function} [aSpec.callback]
> +   *        Callback used for opening the tab
> +   * @param {string} [aSpec.method="tabClick"]
> +   *        Method used for opening the tab
> +   * @param {number} [aSpec.index]

Please move before method

@@ +868,5 @@
> +   * @param {MozElement} [aSpec.tab]
> +   *        Tab to select
> +   */
> +  selectTab: function tabBrowser_selectTab(aSpec={}) {
> +    var method = aSpec.method || "tabClick";

Please keep to use the real action behind, which will be 'click'.

@@ +870,5 @@
> +   */
> +  selectTab: function tabBrowser_selectTab(aSpec={}) {
> +    var method = aSpec.method || "tabClick";
> +
> +    assert.ok((aSpec.tab || typeof aSpec.index === "number"),

No need for the outer brackets. Add those only around the typeof comparison.

@@ +880,5 @@
> +      return;
> +    }
> +
> +    assert.waitFor(() => !tab.getNode().hasAttribute("busy"),
> +                   "The tab has loaded");

Can you please explain why that is necessary? Maybe good to add a comment. I do not see why we cannot switch a tab while it is e.g. loading.

@@ +952,5 @@
> +    var tabSelected = false;
> +
> +    function checkTabSelected() { tabSelected = true; }
> +    this._controller.window.addEventListener("TabSelect", checkTabSelected);
> +    try {

nit: please separate the try block.

@@ +958,5 @@
> +
> +      assert.waitFor(() => tabSelected, "Tab has been selected");
> +    }
> +    catch (ex) {
> +      assert.fail(ex);

I don't see the value of the catch block. Please remove it.

Comment 8

[Daniela Domnici]

Attachment: bug1071566_V1

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8531220 [details] [diff] [review]
bug1071566_V1

Review of attachment 8531220 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +858,5 @@
> +   * Select a tab using different methods
> +   *
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {MozElement} [aSpec.tab]

Please move it after index

@@ +870,5 @@
> +   */
> +  selectTab: function tabBrowser_selectTab(aSpec={}) {
> +    var method = aSpec.method || "click";
> +
> +    assert.ok(aSpec.tab || (typeof aSpec.index === "number"),

Tab/index is not relevant if we use a callback, so you may want to assert also for "|| aSpec.callback"

@@ +871,5 @@
> +  selectTab: function tabBrowser_selectTab(aSpec={}) {
> +    var method = aSpec.method || "click";
> +
> +    assert.ok(aSpec.tab || (typeof aSpec.index === "number"),
> +              "Either index or tab should be specified");

Given previous comment, you should also update the message to "Either index, tab or callback should be specified"

@@ +872,5 @@
> +    var method = aSpec.method || "click";
> +
> +    assert.ok(aSpec.tab || (typeof aSpec.index === "number"),
> +              "Either index or tab should be specified");
> +    var tab = (typeof aSpec.index === "number") ? this.getTab(aSpec.index) : aSpec.tab;

This initialization and the following if structure is only relevant when you don't use callback.

::: firefox/lib/tests/testTabs.js
@@ +93,5 @@
> + * Bug 1071566
> + */
> +function testTabSelect(){
> +  // Select first tab using selectTab -click method
> +  var selectFirstTab = tabBrowser.selectTab({ index: 1, method: "click"});

The selectTab funtion doesn't return a value, so you don't need to assign it to a variable. Please modify everywhere.

@@ +96,5 @@
> +  // Select first tab using selectTab -click method
> +  var selectFirstTab = tabBrowser.selectTab({ index: 1, method: "click"});
> +
> +  //Check if the first tab is selected
> +  assert.waitFor(function (){

I don't think you need a waitFor here, an assertions would be enough.

@@ +100,5 @@
> +  assert.waitFor(function (){
> +    return tabBrowser.selectedIndex === 1; }, "First tab has been selected");
> +
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectedIndex = 3;

Don't use selectedTab here. When you set it's value, the tab gets selected. You don't want to have the tab selected before selecting it using the new method.

@@ +101,5 @@
> +    return tabBrowser.selectedIndex === 1; }, "First tab has been selected");
> +
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectedIndex = 3;
> +  var selectThirdTab = tabBrowser.selectTab({ index: 3, method: "callback", callback: () => {controller.select(tabBrowser.selectedIndex)}});

* No need to assign to a variable
* With the changes requested in the lib, you don't need to specify the index nor the tab with the callback method
* controller.select(tabBrowser.selectedIndex)}} won't work here. You could use something like controller.click(tabBrowser.getTab(3)) instead
* This line is too long, you should split it

@@ +108,5 @@
> +  expect.equal(tabBrowser.selectedIndex, 3, "Third tab has been selected");
> +
> +  //Select last tab using selectTab via reference
> +  var lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  var selectLastTab = tabBrowser.selectTab({ tab: lastTab, method: "click"});

No need to assign to variable

@@ +111,5 @@
> +  var lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  var selectLastTab = tabBrowser.selectTab({ tab: lastTab, method: "click"});
> +
> +  //Check if the last tab is selected
> +  expect.equal(tabBrowser.selectedIndex, controller.tabs.length - 1, "Last tab has been selected");

Line is too long, please put the message on new line

Comment 10

[Daniela Domnici]

Attachment: selectTab_V1

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8531987 [details] [diff] [review]
selectTab_V1

Review of attachment 8531987 [details] [diff] [review]:
-----------------------------------------------------------------

This was v5 :)
Still needs some small updates.

::: firefox/lib/tabs.js
@@ +872,5 @@
> +    var method = aSpec.method || "click";
> +
> +    assert.ok(aSpec.tab || (typeof aSpec.index === "number") || aSpec.callback,
> +              "Either index, tab or callback should be specified");
> +    if (!aSpec.callback) {

Delimit the if with a blank line before it

@@ +874,5 @@
> +    assert.ok(aSpec.tab || (typeof aSpec.index === "number") || aSpec.callback,
> +              "Either index, tab or callback should be specified");
> +    if (!aSpec.callback) {
> +      var tab = (typeof aSpec.index === "number") ? this.getTab(aSpec.index) : aSpec.tab;
> +  

2 extra spaces

@@ +879,5 @@
> +      // Request tab is already selected
> +      if (tab.getNode().selected) {
> +        return;
> +      }
> +   }

The bracket is not aligned, please add another space before it.

::: firefox/lib/tests/testTabs.js
@@ +93,5 @@
> + * Bug 1071566
> + */
> +function testTabSelect(){
> +  // Select first tab using selectTab -click method
> +  selectFirstTab = tabBrowser.selectTab({ index: 1, method: "click"});

* You don't need to assign this to a variable.
* Remove the space between { and index

@@ +95,5 @@
> +function testTabSelect(){
> +  // Select first tab using selectTab -click method
> +  selectFirstTab = tabBrowser.selectTab({ index: 1, method: "click"});
> +
> +  //Check if the first tab is selected

Whitespace after //

@@ +96,5 @@
> +  // Select first tab using selectTab -click method
> +  selectFirstTab = tabBrowser.selectTab({ index: 1, method: "click"});
> +
> +  //Check if the first tab is selected
> +  assert.waitFor(function (){

Use fat arrow function

@@ +100,5 @@
> +  assert.waitFor(function (){
> +    return tabBrowser.selectedIndex === 1; }, "First tab has been selected");
> +
> +  // Select third tab using selectTab - callback method
> +  selectThirdTab = tabBrowser.selectTab({ method: "callback",

* You don't need to assign this to a variable.
* Remove the space between { and method

@@ +101,5 @@
> +    return tabBrowser.selectedIndex === 1; }, "First tab has been selected");
> +
> +  // Select third tab using selectTab - callback method
> +  selectThirdTab = tabBrowser.selectTab({ method: "callback",
> +                                          callback: () => {

The callback function can remain on the same line, with the }); from the end on a new line

@@ +104,5 @@
> +  selectThirdTab = tabBrowser.selectTab({ method: "callback",
> +                                          callback: () => {
> +                                            controller.click(tabBrowser.getTab(3))}});
> +
> +  //Check if the third tab is selected

Whitespace after //

@@ +105,5 @@
> +                                          callback: () => {
> +                                            controller.click(tabBrowser.getTab(3))}});
> +
> +  //Check if the third tab is selected
> +  assert.waitFor(function (){

Use fat arrow function

@@ +108,5 @@
> +  //Check if the third tab is selected
> +  assert.waitFor(function (){
> +    return tabBrowser.selectedIndex === 3; }, "Third tab has been selected");
> +
> +  //Select last tab using selectTab via reference

Whitespace after //

@@ +110,5 @@
> +    return tabBrowser.selectedIndex === 3; }, "Third tab has been selected");
> +
> +  //Select last tab using selectTab via reference
> +  lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  selectLastTab = tabBrowser.selectTab({ tab: lastTab, method: "click"});

* You don't need to assign this to a variable.
* Remove the space between { and tab

@@ +112,5 @@
> +  //Select last tab using selectTab via reference
> +  lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  selectLastTab = tabBrowser.selectTab({ tab: lastTab, method: "click"});
> +
> +  //Check if the last tab is selected

Whitespace after //

@@ +113,5 @@
> +  lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  selectLastTab = tabBrowser.selectTab({ tab: lastTab, method: "click"});
> +
> +  //Check if the last tab is selected
> +  assert.waitFor(function (){

Use fat arrow function

Comment 12

[Daniela Domnici]

Attachment: patch_V6

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8532023 [details] [diff] [review]
patch_V6

Review of attachment 8532023 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me now.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8532023 [details] [diff] [review]
patch_V6

Review of attachment 8532023 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +860,5 @@
> +   * @param {object} [aSpec]
> +   *        Information about how to open the tab
> +   * @param {function} [aSpec.callback]
> +   *        Callback used for opening the tab
> +   * @param {string} [aSpec.method="tabClick"]

I don't see that 'tabClick' is a valid value.

@@ +863,5 @@
> +   *        Callback used for opening the tab
> +   * @param {string} [aSpec.method="tabClick"]
> +   *        Method used for opening the tab
> +   * @param {number} [aSpec.index]
> +   *        Index of the tab to select

nit: has still not been moved before .method.

@@ +888,5 @@
> +        case "click":
> +          // Issue a mousemove event to allow the tab activation click event to propagate
> +          // Tab activation is disabled if the mouse is hovering over the close button
> +          // See: http://hg.mozilla.org/mozilla-central/file/e5b09585215f/browser/base/content/tabbrowser.xml#l4802
> +          tab.mouseEvent(undefined, undefined, {type: "mousemove"});

How does this prevent us from not clicking on the close button? Especially for smaller tab widths? mouseEvent() would move the cursor to the middle of the tab if no offset is given. That is the exact same position if we only do a click. So I don't see why this extra mousemove helps here?

@@ +899,5 @@
> +          break;
> +        default:
> +          assert.fail("Unknown method - " + aSpec.method);
> +      }
> +    });

I would add an extra check that the right tab has been selected.

@@ +960,5 @@
> +      assert.waitFor(() => tabSelected, "Tab has been selected");
> +    }
> +    finally {
> +      this._controller.window.removeEventListener("TabSelect", checkTabSelected);
> +    }

We should not create new waitForXYZ methods on classes. This should be handled transparently in the selectTab method.

::: firefox/lib/tests/testTabs.js
@@ +97,5 @@
> +  tabBrowser.selectTab({index: 1, method: "click"});
> +
> +  // Check if the first tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === 1;}, "First tab has been selected");

assert.waitFor(() => (tabBrowser.selectedIndex === 1),
               "First tab has been selected");

@@ +102,5 @@
> +
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectTab({method: "callback",
> +                        callback: () => {controller.click(tabBrowser.getTab(3))}
> +                       });

Please fix the indenation and spaces. Better to also move method into the new line.

@@ +106,5 @@
> +                       });
> +
> +  // Check if the third tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === 3;}, "Third tab has been selected");

Same as above.

@@ +115,5 @@
> +
> +  // Check if the last tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === controller.tabs.length - 1;},
> +           "Last tab has been selected");

Same as above.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> > +        case "click":
> > +          // Issue a mousemove event to allow the tab activation click event to propagate
> > +          // Tab activation is disabled if the mouse is hovering over the close button
> > +          // See: http://hg.mozilla.org/mozilla-central/file/e5b09585215f/browser/base/content/tabbrowser.xml#l4802
> > +          tab.mouseEvent(undefined, undefined, {type: "mousemove"});
> 
> How does this prevent us from not clicking on the close button? Especially
> for smaller tab widths? mouseEvent() would move the cursor to the middle of
> the tab if no offset is given. That is the exact same position if we only do
> a click. So I don't see why this extra mousemove helps here?
This doesn't suppose to prevent us from clicking on close button. We added this because when the tab opens, the close button it animates. If it animates under the real mouse, it will set the mOverCloseButton flag to true, now even if we click on the middle of the tab it won't select it because the flag won't set to false without a mousemove event. Removing this code will cause the intermittent failures from bug 880135 to come back.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan from comment #15)
> true, now even if we click on the middle of the tab it won't select it
> because the flag won't set to false without a mousemove event.

So exactly this part is the important one which has been dismissed from the comment in the code. Make sure to explain everything that good in the code, so we can save those extra rounds on Bugzilla and later.

Comment 17

[Daniela Domnici]

Attachment: patch_V7

Updated patch.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8537194 [details] [diff] [review]
patch_V7

Review of attachment 8537194 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +895,5 @@
> +           * We added this because when the tab opens, the close button it animates.
> +           * If it animates under the real mouse, it will set the mOverCloseButton flag to true,
> +           * now even if we click on the middle of the tab it won't select it because
> +           * the flag won't set to false without a mousemove event.
> +           */

The comment is too long for a single phrase. 
/** When the tab opens, the close button animates. 
 * If it animates under the real mouse, it will set 
 * the mOverCloseButton flag to true. 
 * Even with a click in the middle of the tab, the flag doesn't get set
 * to false without a mousemove event and so the tab isn't selected.
 */

::: firefox/lib/tests/testTabs.js
@@ +98,5 @@
> +                        method: "click"});
> +
> +  // Check if the first tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === 1;

You can remove return all together.

@@ +104,5 @@
> +
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectTab({method: "callback", callback:() => {
> +    controller.click(tabBrowser.getTab(3))
> +  }});

tabBrowser.selectTab({
  method: "callback", 
  callback: () => {
    controller.click(tabBrowser.getTab(3));
  }
});

I think this way is more readable and maybe this is what Henrik intended with "move method to a new line".

@@ +109,5 @@
> +
> +  // Check if the third tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === 3;
> +  }, "Third tab has been selected");

Same here with return

@@ +118,5 @@
> +                        method: "click"});
> +
> +  // Check if the last tab is selected
> +  assert.waitFor(() => {
> +    return tabBrowser.selectedIndex === controller.tabs.length - 1;

Same here, use brackets.

Comment 19

[Daniela Domnici]

Attachment: patch_V8

Updated patch

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8538544 [details] [diff] [review]
patch_V8

Review of attachment 8538544 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Daniela. Passing to Henrik for the final look.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8538544 [details] [diff] [review]
patch_V8

Review of attachment 8538544 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +863,5 @@
> +   *        Callback used for opening the tab
> +   * @param {number} [aSpec.index]
> +   *        Index of the tab to select
> +   * @param {string} [aSpec.method="click"]
> +   *        Method used for opening the tab

Please list the possible values in the description.

@@ +878,5 @@
> +    if (!aSpec.callback) {
> +      var tab = (typeof aSpec.index === "number") ? this.getTab(aSpec.index)
> +                                                  : aSpec.tab;
> +
> +      // Request tab is already selected

nit: requested

::: firefox/lib/tests/testTabs.js
@@ +93,5 @@
> + * Bug 1071566
> + */
> +function testTabSelect(){
> +  // Select first tab using selectTab -click method
> +  tabBrowser.selectTab({index: 1,

Index starts usually with 0, how can this be the first tab we select here?

@@ +94,5 @@
> + */
> +function testTabSelect(){
> +  // Select first tab using selectTab -click method
> +  tabBrowser.selectTab({index: 1,
> +                        method: "click"});

Click is default. so you don't necessarily need that.

@@ +103,5 @@
> +
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectTab({
> +    method: "callback",
> +    callback:() => {

nit: missing blank after colon.

@@ +104,5 @@
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectTab({
> +    method: "callback",
> +    callback:() => {
> +      controller.click(tabBrowser.getTab(3));

Does tabBrowser.getTab(3).click() not work?

@@ +113,5 @@
> +  assert.waitFor(() => (tabBrowser.selectedIndex === 3),
> +                 "Third tab has been selected");
> +
> +  // Select last tab using selectTab via reference
> +  lastTab = tabBrowser.getTab(controller.tabs.length - 1);

tabBrowser.length

@@ +115,5 @@
> +
> +  // Select last tab using selectTab via reference
> +  lastTab = tabBrowser.getTab(controller.tabs.length - 1);
> +  tabBrowser.selectTab({tab: lastTab,
> +                        method: "click"});

click is default and not needed.

@@ +118,5 @@
> +  tabBrowser.selectTab({tab: lastTab,
> +                        method: "click"});
> +
> +  // Check if the last tab is selected
> +  assert.waitFor(() => (tabBrowser.selectedIndex === controller.tabs.length - 1),

Same as above.

Comment 22

[Daniela Domnici]

Attachment: patch_V9

Updated patch

Comment 23

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8539289 [details] [diff] [review]
patch_V9

Review of attachment 8539289 [details] [diff] [review]:
-----------------------------------------------------------------

Changes requested were added.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8539289 [details] [diff] [review]
patch_V9

Review of attachment 8539289 [details] [diff] [review]:
-----------------------------------------------------------------

Just a nit to fix. Otherwise it looks good.

::: firefox/lib/tests/testTabs.js
@@ +103,5 @@
> +  // Select third tab using selectTab - callback method
> +  tabBrowser.selectTab({
> +    method: "callback",
> +    callback: () => {
> +      tabBrowser.getTab(3).click();

This is still not getting the 3rd but the 4th tab.

Comment 25

[Daniela Domnici]

Attachment: patch_V10

Updated patch.Thanks Henrik for your reviews.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8540122 [details] [diff] [review]
patch_V10

Review of attachment 8540122 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/d20ee8375530 (default)
Thanks Daniela!

Comment 27

[Teodor Druta]

We should really backport this at least to mozilla-release and mozilla-beta.
It blocks Bug 1122516 for those branches.

Comment 28

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/63a68316134e (beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/8ebd2193b430 (release)

We need a new patch for esr.

Comment 29

[Daniela Domnici]

Attachment: patch_V1_esr31

Created new patch for esr31 branch.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555236 [details] [diff] [review]
patch_V1_esr31

Review of attachment 8555236 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/qa/mozmill-tests/rev/810b7afc6635 (esr31)

Comment 31

[Andreea Matei [:AndreeaMatei]]

I think this caused some failures on esr, we need to better check this.

http://mozmill-daily.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2015-01-26&to=2015-01-28&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatus

https://hg.mozilla.org/qa/mozmill-tests/rev/84cd03fab97d (esr31)

Comment 32

[Daniela Domnici]

Attachment: patch_V2_esr31

Updated patch for esr31 branch.
I`ve did functional and remote testruns. Here are the results:
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/4d9c8aea57549df57dc5846cc0031430

Remote:  http://mozmill-crowd.blargon7.com/#/remote/report/4d9c8aea57549df57dc5846cc0025b60

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8557836 [details] [diff] [review]
patch_V2_esr31

Review of attachment 8557836 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/qa/mozmill-tests/rev/bb9289e06449 (esr31)