821695 - Do not load videocontrols.xml if the audio/video tag does not need it.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 821695 - Do not load videocontrols.xml if the audio/video tag does not need it.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker, but looks safe enough to uplift

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Alexandre Poirot [:ochameau]]

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

[Alexandre Poirot [:ochameau]]

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!?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would expect this to break dynamic setting of "controls", or at least trigger assertions in a desktop debug build. Does it not?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[[:fabrice] Fabrice Desré]

Where are we at with this patch? Roc, do you plan to give r+ or are you waiting for an updated version?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Gabriele Svelto [:gsvelto]]

This is a pretty big win for startup performance (~500ms) and makes refreshing the preview a cheaper operation too, see bug 834928 comment 3.

Comment 12

[Alexandre Poirot [:ochameau]]

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

Comment 13

[Andrew Overholt [:overholt]]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Dale Harvey (:daleharvey)]

Awesome, thanks roc, in my testing this makes a huge improvement to any app thats using videos without controls

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e521791fe3cc

Why are we landing this on Aurora?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

[Dale Harvey (:daleharvey)]

I can investigate this

Comment 19

[Andrew Overholt [:overholt]]

(In reply to Dale Harvey (:daleharvey) from comment #18)
> I can investigate this

Do you still have time for this, Dale?

Comment 20

[Dale Harvey (:daleharvey)]

yup working on it right now

Comment 21

[Dale Harvey (:daleharvey)]

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

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Jet, somebody from the layout could take a look?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These are only UNEXPECTED PASSes. Probably we just need to mark them as passing.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Rerunning mochitests with expected-failures (hopefully) changed to passes:
https://tbpl.mozilla.org/?tree=Try&rev=417b4117dc3a

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try looks good. Let's land this.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e99ec356f47b

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e99ec356f47b

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/823769a187c0
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/4d5f18e9a917

Comment 30

[amiller]

Does not make sense to create a regression issue.

Comment 31

[amiller]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 32

[Mike Habicher [:mikeh] (high bugzilla latency)]

This change has no visible effect on the UI.

Comment 33

[amiller]

Verified fixed per comment 32.