Closed Bug 1428847 Opened 6 years ago Closed 6 years ago

Move is_hidden and is_visible into BrowserTestUtils

Categories

(Firefox :: General, enhancement, P5)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: johannh, Assigned: kanika16047)

References

Details

Attachments

(1 file, 4 obsolete files)

There's a sad amount of duplicate is_hidden and is_visible functions in our tests (https://searchfox.org/mozilla-central/search?q=(is_hidden%7Cis_visible)&case=false&regexp=true&path=head.js). We should probably move it into BTU.
Assignee: nobody → kanika16047
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review235814


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/siteIdentity/head.js:132
(Diff revision 1)
>      const insecureConnectionIcon = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled");
>      if (!insecureConnectionIcon) {
>        // HTTP request, there should be no MCB classes for the identity box and the non secure icon
>        // should always be visible regardless of MCB state.
>        ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page");
>        ok(is_hidden(connectionIcon), "connection icon should be hidden");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:136
(Diff revision 1)
>        ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page");
>        ok(is_hidden(connectionIcon), "connection icon should be hidden");
>      } else {
>        // HTTP request, there should be a broken padlock shown always.
>        ok(classList.contains("notSecure"), "notSecure on HTTP page");
>        ok(!is_hidden(connectionIcon), "connection icon should be visible");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:154
(Diff revision 1)
>      is(classList.contains("mixedDisplayContent"), passiveLoaded && !(activeLoaded || activeBlocked),
>         "identityBox has expected class for passiveLoaded && !(activeLoaded || activeBlocked)");
>      is(classList.contains("mixedDisplayContentLoadedActiveBlocked"), passiveLoaded && activeBlocked,
>         "identityBox has expected class for passiveLoaded && activeBlocked");
>  
>      ok(!is_hidden(connectionIcon), "connection icon should be visible");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:246
(Diff revision 1)
>      let promiseViewShown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "ViewShown");
>      doc.getElementById("identity-popup-security-expander").click();
>      await promiseViewShown;
>      is(Array.filter(doc.getElementById("identity-popup-securityView")
>                         .querySelectorAll("[observes=identity-popup-mcb-learn-more]"),
>                      element => !is_hidden(element)).length, 1,

Error: 'is_hidden' is not defined. [eslint: no-undef]
Status: NEW → ASSIGNED
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review236116

Thank you for picking this up!

I'm holding off on fully reviewing this patch for now. Please upload an updated patch with fixes after you run the mochitests locally. It's a good idea to always check that your changes to test files are not breaking the tests. Here's how you can do that:

./mach mochitest <path_to_file/path_to_folder>

If you have made changes to a head.js file, it's usually a good idea to run all mochitests that are in the folder containing your head.js to check if all the tests are passing. An head.js file is a support file that helps run other tests in a folder.

::: browser/base/content/test/permissions/head.js
(Diff revision 1)
>    if (element.parentNode != element.ownerDocument)
>      return is_hidden(element.parentNode);
>  
>    return false;
>  }
> -

Please add the extra line you removed from here

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:228
(Diff revision 1)
> +    if (style.display == "-moz-popup")
> +      return ["hiding", "closed"].includes(element.state);
> +
> +    // Hiding a parent element will hide all its children
> +    if (element.parentNode != element.ownerDocument)
> +      return is_hidden(element.parentNode);

This should be BrowserTestUtils.is_hidden ...

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:244
(Diff revision 1)
> +    if (style.display == "-moz-popup" && element.state != "open")
> +      return false;
> +
> +    // Hiding a parent element will hide all its children
> +    if (element.parentNode != element.ownerDocument)
> +      return is_visible(element.parentNode);

