Closed Bug 1400323 Opened 7 years ago Closed 6 years ago

Intermittent browser/components/places/tests/browser/browser_stayopenmenu.js | Bookmarks Menu Button's Popup should still be open. -

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Platform:
Unspecified
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox57 --- disabled
firefox58 --- disabled
firefox62 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: tawn)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [fxsearch][stockwell disabled])

Attachments

(1 file, 1 obsolete file)

Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden)
59 bytes, text/x-review-board-request
mak
: review+
Details 
Filed by: archaeopteryx [at] coole-files.de

https://treeherder.mozilla.org/logviewer.html#?job_id=131315462&repo=autoland

https://queue.taskcluster.net/v1/task/H2vt-g-ESQGUxZ5F1XHcbw/runs/0/artifacts/public/logs/live_backing.log

This issue already hits both autoland and inbound.

10:04:51     INFO -  947 INFO Entering test bound testStayopenBookmarksClicks
10:04:51     INFO -  Buffered messages logged at 10:04:49
10:04:51     INFO -  948 INFO Popupshown on Bookmarks-Menu-Button
10:04:51     INFO -  Buffered messages logged at 10:04:50
10:04:51     INFO -  949 INFO TEST-PASS | browser/components/places/tests/browser/browser_stayopenmenu.js | Bookmark ctrl-click opened new tab. -
10:04:51     INFO -  Buffered messages logged at 10:04:51
10:04:51     INFO -  950 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-opacity’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 438}]
10:04:51     INFO -  951 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-transform’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 439}]
10:04:51     INFO -  952 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-transform’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 447}]
10:04:51     INFO -  953 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-opacity’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 451}]
10:04:51     INFO -  954 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-transform’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 453}]
10:04:51     INFO -  955 INFO Console message: [JavaScript Warning: "Unknown property ‘-moz-window-transform’.  Declaration dropped." {file: "chrome://global/content/xul.css" line: 459}]
10:04:51     INFO -  956 INFO Console message: [JavaScript Warning: "Unknown pseudo-class or pseudo-element ‘-moz-tree-line’.  Ruleset ignored due to bad selector." {file: "chrome://global/content/xul.css" line: 658}]
10:04:51     INFO -  Buffered messages finished
10:04:51    ERROR -  957 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_stayopenmenu.js | Bookmarks Menu Button's Popup should still be open. -
10:04:51     INFO -  Stack trace:
10:04:51     INFO -  chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_stayopenmenu.js:testStayopenBookmarksClicks:105
Hi custom.firefox.lady, bug 260611 added this tests which fails (relatively) frequent, at least 4 days during the last day on OS X 10.10 debug automation:

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1400323

