Closed Bug 1261536 Opened 4 years ago Closed Last year
Synthetic video document should be created after we have a window
58 bytes, text/x-review-board-request
Spawned from bug 1248507 comment 5 by JW Wang: > It [video document being created before we have a window] sounds like the same issue as > https://bugzilla.mozilla.org/show_bug.cgi?id=198301#c9. Can we have the same fix for the video > document as we did for the image document? > > Btw, it will also fix bug 608634 which I failed to fix due to lack of decent knowledge about > page/document load. Currently, when directly opening a video file (through File-Open or a URL pointing at a file), a synthetic VideoDocument is created before we know which window to associate it with. It works fine, but it prevents some early actions, like sending a notification, or showing a web console message. Bug 1248507 is my main motivation here, as I would like to collect diagnostic information before/while loading the file, and optionally notify the user about potential issues (e.g. missing decoder libraries). As JW noticed, Boris made a similar decision in the case of image loading in bug 198301 (in 2003!), and also this should help with bug 608634.
Review commit: https://reviewboard.mozilla.org/r/43963/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43963/
Attachment #8737425 - Flags: review?(bzbarsky)
Attachment #8737425 - Flags: review?(bzbarsky) → review+
Comment on attachment 8737425 [details] MozReview Request: Bug 1261536 - Create a synthetic document after the window is set - r?bz https://reviewboard.mozilla.org/r/43963/#review40925 ::: dom/html/VideoDocument.cpp:77 (Diff revision 1) > +#endif > + CreateSyntheticVideoDocument(); > + NS_ASSERTION(NS_SUCCEEDED(rv), "failed to create synthetic video document"); > + } > + > if (!nsContentUtils::IsChildOfSameType(this) && These bits can probably move into CreateSyntheticVideoDocument now, no? Followup bug is probably fine for that. r=me
Gah. I hate mozreview. It totally lost which lines I had highlighted... I meant the bits inside the "IsChildOfSameType(this)" if, which add various stylesheets and stuff.
Thank you for the prompt review. I'll remember to use the old review system next time. ;-) I assumed you were talking about the 'if' block, and opened bug 1262058 for that.
Backed out in https://hg.mozilla.org/mozilla-central/rev/e787670d68a4 for causing bug 1262130.
4 years ago
Priority: -- → P2
4 years ago
Priority: P2 → P1
(Marking since this shows up in a semi-unrelated QX bugtree, please ignore.)
Dropping to P2: It's not that critical. It is probably the right thing to do eventually, but we can live with what's there for now -- it's been working fine for more than 13 years! Initially I thought I needed it for DecoderDoctor, but it was not necessary. Reassigning to "nobody"; I'll keep it in my backlog, but in the meantime someone else can take over if they wish...
Assignee: gsquelart → nobody
Priority: P1 → P2
Mass change P2 -> P3
Priority: P2 → P3
FWIW, bug 1262130 recently got tickled by another landing, but only on 52 and older. It's possible that some of the recent video controls work done by ralin made it more resilient on 53. Maybe worth trying to dust this off and see if it can land and stick now?
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2844aaceec5f Create a synthetic document after the window is set - r=bz
This is needed by bug 1431255; :bz asks me to rebase and land this instead.
Assignee: nobody → gsquelart
You didn't need the GetRootElement() check in there. The !InitialSetupHasBeenDone() check does that already, right?
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] (vacation Aug 16-27) from comment #15) > You didn't need the GetRootElement() check in there. The > !InitialSetupHasBeenDone() check does that already, right? Right. I didn't try to make sense of the code while doing the rebase. Should I push a follow-up (w/ r=bz) on this bug or open up a new one?
Followup in this bug is fine with me.
You need to log in before you can comment on or make changes to this bug.