See above. This should be BrowserTestUtils.is_visible
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review-
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review236354


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/siteIdentity/head.js:132
(Diff revision 1)
>      const insecureConnectionIcon = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled");
>      if (!insecureConnectionIcon) {
>        // HTTP request, there should be no MCB classes for the identity box and the non secure icon
>        // should always be visible regardless of MCB state.
>        ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page");
>        ok(is_hidden(connectionIcon), "connection icon should be hidden");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:136
(Diff revision 1)
>        ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page");
>        ok(is_hidden(connectionIcon), "connection icon should be hidden");
>      } else {
>        // HTTP request, there should be a broken padlock shown always.
>        ok(classList.contains("notSecure"), "notSecure on HTTP page");
>        ok(!is_hidden(connectionIcon), "connection icon should be visible");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:154
(Diff revision 1)
>      is(classList.contains("mixedDisplayContent"), passiveLoaded && !(activeLoaded || activeBlocked),
>         "identityBox has expected class for passiveLoaded && !(activeLoaded || activeBlocked)");
>      is(classList.contains("mixedDisplayContentLoadedActiveBlocked"), passiveLoaded && activeBlocked,
>         "identityBox has expected class for passiveLoaded && activeBlocked");
>  
>      ok(!is_hidden(connectionIcon), "connection icon should be visible");

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: browser/base/content/test/siteIdentity/head.js:246
(Diff revision 1)
>      let promiseViewShown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "ViewShown");
>      doc.getElementById("identity-popup-security-expander").click();
>      await promiseViewShown;
>      is(Array.filter(doc.getElementById("identity-popup-securityView")
>                         .querySelectorAll("[observes=identity-popup-mcb-learn-more]"),
>                      element => !is_hidden(element)).length, 1,

Error: 'is_hidden' is not defined. [eslint: no-undef]
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review236116

I see that many of the mochitests are failing but I cannot understand why. Can you please tell if I need to import BTU wherever I'm using BTU.is_visible/is_hidden? Although I don't see it imported in other files.
Hi Kanika, it looks like you created a new changeset for the changes instead of amending your existing changeset. Please upload your original patch after amending it with the suggested changes for review.

Here's how you can do that:

- Select your original changeset by running 'hg up <changeset_ID>'. You can get the changeset ID of your original patch by running 'hg wip'.

- Make necessary changes to files based on my suggestions in Comment 3.

- Run 'hg commit --amend -m"your_commit_message" ' to commit those changes.

- Run 'hg push review' to submit that changeset.


I'll review your patch once this is done and we can figure out why the tests are failing. Thanks!
Attachment #8962032 - Flags: review?(prathikshaprasadsuman)
Attachment #8962032 - Attachment is obsolete: true
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review237050

Thank you! There's a few things left to do before we can land this bug.


Please change the commit message to make it sound imperative. Something like:

"Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. r?prathiksha"


And please rebase this patch on top of central.

::: browser/base/content/test/general/browser_bug633691.js:9
(Diff revision 2)
>  
>  add_task(async function test() {
>    const URL = "data:text/html,<iframe width='700' height='700'></iframe>";
>    await BrowserTestUtils.withNewTab({ gBrowser, url: URL }, async function(browser) {
>      await ContentTask.spawn(browser,
>                              { is_element_hidden_: is_element_hidden.toSource(),

This test needs a little bit of work before it can run correctly. So I think it's a good idea to leave it as it is for now and solve it in a follow up bug!

I guess that also means not removing is_hidden from browser/base/content/test/general/head.js. Could you please add it back there? :)

::: browser/base/content/test/permissions/head.js:1
(Diff revision 2)
>  ChromeUtils.import("resource:///modules/SitePermissions.jsm", this);

Please remove this file

::: browser/base/content/test/permissions/head.js:1
(Diff revision 2)
>  ChromeUtils.import("resource:///modules/SitePermissions.jsm", this);

Also, remove head.js from the support-files section of the browser.ini file that's in the same folder.

::: browser/base/content/test/webextensions/head.js
(Diff revision 2)
>    // generates references to the former and generic add-ons manager code
>    // generates referces to the latter.
>    return (icon == "chrome://browser/content/extension.svg" ||
>            icon == "chrome://mozapps/skin/extensions/extensionGeneric.svg");
>  }
> -

Please add a new line here

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:216
(Diff revision 2)
>                                    PROCESSSELECTOR_CONTRACTID, null);
>        }
>      }
>      return Promise.all(promises).then(() => tab);
>    },
>  

Add a JS-doc style comment here

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:232
(Diff revision 2)
> +    if (element.parentNode != element.ownerDocument)
> +      return BrowserTestUtils.is_hidden(element.parentNode);
> +
> +    return false;
> +  },
> +

