Update 'firefox/plugins/flash/cookies/flash_cookie.html' to wait for the flash objects full load.

RESOLVED FIXED

Status

Mozilla QA
Infrastructure
--
blocker
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Teodor Druta, Assigned: Teodor Druta, Mentored)

Tracking

Version 2
Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8508651 [details] [diff] [review]
FlashCookieTestCaseData.patch

Update 'firefox/plugins/flash/cookies/flash_cookie.html' javscript code to wait for full load of the flash objects before calling the initializing "getCookie()" function.
Attachment #8508651 - Flags: review?
Group: mozilla-employee-confidential
(Assignee)

Updated

3 years ago
Attachment #8508651 - Flags: review?(andrei.eftimie)
Attachment #8508651 - Flags: review?(andreea.matei)
Attachment #8508651 - Flags: review?

Comment 1

3 years ago
Comment on attachment 8508651 [details] [diff] [review]
FlashCookieTestCaseData.patch

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

Indeed this properly waits for the flash files to be loaded.

While the setInterval code could be refactored, I'm not sure we would not actually overcomplicate it.
So I'm fine with it as is.

Please do increase the interval a bit, as 10ms might be too fast.

With that change please ask a review from Henrik.

::: firefox/plugins/flash/cookies/flash_cookie.html
@@ +12,5 @@
> +              if (document.getElementById("get_flash_cookie").PercentLoaded() == 100) {
> +                clearInterval(getFlashCookieInterval);
> +                getCookie();
> +              }
> +            }, 10);

Increase this a bit, maybe to 50ms

@@ +14,5 @@
> +                getCookie();
> +              }
> +            }, 10);
> +          }
> +        }, 10);

Same here.
Attachment #8508651 - Flags: review?(andrei.eftimie)
Attachment #8508651 - Flags: review?(andreea.matei)
Attachment #8508651 - Flags: review+
(Assignee)

Comment 2

3 years ago
Created attachment 8512647 [details] [diff] [review]
FlashCookieTestCaseData.patch

> Increase this a bit, maybe to 50ms
Updated patch with the new interval value.

I think we could refactor the code using Promise objects, but it would add more code and the end result will be the same.
Attachment #8508651 - Attachment is obsolete: true
Attachment #8512647 - Flags: review?(andrei.eftimie)
Attachment #8512647 - Flags: review?(andreea.matei)
Comment on attachment 8512647 [details] [diff] [review]
FlashCookieTestCaseData.patch

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

