Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684513002:
Markdown docs for TestBrowserDialog (Closed)
DescriptionMarkdown docs for TestBrowserDialog
TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt:
`TestBrowserDialog` provides a way to register an `InProcessBrowserTest`
testing harness with a framework that invokes Chrome browser dialogs in
a consistent way.
The contents are taken from the public doc on chromium.org at https://goo.gl/EFz4r2
BUG=654151
Review-Url: https://codereview.chromium.org/2684513002
Cr-Commit-Position: refs/heads/master@{#449200}
Committed: https://chromium.googlesource.com/chromium/src/+/f38739a13a880a4641302d42914d44714c0ad7dc
Patch Set 1 #Patch Set 2 : margdownify #Patch Set 3 : nits #
Total comments: 24
Patch Set 4 : respond to comments #Messages
Total messages: 13 (8 generated)
Description was changed from ========== Markdown docs for TestBrowserDialog BUG=654151 ========== to ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. BUG=654151 ==========
tapted@chromium.org changed reviewers: + patricialor@chromium.org
Description was changed from ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. BUG=654151 ========== to ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. BUG=654151 ==========
Description was changed from ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. BUG=654151 ========== to ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. The contents are taken from the public doc on chromium.org at https://goo.gl/EFz4r2 BUG=654151 ==========
Hi Patti, could you please review for grammar/correctness/readability/etc. Note I'm using the google Markdown style guide at https://github.com/google/styleguide/blob/gh-pages/docguide/style.md Thanks!
lgtm, assume all suggestions are optional! :) https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... File docs/testing/test_browser_dialog.md (right): https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:14: ## How to use it Replace this heading with "How to register a dialog"? https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:16: There’s a chance a dialog already has a test harness inheriting, using, or with "If registering an existing dialog, there's a chance it already..." https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:23: Change this to inherit from DialogBrowserTest, and add an override of Delete "add an". https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:38: then add test invocations using the usual GTest Macros. In Capital T for "Then". https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:38: then add test invocations using the usual GTest Macros. In I prefer "...Macros, in `foo...", but up to you :) https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:51: customized. Maybe add "See {#Testing-additional-dialog-"styles"}." for string use explanation? https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:95: example: [ExtensionBrowserTest]) then a template is provided. E.g. "...then the SupportsTestDialog<> template is provided."? https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:108: ### If you have more than one dialog "style" to test 'Testing additional dialog "styles"' (Might be nicer for this to be shorter if linking this section as suggested above ^) https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:159: ongoing maintenance Nit: Full stops for this and parent bullet? o_O https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:161: * Maintaining a working end-to-end build target executable is hard "An alternative solution was to maintain a working end-to-end build target executable to do this, but this is hard:" https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:201: cause crashes (e.g. [http://crrev.com/426302](http://crrev.com/426302)) Also add the title of the commit here like you've done in the next bullet point? "MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window." (Or maybe just "Support modal windows with a null parent window".) You could replace the [] part of the link with it, but seeing the crrev domain might be also useful to devs reading this, so not sure. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:275: [ExtensionBrowserTest]: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_brow... Did you want to add the public doc & the bug as well in a "See also" section? (Even though you have the bug linked above somewhere as well)
Thanks patti! https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... File docs/testing/test_browser_dialog.md (right): https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:14: ## How to use it On 2017/02/08 23:43:53, Patti Lor wrote: > Replace this heading with "How to register a dialog"? Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:16: There’s a chance a dialog already has a test harness inheriting, using, or with On 2017/02/08 23:43:53, Patti Lor wrote: > "If registering an existing dialog, there's a chance it already..." Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:23: Change this to inherit from DialogBrowserTest, and add an override of On 2017/02/08 23:43:53, Patti Lor wrote: > Delete "add an". Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:38: then add test invocations using the usual GTest Macros. In On 2017/02/08 23:43:53, Patti Lor wrote: > Capital T for "Then". Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:38: then add test invocations using the usual GTest Macros. In On 2017/02/08 23:43:53, Patti Lor wrote: > I prefer "...Macros, in `foo...", but up to you :) Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:51: customized. On 2017/02/08 23:43:53, Patti Lor wrote: > Maybe add "See {#Testing-additional-dialog-"styles"}." for string use > explanation? Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:95: example: [ExtensionBrowserTest]) then a template is provided. E.g. On 2017/02/08 23:43:53, Patti Lor wrote: > "...then the SupportsTestDialog<> template is provided."? Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:108: ### If you have more than one dialog "style" to test On 2017/02/08 23:43:53, Patti Lor wrote: > 'Testing additional dialog "styles"' (Might be nicer for this to be shorter if > linking this section as suggested above ^) Done. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:159: ongoing maintenance On 2017/02/08 23:43:53, Patti Lor wrote: > Nit: Full stops for this and parent bullet? o_O Done. (yeah these should all be complete sentences :) https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:161: * Maintaining a working end-to-end build target executable is hard On 2017/02/08 23:43:53, Patti Lor wrote: > "An alternative solution was to maintain a working end-to-end build target > executable to do this, but this is hard:" I went with * An alternative is to maintain a working end-to-end build target executable to do this, but this has additional costs (and is hard). https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:201: cause crashes (e.g. [http://crrev.com/426302](http://crrev.com/426302)) On 2017/02/08 23:43:53, Patti Lor wrote: > Also add the title of the commit here like you've done in the next bullet point? > "MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window." > > (Or maybe just "Support modal windows with a null parent window".) You could > replace the [] part of the link with it, but seeing the crrev domain might be > also useful to devs reading this, so not sure. added * Supporting window-modal dialogs with a null parent window. https://codereview.chromium.org/2684513002/diff/40001/docs/testing/test_brows... docs/testing/test_browser_dialog.md:275: [ExtensionBrowserTest]: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_brow... On 2017/02/08 23:43:53, Patti Lor wrote: > Did you want to add the public doc & the bug as well in a "See also" section? > (Even though you have the bug linked above somewhere as well) hm - I feel like the bug and doc are more like historical artifacts rather than a source for more information. (e.g. the doc mostly repeats what's here).
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from patricialor@chromium.org Link to the patchset: https://codereview.chromium.org/2684513002/#ps60001 (title: "respond to 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": 60001, "attempt_start_ts": 1486610244279470,
"parent_rev": "d1065b766d40d754475f9270eea5ed3ce295e81b", "commit_rev":
"f38739a13a880a4641302d42914d44714c0ad7dc"}
Message was sent while issue was closed.
Description was changed from ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. The contents are taken from the public doc on chromium.org at https://goo.gl/EFz4r2 BUG=654151 ========== to ========== Markdown docs for TestBrowserDialog TestBrowserDialog landed in r440240 and is ready for wider use. Excerpt: `TestBrowserDialog` provides a way to register an `InProcessBrowserTest` testing harness with a framework that invokes Chrome browser dialogs in a consistent way. The contents are taken from the public doc on chromium.org at https://goo.gl/EFz4r2 BUG=654151 Review-Url: https://codereview.chromium.org/2684513002 Cr-Commit-Position: refs/heads/master@{#449200} Committed: https://chromium.googlesource.com/chromium/src/+/f38739a13a880a4641302d42914d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f38739a13a880a4641302d42914d... |
