Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(336)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by Wladimir Palant
Modified:
5 years, 1 month ago
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
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (2014-11-17 18:58:44 UTC) #8
Wladimir Palant
Tom, are you fine with the final patch?
5 years, 2 months ago (2014-12-11 17:18:25 UTC) #9
tschuster
5 years, 2 months ago (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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5