831406 - Land Flash private browsing tests in testcase-data repository

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Infrastructure, defect)

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

As per bug 827752, these tests need to be landed in the QA litmus data repository.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Matt, do you have an ETA for that? The repository for it is http://hg.mozilla.org/qa/litmus-data/.

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: HTML/SWF/AS/FLA files for Flash/private browsing test

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

The repository has been moved meanwhile and is now located at:
http://hg.mozilla.org/qa/testcase-data/

Whenever you think it's ready for review please set the appropriate flag on the attachment.

Comment 4

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 708358 [details] [diff] [review]
HTML/SWF/AS/FLA files for Flash/private browsing test

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

Please try to make this test as simple as possible. We do not need a fancy styling which could cause other problems to appear. It will be even fine without any CSS decorations. I think it would be great if we can have both cases, here I mean get and set, in a single test file. That will save us a lot given that most code is duplicated across the files. Beside that also remove all the .DS_Store files added to this patch by OS X.

::: firefox/plugins/flash/get_flash_cookie.as
@@ +1,1 @@
> +

What is that file?

::: firefox/plugins/flash/set_flash_cookie.as
@@ +1,1 @@
> +ExternalInterface.addCallback("click", click);

What is that file? Can you please explain?

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks for the feedback. My response:

- I don't know how to make this test any simpler. It requires a data value input in one page, a click, and another click on a different page. The inclusion of the Flash logo is only to indicate that we are using Flash. It can be hidden.

- As above, we require two pages, not one. The test asks that you run one page (set_flash_cookie) in a private browsing window, and the other (get_flash_cookie) in a normal window.

- The .as files are the code that gets compiled into the SWF, using the .fla files as project files.

- The code snippet you reference is ActionScript, as per previous point. The line you reference is the code required to enable communication between JavaScript and ActionScript inside the SWF.

- The CSS in the HTML files here was auto-written by Flash authoring tool's auto-HTML generation. It will be removed. It does nothing.

- As above, I will remove the .DS_Store files that came along for the ride, once we settle on any other outstanding issues with the test.

Comment 6

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Updated test files, as above

Incorporated changes from comment 4

Comment 7

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

