Closed
Bug 759703
Opened 12 years ago
Closed 12 years ago
Intermittent browser_pluginnotification.js | Test 16b, Should have a click-to-play notification | Test 16c, Should not have a click-to-play notification | Test 16c, Plugin should be activated
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla16
People
(Reporter: emorley, Assigned: keeler)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 2 obsolete files)
14.21 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Rev3 Fedora 12x64 mozilla-inbound debug test mochitest-other on 2012-05-30 02:45:07 PDT for push 8f8639307984 slave: talos-r3-fed64-016 https://tbpl.mozilla.org/php/getParsedLog.php?id=12190816&tree=Mozilla-Inbound { TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16a, Should not have a click-to-play notification --DOMWINDOW == 42 (0x77acb50) [serial = 1036] [outer = 0x7761f50] [url = chrome://mochitests/content/browser/browser/base/content/test/plugin_clickToPlayAllow.html] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16b, Should have a click-to-play notification Stack trace: JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16b :: line 477 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16b, Plugin should not be activated TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16c, Should not have a click-to-play notification Stack trace: JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16c :: line 488 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16c, Plugin should be activated Stack trace: JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16c :: line 491 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16d, Should have a click-to-play notification TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16d, Plugin should not be activated WARNING: NS_ENSURE_TRUE(mMutable) failed: file ../../../../netwerk/base/src/nsSimpleURI.cpp, line 258 INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | finished in 7056ms }
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 3•12 years ago
|
||
Ideally, we'd want some kind of event when the plugin is actually in the page and ready to be clicked on. The only event I could find that actually fired was "focus" ("load" doesn't seem to fire for objects or embeds (?!)). So, I set up an interval that would send tab key events. When the plugin shows up in the page, it will eventually be focused by the tabbing, and the listener will go on to the next test. If the plugin doesn't show up after 5 seconds' worth of tabbing, the test moves on. Also, I thought it would be a good idea to get rid of all other timeouts, because the same thing could happen with them. Most of the time we're waiting for a plugin to be activated, so I added other intervals that basically do the same thing.
Attachment #630199 -
Flags: feedback?(jaws)
Comment 4•12 years ago
|
||
Comment on attachment 630199 [details] [diff] [review] fix idea Review of attachment 630199 [details] [diff] [review]: ----------------------------------------------------------------- Interesting solution. Maybe this will work, but the failures are very infrequent so I'm not sure how useful this change is. What do you think? ::: browser/base/content/test/browser_pluginnotification.js @@ +476,5 @@ > > prepareTest(test16a, gTestRoot + "plugin_bug743421.html"); > } > > +function sendTab(aWindow) { please rename this to sendTabKey to disambiguate with browser tabs. @@ +532,5 @@ > + var interval = setInterval(function() { > + if (tries < 50) { > + sendTab(gTestBrowser.contentWindow); > + } else { > + callback(); We shouldn't be hitting these |else| blocks here nor in the other test functions within this file. Can you fail a test if it reaches one of these |else| blocks or if |tries| goes over its limit?
Attachment #630199 -
Flags: feedback?(jaws)
Updated•12 years ago
|
Attachment #630199 -
Flags: feedback+
Assignee | ||
Comment 5•12 years ago
|
||
Thanks for the feedback. I'll make the changes when I get a chance. I think it's worth it to fix for two reasons: 1) it makes the test faster, because it only waits as long as it has to (within 100ms or whatever we want to set that number to) and 2) fewer intermittent oranges means a better test suite means better development, so it's good for the project.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•12 years ago
|
Attachment #630199 -
Flags: feedback+ → review+
Assignee | ||
Comment 7•12 years ago
|
||
I refactored things, and I think this is a much better solution. We no longer need that tab-sending business. I wasn't quite sure what you meant about the else blocks, but I made sure the test failed if it waited too long (tries hit the limit). I also sped up the interval, so the test goes even faster.
Attachment #631027 -
Flags: review?(jaws)
Assignee | ||
Updated•12 years ago
|
Attachment #630199 -
Attachment is obsolete: true
Comment 8•12 years ago
|
||
Comment on attachment 631027 [details] [diff] [review] patch Review of attachment 631027 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_pluginnotification.js @@ +252,5 @@ > ok(!objLoadingContent.activated, "Test 9a, Plugin with id=" + plugin2.id + " should not be activated"); > > EventUtils.synthesizeMouse(plugin1, 100, 100, { }); > + var objLoadingContent = plugin1.QueryInterface(Ci.nsIObjectLoadingContent); > + var condition = function() { return objLoadingContent.activated; }; This could use an expression closure here if you wanted. E.g. |var condition = function() objLoadingContent.activated;| See https://developer.mozilla.org/en/New_in_JavaScript_1.8#Expression_closures_%28Merge_into_own_page.2Fsection%29 for more information. No added benefit, but I just thought I could mention this since it's cool and is less typing :)
Attachment #631027 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Decided to use expression closures - carrying over r+ Try run: https://tbpl.mozilla.org/?tree=Try&rev=115fa4447f2b
Assignee: nobody → dkeeler
Attachment #631027 -
Attachment is obsolete: true
Attachment #631138 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/700a03cd53d4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•