Can you take a look at the issue and maybe fix it, please? Thank you.
Blocks: 260611
Flags: needinfo?(stayopenmenu)
I authored the original tests, but Marco [::mak] and Mark (:standard8) collaborated on making them work with OSX, since I have no access or experience with Mac. (See Bug 260611 comment #123 etc.) So it's unlikely I can be of much help on this, sorry.
Flags: needinfo?(stayopenmenu)
Mark, can you take a look at this, please?
Flags: needinfo?(standard8)
At a first look I can't see anything obvious. The screenshot shows the menu as being closed already. My first (and currently only) guess is that removing the tab from behind the scenes might affect the menu in some weird manner, but I'd be surprised based on what I've seen with tests in the past.

The timing of debug builds is unlikely to affect this as everything is happening < 1 second according to the timestamps.

I can see if I can get back to this in a week or two, once 57 items have eased off, although there's more frequent intermittents that we need to fix in the meantime.
Flags: needinfo?(standard8)
Priority: P5 → P2
Whiteboard: [fxsearch]
this bug has increased in the last week, all on debug, mostly on osx.

here is a osx debug log:
https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=134824435

and related text to the failure:
20:23:21     INFO - TEST-START | browser/components/places/tests/browser/browser_stayopenmenu.js
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x12797e000 == 39 [pid = 756] [id = {0d9329dd-8ac3-fc4e-b15e-fa38cb87d05d}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1242cd800 == 38 [pid = 756] [id = {1b4ee6c2-fa6c-4b45-a52f-c6705a8ffd51}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x138d5b000 == 37 [pid = 756] [id = {bcb7ef6e-fda7-a349-86d0-74324515e4c5}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1396a1800 == 36 [pid = 756] [id = {4d357423-be3c-964d-8b30-229062941762}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x137920000 == 35 [pid = 756] [id = {b3129e00-6ed0-fc44-8f85-bd5b6a0c9c22}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1394b4800 == 34 [pid = 756] [id = {87c56a9d-1a2f-174a-a29e-ff4e96db9b11}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x116a8c800 == 33 [pid = 756] [id = {ef0da24b-e641-8d4b-aecd-063bae1bfacf}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x12f793800 == 32 [pid = 756] [id = {c08087b9-bd0a-1349-a7bd-253b855b4323}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1373e2800 == 31 [pid = 756] [id = {4f51d68f-0fae-fa41-a561-5823459e9dda}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x12787b800 == 30 [pid = 756] [id = {b247e50b-68c7-d24a-8d92-866456330019}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1373f3000 == 29 [pid = 756] [id = {4e4b64ab-0595-a14d-b73f-6bfcbfc07f7e}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x1373e1800 == 28 [pid = 756] [id = {bd606238-ce9d-c74d-8898-ba3f94ae94a0}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x127869000 == 27 [pid = 756] [id = {00348339-71fc-814a-8673-08d466175a37}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x137911000 == 26 [pid = 756] [id = {1a7d9ecf-5c01-0f49-93e2-46562081e3d1}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x123daa800 == 25 [pid = 756] [id = {1b272d92-872e-bc43-a672-f584bd1cb6a6}]
20:23:22     INFO - GECKO(756) | --DOCSHELL 0x12d3e0000 == 24 [pid = 756] [id = {a6f3b5c1-7013-4742-816c-cb7533a145a0}]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 141 (0x11a675000) [pid = 756] [serial = 54] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 140 (0x118375000) [pid = 756] [serial = 68] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 139 (0x1172e5000) [pid = 756] [serial = 46] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 138 (0x1289bf000) [pid = 756] [serial = 64] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 137 (0x1288d8800) [pid = 756] [serial = 41] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 136 (0x124b3c800) [pid = 756] [serial = 24] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 135 (0x1252e8800) [pid = 756] [serial = 39] [outer = 0x0] [url = chrome://browser/content/bookmarks/bookmarksPanel.xul]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 134 (0x12f613800) [pid = 756] [serial = 14] [outer = 0x0] [url = about:blank]
20:23:22     INFO - GECKO(756) | --DOMWINDOW == 133 (0x122385800) [pid = 756] [serial = 16] [outer = 0x0] [url = about:blank]
20:23:23     INFO - GECKO(756) | --DOMWINDOW == 5 (0x129109800) [pid = 758] [serial = 55] [outer = 0x0] [url = about:blank]
20:23:23     INFO - GECKO(756) | --DOMWINDOW == 4 (0x1222c4000) [pid = 758] [serial = 53] [outer = 0x0] [url = about:blank]
20:23:23     INFO - GECKO(756) | ++DOCSHELL 0x120434800 == 2 [pid = 758] [id = {55fa7990-114d-7942-b474-2dd4b3283c24}]
20:23:23     INFO - GECKO(756) | ++DOMWINDOW == 5 (0x1206d0000) [pid = 758] [serial = 60] [outer = 0x0]
20:23:23     INFO - GECKO(756) | ++DOMWINDOW == 6 (0x128b07000) [pid = 758] [serial = 61] [outer = 0x1206d0000]
20:23:23     INFO - GECKO(756) | [Parent 756, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/netwerk/base/nsChannelClassifier.cpp, line 344
20:23:23     INFO - GECKO(756) | ++DOMWINDOW == 7 (0x117d20800) [pid = 758] [serial = 62] [outer = 0x1206d0000]
20:23:23     INFO - GECKO(756) | --DOCSHELL 0x129078800 == 1 [pid = 758] [id = {c861fe8a-cc38-8945-8d48-d9da570c90f4}]
20:23:24     INFO - TEST-INFO | started process screencapture
20:23:24     INFO - TEST-INFO | screencapture: exit 0
20:23:24     INFO - Buffered messages logged at 20:23:21
20:23:24     INFO - Entering test bound test_setup
20:23:24     INFO - Bookmarks toolbar made visible
20:23:24     INFO - Leaving test bound test_setup
20:23:24     INFO - Entering test bound testStayopenBookmarksClicks
20:23:24     INFO - Buffered messages logged at 20:23:22
20:23:24     INFO - Popupshown on Bookmarks-Menu-Button
20:23:24     INFO - Buffered messages logged at 20:23:23
20:23:24     INFO - TEST-PASS | browser/components/places/tests/browser/browser_stayopenmenu.js | Bookmark ctrl-click opened new tab. - 
20:23:24     INFO - Buffered messages finished
20:23:24     INFO - TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_stayopenmenu.js | Bookmarks Menu Button's Popup should still be open. - 
20:23:24     INFO - Stack trace:
20:23:24     INFO - chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_stayopenmenu.js:testStayopenBookmarksClicks:105
20:23:24     INFO - GECKO(756) | --DOMWINDOW == 132 (0x12c849800) [pid = 756] [serial = 89] [outer = 0x0] [url = about:mozilla]
20:23:24     INFO - GECKO(756) | --DOMWINDOW == 131 (0x1298dc800) [pid = 756] [serial = 132] [outer = 0x0] [url = about:buildconfig]


I don't think the log is a lot of help necessarily, but I do see some info statements that might help pinpoint the problem.

:standard8, is this something you could help set the proper priority and ensure the right developer(s) knows about this?
Flags: needinfo?(standard8)
Whiteboard: [fxsearch] → [fxsearch][stockwell needswork]
The logs aren't useful, the only thing I can think of is timing. I'm not sure it increased, but maybe a peak.

Might be better for someone who knows a bit more about the menu system to take a look at this. I'll pass it onto Panos.
Flags: needinfo?(standard8) → needinfo?(past)
I believe Paolo has some experience there. Could you please take a look?
Flags: needinfo?(past) → needinfo?(paolo.mozmail)
So, this fails almost exclusively on OS X 10.10. Like Mark, I've looked at the test and it seems correct in when and how it waits for events to happen.

The only thing that comes to my mind is that a background event in the operating system is causing all popups to be closed at a random point. In this case, I don't know if there is anything we can do in the code, but Neil may remember if he's ever observed anything similar with popups in infrastructure before.
Flags: needinfo?(paolo.mozmail) → needinfo?(enndeakin)
OS: Unspecified → Mac OS X
When I debug this, it suggests that the popup is closing due to the menuitem being clicked with the mouse. This is what the test appears to be testing, so that behaviour is correct. The bug that added this 260611 suggests that it is modifying how the menus close when their items are clicked.

Note also bug 1401364 where this same test times out. That test timeout is easy to reproduce.

Also, this suspicious bit of code from the original bug:

setTimeout(() => {
        target.removeAttribute("closemenu");
}, 500);

I didn't look at it in detail but I would guess that patch in bug 260611 isn't correct.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #14)
> The bug that added this 260611 suggests that
> it is modifying how the menus close when their items are clicked.

Right, we don't want to close the menu when one of its items is opened in a new tab by any means.

> Also, this suspicious bit of code from the original bug:
> 
> setTimeout(() => {
>         target.removeAttribute("closemenu");
> }, 500);

That was indeed added to try to workaround a Macos problem, it looks like there is a long timeout between the click event and the time at which the menus are actually auto-closed, that on other platforms doesn't exist.
Though, it's indeed possible that same timeout is the cause of this intermittent, we should probably wait for the attribute to be removed before moving to the next test?

> I didn't look at it in detail but I would guess that patch in bug 260611
> isn't correct.

we're happy to evaluate alternatives if you can suggest a different approach. We want to be able to control whether the menus should close or not, but at the same time, making closing popups completely "manual" looks scary, so the bug retained the usual behavior, and disabled auto closing when the pref if set and the items are opened in a new tab.
(In reply to Marco Bonardo [::mak] from comment #15)
> > Also, this suspicious bit of code from the original bug:
> > 
> > setTimeout(() => {
> >         target.removeAttribute("closemenu");
> > }, 500);
> 
> That was indeed added to try to workaround a Macos problem, it looks like
> there is a long timeout between the click event and the time at which the
> menus are actually auto-closed, that on other platforms doesn't exist.
> Though, it's indeed possible that same timeout is the cause of this
> intermittent, we should probably wait for the attribute to be removed before
> moving to the next test?

> we're happy to evaluate alternatives if you can suggest a different
> approach.

*If* the problem is that the timeout is not always long enough, I'd like to get your opinion on an alternative. I was thinking that there's really no rush to remove the attr as long as we're doing the correct thing for the user. So maybe adding:

if (!modiKey) {
  target.removeAttribute("closemenu");
}

to onMouseUp

while deleting that attr removal code (including the timeout) from onClick might be feasible. (It would not necessarily remove the attr from the same menuitem we previously set it on, just ensure it is not present when the menu should close.) But I don't know how the bookmark menus get built/rebuilt. Once the menu chain is closed, we don't want to leave incorrect closemenu attr's lying around (especially since the user may change the pref). If the menu gets rebuilt to its default state next time it's opened, it doesn't really matter. This approach might be too close to "brute forcing" for your comfort; just throwing the idea on the table.
Flags: needinfo?(mak77)
I didn't actually look at the code in detail; I only noticed that a timeout was added by that bug which generally raises red flags when tests fail intermittently. But I have now looked at it in more detail.

If the test is relying on clicking on several menuitems in sequence while timers are occurring that change the closemenu attribute, then that sounds like an issue if you aren't waiting for those timers. It could, for instance, run the entire test and then fire the timers all at once. Or, several of them could run in quick succession while a test step in occurring. Meaning that the closemenu attribute could change at any time.

Within the menu code, the closemenu attribute is checked after the menu has finished blinking but before the popup has started hiding. I would suggest instead just resetting the closemenu attribute during the popuphiding or popuphidden event rather than using a timer.
This test keeps failing.
::mak, do you think you can look into this?
(In reply to custom.firefox.lady from comment #16)
> *If* the problem is that the timeout is not always long enough, I'd like to
> get your opinion on an alternative.

comment 17 has one involving popuphiding. But I don't recall if we already investigated that originally.

The problem is not "long" timeouts, the problem is the test doesn't wait for any of these timeouts, and thus it can easily fail when they don't fire as (not) expected.

> But I don't know how the bookmark menus get
> built/rebuilt. Once the menu chain is closed, we don't want to leave
> incorrect closemenu attr's lying around 

Right. IIRC the bookmarks popups are never destroyed/rebuilt, but most often just updated.
(In reply to Henrietta Maior [:henrietta_maior] from comment #20)
> This test keeps failing.
> ::mak, do you think you can look into this?

I think we should temporarily disable the test on macosx. The reason of the failure is known, we just don't have resources to fix it properly for now.
Flags: needinfo?(mak77)
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/511604f49a82
Disable test browser_stayopenmenu.js on osx/debug for on-going intermittent failures; r=me,test-only
https://hg.mozilla.org/mozilla-central/rev/511604f49a82
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
This is not fixed, it's just disabled, right?
Status: RESOLVED → REOPENED
status-firefox58: fixed → disabled
Resolution: FIXED → ---
Whiteboard: [fxsearch][stockwell needswork] → [fxsearch][stockwell disabled]
yes, this is correct- we should have marked the bug as leave-open
(In reply to Marco Bonardo [::mak] from comment #21)
> (In reply to custom.firefox.lady from comment #16)
> comment 17 has one involving popuphiding. But I don't recall if we already
> investigated that originally.

I originally decided against using popuphiding due to it causing incorrect menu behavior in an edge case.

> The problem is not "long" timeouts, the problem is the test doesn't wait for
> any of these timeouts, and thus it can easily fail when they don't fire as
> (not) expected.

Understand that now; submitted patch avoids the need for timeouts.

> IIRC the bookmarks popups are never destroyed/rebuilt, but most often
> just updated.

Which means we need to use popuphiding for cleanup in addition to handling above edge case.

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9a8e48d8ebdcd48deb3eb1c697e1ab121591f9
Seems to prevent the issue with browser_stayopenmenu.js mentioned in Bug 260611 comment #117; could you  examine the try results as well?
Comment on attachment 8973488 [details]
Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden)

https://reviewboard.mozilla.org/r/241858/#review249902

::: browser/base/content/browser-places.js:768
(Diff revision 1)
> +      // while menu was kept open, but now menu should close.
> +      target.removeAttribute("closemenu");
> +    }
> +  },
> +
> +  onPopupHidden(aEvent) {

Alternatively, would it be possible to add a popuphidden handler only to the specific popup where closemenu was set? So basically when we setAttribute("closemenu", "none"), we also add a {once} popuphidden listener only for that popup, and check it only handles it (it would also know where closemenu was set, so it shouldn't even walk children).
Then potentially we could avoid having a popuphidden listener looping every child of every bookmark popup.

::: browser/base/content/browser.xul:1024
(Diff revision 1)
>          <toolbarbutton id="bookmarks-toolbar-button"
>                         class="toolbarbutton-1"
>                         flex="1"
>                         label="&bookmarksToolbarItem.label;"
>                         oncommand="PlacesToolbarHelper.onPlaceholderCommand();"/>
> +                       onpopuphidden="BookmarksEventHandler.onPopupHidden(event);"

there is a mistake here, that is causing some of the mochitest-browser failures. The attribute has been added AFTER the element tag closing, it should be moved above oncommand.

::: browser/components/places/tests/browser/browser.ini:78
(Diff revision 1)
>  subsuite = clipboard
>  [browser_sidebar_open_bookmarks.js]
>  [browser_sidebarpanels_click.js]
>  skip-if = (os == "mac" && debug) # Bug 1423667
>  [browser_sort_in_library.js]
>  [browser_stayopenmenu.js]

Once the try results are green, it would be great to find among them the one running this test, and retriggering it a few (20) times to check for intermittents
Attachment #8973488 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [::mak] from comment #31)
> Comment on attachment 8973488 [details]
> Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing
> setTimeout on Mac.
> 
> https://reviewboard.mozilla.org/r/241858/#review249902
> 
> ::: browser/base/content/browser-places.js:768
> (Diff revision 1)
> > +      // while menu was kept open, but now menu should close.
> > +      target.removeAttribute("closemenu");
> > +    }
> > +  },
> > +
> > +  onPopupHidden(aEvent) {
> 
> Alternatively, would it be possible to add a popuphidden handler only to the
> specific popup where closemenu was set? So basically when we
> setAttribute("closemenu", "none"), we also add a {once} popuphidden listener
> only for that popup, and check it only handles it (it would also know where
> closemenu was set, so it shouldn't even walk children).
> Then potentially we could avoid having a popuphidden listener looping every
> child of every bookmark popup.

This will set a {once} el for /each/ menuitem clicked while menu is kept open, but sure, we can do it that way. Although, upon further experimentation (on my Linux build - no idea if this is OS specific), I see no evidence that these dynamically added attributes persist after popuphidden anyway. So we'd likely be doing this clean up as a "good practice" rather than for any real benefit. OTOH, if we don't clean up and they do get persisted (now or in the future), it would cause hard-to-detect bugs where the menu stays open when it should not. Go the safest route (clean up) or the more efficient (skip it)? Your thoughts, please.
Flags: needinfo?(mak77)
Attached patch including the cleanup code, in case that is helpful.
Attachment #8973488 - Attachment is obsolete: true
Attachment #8976766 - Flags: feedback?(mak77)
Got a chance to test this further, and we definitely do need the cleanup code. While Ctrl-clicking works fine without the cleanup, non-modified left click would behave incorrectly in certain scenarios. So previously attached patch is ready for review.
Flags: needinfo?(mak77)
Comment on attachment 8976766 [details] [diff] [review]
Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden)

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

Yes, there could be smarter ways to handle this, like tracking which menuitems have the attribute and adding a single popuphidden that loops through that... But I don't think in practice it matters and the code is quite readable and simple as-is. Let's not complicate it just for unknown gains, we can always get fancy in the future if necessary.

r=me with a green try. If you need help issuing a Try run just needinfo me. When you push to try please also enable the -test-verify- harness in addition to the mochitest-browser-e10s one.
Attachment #8976766 - Flags: feedback?(mak77) → feedback+
Assignee: nobody → stayopenmenu
Status: REOPENED → ASSIGNED
Comment on attachment 8976766 [details] [diff] [review]
Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden)

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

Yes, there could be smarter ways to handle this, like tracking which menuitems have the attribute and adding a single popuphidden that loops through that... But I don't think in practice it matters and the code is quite readable and simple as-is. Let's not complicate it just for unknown gains, we can always get fancy in the future if necessary.

r=me with a green try. If you need help issuing a Try run just needinfo me. When you push to try please also enable the -test-verify- harness in addition to the mochitest-browser-e10s one.
Attachment #8976766 - Flags: feedback+ → review+
(In reply to Marco Bonardo [::mak] from comment #36)
> r=me with a green try. If you need help issuing a Try run just needinfo me.
> When you push to try please also enable the -test-verify- harness in
> addition to the mochitest-browser-e10s one.

Should I do something like the following? (obtained from trychooser)
./mach try -b do -p linux64,macosx64,win64 -u test-verify-e10s,mochitest-e10s-bc -t none --artifact
Flags: needinfo?(mak77)
yes, that sounds good.
Flags: needinfo?(mak77)
A lot of orange on the try push, but I'm not sure how to interpret those results. Would you have a look when you get a chance?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=629d889cffea2aa70f4462e7151fcbf1abb85fc1
Flags: needinfo?(mak77)
Those failures look strange, maybe you were unfortunate and got a broken tree... I'd suggest to pull again from mozilla-central, rebase the patch on top of it and push it again.
Flags: needinfo?(mak77)
This one looks better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ce40cde234b2a09a42c0da7169c09417445505f

Should I push rebased patch to moz-review (see no way there to set as already reviewed) or export/attach it?

(In reply to Marco Bonardo [::mak] from comment #31)
> >  [browser_stayopenmenu.js]
> 
> Once the try results are green, it would be great to find among them the one
> running this test, and retriggering it a few (20) times to check for
> intermittents

I haven't the slightest idea how to do that, sorry.
Flags: needinfo?(mak77)
(In reply to custom.firefox.lady [:tawn] from comment #41)
> This one looks better:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6ce40cde234b2a09a42c0da7169c09417445505f
> 
> Should I push rebased patch to moz-review (see no way there to set as
> already reviewed) or export/attach it?

if you push an update to an existing reviewed patch, it's automatically marked as reviewed. The only important thing is that the mozreview id in the commit message is preserved. Using amend and evolve won't mess up ids.
Anyway it wouldn't be a big deal to re-review.

> (In reply to Marco Bonardo [::mak] from comment #31)
> > >  [browser_stayopenmenu.js]
> > 
> > Once the try results are green, it would be great to find among them the one
> > running this test, and retriggering it a few (20) times to check for
> > intermittents
> 
> I haven't the slightest idea how to do that, sorry.

Well, I think the only way is to open the raw log for each BC, search for the test name. once found, being logged in to treeherder allows to retrigger a test by selecting it and picking the retrigger icon. Though, I don't know if you have the rights to do that. I did it for you.
Flags: needinfo?(mak77)
Comment on attachment 8973488 [details]
Bug 1400323 - Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden)

https://reviewboard.mozilla.org/r/241858/#review253466
Attachment #8973488 - Flags: review?(mak77) → review+
Attachment #8976766 - Attachment is obsolete: true
Thanks, it should autoland soon.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/a6f0fa535135
Refactor code for openInTabClosesMenu pref to avoid needing setTimeout on Mac. (cleanup on popuphidden) r=mak
https://hg.mozilla.org/mozilla-central/rev/a6f0fa535135
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: