Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1051653006:
Add sheriff documentation for rebaselining. (Closed)
DescriptionAdd sheriff documentation for rebaselining.
Copied from notes Robert emailed me.
NOTRY=true
DOCS_PREVIEW= https://skia.org/?cl=1051653006
Committed: https://skia.googlesource.com/skia/+/fde1c85696656e5ccaa938114751e3f6bab6a90f
Patch Set 1 #Patch Set 2 : Changes to appearance #
Total comments: 16
Patch Set 3 : Respond to rmistry's comments on ps 2. #Patch Set 4 : Better link; another attempt to indent. #Patch Set 5 : Another indentation attempt #
Total comments: 2
Patch Set 6 : Follow Ravi's suggestion from ps 5 #Patch Set 7 : No top level numbers #Messages
Total messages: 18 (4 generated)
scroggo@google.com changed reviewers: + rmistry@google.com, robertphillips@google.com
https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:125: * DEPS roll #, I was trying to indent this further, but did not know how. Is there documentation how to generate the documentation somewhere?
https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:18: + [What to do if DEPS roll fails to land](#deps_roll_failures) Also add a link here to your new section. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:117: ### Rebaseline Layout Tests (i.e., add suppressions) When would the sheriff need to do this? Maybe you can add a line in the above section ("What to do if DEPS roll fails to land") explaining when this would need to happen and link to this new section. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:120: * goto crbug.com Making this a link might be useful. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:125: * DEPS roll #, On 2015/04/03 13:43:13, scroggo wrote: > I was trying to indent this further, but did not know how. Is there > documentation how to generate the documentation somewhere? The documentation is here: https://skia.org/dev/tools/markdown but I do not see anything about indenting it. I think what had worked for me was indenting by two spaces, if that does not work then try to do it how the first Contents section does it. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:126: * helpful message about what went wrong (e.g., “Changes to how lighting is scaled in Skia r#### changed the following images:”) Nit: helpful -> Helpful https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:127: * layout tests effected Nit: layout -> Layout https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:135: 2. Edit skia/skia_test_expectations.txt A link to the file in googlesource might be useful.
https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:18: + [What to do if DEPS roll fails to land](#deps_roll_failures) On 2015/04/03 14:54:33, rmistry wrote: > Also add a link here to your new section. Done. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:117: ### Rebaseline Layout Tests (i.e., add suppressions) On 2015/04/03 14:54:33, rmistry wrote: > When would the sheriff need to do this? > Maybe you can add a line in the above section ("What to do if DEPS roll fails to > land") explaining when this would need to happen and link to this new section. Done. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:120: * goto crbug.com On 2015/04/03 14:54:33, rmistry wrote: > Making this a link might be useful. Done. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:126: * helpful message about what went wrong (e.g., “Changes to how lighting is scaled in Skia r#### changed the following images:”) On 2015/04/03 14:54:34, rmistry wrote: > Nit: helpful -> Helpful Done. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:127: * layout tests effected On 2015/04/03 14:54:33, rmistry wrote: > Nit: layout -> Layout Done. https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:135: 2. Edit skia/skia_test_expectations.txt On 2015/04/03 14:54:34, rmistry wrote: > A link to the file in googlesource might be useful. Done. https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:128: * DEPS roll #, This sure looks to me like the example of Fee Fie Fo Fum at https://skia.org/dev/tools/markdown, but it does not seem to work :( (I also tried using "+" like the contents, but no luck...)
On 2015/04/06 17:44:53, scroggo wrote: > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > File site/dev/sheriffing/index.md (right): > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:18: + [What to do if DEPS roll fails to > land](#deps_roll_failures) > On 2015/04/03 14:54:33, rmistry wrote: > > Also add a link here to your new section. > > Done. > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:117: ### Rebaseline Layout Tests (i.e., add > suppressions) > On 2015/04/03 14:54:33, rmistry wrote: > > When would the sheriff need to do this? > > Maybe you can add a line in the above section ("What to do if DEPS roll fails > to > > land") explaining when this would need to happen and link to this new section. > > Done. > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:120: * goto http://crbug.com > On 2015/04/03 14:54:33, rmistry wrote: > > Making this a link might be useful. > > Done. > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:126: * helpful message about what went wrong (e.g., > “Changes to how lighting is scaled in Skia r#### changed the following images:”) > On 2015/04/03 14:54:34, rmistry wrote: > > Nit: helpful -> Helpful > > Done. > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:127: * layout tests effected > On 2015/04/03 14:54:33, rmistry wrote: > > Nit: layout -> Layout > > Done. > > https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:135: 2. Edit skia/skia_test_expectations.txt > On 2015/04/03 14:54:34, rmistry wrote: > > A link to the file in googlesource might be useful. > > Done. > > https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... > File site/dev/sheriffing/index.md (right): > > https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:128: * DEPS roll #, > This sure looks to me like the example of Fee Fie Fo Fum at > https://skia.org/dev/tools/markdown, but it does not seem to work :( (I also > tried using "+" like the contents, but no luck...) +jcgregorio Maybe Joe remembers how to do sublists using markdown.
jcgregorio@google.com changed reviewers: + jcgregorio@google.com
https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:125: * DEPS roll #, On 2015/04/03 14:54:33, rmistry wrote: > On 2015/04/03 13:43:13, scroggo wrote: > > I was trying to indent this further, but did not know how. Is there > > documentation how to generate the documentation somewhere? > > The documentation is here: https://skia.org/dev/tools/markdown > but I do not see anything about indenting it. I think what had worked for me was > indenting by two spaces, if that does not work then try to do it how the first > Contents section does it. Yes, it is two spaces, see the nested list on this page: https://skia.org/dev/tools/markdown.md
https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/20001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:125: * DEPS roll #, On 2015/04/06 17:57:21, jcgregorio wrote: > On 2015/04/03 14:54:33, rmistry wrote: > > On 2015/04/03 13:43:13, scroggo wrote: > > > I was trying to indent this further, but did not know how. Is there > > > documentation how to generate the documentation somewhere? > > > > The documentation is here: https://skia.org/dev/tools/markdown > > but I do not see anything about indenting it. I think what had worked for me > was > > indenting by two spaces, if that does not work then try to do it how the first > > Contents section does it. > > Yes, it is two spaces, see the nested list on this page: > > https://skia.org/dev/tools/markdown.md Yeah, that's the page I'm looking at, and my latest CL I *think* I'm doing the same thing, but it does not appear to be working.
https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... File site/dev/sheriffing/index.md (right): https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... site/dev/sheriffing/index.md:128: * DEPS roll #, On 2015/04/06 17:44:53, scroggo wrote: > This sure looks to me like the example of Fee Fie Fo Fum at > https://skia.org/dev/tools/markdown, but it does not seem to work :( (I also > tried using "+" like the contents, but no luck...) Maybe a solution is to remove spaces from the top level bullets and to only have them for the sub bullets. i.e. * Description: * DEPS roll #,
On 2015/04/06 18:06:43, rmistry wrote: > https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... > File site/dev/sheriffing/index.md (right): > > https://codereview.chromium.org/1051653006/diff/80001/site/dev/sheriffing/ind... > site/dev/sheriffing/index.md:128: * DEPS roll #, > On 2015/04/06 17:44:53, scroggo wrote: > > This sure looks to me like the example of Fee Fie Fo Fum at > > https://skia.org/dev/tools/markdown, but it does not seem to work :( (I also > > tried using "+" like the contents, but no luck...) > > Maybe a solution is to remove spaces from the top level bullets and to only have > them for the sub bullets. > i.e. > > * Description: > * DEPS roll #, I thought I had tried that before... that mimics patch set 1, where the first sub bullets (e.g. go to crbug.com, Description, etc) get upgraded to numbers (this is patch set 6 - I do not think there's a way to see all the different patch sets from the link). I tried taking out the numbers (ps 7) and just using asterisks (where the lines I originally wanted numbered are less indented), and I get back to an earlier bad version, where DEPS roll etc are no more indented that Description.
LGTM
The CQ bit was checked by scroggo@google.com
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051653006/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/fde1c85696656e5ccaa938114751e3f6bab6a90f
Message was sent while issue was closed.
Failed to apply the patch.
Message was sent while issue was closed.
On 2015/04/07 13:41:29, I haz the power (commit-bot) wrote: > Failed to apply the patch. That was strange. It landed successfully but then said failed to apply the patch. |
