Change manifest.js to use SpecialPowers.pushPrefEnv

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: jwwang)

Tracking

Trunk
mozilla40
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(URL)

Attachments

(1 attachment, 13 obsolete attachments)

7.58 KB, patch
eflores
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Follow-up from bug 898696.

That file should be converted to use SpecialPowers.pushPrefEnv.

From bug 898696, comment 13:
"media.opus.enabled defaults to true now anyway, so you can remove that code path from manifest.js."

From bug 868439, comment 18:
"
Ok, I think I know what's going on with test_access_control.html. On Android the media.preload.default is 1, while on desktop it is 2. 
I put the prefs setting in manifest.js inside the MediaTestManager.runTests function, but that is not called at all by test_access_control.html, hence the time out there.
I fixed that by setting the prefs in the test_access_control.html file itself and combine it with a function to test on. There are more of those cases with this problem:
http://mxr.mozilla.org/mozilla-central/search?string=+src=%22manifest.js%22
test_autoplay.html, test_bug463162.xhtml, test_error_in_video_document.html, test_delay_load.html, test_defaultMuted.html, test_contentDuration7.html, test_closing_connections.html, test_can_play_type_webm.html, test_can_play_type_wave.html, test_can_play_type_ogg.html,  test_can_play_type_no_webm.html, test_can_play_type_no_wave.html, test_can_play_type_no_ogg.html, test_can_play_type.html, test_paused.html, test_no_load_event.html,  test_networkState.html, test_mixed_principals.html, test_load_source.html, test_load.html, test_error_on_404.html, test_seekable2.html, test_source.html, test_reactivate.html, test_referer.html, test_source_media.html, test_bug726904.html, test_source_null.html, test_streams_element_capture_reset.html, test_too_many_elements.html, test_standalone.html, test_source_write.html, test_streams_autoplay.html, test_seekable3.html, test_reset_src.html,  test_dash_detect_stream_switch.html, test_streams_gc.html, test_can_play_type_no_dash.html, test_can_play_type_dash.html, test_can_play_type_mpeg.html and test_streams_srcObject.html
"

All the files that were mentioned in that comment need to be adjusted when manifest.js uses SpecialPowers.pushPrefEnv, because that function is asynchronous.

cpearce mentioned on irc that it would be nice if it would be possible if pushPrefEnv (or the set**Pref functions) could be called synchronously (by spinning the event loop).
That would definitely help here, if that would be possible. Then, all those test files don't need to be adjusted.
(Reporter)

Comment 1

5 years ago
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #0)
> cpearce mentioned on irc that it would be nice if it would be possible if
> pushPrefEnv (or the set**Pref functions) could be called synchronously (by
> spinning the event loop).
> That would definitely help here, if that would be possible. Then, all those
> test files don't need to be adjusted.

I tried to implement something like that in bug 907502, but that bug will probably become WONTFIX (explanation, see bug 621363, comment 4), so that's not an option.
(Reporter)

Comment 2

4 years ago
Created attachment 8371896 [details] [diff] [review]
manifest.diff
(Reporter)

Comment 4

4 years ago
Created attachment 8372219 [details] [diff] [review]
manifest.diff

This should fix the failures, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fa7a7819a082
Attachment #8371896 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
Created attachment 8372260 [details] [diff] [review]
manifest.diff

Hopefully this fixes the failure in /test_streams_gc.html
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8e48d4dd8d93
Attachment #8372219 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
Created attachment 8372300 [details] [diff] [review]
manifest.diff

That didn't fix it, this should, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2401d1315bf1
Attachment #8372260 - Attachment is obsolete: true
(Reporter)

Comment 7

4 years ago
Created attachment 8374394 [details] [diff] [review]
manifest.diff

The previous patch seemed to cause a crash in b2g ICS emulator opt.
/tests/content/media/test/test_reactivate.html | application crashed [@ android::OmxDecoder::ReadAudio(MPAPI::AudioFrame*, long long)]

