Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5745141503492096: Issue 1561 - Move unit tests out of the browser`s content area (Closed)

Created:
Nov. 14, 2014, 11:11 p.m. by Wladimir Palant
Modified:
Jan. 10, 2015, 12:47 a.m.
Reviewers:
tschuster
Visibility:
Public.

Description

This patch got a lot larger than I expected, sorry about that. Some unit tests had to be adjusted because the chrome <=> content security boundary moved so e.g. frames created by unit tests need to communicate to the test differently. However, QUnit also got better at catching errors in these frames apparently. Pop-up blocker tests aren't currently e10s-compatible but I plan to address this separately.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Final patch #

Total comments: 5

Patch Set 3 : Addressed comment #

Patch Set 4 : Rebased patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -56 lines) Patch
M chrome.manifest View 1 chunk +1 line, -0 lines 0 comments Download
A chrome/content/harness.js View 1 chunk +13 lines, -0 lines 0 comments Download
A chrome/content/harness.xul View 1 chunk +39 lines, -0 lines 0 comments Download
M chrome/content/index.html View 1 chunk +17 lines, -21 lines 0 comments Download
A chrome/content/settings.xul View 1 chunk +37 lines, -0 lines 0 comments Download
M chrome/content/tests/elemhide.js View 1 2 2 chunks +10 lines, -5 lines 0 comments Download
M chrome/content/tests/policy.js View 1 2 3 5 chunks +68 lines, -15 lines 0 comments Download
M chrome/content/tests/popupBlocker.js View 1 2 3 chunks +14 lines, -14 lines 0 comments Download
M chrome/content/tests/ui/icon_position.js View 1 1 chunk +1 line, -1 line 0 comments Download
A chrome/locale/en-US/harness.dtd View 1 chunk +1 line, -0 lines 0 comments Download
A chrome/locale/en-US/settings.dtd View 1 chunk +3 lines, -0 lines 0 comments Download
M lib/main.js View 1 chunk +98 lines, -0 lines 0 comments Download
M metadata.gecko View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 10
Wladimir Palant
Nov. 14, 2014, 11:11 p.m. (2014-11-14 23:11:10 UTC) #1
Wladimir Palant
This fixes the remaining issues, only remaining problem being that pop-up blocker tests still don`t ...
Nov. 15, 2014, 11:53 p.m. (2014-11-15 23:53:42 UTC) #2
tschuster
On 2014/11/15 23:53:42, Wladimir Palant wrote: > This fixes the remaining issues, only remaining problem ...
Nov. 16, 2014, 12:47 p.m. (2014-11-16 12:47:55 UTC) #3
tschuster
LGTM http://codereview.adblockplus.org/5745141503492096/diff/5629499534213120/chrome/content/settings.xul File chrome/content/settings.xul (right): http://codereview.adblockplus.org/5745141503492096/diff/5629499534213120/chrome/content/settings.xul#newcode30 chrome/content/settings.xul:30: <menuitem value="" label="&tests.all;"/> This is super awesome, being ...
Nov. 16, 2014, 12:50 p.m. (2014-11-16 12:50:52 UTC) #4
tschuster
http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/tests/elemhide.js File chrome/content/tests/elemhide.js (right): http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/tests/elemhide.js#newcode129 chrome/content/tests/elemhide.js:129: frame.addEventListener("frameready", function() When I first saw this I didn't ...
Nov. 16, 2014, 12:53 p.m. (2014-11-16 12:53:54 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/index.html File chrome/content/index.html (right): http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/index.html#newcode28 chrome/content/index.html:28: result = ["tests/" + test + ".js" for (test ...
Nov. 17, 2014, 2:28 p.m. (2014-11-17 14:28:13 UTC) #6
tschuster
On 2014/11/17 14:28:13, Wladimir Palant wrote: > http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/index.html > File chrome/content/index.html (right): > > http://codereview.adblockplus.org/5745141503492096/diff/5741031244955648/chrome/content/index.html#newcode28 ...
Nov. 17, 2014, 4:28 p.m. (2014-11-17 16:28:36 UTC) #7
Wladimir Palant
Ok, if there is no documented decision I'd rather go with the backwards-compatible variant for ...
Nov. 17, 2014, 6:58 p.m. (2014-11-17 18:58:44 UTC) #8
Wladimir Palant
Tom, are you fine with the final patch?
Dec. 11, 2014, 5:18 p.m. (2014-12-11 17:18:25 UTC) #9
tschuster
Dec. 13, 2014, 12:15 p.m. (2014-12-13 12:15:25 UTC) #10
On 2014/12/11 17:18:25, Wladimir Palant wrote:
> Tom, are you fine with the final patch?

Yes! Sorry, I must have missed the new patch.
LGTM

Powered by Google App Engine
This is Rietveld