1220124 - chrome.tabs.onHighlighted undefined

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Goran Sumzer]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151016093648

Steps to reproduce:

called chrome.tabs.onHighlighted from background.js


Actual results:

ReferenceError: reference to undefined property chrome.tabs.onHighlighted

Comment 1

[Matthew Wein [:mattw]]

Could I get some clarification on how onHighlighted should function? The Firefox Doc says that the event should be "fired when the highlighted or selected tabs in a window changes."  Does this mean that whenever the mouse hovers over a tab it should fire, in addition to it firing whenever a tab is clicked on?

Comment 2

[Kris Maglione [:kmag]]

In our case, it should probably just be an alias for onActivated.

Chrome has a concept of highlighted tabs, where you can select multiple tabs to perform an operation on. So, if you click tab 1, hold shift, and then click tab 3, tabs 1-3 become highlighted, and tab 3 becomes active. If you then right click and click "Close Tabs", all three tabs are closed.

Since we don't have a similar concept, we only need to treat the currently active tab as highlighted.

Comment 3

[Matthew Wein [:mattw]]

Attachment: WIP: implement chrome.tabs.onHighlighted

I tried to write a test for this but I wasn't able to get my highlight listener to fire. I tried |browser.tabs.update(tabs[0].id, { active: true });| which selected the tab but the listener didn't get called.  Do you have some advice on how to write a simple test for this?
<none>

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8713899 [details] [diff] [review]
WIP: implement chrome.tabs.onHighlighted

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

The tests for this are going to be a bit tricky. If there were already tests for onActivated, you could base yours on them... but since there aren't, we should probably add them at the same time.

The easiest thing will probably be to base your tests on the browser_ext_tabs_create.js tests, and possibly a bit on the browser_ext_pageAction_context.js tests.

So, essentially, create a helper function that returns a promise when both the activated and highlighted events have happened, e.g.:

let promiseActivated = (tabId, windowId) => {
  let activatedPromise = new Promise(resolve => {
    let onActivated = details => {
      if (details.tabId === tabId && details.windowId === windowId) {
        browser.tabs.onActivated.removeListener(onActivated);
        browser.test.pass(`Got onActivated event for tab ${tabId}, window ${windowId}`);
        resolve();
      }
    };
    browser.tabs.onActivated.addListener(onActivated);
  });

  let highlightedPromise = new Promise(resolve => {
    let onHighlighted = details => {
      if (details.tabId === tabId && details.windowId === windowId) {
        browser.tabs.onHighlighted.removeListener(onHighlighted);
        browser.test.pass(`Got onHighlighted event for tab ${tabId}, window ${windowId}`);
        resolve();
      }
    };
    browser.tabs.onHighlighted.addListener(onHighlighted);
  });

  return Promise.all([activatedPromise, highlightedPromise]);
};

And then use the tabs API to create several tabs (making sure you get a highlighted and activated event after each create call), and then use browser.tabs.update to change the active tab once or twice.

Comment 5

[Matthew Wein [:mattw]]

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

Comment 6

[Matthew Wein [:mattw]]

Attachment: Add support for chrome.tabs.onhighlight

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8714927 [details] [diff] [review]
Add support for chrome.tabs.onhighlight

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

This looks good. Thanks.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js
@@ +60,5 @@
> +          "onActivated",
> +          "onHighlighted",
> +        ]);
> +      }).then(() => {
> +        browser.test.log(`Highlighting tab ${tabIds[0]}`);

Would be good to check that the events are for the right tabs here. And for the events below too.

@@ +88,5 @@
> +          "onHighlighted",
> +        ]);
> +      }).then(() => {
> +        for (let i = 0; i < tabIds.length; i++) {
> +          promiseTabs.remove(tabIds[i]);

We should probably do something like `return Promise.all(tabIds.map(id => promiseTabs.remove(id)))` here, just to be sure the tabs are gone when we end the test. We'll wind up with intermittent failures if they're not.

Comment 8

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=718c5600588e

Comment 9

[Matthew Wein [:mattw]]

Attachment: Add support for chrome.tabs.onhighlight

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8715538 [details] [diff] [review]
Add support for chrome.tabs.onhighlight

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

Sorry, I thought I'd reviewed this already.

It looks good, just needs a bit of minor cleanup. Thanks.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js
@@ +16,5 @@
> +        };
> +      });
> +      return result;
> +    }
> +    let promiseTabs = wrapAPI(browser.tabs);

This isn't necessary anymore. We can just use browser.tabs directly, since they now return promises.

@@ +78,5 @@
> +            "onActivated",
> +            "onHighlighted",
> +          ]);
> +        }).then(() => {
> +          resolve();

No need to use `new Promise` here. This code is equivalent to:

    function openTab(windowId) {
      browser.tabs.create({ windowId }).then(tab => {
        tabIds.push(tab.id);
        browser.test.log(`Opened tab ${tab.id}`);
        return expectEvents(tab.id, [
          "onActivated",
          "onHighlighted",
        ]);
      });
    }

@@ +96,5 @@
> +            "onActivated",
> +            "onHighlighted",
> +          ]);
> +        }).then(() => {
> +          resolve();

Same as above. This is equivalent to:

   function highlightTab(tabId) {
      browser.test.log(`Highlighting tab ${tabId}`);
      return browser.tabs.update(tabId, { active: true }).then(tab => {
        browser.test.assertEq(tab.id, tabId, `Tab ${tab.id} highlighted`);
        return expectEvents(tab.id, [
          "onActivated",
          "onHighlighted",
        ]);
      });
    }

Comment 11

[Matthew Wein [:mattw]]

Attachment: Add support for chrome.tabs.onHighlighted.

Comment 12

[Matthew Wein [:mattw]]

Thanks for the helpful feedback.

Comment 13

[Matthew Wein [:mattw]]

Attachment: Implement chrome.commands.getAll

Comment 14

[Matthew Wein [:mattw]]

(In reply to Matthew Wein [:mattw] from comment #13)
> Created attachment 8723346 [details] [diff] [review]
> Implement chrome.commands.getAll

This patch shouldn't have gotten added to this bug. I'm not sure how I mixed this up but I'll resolve it now.

Comment 15

[Matthew Wein [:mattw]]

Attachment: Add support for chrome.tabs.onhighlight. try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n --post-to-bugzilla

Comment 16

[Matthew Wein [:mattw]]

Attachment: Add support for chrome.tabs.onHighlighted

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90494d4d76ab

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/764c86198573

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Followup push to fix eslint issues: https://hg.mozilla.org/integration/mozilla-inbound/rev/764c86198573

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for e10s browser_ext_tabs_onHighlighted.js permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=22601997&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7882aa8df614

Comment 21

[Kris Maglione [:kmag]]

Hrm. That error again.

It happens when we wind up with moz-extension: URLs from disabled extensions in about:newtab, and the test machine happens to have its mouse cursor over one of them when the new tab page opens.

I'll take care of it.

Comment 22

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/57774ff596857a1567ff5e40ce44cbbe2ec8308a
Bug 1220124 - Add support for chrome.tabs.onhighlight. r=kmag

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57774ff59685

Comment 24

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onHighlighted