Split out platform-specific webexposed tests. [chromium/src : master]

Paul Meyer (Gerrit)

unread,

Oct 16, 2017, 1:43:43 PM10/16/17

to blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

This change is ready for review.

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Rick Byers (Gerrit)

unread,

Oct 17, 2017, 5:37:56 PM10/17/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Looks good overall, just a few small suggestions

View Change

6 comments:

File third_party/WebKit/LayoutTests/resources/global-interface-listing.js:
- Patch Set #10, Line 95: // a new platform-specific interface.
  Suggest adding something here about WHY we have this split. Eg. "This enables us to keep the churn on the platform-specific expectations files to a bare minimum to make updates in the common (platform-neutral) case as simple as possible".
- Patch Set #10, Line 96: platformSpecificInterfaces
  Aside: I'm surprised PaymentRequest isn't here, I wonder why!
- Patch Set #10, Line 185: interfaceNames
  nit: make this line easier to read with some line splitting
- Patch Set #10, Line 201: filterPlatformSpecificProperty
  break long line
  Also "(i, p) => filterPlatformSpecificProperty(i, p)" might be slightly clearer/shorter than relying on "bind(null".
File third_party/WebKit/LayoutTests/webexposed/global-interface-listing.html:
- Patch Set #10, Line 10: false
  nit: since this is cross file, use a comment or local to indicate what 'false' means. Eg. 'var platformSpecific = false;'
File third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing-worker.js:
- Patch Set #10, Line 32: false
  Do we want to add platform-specific versions of all the worker cases?
  I guess I can see a compelling argument that the cost (having a multitude more files to update when changing a platform-specific API) isn't worth the benefit (risk of accidentally shipping an API that is BOTH worker-specific and platform-specific). But we should at least make that argument in a comment in case we want to revisit it later.
  Note that we could also consider factoring the work-specific APIs in a way similar to how you're factoring the platform-specific APIs, though the value is lower because updating them automatically is easier (can rely on run-webkit-tests --reset-results).

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Paul Meyer (Gerrit)

unread,

Oct 19, 2017, 11:20:05 AM10/19/17

to blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

PTAL

View Change

6 comments:

File third_party/WebKit/LayoutTests/resources/global-interface-listing.js:
- Patch Set #10, Line 95: // a new platform-specific interface. This enables us to keep the churn on the
  Suggest adding something here about WHY we have this split. Eg. […]
  Done
- Patch Set #10, Line 96: latform-specific expectati

- Aside: I'm surprised PaymentRequest isn't here, I wonder why!

- I'm seeing it in all of the platform-specific expectation files, so I'm guessing it isn't platform-specific anymore.
- Patch Set #10, Line 185: ME: List inter

- nit: make this line easier to read with some line splitting

- Done
- Patch Set #10, Line 201: nction(propertyKey) {
  break long line […]
  Done
File third_party/WebKit/LayoutTests/webexposed/global-interface-listing.html:

- Patch Set #10, Line 10: false

- nit: since this is cross file, use a comment or local to indicate what 'false' means. Eg. […]
  Done
File third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing-worker.js:

- Patch Set #10, Line 32:

- Do we want to add platform-specific versions of all the worker cases? […]
  I've added a comment explaining why this test is not split into platform-specific vs. platform-neutral.

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Rick Byers (Gerrit)

unread,

Oct 19, 2017, 11:57:51 AM10/19/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Patch set 11:Code-Review +1

View Change

2 comments:

File third_party/WebKit/LayoutTests/resources/global-interface-listing.js:
- Patch Set #10, Line 96: latform-specific expectati
  I'm seeing it in all of the platform-specific expectation files, so I'm guessing it isn't platform-s […]
  Yep, I guess I just forgot that they shipped on desktop now. Yay!
File third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing-worker.js:

- Patch Set #10, Line 32:

- I've added a comment explaining why this test is not split into platform-specific vs. […]
  LGTM, thanks. I agree this is the right cost/benefit trade off for now.

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Paul Meyer (Gerrit)

unread,

Oct 19, 2017, 3:20:45 PM10/19/17

to blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Patch set 12:Commit-Queue +2

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 19, 2017, 3:21:26 PM10/19/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

CQ is trying da patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"rebased" https://chromium-review.googlesource.com/c/703097/12

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/703097/12

Bot data: {"action": "start", "triggered_at": "2017-10-19T19:20:42.0Z", "cq_cfg_revision": "fc2b2f04ed20d88113c10951036ca403891c435f", "revision": "cd8c55d1703c66dd1d1b06aa484c3ed98e83f9df"}

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 19, 2017, 6:41:50 PM10/19/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/569929)

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Paul Meyer (Gerrit)

unread,

Oct 21, 2017, 9:20:59 PM10/21/17

to blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Patch set 15:Commit-Queue +2

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 21, 2017, 9:21:13 PM10/21/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

CQ is trying da patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.

"updating mac expectations" https://chromium-review.googlesource.com/c/703097/15

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/703097/15

Bot data: {"action": "start", "triggered_at": "2017-10-22T01:20:54.0Z", "cq_cfg_revision": "573b6c39d3de239a70e8fa672647b63bf0bd1f89", "revision": "5eb430943bf92062582a2de601f58594665c6aa4"}

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 21, 2017, 11:40:47 PM10/21/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Try jobs failed on following builders:

  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/571319)

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Paul Meyer (Gerrit)

unread,

Oct 22, 2017, 10:39:55 AM10/22/17

to blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, Commit Bot, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Patch set 16:Commit-Queue +2

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 22, 2017, 10:40:00 AM10/22/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

CQ is trying da patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.

"updating mac expectations" https://chromium-review.googlesource.com/c/703097/16

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/703097/16

Bot data: {"action": "start", "triggered_at": "2017-10-22T14:39:46.0Z", "cq_cfg_revision": "573b6c39d3de239a70e8fa672647b63bf0bd1f89", "revision": "5080d0041dfb7f0d6d4126bef801e76edb4756bb"}

View Change

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 22, 2017, 11:54:16 AM10/22/17

to Paul Meyer, blink-...@chromium.org, falken...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, mac-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, Rick Byers, chromium...@chromium.org, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki

Commit Bot merged this change.

View Change

Approvals:
  Rick Byers: Looks good to me
  Paul Meyer: Commit

Split out platform-specific webexposed tests.

This patch will split all platform-specific interfaces out of existing
global-interface-listing layout tests into a separate 
global-interface-listing-platform-specific test, using a
platform-specific interface whitelist. This will remove the need to
update expectation files for each platform separately when adding a new
web interface to all platforms (which is usually the case).

Bug: 711292
Change-Id: I5995684f8df82ccbf19678086a29fcb6e633406f
Reviewed-on: https://chromium-review.googlesource.com/703097
Commit-Queue: Paul Meyer <paul...@chromium.org>
Reviewed-by: Rick Byers <rby...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510706}
---
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/resources/global-interface-listing-worker.js
D third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt
D third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
A third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-platform-specific-expected.txt
D third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt
A third_party/WebKit/LayoutTests/platform/mac/webexposed/global-interface-listing-platform-specific-expected.txt
A third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-platform-specific-expected.txt
A third_party/WebKit/LayoutTests/platform/win/webexposed/global-interface-listing-platform-specific-expected.txt
M third_party/WebKit/LayoutTests/resources/global-interface-listing.js
M third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt
R third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
A third_party/WebKit/LayoutTests/webexposed/global-interface-listing-platform-specific.html
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing.html
M third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing-worker.js
20 files changed, 364 insertions(+), 10,437 deletions(-)

To view, visit change 703097. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward