Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075153003:
[Findit] Add fracas analysis result feedback page. (Closed)
|
Created:
4 years, 6 months ago by Sharu Jiang Modified:
4 years, 5 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, aarya, mmandlis, lijeffrey, chanli Base URL:
https://chromium.googlesource.com/infra/infra.git@master Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Findit] Add fracas analysis result feedback page.
How to see the page:
1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard
2. Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results.
BUG=615267
Committed: https://chromium.googlesource.com/infra/infra/+/7cb2d336593b1c1aac029a9fd53e63d203d1fa3c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix nits. #
Total comments: 2
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Fix nits. #
Total comments: 12
Patch Set 5 : Address comments. #Patch Set 6 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== polish. Modifiy crash_analysis Add feedback html. delete print. Add fiels. Merge branch 'master' of https://chromium.googlesource.com/infra/infra into only-dashboard Merge branch 'master' of https://chromium.googlesource.com/infra/infra into only-dashboard Use urlsafe. Fix comments. Fix bug. Address comments. Fix tests. add traiged-unsure. Fix bugs. feedback page. Fix nit. Add test files. Fracas dashboard add triage_status Add dashboard handler and template fracas dashboard. BUG= ========== to ========== [Findit] Add fracas analysis result feedback page. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results. BUG=615267 ==========
katesonia@chromium.org changed reviewers: + mbarbella@google.com, stgao@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
This cl doesn't include the triage button(correct, incorrect, unsure), it only shows the findit result, I will include that in another cl) PTAL :)
mbarbella@chromium.org changed reviewers: + mbarbella@chromium.org
https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/handle... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/handle... appengine/findit/handlers/crash/fracas_result_feedback.py:29: def _GetCrashDataFromAnalysis(self, analysis): Could this be useful anywhere else? Would it make more sense to move something like this into the entity class? https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... appengine/findit/templates/crash/fracas_result_feedback.html:32: function ConstructMonorailUrl() { Any reason why this has to be done in script? It feels a bit hacky. Just use a form. https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... appengine/findit/templates/crash/fracas_result_feedback.html:71: <b> Historical metadata (last 20 versions):</b> Could you post a screenshot of what this looks like (or link to staging)? I'm having some trouble visualizing this from the template. I will say that it seems non-intuitive to have each row as a different feature. This is usually the exact opposite of what you'd want.
Description was changed from ========== [Findit] Add fracas analysis result feedback page. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results. BUG=615267 ========== to ========== [Findit] Add fracas analysis result feedback page. How to see the page: 1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard 2. Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results. BUG=615267 ==========
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/handle... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/handle... appengine/findit/handlers/crash/fracas_result_feedback.py:29: def _GetCrashDataFromAnalysis(self, analysis): On 2016/06/17 22:14:32, Martin Barbella wrote: > Could this be useful anywhere else? Would it make more sense to move something > like this into the entity class? Done. https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... appengine/findit/templates/crash/fracas_result_feedback.html:32: function ConstructMonorailUrl() { On 2016/06/17 22:14:32, Martin Barbella wrote: > Any reason why this has to be done in script? It feels a bit hacky. Just use a > form. This is I want to get 'window.location.href' and I don't want to hard-code a long url (especially with some special characters) in <a href="...">. If I understand correctly, use a form to input those parameters? https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... appengine/findit/templates/crash/fracas_result_feedback.html:71: <b> Historical metadata (last 20 versions):</b> On 2016/06/17 22:14:32, Martin Barbella wrote: > Could you post a screenshot of what this looks like (or link to staging)? I'm > having some trouble visualizing this from the template. I will say that it seems > non-intuitive to have each row as a different feature. This is usually the exact > opposite of what you'd want. I already deployed a version. 2 steps to open it. 1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard 2. Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results.
https://codereview.chromium.org/2075153003/diff/100001/appengine/findit/handl... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/100001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. Hm, my comments in the original CL seems not addressed at all. Same for other files. https://codereview.chromium.org/2067373002/#ps80001 Would you please address comments from there first, and then ask me for another round of review of this CL?
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
https://codereview.chromium.org/2075153003/diff/100001/appengine/findit/handl... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/100001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/06/20 05:02:51, stgao wrote: > Hm, my comments in the original CL seems not addressed at all. Same for other > files. > https://codereview.chromium.org/2067373002/#ps80001 > > Would you please address comments from there first, and then ask me for another > round of review of this CL? Sorry, forgot the comments there ><
Patchset #3 (id:180001) has been deleted
LGTM with a few nits. https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/60001/appengine/findit/templa... appengine/findit/templates/crash/fracas_result_feedback.html:32: function ConstructMonorailUrl() { On 2016/06/17 23:58:22, sharu jiang wrote: > On 2016/06/17 22:14:32, Martin Barbella wrote: > > Any reason why this has to be done in script? It feels a bit hacky. Just use a > > form. > > This is I want to get 'window.location.href' and I don't want to hard-code a > long url (especially with some special characters) in <a href="...">. > > If I understand correctly, use a form to input those parameters? Should be fine as-is based on that. Still, script could be used to populate the one parameter that needs window.location.herf if you want to avoid implementing things like CreateUrl, but it's not that big of a deal. https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:36: parameters.components = 'Tool>Test>Findit'; Shouldn't this be Tools? https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:42: '\n\nWhat is the bug or feature'); Nit: end with ?
https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:36: parameters.components = 'Tool>Test>Findit'; On 2016/06/22 17:23:12, Martin Barbella wrote: > Shouldn't this be Tools? Oops, changed it. https://codereview.chromium.org/2075153003/diff/200001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:42: '\n\nWhat is the bug or feature'); On 2016/06/22 17:23:12, Martin Barbella wrote: > Nit: end with ? Done.
lgtm once comments are addressed https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:26: 'cannot find analysis for crash %s' % analysis.signature) Will an unexpected exception be raised here? https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:33: 'regression_range': analysis.result.get('regression_range', None), Could be even simplified by: dict_var.get('key') https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:50: def HandlePost(self): # pragma: no cover This is not needed. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... File appengine/findit/handlers/crash/test/fracas_result_feedback_test.py (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/test/fracas_result_feedback_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. 2016? Please check the other new files. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:15: <script src="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.1/jquery-ui.min.js"></script> Are all these jquery libs needed? Suggestion: when reusing existing code or html template, clean up before adding new code. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:75: <td align="center">Version</td> Why do we add all data in the thead instead of tbody?
https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... File appengine/findit/handlers/crash/fracas_result_feedback.py (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:26: 'cannot find analysis for crash %s' % analysis.signature) On 2016/06/23 07:48:55, stgao wrote: > Will an unexpected exception be raised here? Ouch, done. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:33: 'regression_range': analysis.result.get('regression_range', None), On 2016/06/23 07:48:55, stgao wrote: > Could be even simplified by: dict_var.get('key') Done. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/fracas_result_feedback.py:50: def HandlePost(self): # pragma: no cover On 2016/06/23 07:48:55, stgao wrote: > This is not needed. Done. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... File appengine/findit/handlers/crash/test/fracas_result_feedback_test.py (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/handl... appengine/findit/handlers/crash/test/fracas_result_feedback_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/23 07:48:55, stgao wrote: > 2016? > > Please check the other new files. Done. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... File appengine/findit/templates/crash/fracas_result_feedback.html (right): https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:15: <script src="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.1/jquery-ui.min.js"></script> On 2016/06/23 07:48:55, stgao wrote: > Are all these jquery libs needed? > > Suggestion: when reusing existing code or html template, clean up before adding > new code. Done. https://codereview.chromium.org/2075153003/diff/220001/appengine/findit/templ... appengine/findit/templates/crash/fracas_result_feedback.html:75: <td align="center">Version</td> On 2016/06/23 07:48:55, stgao wrote: > Why do we add all data in the thead instead of tbody? Done.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mbarbella@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2075153003/#ps240001 (title: "Address comments.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2043973002 Patch 140001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org, mbarbella@chromium.org Link to the patchset: https://codereview.chromium.org/2075153003/#ps260001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Add fracas analysis result feedback page. How to see the page: 1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard 2. Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results. BUG=615267 ========== to ========== [Findit] Add fracas analysis result feedback page. How to see the page: 1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard 2. Click the signature of a crash, the page will be redirected to the feedback page of this crash. This page will only show the findit results. BUG=615267 Committed: https://chromium.googlesource.com/infra/infra/+/7cb2d336593b1c1aac029a9fd53e6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001) as https://chromium.googlesource.com/infra/infra/+/7cb2d336593b1c1aac029a9fd53e6...
Message was sent while issue was closed.
CQ bit was unchecked. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
