Closed Bug 1243838 Opened 6 years ago Closed 5 years ago

Fix test_contextmenu.html to run in e10s

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

The test may need to be rewritten as a browser-chrome test.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8715074 [details]
MozReview Request: Bug 1243838 - Rewrite test_contextmenu.html (-> browser_contextmenu.html) to run in e10s. r?mconley

https://reviewboard.mozilla.org/r/33331/#review30133

Hey Jared,

This looks good - especially because I can tell it was probably a pain in the butt to convert.

I have some recommendations on ways we can get rid of some global variables in the test scope, and some other minor notes - but I don't think it should block landing and enabling this test.

Address the ones you feel are justified, push back against the ones you don't, and please file bugs for the ones that are justified but probably not worth doing right now.

Thanks!

::: browser/base/content/test/general/browser_contextmenu.js:3
(Diff revision 1)
> +let tab;
> +let contextMenu;

Does these really need to be global?

::: browser/base/content/test/general/browser_contextmenu.js:5
(Diff revision 1)
> +let inspectItems = [];

It seems to me that we're using these inspectItems all over the place when checking the context menu. Can't we just alter test_contextmenu to check for the inspectItems items if the devtools.inspector.enabled pref is set to true?

I think that'd simplify things a little bit.

::: browser/base/content/test/general/browser_contextmenu.js:6
(Diff revision 1)
> +let loginFillItems = [

Should we perhaps name this LOGIN_FILL_ITEMS or something to help indicate that this is kind of like a test constant that will be used in a number of tests?

::: browser/base/content/test/general/browser_contextmenu.js:17
(Diff revision 1)
> +  const example_base = "http://example.com/browser/browser/base/content/test/general/";

One thing I wonder about is if we should have just a single add_task, and an array of context menu test object things that the single add_task can iterate to run. That way, you don't need to many globals, and you don't need to put the clean-up of removing the tab in its own task at the end of the test file.

::: browser/base/content/test/general/browser_contextmenu.js:39
(Diff revision 1)
> +let plainTextItems;
> +add_task(function* test_plaintext() {
> +  plainTextItems = ["context-navigation",   null,
> +                        ["context-back",         false,
> +                         "context-forward",      false,
> +                         "context-reload",       true,
> +                         "context-bookmarkpage", true], null,
> +                    "---",                  null,
> +                    "context-savepage",     true,
> +                    "---",                  null,
> +                    "context-viewbgimage",  false,
> +                    "context-selectall",    true,
> +                    "---",                  null,
> +                    "context-viewsource",   true,
> +                    "context-viewinfo",     true
> +                   ].concat(inspectItems);
> +  yield test_contextmenu("#test-text", plainTextItems);
> +});

Can we combine this test with test_plaintext2 so that we don't have to keep plainTextItems around in the global scope?

::: browser/base/content/test/general/browser_contextmenu.js:294
(Diff revision 1)
> +  // Disabled since the spell checker completes before
> +  // this test is started.
> +  todo(false, "spell checker tests are failing");
> +  return;

Can you please file a bug / bugs to get this and the other spellcheck bugs enabled?

::: browser/base/content/test/general/browser_contextmenu.js:508
(Diff revision 1)
> +        Services.prefs.setBoolPref("full-screen-api.allow-trusted-requests-only", false);
> +        Services.prefs.setCharPref("full-screen-api.transition-duration.enter", "0 0");
> +        Services.prefs.setCharPref("full-screen-api.transition-duration.leave", "0 0");

If you use pushPrefs from head.js instead, I don't think you have to worry about cleaning up / reverting your prefs later.

::: browser/base/content/test/general/browser_contextmenu.js:685
(Diff revision 1)
> +          let doc = content.document;
> +          let win = doc.defaultView;
> +          win.getSelection().removeAllRanges();

Why is it necessary to remove all ranges first? Don't we expect to have no selected ranges at this time?

::: browser/base/content/test/general/browser_contextmenu.js:723
(Diff revision 1)
> +        Services.prefs.setBoolPref("plugins.click_to_play", true);

Again, pushPrefs means you don't need to worry about resetting this pref back to the default.

::: browser/base/content/test/general/browser_contextmenu.js:799
(Diff revision 1)
> +function* test_contextmenu(selector, menuItems, options={}) {

Let's add a docstring above this function to help describe what it does.

::: browser/base/content/test/general/browser_contextmenu.js:800
(Diff revision 1)
> +  contextMenu = document.getElementById("contentAreaContextMenu")

Missing semi-colon.

Does contextMenu really need to be global? Can't this function use its own reference to it?

::: browser/base/content/test/general/browser_contextmenu.js:808
(Diff revision 1)
> +  yield ContentTask.spawn(tab.linkedBrowser,

Do we need to use tab here, or can we use gBrowser.selectedBrowser? Will we ever be doing a test for anything except the selected browser? I suspect not...

::: browser/base/content/test/general/browser_contextmenu.js:816
(Diff revision 1)
> +    element.focus();

Won't focusing this element automatically cause a blur on anything that was previously focused? Why do we need to do the blur manually?

::: browser/base/content/test/general/browser_contextmenu.js:861
(Diff revision 1)
> +function selectText(selector) {

DocString for this please

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:498
(Diff revision 1)
> +      Services.console.logStringMessage(`selecting ${target} at ${offsetX}, ${offsetY} (centered: ${event.centered})`);

Just double-checking - do you mean to keep this around?
Attachment #8715074 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> ::: browser/base/content/test/general/browser_contextmenu.js:3
> (Diff revision 1)
> > +let tab;
> > +let contextMenu;
> 
> Does these really need to be global?

Yes, because the global `contextMenu` is referenced from contextmenu_common.js which is imported as a subscript by the test.
 
> ::: browser/base/content/test/general/browser_contextmenu.js:5
> (Diff revision 1)
> > +let inspectItems = [];
> 
> It seems to me that we're using these inspectItems all over the place when
> checking the context menu. Can't we just alter test_contextmenu to check for
> the inspectItems items if the devtools.inspector.enabled pref is set to true?
> 
> I think that'd simplify things a little bit.

Yeah, it looks like we can do this.

> ::: browser/base/content/test/general/browser_contextmenu.js:6
> (Diff revision 1)
> > +let loginFillItems = [
> 
> Should we perhaps name this LOGIN_FILL_ITEMS or something to help indicate
> that this is kind of like a test constant that will be used in a number of
> tests?

Sure.

> ::: browser/base/content/test/general/browser_contextmenu.js:17
> (Diff revision 1)
> > +  const example_base = "http://example.com/browser/browser/base/content/test/general/";
> 
> One thing I wonder about is if we should have just a single add_task, and an
> array of context menu test object things that the single add_task can
> iterate to run. That way, you don't need to many globals, and you don't need
> to put the clean-up of removing the tab in its own task at the end of the
> test file.

Perhaps. I'm indifferent here, I don't see a major issue with the globals as the test is not included by other code, so I would prefer to leave this as-is.

> ::: browser/base/content/test/general/browser_contextmenu.js:39
> (Diff revision 1)
> > +let plainTextItems;
> > +add_task(function* test_plaintext() {
> > +  plainTextItems = ["context-navigation",   null,
> > +                        ["context-back",         false,
> > +                         "context-forward",      false,
> > +                         "context-reload",       true,
> > +                         "context-bookmarkpage", true], null,
> > +                    "---",                  null,
> > +                    "context-savepage",     true,
> > +                    "---",                  null,
> > +                    "context-viewbgimage",  false,
> > +                    "context-selectall",    true,
> > +                    "---",                  null,
> > +                    "context-viewsource",   true,
> > +                    "context-viewinfo",     true
> > +                   ].concat(inspectItems);
> > +  yield test_contextmenu("#test-text", plainTextItems);
> > +});
> 
> Can we combine this test with test_plaintext2 so that we don't have to keep
> plainTextItems around in the global scope?

These two tests are checking that the menu remains constant before and after some actions, so we can't really combine them.

> ::: browser/base/content/test/general/browser_contextmenu.js:294
> (Diff revision 1)
> > +  // Disabled since the spell checker completes before
> > +  // this test is started.
> > +  todo(false, "spell checker tests are failing");
> > +  return;
> 
> Can you please file a bug / bugs to get this and the other spellcheck bugs
> enabled?

Will do.

> ::: browser/base/content/test/general/browser_contextmenu.js:508
> (Diff revision 1)
> > +        Services.prefs.setBoolPref("full-screen-api.allow-trusted-requests-only", false);
> > +        Services.prefs.setCharPref("full-screen-api.transition-duration.enter", "0 0");
> > +        Services.prefs.setCharPref("full-screen-api.transition-duration.leave", "0 0");
> 
> If you use pushPrefs from head.js instead, I don't think you have to worry
> about cleaning up / reverting your prefs later.

Cool, I didn't know about this before.

> ::: browser/base/content/test/general/browser_contextmenu.js:685
> (Diff revision 1)
> > +          let doc = content.document;
> > +          let win = doc.defaultView;
> > +          win.getSelection().removeAllRanges();
> 
> Why is it necessary to remove all ranges first? Don't we expect to have no
> selected ranges at this time?

I was just following the test_contextmenu.html code.

> ::: browser/base/content/test/general/browser_contextmenu.js:816
> (Diff revision 1)
> > +    element.focus();
> 
> Won't focusing this element automatically cause a blur on anything that was
> previously focused? Why do we need to do the blur manually?

Ditto.
https://hg.mozilla.org/integration/fx-team/rev/6226f2e8a41b637c428862cf6931462bf59938ff
Bug 1243838 - Rewrite test_contextmenu.html (-> browser_contextmenu.html) to run in e10s. r=mconley
https://hg.mozilla.org/mozilla-central/rev/6226f2e8a41b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.