Closed Bug 915824 Opened 10 years ago Closed 9 years ago

move more CSP tests from content/base/test into csp subdir


(Core :: DOM: Core & HTML, enhancement)

Not set





(Reporter: geekboy, Assigned: yeukhon)


(Blocks 1 open bug)



(1 file, 7 obsolete files)

Most of the CSP tests were moved in bug 903080, but a few got missed 'cause they're named stealthily.

Blocks: CSP
We should rename these tests to be descriptive while we're at it - just have to make sure the bug # is in a comment, preferably at the top of the test_*.html file.
I will flag one but there are five parts (each move is a single patch), but they depend on the order (which isn't nice but we can change that if we want to...)
Assignee: nobody → yeukhon
Attachment #8368955 - Flags: feedback?(sstamm)
Brief try build (in-progress):

Local builds look good (tested for each patch)
A quick comment: redirect tests have been "re-implemented" in csp/ directory with a quick glance. Therefore I added xcsp prefix to the file names. Injection is assosicated with XCSP at the time so it was named with xcsp prefix. The other two without xcsp.. I am just waiting to see if we should add xcsp or not..

In general, I think *it might be a good idea* to name XCSP-only tests with xcsp so that when we remove XCSP support from Firefox (I assume this is what we will do!) we can easily remove xcsp...

Sorry for the spam. Thanks!
did you use hg rename/move/mv or did you just delete and addremove the files?
I did hg move (addremove got me in trouble, we can talk about that one on IRC). Or I think I did hg move for most of them. Do they not just show up as R and A in the end?

I also have to fix comments and file path after I did hg move.
Look at your raw diffs... they're different for the different techniques.  

regarding xcsp: if the test makes ZERO sense for the spec compliant header, that's fine, use "xcsp" in the file name.  If we can convert it to work with CSP 1.0 or 1.1, then don't because we'll end up tweaking the tests to use the new parser/syntax/etc.
Comment on attachment 8368955 [details] [diff] [review]

Review of attachment 8368955 [details] [diff] [review]:

As we discussed in IRC last Friday, please collapse the moves into one patch and use hg mv to relocate the files.  (Please also be sure you've got a recent version of mercurial as briansmith suggested older hg versions can mess up history of mv'ed files.
Attachment #8368955 - Flags: feedback?(sstamm)
Attached patch move_csp_test_v2.patch (obsolete) — Splinter Review
As discussed, I am using Mercurial Distributed SCM (version 2.8.2).

Try looks good:
Attachment #8368955 - Attachment is obsolete: true
Attachment #8368956 - Attachment is obsolete: true
Attachment #8368957 - Attachment is obsolete: true
Attachment #8368958 - Attachment is obsolete: true
Attachment #8368960 - Attachment is obsolete: true
Attachment #8370587 - Flags: review?(sstamm)
Comment on attachment 8370587 [details] [diff] [review]

Review of attachment 8370587 [details] [diff] [review]:

r=me.  Please make sure the bug #s are preserved in the test files you renamed to not have bug numbers.

::: content/base/test/csp/test_301_redirect.html
@@ +70,5 @@
>  SimpleTest.waitForExplicitFinish();
>  // save this for last so that our listeners are registered.
> +document.getElementById('content_iframe').src = 'file_redirect_content.sjs?301';

Please maintain the bug number in this file somewhere (in a comment at the top would probably be fine).  Actually, please make sure the bug number is referenced somewhere in each file where you change the name from a bug number to its functionality (this way it's easier to search by bug number).

::: content/base/test/csp/test_csp_report.html
@@ +1,5 @@
>  <html>
> +<!--
> +
> +-->

Yeah, like this.  This is good.
Attachment #8370587 - Flags: review?(sstamm) → review+
Attached patch move_csp_test_v3.patch (obsolete) — Splinter Review
Carrying over r+ from Sid.

Added bug numbers to all sjs and html files except ^headers^.

Try looks good:
Attachment #8370587 - Attachment is obsolete: true
Attachment #8375192 - Flags: review+
Keywords: checkin-needed
This effectively re-enabled a lot of tests that were disabled on B2G because their platform-specific manifests weren't updated. Backed out.
Modified b2g.json, b2g-desktop.json and b2g-debug.json to reflect the name changes.

Try looks good, all green:
Attachment #8377019 - Flags: review+
Attachment #8375192 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.