Add a JS-doc style comment here also
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review-
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review238374

::: browser/base/content/test/permissions/head.js:1
(Diff revision 2)
>  ChromeUtils.import("resource:///modules/SitePermissions.jsm", this);

ok, no, looks like this file still has an import. Please don't remove this file. There's also no need to change the support section of head.js in that case. Sorry :)
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

I'm also tagging Johann for a final review so we can be sure here.
Attachment #8961443 - Flags: review- → review?(jhofmann)
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review239148

I think Prathiksha has mentioned everything that's left to do here. We should defer fixing browser_bug633691.js to a follow-up bug and the functions in BTU still need JSDoc at the top (see the other functions in that file for reference).

Thank you for working on this!
Attachment #8961443 - Flags: review?(jhofmann)
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review239148

Thank you. I have pushed in an updated patch. :)
Please take a look for the final review.
And it'd be great to file that bug and work on it too!
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review238374

> ok, no, looks like this file still has an import. Please don't remove this file. There's also no need to change the support section of head.js in that case. Sorry :)

Thanks a lot for the review! :)
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review239642

Almost done, thanks!

Some things to note:

It's always a good idea to run eslint before submitting a patch for review. Here's how you can do that:
./mach eslint <path_to_file/path_to_folder>

We should probably also do a try run to make sure that the patch does not break anything(https://wiki.mozilla.org/ReleaseEngineering/TryServer) and this looks like a great opportunity for you to trigger your first try run. To do that, you'll need to first get Level 1 access. You can read about how to get commit access here(https://www.mozilla.org/en-US/about/governance/policies/commit/). Once you have filed a bug for it, feel free to CC me or Johann to vouch for you. :)

::: browser/base/content/test/general/browser_bug633691.js:10
(Diff revision 3)
>  add_task(async function test() {
>    const URL = "data:text/html,<iframe width='700' height='700'></iframe>";
>    await BrowserTestUtils.withNewTab({ gBrowser, url: URL }, async function(browser) {
>      await ContentTask.spawn(browser,
>                              { is_element_hidden_: is_element_hidden.toSource(),
> -                              is_hidden_: is_hidden.toSource() },
> +                              is_hidden_: BrowserTestUtils.is_hidden.toSource() },

Please revert this change

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:224
(Diff revision 3)
> +   *
> +   * @param {Element} element
> +   *        The element which is to be checked.
> +   *
> +   * @return {boolean}
> +   *        

trailing spaces here, please remove this line :)

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:249
(Diff revision 3)
> +   *
> +   * @param {Element} element
> +   *        The element which is to be checked.
> +   *
> +   * @return {boolean}
> +   *        

trailing spaces here also, remove the line
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review-
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review240408

Please run eslint before submitting a patch for review. You can avoid all the trailing spaces and other linting errors that way. Here's how you can do that:
./mach eslint <path_to_file/path_to_folder>

I will r+ this patch once we have good try results since this patch includes a lot of changes to test files. You can schedule a try run once you get your commit access on reviewboard under the automation menu. Feel free to ask questions if you get stuck.

::: browser/base/content/test/permissions/head.js
(Diff revision 5)
>  ChromeUtils.import("resource:///modules/SitePermissions.jsm", this);
> -

I think you need a newline at the end of this file

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:223
(Diff revision 5)
> +   * Checks if a DOM element is hidden.
> +   *
> +   * @param {Element} element
> +   *        The element which is to be checked.
> +   *
> +   * @return {boolean}      

trailing whitespaces here

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:247
(Diff revision 5)
> +   * Checks if a DOM element is visible.
> +   *
> +   * @param {Element} element
> +   *        The element which is to be checked.
> +   *
> +   * @return {boolean}       

trailing whitespaces here also
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review-
Hi Kanika, I just saw your try results and it looks like mochitests weren't included in your last try run. Please schedule a new try run with this as your try syntax:

try: -b do -p linux64,macosx64,win64 -u mochitests -t none --artifact
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review242476

Thank you for working on this. Just a few things left to change.

Please rebase your patch on central to avoid toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js from failing on try. :)

::: browser/base/content/test/general/head.js:477
(Diff revision 6)
> -  ok(is_visible(element), msg || "Element should be visible");
> +  ok(BrowserTestUtils.is_visible(element), msg || "Element should be visible");
>  }
>  
>  function is_element_hidden(element, msg) {
>    isnot(element, null, "Element should not be null, when checking visibility");
> -  ok(is_hidden(element), msg || "Element should be hidden");
> +  ok(BrowserTestUtils.is_hidden(element), msg || "Element should be hidden");

Please revert this change to stop browser/base/content/test/general/browser_bug633691.js from failing :)

::: toolkit/mozapps/extensions/test/browser/head.js:476
(Diff revision 6)
> -  if (aElement.parentNode != aElement.ownerDocument)
> -    return is_hidden(aElement.parentNode);
> -
> -  return false;
> -}
>  

And remove the extra line from here
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review-
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review242908

Looks great, thank you!
Attachment #8961443 - Flags: review?(prathikshaprasadsuman) → review+
Thank you :)
Is there a need for setting the check-in needed flag?
(In reply to kanika16047 from comment #25)
> Thank you :)
> Is there a need for setting the check-in needed flag?