::: firefox/plugins/flash/cookies/flash_cookie.html
@@ +7,5 @@
> +      window.onload = function() {
> +        var setFlashCookieInterval = setInterval(function() {
> +          if (document.getElementById("set_flash_cookie").PercentLoaded() == 100) {
> +            clearInterval(setFlashCookieInterval);
> +            var getFlashCookieInterval = setInterval(function() {

I just checked why we couldn't do this waiting in test itself, and PercentLoaded function can't be called from the test so it make sense to have this here. 

Elements are ready on the onload event, please cache them, there is no need to query them on each interval loop. 
Also if you really have to wait for both objects, you could do that in the same interval, you don't need two nested setInterval functions. 

   window.onload = function() {
     var setFlashCookie = document.getElementById("set_flash_cookie");
     var getFlashCookie document.getElementById("get_flash_cookie");
     var waitForFlash = setInterval(function() {
       if (setFlashCookie.PercentLoaded() == 100 && getFlashCookie.PercentLoaded() == 100) {
         getCookie();
	 clearInterval(waitForFlash);
       }
     }, 50);
Blocks: 1088561
(Assignee)

Comment 4

3 years ago
Created attachment 8513408 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch

Thanks for the review, Cosmin.
I have attached a new patch, updated the code to use a single interval, cached elements to variables.
Attachment #8512647 - Attachment is obsolete: true
Attachment #8512647 - Flags: review?(andrei.eftimie)
Attachment #8512647 - Flags: review?(andreea.matei)
Attachment #8513408 - Flags: review?(andrei.eftimie)
Attachment #8513408 - Flags: review?(andreea.matei)

Comment 5

3 years ago
Comment on attachment 8513408 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch

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

Looks good. Thanks Cosmin for the input here!

Please fix the mentioned nit.
Also the commit message should be for the changes in general you are doing, not the latest iteration, so please also revert that.
Something along the lines: "Properly wait for the flash files to load"

With these 2 changes ask a review from Henrik.

::: firefox/plugins/flash/cookies/flash_cookie.html
@@ +6,5 @@
>      <script>
> +      window.onload = function() {
> +        var setFlashCookie = document.getElementById("set_flash_cookie");
> +        var getFlashCookie = document.getElementById("get_flash_cookie");
> +        

nit: please remove trailing whitespace
Attachment #8513408 - Flags: review?(andrei.eftimie)
Attachment #8513408 - Flags: review?(andreea.matei)
Attachment #8513408 - Flags: review+
(Assignee)

Comment 6

3 years ago
Created attachment 8513417 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch
Attachment #8513408 - Attachment is obsolete: true
Attachment #8513417 - Flags: review?(andrei.eftimie)
Attachment #8513417 - Flags: review?(andreea.matei)

Updated

3 years ago
Attachment #8513417 - Flags: review?(hskupin)
Attachment #8513417 - Flags: review?(andrei.eftimie)
Attachment #8513417 - Flags: review?(andreea.matei)
Attachment #8513417 - Flags: review+
Comment on attachment 8513417 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch

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

With the comments fixed r=me.

::: firefox/plugins/flash/cookies/flash_cookie.html
@@ +3,5 @@
>    <head>
>      <title>Flash Cookie</title>
>      <meta charset="utf-8">
>      <script>
> +      window.onload = function() {

nit: blank after function

@@ +5,5 @@
>      <meta charset="utf-8">
>      <script>
> +      window.onload = function() {
> +        var setFlashCookie = document.getElementById("set_flash_cookie");
> +        var getFlashCookie = document.getElementById("get_flash_cookie");

Lets move those two lines into the global script scope, so they can be used by the other methods too. No need to define them everywhere again. When doing that please also sort alphabetically.

@@ +7,5 @@
> +      window.onload = function() {
> +        var setFlashCookie = document.getElementById("set_flash_cookie");
> +        var getFlashCookie = document.getElementById("get_flash_cookie");
> +
> +        var waitForFlash = setInterval(function() {

nit: blank after function

@@ +8,5 @@
> +        var setFlashCookie = document.getElementById("set_flash_cookie");
> +        var getFlashCookie = document.getElementById("get_flash_cookie");
> +
> +        var waitForFlash = setInterval(function() {
> +          if (setFlashCookie.PercentLoaded() == 100 && getFlashCookie.PercentLoaded() == 100) {

Please move the second comparison to the next line to make it better readable. Maybe due getFlashCookie first.
Attachment #8513417 - Flags: review?(hskupin) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8513503 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch

Added blanks after "function ()".
Moved getFlashCookie and setFlashCookie variables to the global scope.
Moved the if comparison from one line to two lines.
Attachment #8513417 - Attachment is obsolete: true
Attachment #8513503 - Flags: review?(andrei.eftimie)
Attachment #8513503 - Flags: review?(andreea.matei)

Comment 9

3 years ago
Comment on attachment 8513503 [details] [diff] [review]
FlashCookieTestCaseDatav2.patch

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

Landed:
https://hg.mozilla.org/qa/testcase-data/rev/1d6e847b3d2f

Should be picked up by the cron job soon and changes be visible at: http://www.mozqa.com/data/firefox/plugins/flash/cookies/flash_cookie.html
Attachment #8513503 - Flags: review?(andrei.eftimie)
Attachment #8513503 - Flags: review?(andreea.matei)
Attachment #8513503 - Flags: review+
Attachment #8513503 - Flags: checkin+

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.