But there was also an issue with the patch that caused a js error in chrome. This fixes that, I'll do a new try run with this patch, but I first want to know what Chris thinks of this patch.
Chris, perhaps you want the pushPrefenv function in a mediaSetPrefs function (or something like that).
Attachment #8372300 - Attachment is obsolete: true
Attachment #8374394 - Flags: review?(cpearce)
Comment on attachment 8374394 [details] [diff] [review]
manifest.diff

Review of attachment 8374394 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should create a setMediaTestsPrefs(callback_to_start_test) function that sets the prefs and calls the callback upon completion. We should use that instead of setting the prefs manually where we need to.

You may be able to have the function take an optional argument which is a list of extra prefs to also set. So we can pass in extra prefs to set (like the media cache size as needed in test_closing_connections.html).

You don't need to set the apple and gstreamer prefs any more.

::: content/media/test/manifest.js
@@ +642,5 @@
>      this.numTestsRunning = 0;
>      // Always wait for explicit finish.
>      SimpleTest.waitForExplicitFinish();
> +
> +    SpecialPowers.pushPrefEnv({"set": [["media.preload.default", 2], ["media.preload.auto", 3], ["media.gstreamer.enabled", true], ["media.apple.mp3.enabled", true]]}, function() {this.nextTest()}.bind(this));

You shouldn't need to set media.gstreamer.enabled and media.apple.mp3.enabled any more.

@@ +647,3 @@
>    }
>  
> +  this.setPrefs

uh, I'm no JS wizard, but this line looks like a typo?

