593645 - use popup.triggerNode instead of document.popupNode for tab context menu

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Frank Yan (:fryn)]

Attachment: part 1 of 2 (wait for part 2 to check this in)

Since bug 383930 was fixed and according to Neil Deakin's blog post (http://enndeakin.wordpress.com/2010/08/08/status-update-for-august-7/), document.popupNode is now deprecated. Let's use the new popup.triggerNode instead.

Comment 1

[Frank Yan (:fryn)]

Dao, since we now only show the tab context menu on the tabs themselves, can we also get rid of TabContextMenu.contextTab and just directly use aPopupMenu.triggerNode?

Comment 2

[Dão Gottwald [:dao]]

That sounds like it would complicate code currently accessing TabContextMenu.contextTab without direct access to the popup node.

Comment 3

[Frank Yan (:fryn)]

turns out that some tests rely on being able to set document.popupNode arbitarily, so I'll have to fix the tests.

Low priority, so I'll come back to this later.

Comment 4

[(no longer active)]

Indeed.  I landed it <http://hg.mozilla.org/projects/cedar/rev/a4a1307fc4e1>, it broke tests <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301271675.1301274934.12522.gz> and Karl backed it out <http://hg.mozilla.org/projects/cedar/rev/bbacb90f4c63>.

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 472216 [details] [diff] [review]
part 1 of 2 (wait for part 2 to check this in)

(In reply to comment #4)

Yeah, I warned about that. The tests shouldn't be using document.popupNode like that, even though it's the easy way. popup.triggerNode wasn't available then, so also no need to blame 'em. I'll get to this later.

Comment 6

[Frank Yan (:fryn)]

Attachment: part 2 of 2: fix tests

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests

oops, forgot to fill the requestee field.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests

>+  let tab = gBrowser.tabs[0];
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>+  menu.hidePopup();

Can you add a helper function for this in head.js?

Comment 9

[Frank Yan (:fryn)]

Attachment: part 2 of 2: fix tests v2

(In reply to Dão Gottwald [:dao] from comment #8)
... 
> Can you add a helper function for this in head.js?

Done.

Comment 10

[Frank Yan (:fryn)]

Attachment: part 2 of 2: fix tests v2

Added line to Makefile.in that I forgot.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 589448 [details] [diff] [review]
part 2 of 2: fix tests v2

>+++ b/browser/base/content/test/browser_visibleTabs_contextMenu.js

> function popup(tab) {
>-  document.popupNode = tab;
>-  TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>   is(TabContextMenu.contextTab, tab, "TabContextMenu context is the expected tab");
>-  TabContextMenu.contextTab = null;
>+  menu.hidePopup();
> }

>+++ b/browser/base/content/test/head.js

>+function updateTabContextMenu() {
>+  let tab = gBrowser.selectedTab;
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>+  menu.hidePopup();
>+}

updateTabContextMenu can take an optional tab argument and integrate the test that popup() above is doing.

Comment 12

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/fx-team/rev/224d7c74206f
https://hg.mozilla.org/integration/fx-team/rev/8be167cb61af

Comment 13

[Frank Yan (:fryn)]

Pushed with review comment addressed.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/224d7c74206f
https://hg.mozilla.org/mozilla-central/rev/8be167cb61af

Comment 15

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/mozilla-central/search?string=.popupNode&case=1
"Found 72 matching lines in 30 files"

Should this bug (have) fix these too?
Should they be fixed in a separate bug(s)?
Should they not be fixed?

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #15)

Increased to "Found 106 matching lines in 47 files".