Closed Bug 831406 Opened 8 years ago Closed 8 years ago

Land Flash private browsing tests in testcase-data repository

Categories

(Mozilla QA Graveyard :: Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mwobensmith, Assigned: daniela.p98911)

Details

Attachments

(3 files, 9 obsolete files)

As per bug 827752, these tests need to be landed in the QA litmus data repository.
Matt, do you have an ETA for that? The repository for it is http://hg.mozilla.org/qa/litmus-data/.
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.
Assignee: nobody → mwobensmith
Blocks: 827752
Status: NEW → ASSIGNED
No longer depends on: 827752
OS: Mac OS X → All
Hardware: x86 → All
Summary: Land Flash private browsing tests in litmus data → Land Flash private browsing tests in testcase-data repository
Attachment #708358 - Flags: review+
Attachment #708358 - Flags: review+ → review?(hskupin)
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?
Attachment #708358 - Flags: review?(hskupin) → review-
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.
Attached patch Updated test files, as above (obsolete) — Splinter Review
Incorporated changes from comment 4
Attachment #708358 - Attachment is obsolete: true
Attachment #709980 - Flags: review?(hskupin)
(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 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.
Attachment #709980 - Flags: review?(hskupin) → review-
Matt, would you mind if Daniela helps out with the updates so we can make progress on the appropriate Mozmill tests?
Flags: needinfo?(mwobensmith)
I don't mind at all. :)

I'm working on making the changes requested, and will upload that very soon.
Flags: needinfo?(mwobensmith)
Attached patch patch v2.0 (obsolete) — Splinter Review
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.
Attachment #709980 - Attachment is obsolete: true
Attachment #713396 - Flags: feedback?(hskupin)
Attachment #713396 - Flags: feedback?(dave.hunt)
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
Attachment #713396 - Flags: feedback?(hskupin)
Attachment #713396 - Flags: feedback?(dave.hunt)
Attachment #713396 - Flags: feedback+
Attached patch patch v2.1 (obsolete) — Splinter Review
Made the changes per review
Attachment #714275 - Flags: review?(andreea.matei)
Comment on attachment 714275 [details] [diff] [review]
patch v2.1

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

This is missing the html file.
Attachment #714275 - Flags: review?(andreea.matei)
Attached patch patch v2.2 (obsolete) — Splinter Review
Sorry about that, I forgot to add the *.html file to the new patch
Attachment #713396 - Attachment is obsolete: true
Attachment #714275 - Attachment is obsolete: true
Attachment #714339 - Flags: review?(andreea.matei)
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.
Attachment #714339 - Flags: review?(andreea.matei) → review+
Attachment #714339 - Flags: review?(hskupin)
Attachment #714339 - Flags: review?(dave.hunt)
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.
Attachment #714339 - Flags: review?(hskupin)
Attachment #714339 - Flags: review?(dave.hunt)
Attachment #714339 - Flags: review-
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.
Flags: needinfo?(mwobensmith)
Daniela, can you please have a look at the following addon how it performs this task? 

https://addons.mozilla.org/addon/betterprivacy/
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.
Attachment #715780 - Flags: review?(hskupin)
Flags: needinfo?(mwobensmith)
Attached patch patch v1.3 (obsolete) — Splinter Review
Patch without the project files and *.as files. I will add those as a *.tar archive to this bug per comment #17
Attachment #715780 - Attachment is obsolete: true
Attachment #715780 - Flags: review?(hskupin)
Attachment #716497 - Flags: review?(hskupin)
Attachment #716497 - Flags: review?(andreea.matei)
Attached file Assets *.tar archive
Assets
Attachment #714339 - Attachment is obsolete: true
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.
Attachment #716497 - Flags: review?(hskupin)
Attachment #716497 - Flags: review?(andreea.matei)
Attachment #716497 - Flags: review-
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
Attached patch patch v1.4 (obsolete) — Splinter Review
Made changes per review
Attachment #716497 - Attachment is obsolete: true
Attachment #717062 - Flags: review?(hskupin)
Attachment #717062 - Flags: review?(andreea.matei)
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.
Attachment #717062 - Flags: review?(hskupin)
Attachment #717062 - Flags: review?(andreea.matei)
Attachment #717062 - Flags: review+
Attached patch patch v1.5 (obsolete) — Splinter Review
Added the modified HTML file to the patch
Attachment #717062 - Attachment is obsolete: true
Attachment #718340 - Flags: review?(hskupin)
Attachment #718340 - Flags: review?(andreea.matei)
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.
Attachment #718340 - Flags: review?(hskupin)
Attachment #718340 - Flags: review?(andreea.matei)
Attachment #718340 - Flags: review+
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.
Keywords: checkin-needed
Assignee: mwobensmith → dpetrovici
Attached patch patch v1.6Splinter Review
Added an input field so that the result of the set operation is displayed
Attachment #718340 - Attachment is obsolete: true
Attachment #718942 - Flags: review?(hskupin)
Attachment #718942 - Flags: review?(andreea.matei)
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.
Attachment #718942 - Flags: review?(hskupin)
Attachment #718942 - Flags: review?(andreea.matei)
Attachment #718942 - Flags: review+
http://hg.mozilla.org/qa/testcase-data/rev/e1349dd3ee18
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
No longer blocks: 827752
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.