To get your patch landed? Yes. :)

I scheduled another try run for you today and it looks like toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js still needs a fix, and sorry, I wasn't very clear in my review about what needs to get fixed there. Could you look into it before setting the checkin-needed flag? I'm very busy this week. You can ping Johann on IRC if you have any questions.
Hi Kanika, are you still working on this? I'd be happy to help you push this over the finish line :)
Flags: needinfo?(kanika16047)
Hi, I'm really sorry I got busy with the exams. Yes, I'm continuing working on this. Thank you for understanding.
Flags: needinfo?(kanika16047)
Ok, sure, no worries!
Hi, I ran ./mach mochitest browser/base/content/test/general/browser_bug633691.js after rebasing with the latest code and the test ran successfully. Should I push the patch for review to be able to trigger another try run?
(In reply to kanika16047 from comment #30)
> Hi, I ran ./mach mochitest
> browser/base/content/test/general/browser_bug633691.js after rebasing with
> the latest code and the test ran successfully. Should I push the patch for
> review to be able to trigger another try run?

Yes, you can upload the updated patch(run hg push review again) and schedule another try run on it. If the try results look green, we can then land your latest revision. :)
Comment on attachment 8961443 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/230190/#review248334


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/test/browser/browser_list.js:169
(Diff revision 8)
>    is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden");
>  
>    info("Addon 5");
>    addon = items["Test add-on 5"];
>    addon.parentNode.ensureElementIsVisible(addon);
>    await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link")));

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: toolkit/mozapps/extensions/test/browser/browser_list.js:225
(Diff revision 8)
>    is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden");
>  
>    info("Addon 8");
>    addon = items["Test add-on 8"];
>    addon.parentNode.ensureElementIsVisible(addon);
>    await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link")));

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: toolkit/mozapps/extensions/test/browser/browser_list.js:247
(Diff revision 8)
>    is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden");
>  
>    info("Addon 9");
>    addon = items["Test add-on 9"];
>    addon.parentNode.ensureElementIsVisible(addon);
>    await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link")));

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: toolkit/mozapps/extensions/test/browser/browser_list.js:524
(Diff revision 8)
>    is(Object.keys(items).length, EXPECTED_ADDONS, "Should be the right number of add-ons installed");
>  
>    info("Addon 10");
>    let addon = items["Test add-on 10"];
>    addon.parentNode.ensureElementIsVisible(addon);
>    await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link")));

Error: 'is_hidden' is not defined. [eslint: no-undef]

::: toolkit/mozapps/extensions/test/browser/browser_list.js:545
(Diff revision 8)
>    is(get_node(addon, "error-link").href, infoURL, "Error link should be correct");
>  
>    info("Addon 11");
>    addon = items["Test add-on 11"];
>    addon.parentNode.ensureElementIsVisible(addon);
>    await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link")));

