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

Issue 29891680: Issue 6986 - Don't use chromium remote interface on Windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by hub
Modified:
1 year, 3 months ago
Reviewers:
Sebastian Noack, tlucas, geo
CC:
Manish Jethani
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Donc run headless on Windows #

Total comments: 2

Patch Set 3 : Fixed nit #

Total comments: 2

Patch Set 4 : Don't launch chromiun in single process on Windows #

Patch Set 5 : Default to Chrome webdriver on Windows. #

Patch Set 6 : Removed the changes in chrome_remote_interface #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -1 line) Patch
M test/runners/chromium_process.js View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M test_runner.js View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 21
hub
1 year, 4 months ago (2018-09-25 21:59:59 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js#newcode62 test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700); fs.constants.S_IRWXU is 0o700. This change has no ...
1 year, 4 months ago (2018-09-25 22:12:47 UTC) #2
hub
https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js#newcode62 test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700); On 2018/09/25 22:12:46, Sebastian Noack wrote: > ...
1 year, 4 months ago (2018-09-26 01:03:25 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js#newcode62 test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700); On 2018/09/26 01:03:25, hub wrote: > On ...
1 year, 4 months ago (2018-09-26 01:10:51 UTC) #4
geo
https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromium_remote_process.js#newcode62 test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700); On 2018/09/26 01:10:51, Sebastian Noack wrote: > ...
1 year, 4 months ago (2018-09-26 13:34:09 UTC) #5
geo
I've tested the patch in combination with this fix:https://hg.adblockplus.org/adblockpluscore/rev/50ac4b73ce21 Now I'm getting the following error: ...
1 year, 4 months ago (2018-09-26 22:00:09 UTC) #6
hub
On 2018/09/26 22:00:09, geo wrote: > I've tested the patch in combination with this > ...
1 year, 4 months ago (2018-09-27 12:47:05 UTC) #7
Sebastian Noack
FWIW, in adblockpluschrome the browser tests pass on Windows now. We use the exact same ...
1 year, 4 months ago (2018-09-27 15:53:25 UTC) #8
hub
On 2018/09/27 15:53:25, Sebastian Noack wrote: > FWIW, in adblockpluschrome the browser tests pass on ...
1 year, 4 months ago (2018-09-27 16:52:39 UTC) #9
hub
I have updated the patch to disable headless on Chrome on Windows.
1 year, 4 months ago (2018-09-27 17:01:02 UTC) #10
Sebastian Noack
Geo, can you try if the tests pass on Windows after applying the latest patch ...
1 year, 4 months ago (2018-09-28 15:37:00 UTC) #11
hub
Updated patch https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromium_process.js File test/runners/chromium_process.js (right): https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromium_process.js#newcode36 test/runners/chromium_process.js:36: let {platform} = process; On 2018/09/28 15:37:00, ...
1 year, 4 months ago (2018-09-28 15:56:35 UTC) #12
geo
https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromium_remote_process.js#newcode72 test/runners/chromium_remote_process.js:72: "--headless", "--single-process", "--disable-gpu", "--no-sandbox", I've tested the latest patch, ...
1 year, 4 months ago (2018-10-01 13:19:04 UTC) #13
hub
Updated patch https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromium_remote_process.js File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromium_remote_process.js#newcode72 test/runners/chromium_remote_process.js:72: "--headless", "--single-process", "--disable-gpu", "--no-sandbox", On 2018/10/01 13:19:03, ...
1 year, 3 months ago (2018-10-30 23:39:45 UTC) #14
hub
updated patch Now we default to webdriver Chromium on windows.
1 year, 3 months ago (2018-11-08 21:25:40 UTC) #15
geo
On 2018/11/08 21:25:40, hub wrote: > updated patch > > Now we default to webdriver ...
1 year, 3 months ago (2018-11-08 21:32:01 UTC) #16
hub
Added Tristan as a reviewer since it is "Automation" Manish in CC
1 year, 3 months ago (2018-11-09 18:20:38 UTC) #17
Sebastian Noack
Are the changes in chromium_remote_process.js necessary if we only use the web driver implementation on ...
1 year, 3 months ago (2018-11-09 19:07:46 UTC) #18
hub
On 2018/11/09 19:07:46, Sebastian Noack wrote: > Are the changes in chromium_remote_process.js necessary if we ...
1 year, 3 months ago (2018-11-09 19:50:43 UTC) #19
hub
updated patch
1 year, 3 months ago (2018-11-09 20:03:10 UTC) #20
Sebastian Noack
1 year, 3 months ago (2018-11-09 20:17:27 UTC) #21
LGTM
Sign in to reply to this message.

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