(In reply to Matt Wobensmith from comment #5)
> - As above, we require two pages, not one. The test asks that you run one
> page (set_flash_cookie) in a private browsing window, and the other
> (get_flash_cookie) in a normal window.

Well, you can have the same page open in the private browsing window and in a normal window. Why do we need different URLs?

> - The .as files are the code that gets compiled into the SWF, using the .fla
> files as project files.

We usually don't add project files to the repository. You might attach them here as a tarball so we can have a reference if we have to update the test later. Keep in mind it's a public facing website.

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 709980 [details] [diff] [review]
Updated test files, as above

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

This is only a diff of the latest two versions of the patch. Please attach the full patch as you get via 'hg export' or 'hg qdiff', but wait until we agreed on the remaining issues.

Comment 9

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Matt, would you mind if Daniela helps out with the updates so we can make progress on the appropriate Mozmill tests?

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

I don't mind at all. :)

I'm working on making the changes requested, and will upload that very soon.

Comment 11

[Daniela Petrovici]

Attachment: patch v2.0

I have created one single file that sets and gets the cookie. I have tested it manually with the steps required for bug 827752 and it is working. Please tell me if I need to add anything else here.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 713396 [details] [diff] [review]
patch v2.0

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

A few nits, but this works well for me. Thanks Daniela!

::: firefox/plugins/flash/flashCookie.html
@@ +1,1 @@
> +<!DOCTYPE html>

Rename this to flash_cookie.html to be consistent with other files in the repository.

@@ +3,5 @@
> +  <head>
> +    <title>Flash Cookie</title>
> +    <meta charset="utf-8">
> +  </head>
> +<body>

nit: indent lines from <body> to </body> an additional two spaces

@@ +21,5 @@
> +    <param name="movie" value="get_flash_cookie.swf" />
> +    <param name="allowScriptAccess" value="sameDomain" />
> +  </object>
> +  <script>
> +     function createCookie() {

nit: indent should be two spaces (not three)

@@ +22,5 @@
> +    <param name="allowScriptAccess" value="sameDomain" />
> +  </object>
> +  <script>
> +     function createCookie() {
> +        var cookieValue = document.getElementById("cookieValue").value;

nit: check indentation here too

@@ +30,5 @@
> +     function getCookie() {
> +       var getFlashCookie = document.getElementById("get_flash_cookie");
> +       getFlashCookie.click();
> +     }
> +     function fromFlash(arg) {

Can we rename this argument aCookieValue

Comment 13

[Daniela Petrovici]

Attachment: patch v2.1

Made the changes per review

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 714275 [details] [diff] [review]
patch v2.1

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

This is missing the html file.

Comment 15

[Daniela Petrovici]

Attachment: patch v2.2

Sorry about that, I forgot to add the *.html file to the new patch

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 714339 [details] [diff] [review]
patch v2.2

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

Works fine for me, tested on Linux and OS X. I will leave Henrik and Dave do the final check giving that is a testcase-data change.

Comment 17

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 714339 [details] [diff] [review]
patch v2.2

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

That looks fine but I'm still missing the part where we remove the cookie which has been set before. That means you will be able to run the test page once but each further try would make the test inaccurate. We might have to ask for feedback from Matt.

Also please attach the development files for the swf files as assets separately to this bug.

Comment 18

[Daniela Petrovici]

I think that the removing cookie functionality needs to be done by the flash files since we are setting the cookie through them. Matt, can you please help us with this?

I will add the modified *.fla project files when they are created.

Comment 19

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Daniela, can you please have a look at the following addon how it performs this task? 

https://addons.mozilla.org/addon/betterprivacy/

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Flash private browsing tests and source

I took Daniela's changes, and added the following:

- A link to clear the current Flash cookie
- Renamed "create" to "set" so that all nomenclature is consistent
- Added a span field next to "set" link so that both "set" and "get" links have their own confirmation message

The way a person would run this test should be:
1. Open page in private browsing mode
2. Enter value in form field
3. Click "Set Flash Cookie" and verify value is displayed
4. Click "Get Flash Cookie" and verify value is displayed
5. Close page and reopen in non-private browsing mode.
6. Click "Get Flash Cookie" and verify previous value is NOT displayed

Additionally, I have added a link to "Clear Flash Cookie" but I think that this is not useful. We only care about the above scenario. If the steps are run in order, then the action of clearing the Flash cookie explicitly isn't needed. The primary objective of this test is to verify that a Flash cookie in private browsing mode is never persisted in a non-private browsing mode.

Comment 21

[Daniela Petrovici]

Attachment: patch v1.3

Patch without the project files and *.as files. I will add those as a *.tar archive to this bug per comment #17

Comment 22

[Daniela Petrovici]

Attachment: Assets *.tar archive

Assets

Comment 23

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 716497 [details] [diff] [review]
patch v1.3

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

r- because it doesn't work for me. Whether setting or getting the value failed. Also please create a sub folder called like 'cookies' under flash were we can stick the files into. I don't want to mess up the base flash folder which already contains video files.

Also the current UI is not very self-explanatory. Please add a label to the existing input field for what it is used. Also change the other span elements into input fields as well, so users can see that something will be shown after the action.

Comment 24

[Daniela Petrovici]

A web server is needed to get and set the cookie through this page. I have used xampp (http://www.apachefriends.org/en/xampp-linux.html) and the steps to run this are below:
1) Download the webserver
2) Follow the steps to install it
3) Start the server by using sudo /opt/lampp/lampp start
4) Copy the files from this patch to /opt/lampp/htdocs/
5) Open the flash_cookie.html with http://localhost/flash_cookie.html

I will add an updated version of the patch (with the above changes) today

Comment 25

[Daniela Petrovici]

Attachment: patch v1.4

Made changes per review

Comment 26

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 717062 [details] [diff] [review]
patch v1.4

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

Works fine. Daniela, I will attach a modified HTML file. If you like if feel free to update the patch.

Comment 27

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: modified html file

Comment 28

[Daniela Petrovici]

Attachment: patch v1.5

Added the modified HTML file to the patch

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 718340 [details] [diff] [review]
patch v1.5

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

Tests fine for me, so now instead of receiving an error message when the value is not unique, nothing changes in the get field and we have to clear each time.

Comment 30

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Hm, good call Andreea. I missed that, sorry. I think that would be very helpful if it's getting tested manually. It shouldn't be that hard to add another failure field we can use for the error output.

Comment 31

[Daniela Petrovici]

Attachment: patch v1.6

Added an input field so that the result of the set operation is displayed

Comment 32

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 718942 [details] [diff] [review]
patch v1.6

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

Works now with the error message, tested with 2 normal windows and one private.

Comment 33

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

http://hg.mozilla.org/qa/testcase-data/rev/e1349dd3ee18