Error: 'is_hidden' is not defined. [eslint: no-undef]
It seems like you'll have to replace the usage in toolkit/mozapps/extensions/test/browser/browser_list.js as well :)
(In reply to Johann Hofmann [:johannh] from comment #0)
> We should probably move it into BTU.

Since this is also useful for mochitest-plain and mochitest-chrome it would probably be better in TestUtils, or even better, part of SimpleTest.js which is already in most of those test scopes (though I don't understand how much we're allowed to change that).
(In reply to Johann Hofmann [:johannh] from comment #34)
> It seems like you'll have to replace the usage in
> toolkit/mozapps/extensions/test/browser/browser_list.js as well :)

Hi, fixed that but there are a few more test jobs that are failing on try run.
Can you please help me figure out what else needs to be changed?
(In reply to kanika16047 from comment #37)
> (In reply to Johann Hofmann [:johannh] from comment #34)
> > It seems like you'll have to replace the usage in
> > toolkit/mozapps/extensions/test/browser/browser_list.js as well :)
> 
> Hi, fixed that but there are a few more test jobs that are failing on try
> run.
> Can you please help me figure out what else needs to be changed?

I think you just forgot to replace these occurrences:

https://searchfox.org/mozilla-central/search?q=is_hidden&case=false&regexp=false&path=browser_details.js
https://searchfox.org/mozilla-central/search?q=is_hidden&case=false&regexp=false&path=browser_discovery.js

That should be simple to fix :)

Thanks for all the work!
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #36)
> (In reply to Johann Hofmann [:johannh] from comment #0)
> > We should probably move it into BTU.
> 
> Since this is also useful for mochitest-plain and mochitest-chrome it would
> probably be better in TestUtils, or even better, part of SimpleTest.js which
> is already in most of those test scopes (though I don't understand how much
> we're allowed to change that).

Hm, yeah, I agree, but considering that this is a pretty big mentored bug I'd prefer to push that into a follow-up (where we could also figure out the SimpleTest parts). I realize that that would mean touching all that code again, but I'm honestly just happy with the state of having one common implementation somewhere, in thise case BTU. What do you think?
Yeah, that's reasonable.
Hi Kanika, would you have time to fix up the last two occurrences mentioned in comment 38? It would be great to be able to land this.
Flags: needinfo?(kanika16047)
Attachment #8961443 - Attachment is obsolete: true
Comment on attachment 8982213 [details]
Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils

https://reviewboard.mozilla.org/r/248192/#review254460

This looks good but something is not right with the way these commits are organized, can you please squash them into a single commit?

Thanks :)
Attachment #8982213 - Flags: review?(jhofmann)
Comment on attachment 8982212 [details]
Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils

https://reviewboard.mozilla.org/r/248190/#review254462
Attachment #8982213 - Attachment is obsolete: true
(In reply to Johann Hofmann [:johannh] from comment #44)
> Comment on attachment 8982213 [details]
> Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils
> 
> https://reviewboard.mozilla.org/r/248192/#review254460
> 
> This looks good but something is not right with the way these commits are
> organized, can you please squash them into a single commit?
> 
> Thanks :)

Sorry for the delay. Does this look better now? 
I followed http://docs.firefox-dev.tools/contributing/making-prs.html to squash commits.
Flags: needinfo?(kanika16047)
Comment on attachment 8982212 [details]
Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils

https://reviewboard.mozilla.org/r/248190/#review256974

Unfortunately it's not really what we need (the entire patch not a subset of it). Maybe you can ping me on IRC or Slack and we can quickly resolve this together? :)

Thanks!
Attachment #8982212 - Flags: review?(jhofmann)
Attachment #8982212 - Attachment is obsolete: true
Comment on attachment 8985142 [details]
Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm.

https://reviewboard.mozilla.org/r/250832/#review257210

Looks great, thank you!
Attachment #8985142 - Flags: review?(jhofmann) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/684094d78fb4
Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. r=johannh
https://hg.mozilla.org/mozilla-central/rev/684094d78fb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Blocks: 1875090
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: