Closed
Bug 821695
Opened 12 years ago
Closed 12 years ago
Do not load videocontrols.xml if the audio/video tag does not need it.
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-b2g:tef+, blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)
VERIFIED
FIXED
B2G C4 (2jan on)
People
(Reporter: vingtetun, Assigned: roc)
References
Details
(Keywords: perf, Whiteboard: [FFOS_perf])
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
sicking
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
It seems like videcontrols.xml cost 200ms to load on the camera app. It can probably be avoided y not loading it the tag does not request for any controls. See the css rule in https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css#75
Reporter | ||
Updated•12 years ago
|
Blocks: Camera-Startup
Reporter | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 1•12 years ago
|
||
Not a blocker, but looks safe enough to uplift
Assignee: nobody → poirot.alex
blocking-basecamp: ? → -
Comment on attachment 693007 [details] [diff] [review]
Bug 821695 - Do not load videocontrols.xml if the audio/video tag does not need it.
[Triage Comment]
Do check that the video controls still work in B2G webpages.
Also check that they work even if .controls=true is set after the video has started playing.
Attachment #693007 -
Flags: approval-mozilla-b2g18+
Attachment #693007 -
Flags: approval-mozilla-aurora+
Comment 4•12 years ago
|
||
Checked audio and video app.
html5media.info in browser that uses controls.
Tested with this custom page, setting controls=true before/after playing
http://test.techno-barje.fr/video/
Comment 5•12 years ago
|
||
Comment on attachment 693007 [details] [diff] [review]
Bug 821695 - Do not load videocontrols.xml if the audio/video tag does not need it.
Everything looks good, but it looks too simple!?
Attachment #693007 -
Flags: review?(roc)
Assignee | ||
Comment 6•12 years ago
|
||
I would expect this to break dynamic setting of "controls", or at least trigger assertions in a desktop debug build. Does it not?
Assignee | ||
Comment 7•12 years ago
|
||
You should at least get the warning
NS_WARNING("Someone passed native anonymous content directly into frame "
"construction. Stop doing that!");
The problem is that setting '-moz-binding' forces reconstruction of the frames for the videocontrols anonymous content, and reconstructing frames for anonymous content is a bad idea.
But I think we can probably take this patch anyway, given we need it.
Comment 8•12 years ago
|
||
Where are we at with this patch? Roc, do you plan to give r+ or are you waiting for an updated version?
Comment on attachment 693007 [details] [diff] [review]
Bug 821695 - Do not load videocontrols.xml if the audio/video tag does not need it.
Review of attachment 693007 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not sure that uplifting this is still worth the risk. Please renominate if you think it is.
Attachment #693007 -
Flags: approval-mozilla-b2g18+
Comment 11•12 years ago
|
||
This is a pretty big win for startup performance (~500ms) and makes refreshing the preview a cheaper operation too, see bug 834928 comment 3.
blocking-b2g: --- → tef?
Comment 12•12 years ago
|
||
Ok(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> You should at least get the warning
> NS_WARNING("Someone passed native anonymous content directly into frame "
> "construction. Stop doing that!");
Ok so I applied my patch on a firefox desktop debug build and opened:
http://test.techno-barje.fr/video/
Clicked on Play, then Set. Video started playing, controls were working fine.
Then I clicked on Unset and Set again, controls disappears and appeared again, still working.
Here is the log I have seen while doing this:
WARNING: NS_ENSURE_TRUE(resultIndex >= 0) failed: file /mnt/desktop/gecko/toolkit/components/autocomplete/nsAutoCompleteController.cpp, line 1463
++DOMWINDOW == 11 (0x3317b10) [serial = 14] [outer = 0x3eae520]
WARNING: NS_ENSURE_TRUE(currentURI) failed: file /mnt/desktop/gecko/content/base/src/ThirdPartyUtil.cpp, line 96
Finally, I submitted a try run in order to see if tests fail:
https://tbpl.mozilla.org/?tree=Try&rev=a6775caaf73e
Updated•12 years ago
|
Whiteboard: [FFOS_perf]
Comment 13•12 years ago
|
||
We discussed this during triage today and the thoughts are:
- there is an unknown amount of risk associated with this change
- the 500 ms startup benefit is great
- it doesn't look like we're finished here
The decision was to not block on this but please feel free to renom with as much info as you can provide.
blocking-b2g: tef? → -
Updated•12 years ago
|
Assignee: poirot.alex → dale
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 693007 [details] [diff] [review]
Bug 821695 - Do not load videocontrols.xml if the audio/video tag does not need it.
Review of attachment 693007 [details] [diff] [review]:
-----------------------------------------------------------------
Okay, based on that testing I think we can take this.
Attachment #693007 -
Flags: review?(roc) → review+
Comment 15•12 years ago
|
||
Awesome, thanks roc, in my testing this makes a huge improvement to any app thats using videos without controls
blocking-b2g: - → tef?
Keywords: checkin-needed
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e521791fe3cc
Why are we landing this on Aurora?
Keywords: checkin-needed
Comment 17•12 years ago
|
||
This caused mochitest-2 orange. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b25322ec2bf
https://tbpl.mozilla.org/php/getParsedLog.php?id=19355966&tree=Mozilla-Inbound
5021 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["removeformat",""]] "[foo<video></video>bar]" compare innerHTML
5023 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["removeformat",""]] "[foo<video src=abc></video>bar]" compare innerHTML
Comment 18•12 years ago
|
||
I can investigate this
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Updated•12 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → affected
Comment 19•12 years ago
|
||
(In reply to Dale Harvey (:daleharvey) from comment #18)
> I can investigate this
Do you still have time for this, Dale?
Flags: needinfo?(dale)
Comment 21•12 years ago
|
||
Actually not so much, I can reproduce the error, same tests fail when I run locally, however its inside layout which is gonna need someone more experienced in that part of the codebase. deassinging
Assignee: dale → nobody
Assignee | ||
Updated•12 years ago
|
Assignee: bugs → roc
Assignee | ||
Comment 23•12 years ago
|
||
These are only UNEXPECTED PASSes. Probably we just need to mark them as passing.
Assignee | ||
Comment 24•12 years ago
|
||
Those two tests currently fail due to bug 839378 (which I just filed): the videocontrols XBL modify the page DOM (which is bad) causing the test to fail. With this patch, those tests pass, because the controls XBL is not loaded.
Assignee | ||
Comment 25•12 years ago
|
||
Rerunning mochitests with expected-failures (hopefully) changed to passes:
https://tbpl.mozilla.org/?tree=Try&rev=417b4117dc3a
Assignee | ||
Comment 26•12 years ago
|
||
Try looks good. Let's land this.
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 29•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/823769a187c0
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/4d5f18e9a917
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Updated•12 years ago
|
status-b2g18-v1.0.1:
--- → fixed
Comment 31•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Comment 32•12 years ago
|
||
This change has no visible effect on the UI.
You need to log in
before you can comment on or make changes to this bug.
Description
•