1067721 - [e10s] "Save Snapshot As" dialog box doesn't open

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Loic]

STR: play any MP4 video like http://www.w3schools.com/html/mov_bbb.mp4 and right click to select the entry "save snapshot as".

Result: Nightly fails to open the dialog box to save the snapshot.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: [e10s] Make "Save Snapshot As" context menu item work for e10s. r=?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8513781 [details] [diff] [review]
[e10s] Make "Save Snapshot As" context menu item work for e10s. r=?

Grrr, bzexport...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a52dd541eea5

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: bz://1067721/mconley

Comment 7

[Mike Conley (:mconley) (:⚙️)]

/r/91 - Bug 1067721 - [e10s] Make "Save Snapshot As" context menu item work for e10s. r=?
/r/93 - Regression test

Pull down these commits:

hg pull review -r 734c1b478b5416ddabd116288845f25888148e53

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8513781 [details] [diff] [review]
[e10s] Make "Save Snapshot As" context menu item work for e10s. r=?

This is the fix I have in mind. The regression test appears to fail on try, so I'm looking into that next.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8514328 [details] [diff] [review]
Regression test

I re-purposed some pre-existing video support files to do my bidding here, and so I renamed them to be more general.

This test works for both e10s and non-e10s - I was pleasantly surprised that I could use nsIDOMWindowUtils to open a context menu in a framescript.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

It's worth checking if you can just use document.popupNode in the child instead of sending back the CPOW ref

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Felipe Gomes from comment #13)
> It's worth checking if you can just use document.popupNode in the child
> instead of sending back the CPOW ref

Tried, and it wasn't defined. I can't say I'm 100% surprised though - reading through[1], it looks like document.popupNode is on the outs.

Thanks for the review!

[1]: https://developer.mozilla.org/en-US/docs/Web/API/document.popupNode

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8514502 [details] [diff] [review]
Regression test

browser_autocomplete_a11y_label.js was relying on that promisePopupHidden bug to actually hide the popup. When I fixed promisePopupHidden, it broke the assumption that promisePopupHidden actually _hides_ the popup (instead of waiting for it to hide). So I just hide the popup here.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/9ee0da2fce79

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ee0da2fce79

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1067721.html

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0

Verified fixed on:
- latest Nightly, build ID: 20141228030216
- Firefox 36.0a2, build ID: 20141229004004