::: content/media/test/test_access_control.html
@@ +19,5 @@
>  
>  /** Test for Bug 451958 **/
>  
>  function run() {
> +  SpecialPowers.pushPrefEnv({"set": [["media.preload.default", 2], ["media.preload.auto", 3], ["media.gstreamer.enabled", true], ["media.apple.mp3.enabled", true]]}, function() {

Shouldn't need the gstreamer or apple pref here. Ditto for other tests too.
Attachment #8374394 - Flags: review?(cpearce) → feedback+
(Reporter)

Comment 9

4 years ago
(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8374394 [details] [diff] [review]
> @@ +647,3 @@
> >    }
> >  
> > +  this.setPrefs
> 
> uh, I'm no JS wizard, but this line looks like a typo?

Oops, that was some stuff I tried, but wasn't in any of the original patches.
Anyway, thanks for your suggestions, I'll work on a new patch.
(Reporter)

Comment 10

4 years ago
Created attachment 8377550 [details] [diff] [review]
manifest.diff

Updated patch, matching review comments
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b94a3f2a7735
Attachment #8374394 - Attachment is obsolete: true
(Reporter)

Comment 11

4 years ago
Created attachment 8377795 [details] [diff] [review]
manifest.diff

Ok, I made one obvious mistake in the previous patch. This one is ready for review.
Attachment #8377550 - Attachment is obsolete: true
Attachment #8377795 - Flags: review?(cpearce)
Comment on attachment 8377795 [details] [diff] [review]
manifest.diff

Review of attachment 8377795 [details] [diff] [review]:
-----------------------------------------------------------------

Good.
Attachment #8377795 - Flags: review?(cpearce) → review+
(Reporter)

Comment 13

4 years ago
Did an extra tryserver patch to be sure: https://tbpl.mozilla.org/?tree=Try&rev=6e8df0de3a5c
(Reporter)

Comment 14

4 years ago
The tryserver was all right, but I'm worried about the [@ android::OmxDecoder::ReadAudio] crash that seems to be happening occasionally.
See also the latest tryserver run for b2g-emulator: https://tbpl.mozilla.org/?tree=Try&rev=6e8df0de3a5c
I also mentioned this in comment 7.

I don't know what to do about this, this seems like a bug in the Core code.
Flags: needinfo?(cpearce)
This is in code I'm not familiar with.

Sotaro: Can you answer Martijn's question in comment 14 please? Do you know the cause of the crash?
Flags: needinfo?(cpearce) → needinfo?(sotaro.ikeda.g)
I am going to investigate about the crash in a new bug.
Flags: needinfo?(sotaro.ikeda.g)

Updated

4 years ago
Depends on: 976052
(In reply to Chris Pearce (:cpearce) from comment #15)
> This is in code I'm not familiar with.
> 
> Sotaro: Can you answer Martijn's question in comment 14 please? Do you know
> the cause of the crash?

The problem happens when MediaDecoderStateMachine's state is inconsistent with OmxDecoder. It might be bettet to check again after Bug 973408 is fixed.
(Reporter)

Comment 18

4 years ago
Created attachment 8407653 [details] [diff] [review]
manifest.diff

Updated to tip
Attachment #8377795 - Attachment is obsolete: true
(Reporter)

Comment 19

4 years ago
Pushed to try again to see if the crash is gone nowadays: https://tbpl.mozilla.org/?tree=Try&rev=f55b3cd49b87
(Reporter)

Comment 20

4 years ago
The crash doesn't seem to be there anymore, but for some reason content/media/test/test_seekable3.html seems to be timing out.
(Reporter)

Comment 21

4 years ago
Ok, pushed to try again, to see if the timing out doesn't happen anymore: https://tbpl.mozilla.org/?tree=Try&rev=6e3fe1e3fe19
(Reporter)

Comment 22

4 years ago
The time out is still happening. I guess there might be something wrong with the patch. The problem is, I haven't gotten mochitests working on the b2g emulator.
(Assignee)

Comment 23

4 years ago
Which timeout do you refer to?
(Reporter)

Comment 24

4 years ago
The timeouts in content/media/test/test_seekable3.html on Android and B2G.
(Assignee)

Comment 25

4 years ago
test_seekable3.html is not changed by your patch. How does your patch fix the timeout in test_seekable3.html?
(Reporter)

Comment 26

4 years ago
My patch doesn't fix the timeouts, it seems to cause them. Hence, the reason why this patch isn't checked in yet.
(Reporter)

Comment 27

4 years ago
Created attachment 8448692 [details] [diff] [review]
manifest.diff

Ok, I now have a working b2g emulator again and I could reproduce the timeout in test_seekable3.html. It turns out, the video tag in there doesn't have  preload="metadata" (like test_seekable2.html). Once that is added, the timeout doesn't occur.

Chris, let me know if you want to have a tryserver run again for this. I haven't done it, because I already had 2 try runs before and this timeout failure, I was able to test and fix on my b2g emulator build (and the fix is pretty obvious in hindsight).
Attachment #8407653 - Attachment is obsolete: true
Attachment #8448692 - Flags: review?(cpearce)
Comment on attachment 8448692 [details] [diff] [review]
manifest.diff

Review of attachment 8448692 [details] [diff] [review]:
-----------------------------------------------------------------

I'll pass the review to JW Wang on this. He's in charge of the media tests now.
Attachment #8448692 - Flags: review?(cpearce) → review?(jwwang)
(Assignee)

Comment 29

4 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #27)
> Created attachment 8448692 [details] [diff] [review]
> manifest.diff
> 
> Ok, I now have a working b2g emulator again and I could reproduce the
> timeout in test_seekable3.html. It turns out, the video tag in there doesn't
> have  preload="metadata" (like test_seekable2.html). Once that is added, the
> timeout doesn't occur.

This is exactly what your patch trying to fix by providing a consistent 'preload' settings across all platforms. That is setMediaTestsPrefs() in manifest.js. However, there are cases where video tags are instantiated before any code of manifest.js is run, as well as cases where MediaTestManager is not used at all. Is there a way to include the cases listed above to free the test case writer from failing on this subtle difference on different platforms? Maybe SimpleTest.js would be a good place which is included by all media tests.
Flags: needinfo?(martijn.martijn)
(Reporter)

Comment 30

4 years ago
(In reply to JW Wang [:jwwang] from comment #29)
> This is exactly what your patch trying to fix by providing a consistent
> 'preload' settings across all platforms. That is setMediaTestsPrefs() in
> manifest.js. However, there are cases where video tags are instantiated
> before any code of manifest.js is run, as well as cases where
> MediaTestManager is not used at all. Is there a way to include the cases
> listed above to free the test case writer from failing on this subtle
> difference on different platforms? Maybe SimpleTest.js would be a good place
> which is included by all media tests.

See discussion in bug 898696.
I don't think modifying SimpleTest.js for some media tests is the way to go.
I mentioned in bug 898696, comment 11 to put these preferences in prefs_general.js as an alternative.
Flags: needinfo?(martijn.martijn)
(Assignee)

Comment 31

4 years ago
Ok, I see. We should keep testing environment as close to the product environment as possible to find bugs that have impacts on the users. However, there are also pre-conditions to meet for the tests to be valid, that is 'preload' settings for all media tests for example.

I agreed with bug 898696 comment 13 where we use manifest.js whenever possible and set 'preload' manually if necessary for media tests.
(Assignee)

Comment 32

4 years ago
Comment on attachment 8448692 [details] [diff] [review]
manifest.diff

Review of attachment 8448692 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_autoplay.html
@@ +33,5 @@
> +  SimpleTest.finish();
> +}
> +
> +function startTest() {
> +  setMediaTestsPrefs(runTest);

We don't need to change 'preload' settings in this test case.

::: content/media/test/test_bug463162.xhtml
@@ +59,5 @@
>  
>  var t = getPlayableVideo(gSmallTests);
>  
> +function runTest() {
> +  document.getElementById('test').innerHTML = '<video id="a1" preload="metadata" onloadedmetadata="onMetaData(\'a1\');"><sauce/><source type="bad" src="404" onerror="onError(event, \'a1\');"/></video><video id="a2" preload="metadata" onloadedmetadata="onMetaData(\'a2\');"><source onerror="onError(event, \'a2\');"/></video><video id="a3" preload="metadata" onloadedmetadata="onMetaData(\'a3\');"><html:source onerror="onError(event, \'a3\');"/></video><video id="a4" preload="metadata" onloadedmetadata="onMetaData(\'a4\');"><svg:source/><source onerror="onError(event, \'a4\');" type="bad" src="404"/></video>';

The code is hard to read. Please create elements dynamically.

::: content/media/test/test_delay_load.html
@@ +12,5 @@
>  <body>
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=479711">Mozilla Bug 479711</a>
>  <p id="display"></p>
>  <div id="content" style="display: none">
>    

space

::: content/media/test/test_mixed_principals.html
@@ +62,5 @@
> +    // In v2, try loading cross-origin first and then getting redirected to
> +    // our origin.
> +    v2.src = "http://example.org/tests/content/media/test/dynamic_redirect.sjs?key=v2_" + key + "&res=" + resource.name;
> +    v2.load();
> +  

space

::: content/media/test/test_no_load_event.html
@@ +14,4 @@
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=715469">Mozilla Bug 715469</a>
>  <p id="display"></p>
>  <div id="content" style="display: none">
>    

space

@@ +33,5 @@
>      var v = document.createElement("video");
>      v.src = resource.name;
>      v.addEventListener("loadeddata", function(){v.play();}, false);
>      v.controls = "true";
>      

space

@@ +42,2 @@
>        false);
>        

space

@@ +42,4 @@
>        false);
>        
>      v.addEventListener("ended", finished, false);
>      

space

::: content/media/test/test_reset_src.html
@@ +79,5 @@
> +    todo("No video file available.");
> +    SimpleTest.finish();
> +  } else {
> +    v.addEventListener("loadedmetadata", onLoadedMetadata_Video);
> +    v.src = source.name;

I think it is simpler to say |v.preload = "metadata"| without calling setMediaTestsPrefs() for most cases.

::: content/media/test/test_streams_gc.html
@@ +11,5 @@
>  <pre id="test">
> +<script type="application/javascript">
> +function runTest() {
> +  var x = document.createElement('iframe');
> +  x.src = 'streams_gc_iframe.html';

I don't understand why we need an iframe to run the test. Is it because the static audio element that is initialized before the preload changes take effect? If that is the case, we should create media elements dynamically or set 'preload' attribute manually in the audio tag.

Otherwise, it is confusing for the test case writer to know whether to put the test code in an iframe or not.

Btw, I fail to see the need for this test case to employ 'preload' settings.
Attachment #8448692 - Flags: review?(jwwang)
(Reporter)

Comment 33

4 years ago
Created attachment 8454876 [details] [diff] [review]
manifest.diff

Updated to review comments, I have to check this patch on b2g emulator now.
Attachment #8448692 - Attachment is obsolete: true
(Reporter)

Comment 34

4 years ago
Ok, b2g emulator test run passed, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=34df5d7bb6d0
I addressed your review comments.

(In reply to JW Wang [:jwwang] from comment #32)
> ::: content/media/test/test_reset_src.html
> @@ +79,5 @@
> I think it is simpler to say |v.preload = "metadata"| without calling
> setMediaTestsPrefs() for most cases.

You mean for this test file or in general? It seems to me that for the media tests in general, it's now better to use preload="metadata" instead of setting the media.preload.default and media.preload.auto prefs.
(Reporter)

Updated

4 years ago
Attachment #8454876 - Flags: review?(jwwang)
(Assignee)

Comment 35

4 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #34)
> Ok, b2g emulator test run passed, pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=34df5d7bb6d0
> I addressed your review comments.
> 
> (In reply to JW Wang [:jwwang] from comment #32)
> > ::: content/media/test/test_reset_src.html
> > @@ +79,5 @@
> > I think it is simpler to say |v.preload = "metadata"| without calling
> > setMediaTestsPrefs() for most cases.
> 
> You mean for this test file or in general? It seems to me that for the media
> tests in general, it's now better to use preload="metadata" instead of
> setting the media.preload.default and media.preload.auto prefs.

For the general cases. And yes, preload='metadata' is simpler.
(Assignee)

Comment 36

4 years ago
Comment on attachment 8454876 [details] [diff] [review]
manifest.diff

Review of attachment 8454876 [details] [diff] [review]:
-----------------------------------------------------------------

r- for some tests not enabled. We don't know if the changes work.

::: content/media/test/test_bug463162.xhtml
@@ +64,4 @@
>  
> +  var v = document.createElement('video');
> +  v.id = 'a1';
> +  v.preload = 'metadata';

preload is already set to 'metadata' in video tags. Do we really need to change this test?

::: content/media/test/test_delay_load.html
@@ +17,5 @@
>  <div id="log" style="font-size: small;"></div>
>  <pre id="test">
>  <script type="application/javascript">
> +function runTest() {
> +  var x = document.createElement('iframe');

Why do we need an iframe to run the test? This test is not enabled on b2g try.

::: content/media/test/test_error_in_video_document.html
@@ +49,5 @@
> +    var f = document.createElement("iframe");
> +    f.src = t.name;
> +    f.addEventListener("load", function() { SimpleTest.executeSoon(check); }, false);
> +    document.body.appendChild(f);
> +  });

enable this test.

::: content/media/test/test_mixed_principals.html
@@ +71,3 @@
>  
> +function startTest() {
> +  setMediaTestsPrefs(beginTest, runTest);

It should be setMediaTestsPrefs(runTest).
This test is not run at all, please enable it in mochitest.ini.

::: content/media/test/test_streams_gc.html
@@ +6,5 @@
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>    <script type="text/javascript" src="manifest.js"></script>
>  </head>
>  <body onload="doTest()">
> +<audio id="a" preload="metadata"></audio>

This test is not run on your b2g try. Please modify mochitest.ini to enable and verify the change. However it seems to me we don't need to change preload for this test. I wonder if this test failed on b2g for another reason.
Attachment #8454876 - Flags: review?(jwwang) → review-
(Reporter)

Comment 37

4 years ago
(In reply to JW Wang [:jwwang] from comment #36)
> r- for some tests not enabled. We don't know if the changes work.

Ok, I'll try to enable the tests you mentioned in your review comments. Iirc, I did test before with this patch, to see if they could be enabled, but apparently they could not, otherwise, I would have already have them enabled in the patch. But things might have changed by now.

I'll address your review comments in a new patch and I'll run it on tryserver to see the results.

The disabledness of content/media/test/test_delay_load.html on b2g emulator is bug 1021676.

> ::: content/media/test/test_error_in_video_document.html
> @@ +49,5 @@
> > +    var f = document.createElement("iframe");
> > +    f.src = t.name;
> > +    f.addEventListener("load", function() { SimpleTest.executeSoon(check); }, false);
> > +    document.body.appendChild(f);
> > +  });
> 
> enable this test.

This is only disabled on Android only, see bug 608634.

> ::: content/media/test/test_mixed_principals.html
> This test is not run at all, please enable it in mochitest.ini.

This is bug 567954.

> ::: content/media/test/test_streams_gc.html
> @@ +6,5 @@
> >    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> >    <script type="text/javascript" src="manifest.js"></script>
> >  </head>
> >  <body onload="doTest()">
> > +<audio id="a" preload="metadata"></audio>
> 
> This test is not run on your b2g try. Please modify mochitest.ini to enable
> and verify the change. However it seems to me we don't need to change
> preload for this test. I wonder if this test failed on b2g for another
> reason.

This is bug 1021682.
I don't know why this test fails on b2g. This patch was just to rewrite the media test files to use pushPrefEnv. When <script src="manifest.js"> is in the test file, I had to use pushPrefEnv, because those files had automatically their preferences changed. However not all test files necessarily need these pref changes, perhaps. So perhaps preload for this test file is not necessary.

Reenabling tests was not really in the scope of this patch. It's likely it doesn't help for those intermittent failures.
(Reporter)

Comment 38

4 years ago
Created attachment 8455216 [details] [diff] [review]
manifest.diff

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6e0174a3f468
Attachment #8454876 - Attachment is obsolete: true
(Reporter)

Comment 39

4 years ago
Comment on attachment 8455216 [details] [diff] [review]
manifest.diff

Review of attachment 8455216 [details] [diff] [review]:
-----------------------------------------------------------------

ok, all is green, except content/media/test/test_delay_load.html, which is expected.
But the other ones that I enabled might be intermittent failures.
I think it would be better to enable those tests in a different patch/bug, because it's not really in the scope of this bug.

(In reply to JW Wang [:jwwang] from comment #35)
> > You mean for this test file or in general? It seems to me that for the media
> > tests in general, it's now better to use preload="metadata" instead of
> > setting the media.preload.default and media.preload.auto prefs.
> 
> For the general cases. And yes, preload='metadata' is simpler.

Ok, so perhaps it is possible to get rid of setMediaTestsPrefs completely. Not sure if it's feasible. Is that what you want?
Attachment #8455216 - Flags: review?(jwwang)
(Assignee)

Comment 40

4 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #39)
> ok, all is green, except content/media/test/test_delay_load.html, which is
> expected.
> But the other ones that I enabled might be intermittent failures.
> I think it would be better to enable those tests in a different patch/bug,
> because it's not really in the scope of this bug.

Yes, it is not the scope of this bug. But without enabling the tests, we won't know if we are just making irrelevant changes that don't change the test results. To err on the side of caution, I would prefer to hold off the changes until the root cause is known and take appropriate changes.


> Ok, so perhaps it is possible to get rid of setMediaTestsPrefs completely.
> Not sure if it's feasible. Is that what you want?

Yes, I would prefer to take the change in manifest.js only which will take effect automatically for those tests which employ MediaTestManager. For those not, setting preload="metadata" is simpler than calling setMediaTestsPrefs() and code is easier to read.
(Assignee)

Comment 41

4 years ago
Comment on attachment 8455216 [details] [diff] [review]
manifest.diff

Review of attachment 8455216 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.
r- for the issue in test_mixed_principals.html

::: content/media/test/test_mixed_principals.html
@@ +23,5 @@
>  var v2 = document.getElementById("v2");
>  
>  var resource = getPlayableVideo(gSeekTests);
>  
> +function runTest() {

The change is wrong. The video tags are instantiated before the change in preload settings take effect. You should just specify preload="metadata" in the video tag attributes.
Attachment #8455216 - Flags: review?(jwwang) → review-
(Reporter)

Comment 42

4 years ago
Created attachment 8457821 [details] [diff] [review]
902686.diff

(In reply to JW Wang [:jwwang] from comment #41)
> Overall looks good.
> r- for the issue in test_mixed_principals.html
> The change is wrong. The video tags are instantiated before the change in
> preload settings take effect. You should just specify preload="metadata" in
> the video tag attributes.

Yes, this test was failing in the previous patch, but it turns out, it's completely disabled, see bug 567954.
Attachment #8455216 - Attachment is obsolete: true
Attachment #8457821 - Flags: review?(jwwang)
(Assignee)

Comment 43

4 years ago
Comment on attachment 8457821 [details] [diff] [review]
902686.diff

Review of attachment 8457821 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8457821 - Flags: review?(jwwang) → review+
(Assignee)

Comment 44

4 years ago
Btw, you can name the patch as xxx-v1.patch, xxx-v2.patch and so on. It would be more interdiff-friendly to see what changed between different patch versions.
(Reporter)

Comment 45

4 years ago
Created attachment 8457921 [details] [diff] [review]
902686-v2.diff

(In reply to JW Wang [:jwwang] from comment #40)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #39)
> > ok, all is green, except content/media/test/test_delay_load.html, which is
> > expected.
> > But the other ones that I enabled might be intermittent failures.
> > I think it would be better to enable those tests in a different patch/bug,
> > because it's not really in the scope of this bug.
> 
> Yes, it is not the scope of this bug. But without enabling the tests, we
> won't know if we are just making irrelevant changes that don't change the
> test results. To err on the side of caution, I would prefer to hold off the
> changes until the root cause is known and take appropriate changes.

Ok, removed the mochitest.ini changes here.
The changes aren't irrelevant, even if they would not change the test results. SpecialPowers.setBoolPref/setIntPref etc, shouldn't be used at all.
Attachment #8457821 - Attachment is obsolete: true
Attachment #8457921 - Flags: review?(jwwang)
(Assignee)

Updated

4 years ago
Attachment #8457921 - Flags: review?(jwwang) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=44028197&tree=Mozilla-Inbound
(Reporter)

Comment 48

4 years ago
Sorry :(
This makes sense, test_delay_load.html was disabled for b2g only, not Android.
The last try run (comment 39), I mentioned this, but thought for some reason, it was disabled on Android and b2g. 
So by adding that line back (to get it disabled), I thought everything was fine and no new try run would be necessary.
So test_delay_load.html needs to be fixed too.
(Assignee)

Updated

4 years ago
Depends on: 1021676
(Assignee)

Comment 49

4 years ago
Hi Martijn,
Since test_delay_load.html is fixed in bug 1021676, can you try again?
Flags: needinfo?(martijn.martijn)
(Assignee)

Comment 50

3 years ago
Per discussion with Martijn, I will take this bug.
Assignee: martijn.martijn → jwwang
Flags: needinfo?(martijn.martijn)
(Assignee)

Comment 51

3 years ago
Created attachment 8589421 [details] [diff] [review]
902686-v3.diff

rebase and remove unnecessary changes from patch-v2.
Attachment #8457921 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
Comment on attachment 8589421 [details] [diff] [review]
902686-v3.diff

Change manifest.js to use SpecialPowers.pushPrefEnv to have a consistent preload settings on all platforms. Also SpecialPowers.setXXX which is racy on e10s platforms. 

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=721cfa3c627e.
Attachment #8589421 - Flags: review?(edwin)
(Assignee)

Comment 53

3 years ago
Thanks for the review!

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=721cfa3c627e.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f8cbccdf1945
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.