Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783203003:
Add components for directories where we know them (Closed)
|
Created:
3 years, 8 months ago by Sergiy Byelozyorov Modified:
3 years, 8 months ago CC:
abarth-chromium, darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, jshin+watch_chromium.org, mstensho (USE GERRIT), ortuno+watch_chromium.org, rjwright, scheib+watch_chromium.org, shans Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd components for directories where we know them
BUG=679390
Review-Url: https://codereview.chromium.org/2783203003
Cr-Commit-Position: refs/heads/master@{#462448}
Committed: https://chromium.googlesource.com/chromium/src/+/d075a924cbae0c0ec00ec4826da6a607fcd710af
Patch Set 1 #Patch Set 2 : Fix presubmit errors #
Total comments: 13
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Addressed comments #
Messages
Total messages: 43 (28 generated)
This are the components that we have for teams in the spreadsheet. Note that some components can not be added since they do not match teams and presubmit fails.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/30 15:14:45, Sergiy Byelozyorov wrote: > This are the components that we have for teams in the spreadsheet. Note that > some components can not be added since they do not match teams and presubmit > fails. Presubmit log is here: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
On 2017/03/30 15:20:49, Sergiy Byelozyorov wrote: > On 2017/03/30 15:14:45, Sergiy Byelozyorov wrote: > > This are the components that we have for teams in the spreadsheet. Note that > > some components can not be added since they do not match teams and presubmit > > fails. > > Presubmit log is here: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux.... I've fixed those errors by updating team owners in LayoutTests to match those used elsewhere with the same component. The only exception is Blink>WebComponents that is mapped to input-dev@chromium.org and text-encoding-dev@chromium.org for the following dirs: - third_party/WebKit/LayoutTests/fast/input - third_party/WebKit/LayoutTests/fast/encoding and not used anywhere outside. I've ended up using input-dev@chromium.org for both files since it seems to be more generic.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/OWNERS:1: # TEAM: worker-dev@chromium.org The team should still be web-bluetooth@chromium.org and the component should be Blink>Bluetooth. I'm currently trying to get access to the spreadsheet to correct it.
ojan@chromium.org changed reviewers: + ojan@chromium.org
On 2017/04/03 at 15:07:30, sergiyb wrote: > On 2017/03/30 15:20:49, Sergiy Byelozyorov wrote: > > On 2017/03/30 15:14:45, Sergiy Byelozyorov wrote: > > > This are the components that we have for teams in the spreadsheet. Note that > > > some components can not be added since they do not match teams and presubmit > > > fails. > > > > Presubmit log is here: > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux.... > > I've fixed those errors by updating team owners in LayoutTests to match those used elsewhere with the same component. The only exception is Blink>WebComponents that is mapped to input-dev@chromium.org and text-encoding-dev@chromium.org for the following dirs: > > - third_party/WebKit/LayoutTests/fast/input > - third_party/WebKit/LayoutTests/fast/encoding > > and not used anywhere outside. I've ended up using input-dev@chromium.org for both files since it seems to be more generic. I think you've ended up with a bunch of team mappings that are wrong by doing this. I think you'll need to check with the teams in question. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/animation/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/animation/OWNERS:1: # TEAM: paint-dev@chromium.org This doesn't look right. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/cssom/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/cssom/OWNERS:1: # TEAM: dom-dev@chromium.org This doens't look right. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/beacon/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/beacon/OWNERS:1: # TEAM: dom-dev@chromium.org This doesn't look right. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/encoding/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/encoding/OWNERS:2: # COMPONENT: Blink>WebComponents This isn't right. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/input/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/input/OWNERS:2: # COMPONENT: Blink>WebComponents This isn't right.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/OWNERS:1: # TEAM: worker-dev@chromium.org On 2017/04/03 20:19:45, ortuno wrote: > The team should still be mailto:web-bluetooth@chromium.org and the component should be > Blink>Bluetooth. > > I'm currently trying to get access to the spreadsheet to correct it. Done. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/animation/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/animation/OWNERS:1: # TEAM: paint-dev@chromium.org On 2017/04/03 20:47:56, ojan wrote: > This doesn't look right. Reverted and removed component. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/cssom/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/cssom/OWNERS:1: # TEAM: dom-dev@chromium.org On 2017/04/03 20:47:56, ojan wrote: > This doens't look right. Reverted and removed component. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/beacon/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/beacon/OWNERS:1: # TEAM: dom-dev@chromium.org On 2017/04/03 20:47:56, ojan wrote: > This doesn't look right. Reverted and removed component. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/encoding/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/encoding/OWNERS:2: # COMPONENT: Blink>WebComponents On 2017/04/03 20:47:56, ojan wrote: > This isn't right. Reverted and removed component. https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/input/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/input/OWNERS:2: # COMPONENT: Blink>WebComponents On 2017/04/03 20:47:56, ojan wrote: > This isn't right. Removed component.
https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/OWNERS (right): https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/OWNERS:1: # TEAM: worker-dev@chromium.org On 2017/04/04 07:18:30, Sergiy Byelozyorov wrote: > On 2017/04/03 20:19:45, ortuno wrote: > > The team should still be mailto:web-bluetooth@chromium.org and the component > should be > > Blink>Bluetooth. > > > > I'm currently trying to get access to the spreadsheet to correct it. > > Done. Spreadsheet is now deprecated. New changes should be done to the OWNERS files as they are used as a source of truth.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/04 07:19:28, Sergiy Byelozyorov wrote: > https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/bluetooth/OWNERS (right): > > https://codereview.chromium.org/2783203003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/bluetooth/OWNERS:1: # TEAM: > mailto:worker-dev@chromium.org > On 2017/04/04 07:18:30, Sergiy Byelozyorov wrote: > > On 2017/04/03 20:19:45, ortuno wrote: > > > The team should still be mailto:web-bluetooth@chromium.org and the component > > should be > > > Blink>Bluetooth. > > > > > > I'm currently trying to get access to the spreadsheet to correct it. > > > > Done. > > Spreadsheet is now deprecated. New changes should be done to the OWNERS files as > they are used as a source of truth. Friendly ping
bluetooth lgtm! Sorry I didn't realize this was waiting on me.
On 2017/04/05 07:24:56, ortuno wrote: > bluetooth lgtm! Sorry I didn't realize this was waiting on me. No worries, I am also waiting for Ojan to LGTM the rest.
lgtm Looking at the spreadsheet, I see a bunch of rows that aren't listed here that are correct, but I also see a bunch that are wrong (i.e. the component doesn't actually map to something that team triages AFAIK). So, lets fix the two cases I mention here that don't look right, submit this patch, and then reach out to teams to add components. Shruthi, do you want to drive that? https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS (right): https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS:2: # COMPONENT: Blink>DOM This one still doesn't look right to me. Did you check with the layout team? https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS (right): https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS:1: # TEAM: dom-dev@chromium.org Ditto
On 2017/04/05 17:46:02, ojan wrote: > lgtm > > Looking at the spreadsheet, I see a bunch of rows that aren't listed here that > are correct, but I also see a bunch that are wrong (i.e. the component doesn't > actually map to something that team triages AFAIK). > > So, lets fix the two cases I mention here that don't look right, submit this > patch, and then reach out to teams to add components. Shruthi, do you want to > drive that? > > https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS > (right): > > https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS:2: # > COMPONENT: Blink>DOM > This one still doesn't look right to me. Did you check with the layout team? > > https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS > (right): > > https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS:1: # > TEAM: mailto:dom-dev@chromium.org > Ditto SGTM. I will drive adding components to these OWNERS files after this patch lands.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS (right): https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css3-text/css3-text-align-last/OWNERS:2: # COMPONENT: Blink>DOM On 2017/04/05 17:46:02, ojan wrote: > This one still doesn't look right to me. Reverted. > Did you check with the layout team? No, I didn't reach out to individual teams because this CL is supposed to just transfer limited information that we have in the spreadsheet. IMHO cases where a component is not clear (incorrect or conflicts with another team-to-component mapping) should be handled separately. https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS (right): https://codereview.chromium.org/2783203003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/multicol/vertical-lr/balancing/OWNERS:1: # TEAM: dom-dev@chromium.org On 2017/04/05 17:46:02, ojan wrote: > Ditto Done.
The CQ bit was unchecked by sergiyb@chromium.org
The CQ bit was checked by sergiyb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/2783203003/#ps100001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491481063990100,
"parent_rev": "0ab573d311cd7a4a72b86ee8d47a7b70b358432b", "commit_rev":
"d075a924cbae0c0ec00ec4826da6a607fcd710af"}
Message was sent while issue was closed.
Description was changed from ========== Add components for directories where we know them BUG=679390 ========== to ========== Add components for directories where we know them BUG=679390 Review-Url: https://codereview.chromium.org/2783203003 Cr-Commit-Position: refs/heads/master@{#462448} Committed: https://chromium.googlesource.com/chromium/src/+/d075a924cbae0c0ec00ec4826da6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d075a924cbae0c0ec00ec4826da6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
