Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081383004:
SoM: Added notice about chromium webkit failures + documentation links. (Closed)
DescriptionSoM: Added notice about chromium webkit failures + documentation links.
Notices appear at the top for trees with chromium.webkit failures. [Help] links also now exist beside the title per Ojan's suggestion in the bug thread though these currently only appear for webkit failures.
BUG=611277
Committed: https://chromium.googlesource.com/infra/infra/+/418d7294a8c74b2ef50cb355b7f6832e050debc6
Patch Set 1 #
Total comments: 10
Patch Set 2 : Changed in response to feedback. Pending what to do with duplicate logic. #Patch Set 3 : String.includes #
Total comments: 2
Patch Set 4 : Changed logic to just check the tree. #
Messages
Total messages: 16 (5 generated)
zhangtiff@google.com changed reviewers: + martiniss@chromium.org, seanmccullough@chromium.org
PTAL
Description was changed from ========== SoM: Added notice about chromium webkit failures + documentation links. BUG=611277 ========== to ========== SoM: Added notice about chromium webkit failures + documentation links. Notices appear at the top for trees with chromium.webkit failures. [Help] links also now exist beside the title per Ojan's suggestion in the bug thread though these currently only appear for webkit failures. BUG=611277 ==========
https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html:198: // TODO: Find a better way to categorize alerts Usually, with TODOs, you do something like TODO(zhangtiff) (or another username), so that there's someone you can bother if you run into this TODO later. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html:199: return alert.key.indexOf("chromium.webkit") > -1; I believe there's an array.include you can use. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:57: padding-bottom: 10px; Why did this get added? https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:247: _hideWebkitNotice: { Alphabetize https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:459: for (var i = 0; i < alerts.length; i++) { I think there's a fancy for loop you can do, like for x in alerts. But this works if you need to do it. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:460: if (alerts[i].key.indexOf("chromium.webkit") > -1) { I don't really like this logic being duplicated between here and the alert item. Not sure if we can avoid this; sean do you know?
Thanks for the feedback, Stephen! I've made changes for all of them except the ones I responded directly to. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html:199: return alert.key.indexOf("chromium.webkit") > -1; On 2016/06/23 at 01:20:25, martiniss wrote: > I believe there's an array.include you can use. alert.key is a string so I don't think that'd work here, but I imagine there might possibly be a better field for me to use than the key. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:460: if (alerts[i].key.indexOf("chromium.webkit") > -1) { On 2016/06/23 at 01:20:25, martiniss wrote: > I don't really like this logic being duplicated between here and the alert item. Not sure if we can avoid this; sean do you know? One thought I had was to directly add a isWebkitNotice() function to the alerts objects, but I wasn't sure if this was desirable/what the best way to do this would be.
seanmccullough@google.com changed reviewers: + seanmccullough@google.com
https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html:199: return alert.key.indexOf("chromium.webkit") > -1; On 2016/06/23 17:24:57, Tiff wrote: > On 2016/06/23 at 01:20:25, martiniss wrote: > > I believe there's an array.include you can use. > > alert.key is a string so I don't think that'd work here, but I imagine there > might possibly be a better field for me to use than the key. There is an includes() on String too :) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
Alright, now it's down to the duplicate logic. https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html (right): https://codereview.chromium.org/2081383004/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-alert-item.html:199: return alert.key.indexOf("chromium.webkit") > -1; On 2016/06/23 at 17:36:41, seanmccullough1 wrote: > On 2016/06/23 17:24:57, Tiff wrote: > > On 2016/06/23 at 01:20:25, martiniss wrote: > > > I believe there's an array.include you can use. > > > > alert.key is a string so I don't think that'd work here, but I imagine there > > might possibly be a better field for me to use than the key. > > There is an includes() on String too :) > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Ahh, you're right! Submitted another patch for this. Thank you. :)
https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:456: _computeHideWebkitNotice: function(alerts) { Why don't we just always show this to chromium sheriffs, and no one else. So just turn this into `if tree == 'chromium`? sean WDYT? Also, this would prevent android sheriffs from seeing this.
https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:456: _computeHideWebkitNotice: function(alerts) { On 2016/06/23 22:55:07, martiniss wrote: > Why don't we just always show this to chromium sheriffs, and no one else. So > just turn this into `if tree == 'chromium`? sean WDYT? > > Also, this would prevent android sheriffs from seeing this. SGTM
On 2016/06/23 at 23:17:30, seanmccullough wrote: > https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... > File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): > > https://codereview.chromium.org/2081383004/diff/40001/go/src/infra/appengine/... > go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:456: _computeHideWebkitNotice: function(alerts) { > On 2016/06/23 22:55:07, martiniss wrote: > > Why don't we just always show this to chromium sheriffs, and no one else. So > > just turn this into `if tree == 'chromium`? sean WDYT? > > > > Also, this would prevent android sheriffs from seeing this. > > SGTM Good call! I think this would be cleaner to code and cleaner from a user perspective (the notice disappearing when removing all webkit alerts is kinda weird). Pushed a new patch. :)
lgtm
The CQ bit was checked by zhangtiff@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081383004/60001
Message was sent while issue was closed.
Description was changed from ========== SoM: Added notice about chromium webkit failures + documentation links. Notices appear at the top for trees with chromium.webkit failures. [Help] links also now exist beside the title per Ojan's suggestion in the bug thread though these currently only appear for webkit failures. BUG=611277 ========== to ========== SoM: Added notice about chromium webkit failures + documentation links. Notices appear at the top for trees with chromium.webkit failures. [Help] links also now exist beside the title per Ojan's suggestion in the bug thread though these currently only appear for webkit failures. BUG=611277 Committed: https://chromium.googlesource.com/infra/infra/+/418d7294a8c74b2ef50cb355b7f68... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/infra/infra/+/418d7294a8c74b2ef50cb355b7f68... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
