Closed Bug 1261536 Opened 4 years ago Closed Last year

Synthetic video document should be created after we have a window

Categories

(Core :: Audio/Video: Playback, defect, P3)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qx:p-])

Attachments

(1 file)

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.
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.
Blocks: 1262058
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.
https://hg.mozilla.org/mozilla-central/rev/50b8a47e960c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1262130
Backed out in https://hg.mozilla.org/mozilla-central/rev/e787670d68a4 for causing bug 1262130.
Status: RESOLVED → REOPENED
Flags: needinfo?(gsquelart)
Resolution: FIXED → ---
Target Milestone: mozilla48 → ---
(Marking since this shows up in a semi-unrelated QX bugtree, please ignore.)
Whiteboard: [qx:p-]
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
Flags: needinfo?(gsquelart)
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?
Flags: needinfo?(gsquelart)
Pushed by timdream@gmail.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
Blocks: 1431255
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?
https://hg.mozilla.org/mozilla-central/rev/2844aaceec5f
Status: REOPENED → RESOLVED
Closed: 4 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(gsquelart)
Followup in this bug is fine with me.
You need to log in before you can comment on or make changes to this bug.