diff options
| author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2020-10-12 14:27:29 +0200 |
|---|---|---|
| committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2020-10-13 09:35:20 +0000 |
| commit | c30a6232df03e1efbd9f3b226777b07e087a1122 (patch) | |
| tree | e992f45784689f373bcc38d1b79a239ebe17ee23 /chromium/docs | |
| parent | 7b5b123ac58f58ffde0f4f6e488bcd09aa4decd3 (diff) | |
| download | qtwebengine-chromium-85-based.tar.gz | |
BASELINE: Update Chromium to 85.0.4183.14085-based
Change-Id: Iaa42f4680837c57725b1344f108c0196741f6057
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
Diffstat (limited to 'chromium/docs')
64 files changed, 2611 insertions, 377 deletions
diff --git a/chromium/docs/OWNERS b/chromium/docs/OWNERS index 94ca8dea854..f75e5460ad5 100644 --- a/chromium/docs/OWNERS +++ b/chromium/docs/OWNERS @@ -1,2 +1,7 @@ * + +# build@ isn't really the right component for this, but it'll +# do until we create a component for documentation. + +# TEAM: build@chromium.org # COMPONENT: Build diff --git a/chromium/docs/accessibility/autoclick.md b/chromium/docs/accessibility/autoclick.md index dd088bb88ec..fb1783983a3 100644 --- a/chromium/docs/accessibility/autoclick.md +++ b/chromium/docs/accessibility/autoclick.md @@ -46,7 +46,7 @@ ash/autoclick/ ash/system/accessibility/autoclick* - A component extension to provide Accessibility tree information, in -chrome/browser/resources/chromeos/accessibility/autoclick/ +chrome/browser/resources/chromeos/accessibility/accessibility_common/ In addition, there are settings for automatic clicks in chrome/browser/resources/settings/a11y_page/manage_a11y_page.* @@ -64,15 +64,15 @@ out/Release/browser_tests --gtest_filter=”Autoclick*” ### Debugging Developers can add log lines to any of the autoclick C++ files and see output -in the console. To debug the Autoclick extension, the easiest way is from an -external browser. Start Chrome OS on Linux with this command-line flag: +in the console. To debug the Accessibility Common extension, the easiest way is +from an external browser. Start Chrome OS on Linux with this command-line flag: ``` out/Release/chrome --remote-debugging-port=9222 ``` Now open http://localhost:9222 in a separate instance of the browser, and debug -the Autoclick extension background page from there. +the Accessibility Common extension background page from there. ## How it works @@ -132,16 +132,16 @@ may dwell anywhere on the screen to change the scroll location. When the scroll location is changed, the AutoclickController will request the -bounds of the nearest scrollable view from the Autoclick component extension -via the AccessibilityPrivate API. The Autoclick component extension has access -to accessibility tree information, and using a HitTest is able to find the view -at the scroll location, then walks up the tree to find the first view which can -scroll, or stops at the nearest window or dialog bounds. This logic takes place -in autoclick.js, onAutomationHitTestResult_. When the scrolling location is -found, the bounds of the scrollable area are highlighted with a focus ring. -In addition, the bounds are sent back through the AccessibilityPrivate API, -routed to the AutoclickController, which passes it via the -AutoclickMenuBubbleController to the AutoclickScrollBubbleController, which +bounds of the nearest scrollable view from the Accessibility Common extension +via the AccessibilityPrivate API. The Accessibility Common component extension +has access to accessibility tree information, and using a HitTest is able to +find the view at the scroll location, then walks up the tree to find the first +view which can scroll, or stops at the nearest window or dialog bounds. This +logic takes place in autoclick.js, onAutomationHitTestResult_. When the +scrolling location is found, the bounds of the scrollable area are highlighted +with a focus ring. In addition, the bounds are sent back through the +AccessibilityPrivate API, routed to the AutoclickController, which passes it via +the AutoclickMenuBubbleController to the AutoclickScrollBubbleController, which does layout accordingly. ### Bubble Menus: interface and positioning @@ -175,7 +175,7 @@ UI. The scroll bubble starts out anchored to the automatic clicks bubble menu, but if the user selects a new scroll point it will move. When a scroll point is -selected, if the scrollable region found by the Autoclick component extension +selected, if the scrollable region found by the Accessibility Common extension is large enough, the scroll bubble will be anchored near the scroll point itself, similarly to the way the context menu is anchored near the cursor on a right click. When the scrollable region is small, the scroll bubble will be diff --git a/chromium/docs/accessibility/brltty.md b/chromium/docs/accessibility/brltty.md index 32c95be70b4..e05f1d7ad73 100644 --- a/chromium/docs/accessibility/brltty.md +++ b/chromium/docs/accessibility/brltty.md @@ -65,8 +65,8 @@ To upload a change, use repo, something like this: ``` repo start <branch_name> . git commit -a - Bug:chromium:12345 - Test:Write what you tested here + Bug: chromium:12345 + Test: Write what you tested here repo upload . ``` @@ -119,6 +119,11 @@ Next, you will need to uprev the ebuild. Do this by renaming all files from the E.g. Brltty-5.4.ebuild -> brltty-5.6.ebuild +Start a build with your changes by doing + +emerge brltty +(or emerge{$BOARD} brltty). + Note: Manifest has various checksums computed based on the release you uploaded to GCS. Each of these will need to be replaced/updated. This should be enough to kick off a build. It is likely patches won’t apply cleanly. @@ -141,6 +146,29 @@ commits to the file containing the conflict then understanding the history since the last release. If the patch is already upstreamed, you can remove it from the Chrome OS repo. +### Chrome side changes + +Chrome communicates with brltty using libbrlapi. +libbrlapi resides at //third_party/libbrlapi. + +Chrome loads this library dynamically and hard-codes the expected version of the api so in +chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc + +which should match up with the header in +third_party/libbrlapi/brlapi.h. + +During uppreving, if brltty increments its api version, it will be necessary to update the header for libbrlapi, as well as incrementing the supported api version of the libbrlapi shared object. + +First, grab the generated header from your Chrome OS build above. +cp <chromeos root>/build/$BOARD/usr/include/brlapi.h <chrome_root>/third_party/libbrlapi/brlapi.h + +This header contains the specific socket path for Chrome OS which differs from brltty defaults. + +Next, ensure the version in +chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc + +matches the one in the new brltty. + ### Testing Firstly, try to test against brltty on linux. This involves building brltty at diff --git a/chromium/docs/accessibility/patts.md b/chromium/docs/accessibility/patts.md index 637be788539..719d97c238a 100644 --- a/chromium/docs/accessibility/patts.md +++ b/chromium/docs/accessibility/patts.md @@ -101,8 +101,8 @@ To upload the change, use repo upload, something like this: ``` git commit -a - Bug:chromium:12345 - Test:Write what you tested here + Bug: chromium:12345 + Test: Write what you tested here repo upload . ``` diff --git a/chromium/docs/accessibility/tts.md b/chromium/docs/accessibility/tts.md index 353479b54ad..3cbe949e450 100644 --- a/chromium/docs/accessibility/tts.md +++ b/chromium/docs/accessibility/tts.md @@ -41,8 +41,7 @@ platform. (in content/) processes utterances and sends them to the correct output engine. - The [TtsControllerDelegateImpl](https://cs.chromium.org/chromium/src/chrome/browser/speech/tts_controller_delegate_impl.h) -(in chrome/) provides chrome-specific functionality, including making use of -user prefs in Chrome OS. +(in chrome/) provides chrome OS specific functionality. ### Output @@ -76,4 +75,4 @@ user prefs in Chrome OS. - Fuzzer - In content_unittests, content/browser/speech/tts_platform_fuzzer.cc - (currently Windows only).
\ No newline at end of file + (currently Windows only). diff --git a/chromium/docs/adding_to_third_party.md b/chromium/docs/adding_to_third_party.md index 72f32f67a5f..8bbb278cd47 100644 --- a/chromium/docs/adding_to_third_party.md +++ b/chromium/docs/adding_to_third_party.md @@ -175,7 +175,9 @@ Non-Googlers can email one of the people in licensing matters. These reviewers may not be able to +1 a change so look for verbal approval in the comments. (This list does not receive or deliver email, so only use it as a reviewer, not for other communication. Internally, - see cl/221704656 for details about how this is configured.) + see [cl/221704656](https://cl/221704656) for details about how + this is configured.). If you have questions about the third-party process, + ask one of the [//third_party/OWNERS](../third_party/OWNERS) instead. * Lastly, if all other steps are complete, get a positive code review from a member of [//third_party/OWNERS](../third_party/OWNERS) to land the change. diff --git a/chromium/docs/chromedriver_status.md b/chromium/docs/chromedriver_status.md index ccdc14ffe0c..c94b8b187e9 100644 --- a/chromium/docs/chromedriver_status.md +++ b/chromium/docs/chromedriver_status.md @@ -59,3 +59,4 @@ Below is a list of all WebDriver commands and their current support in ChromeDri | POST | /session/{session id}/alert/text | Send Alert Text | Complete | | GET | /session/{session id}/screenshot | Take Screenshot | Complete | | GET | /session/{session id}/element/{element id}/screenshot | Take Element Screenshot | Complete | +| POST | /session/{session id}/print | Print Page | Partially Complete(available for Headless) | [3481](https://bugs.chromium.org/p/chromedriver/issues/detail?id=3481) diff --git a/chromium/docs/clang_format.md b/chromium/docs/clang_format.md index 1fa34d076fc..c14eb33ae0d 100644 --- a/chromium/docs/clang_format.md +++ b/chromium/docs/clang_format.md @@ -1,5 +1,7 @@ # Using clang-format on Chromium C++ Code +[TOC] + *** note NOTE: This page does not apply to the Chromium OS project. See [Chromium Issue 878506](https://bugs.chromium.org/p/chromium/issues/detail?id=878506#c10) @@ -9,15 +11,16 @@ for updates. ## Easiest usage, from the command line To automatically format a pending patch according to -[Chromium style](https://www.chromium.org/developers/coding-style), from -the command line, simply run: ``` git cl format ``` This should work on all -platforms _(yes, even Windows)_ without any set up or configuration: the tool -comes with your checkout. Like other `git-cl` commands, this operates on a diff -relative to the upstream branch. Only the lines that you've already touched in -your patch will be reformatted. You can commit your changes to your git branch -and then run `git cl format`, after which `git diff` will show you what -clang-format changed. Alternatively, you can run `git cl format` with your -changes uncommitted, and then commit your now-formatted code. +[Chromium style](https://www.chromium.org/developers/coding-style), run: +``` git cl format ``` from the command line. This should work on all platforms +without any extra set up: the tool is integrated with depot_tools and the +Chromium checkout. + +Like other `git-cl` commands, this operates on a diff relative to the upstream +branch. Only the lines that changed in a CL will be reformatted. To see what +clang-format would choose, commit any local changes and then run `git cl +format` followed by `git diff`. Alternatively, run `git cl format` and commit +the now-formatted code. ## Editor integrations @@ -44,27 +47,36 @@ For further guidance on editor integration, see these specific pages: * For vim, `:so tools/vim/clang-format.vim` and then hit cmd-shift-i (mac) ctrl-shift-i (elsewhere) to indent the current line or current selection. -## Are robots taking over my freedom to choose where newlines go? - -No. For the project as a whole, using clang-format is just one optional way to -format your code. While it will produce style-guide conformant code, other -formattings would also satisfy the style guide, and all are okay. - -Having said that, many clang-format converts have found that relying on a tool -saves both them and their reviewers time. The saved time can then be used to -discover functional defects in their patch, to address style/readability -concerns whose resolution can't be automated, or to do something else that -matters. - -In directories where most contributors have already adopted clang-format, and -code is already consistent with what clang-format would produce, some teams -intend to experiment with standardizing on clang-format. When these local -standards apply, it will be enforced by a PRESUBMIT.py check. - ## Reporting problems If clang-format is broken, or produces badly formatted code, please file a -[bug]. Assign it to thakis@chromium.org who will route it upstream. +[bug]. Assign it to thakis@chromium.org or dcheng@chromium.org, who will route +it upstream. [bug]: https://code.google.com/p/chromium/issues/entry?comment=clang-format%20produced%20code%20that%20(choose%20all%20that%20apply):%20%0A-%20Doesn%27t%20match%20Chromium%20style%0A-%20Doesn%27t%20match%20blink%20style%0A-%20Riles%20my%20finely%20honed%20stylistic%20dander%0A-%20No%20sane%20human%20would%20ever%20choose%0A%0AHere%27s%20the%20code%20before%20formatting:%0A%0A%0AHere%27s%20the%20code%20after%20formatting:%0A%0A%0AHere%27s%20how%20it%20ought%20to%20look:%0A%0A%0ACode%20review%20link%20for%20full%20files/context:&summary=clang-format%20quality%20problem&cc=thakis@chromium.org&labels=Type-Bug,Build-Tools,OS-?,clang-format + +## Are robots taking over my freedom to choose where newlines go? + +Mostly. At upload time, a presubmit check warns if a CL is not clang-formatted, +but this is a non-blocking warning, and the CL may still be submitted. Even so, +try to prefer clang-format's output when possible: + +- While clang-format does not necessarily format code the exact same way a human + might choose, it produces style-conformat code by design. This can allow + development and review time to be focused on discovering functional defects, + addressing readability/understandability concerns that can't be automatically + fixed by tooling, et cetera. +- Continually fighting the tooling is a losing battle. Most Chromium developers + use clang-format. Large-scale changes will simply run `git cl format` once to + avoid having to deal with the particulars of formatting. Over time, this will + likely undo any carefully-curated manual formatting of the affected lines. + +There is one notable exception where clang-format is often disabled: large +tables of data are often surrounded by `// clang-format off` and `// +clang-format on`. Try to use this option sparingly, as widespread usage makes +tool-assisted refactoring more difficult. + +Again, if clang-format produces something odd, please err on the side of +[reporting an issue](#Reporting-problems): bugs that aren't reported can't be +fixed. diff --git a/chromium/docs/clang_sheriffing.md b/chromium/docs/clang_sheriffing.md index 264a4c595ff..242e8d1c24b 100644 --- a/chromium/docs/clang_sheriffing.md +++ b/chromium/docs/clang_sheriffing.md @@ -17,6 +17,13 @@ document describes some of the processes and techniques for doing that. https://sheriff-o-matic.appspot.com/chromium.clang is the sheriff-o-matic view of that waterfall, which can be easier to work with. +In addition to the waterfall, make sure +[dry run attempts at updating clang](https://chromium-review.googlesource.com/q/owner:thakis%2540chromium.org+%2522roll+clang%2522) +are green. As part of the Clang release process we run upstream LLVM tests. +Ideally these tests are covered by upstream LLVM bots and breakages are +quickly noticed and fixed by the original author of a breaking commit, +but that is sadly not always the case. + [TOC] ## Is it the compiler? diff --git a/chromium/docs/clang_tidy.md b/chromium/docs/clang_tidy.md index 6b1ca7d0e27..112d7b02e1b 100644 --- a/chromium/docs/clang_tidy.md +++ b/chromium/docs/clang_tidy.md @@ -30,13 +30,70 @@ disabled. ### Adding a new check -If you'd like to propose the addition of a new check, please send an email to -cxx@chromium.org describing why you think the check is helpful. If the proposed -check is approved, you may turn it on, though please note that this is only +New checks require review from cxx@chromium.org. If you propose a check and it +gets approved, you may turn it on, though please note that this is only provisional approval: we get signal from users clicking "Not Useful" on comments. If feedback is overwhelmingly "users don't find this useful," the check may be removed. +Traditionally, petitions to add checks include [an +evaluation](https://docs.google.com/document/d/1i1KmXtDD4j_qjhmAdGlJ6UkYXByVX1Kp952Zusdcl5k/edit?usp=sharing) +of the check under review. Crucially, this includes two things: + +- a count of how many times this check fires across Chromium +- a random sample (>30) of places where the check fires across Chromium + +It's expected that the person proposing the check has manually surveyed every +clang-tidy diagnostic in the sample, noting any bugs, odd behaviors, or +interesting patterns they've noticed. If clang-tidy emits FixIts, these are +expected to be considered by the evaluation, too. + +An example of a previous proposal email thread is +[here](https://groups.google.com/a/chromium.org/g/cxx/c/iZ6-Y9ZhC3Q/m/g-8HzqmbAAAJ). + +#### Evaluating: running clang-tidy across Chromium + +Running clang-tidy requires some setup. First, you'll need to sync clang-tidy, +which requires adding `checkout_clang_tidy` to your `.gclient` file: + +``` +solutions = [ + { + 'custom_vars': { + 'checkout_clang_tidy': True, + }, + } +] +``` + +Your next run of `gclient runhooks` should cause clang-tidy to be synced. + +To run clang-tidy across all of Chromium, you'll need a checkout of Chromium's +[build/](https://chromium.googlesource.com/chromium/tools/build) repository. +Once you have that and a Chromium `out/` dir with an `args.gn`, running +clang-tidy across all of Chromium is a single command: + +``` +$ cd ${chromium}/src +$ ${chromium_build}/scripts/slave/recipes/tricium_clang_tidy_wrapper.resources/tricium_clang_tidy.py \ + --base_path $PWD \ + --out_dir out/Linux \ + --findings_file all_findings.json \ + --clang_tidy_binary $PWD/third_party/llvm-build/Release+Asserts/bin/clang-tidy \ + --all +``` + +All clang-tidy checks are run on Linux builds of Chromium, so please set up your +`args.gn` to build Linux. + +`all_findings.json` is where all of clang-tidy's findings will be dumped. The +format of this file is detailed in `tricium_clang_tidy.py`. + +**Note** that the above command will use Chromium's top-level `.clang-tidy` file +(or `.clang-tidy` files scattered throughout `third_party/`, depending on the +files we lint. In order to test a *new* check, you'll have to add it to +Chromium's top-level `.clang-tidy` file. + ### Ignoring a check If a check is invalid on a particular piece of code, clang-tidy supports `// diff --git a/chromium/docs/clangd.md b/chromium/docs/clangd.md index 99559663c4a..a289997a195 100644 --- a/chromium/docs/clangd.md +++ b/chromium/docs/clangd.md @@ -13,8 +13,14 @@ See [instructions](https://clang.llvm.org/extra/clangd/Installation.html#install **Googlers:** clangd has been installed on your glinux by default, just use `/usr/bin/clangd`. -Alternative: use the following command to build clangd from LLVM source, and you -will get the binary at +Alternative: download clangd from the official [Releases](https://github.com/clangd/clangd/releases) +page. + +Note: clangd 10.0.0 does not work with Chromium; use one of the pre-release +versions lower down on the Releases page until a newer version is released. + +If you prefer to build clangd locally, use the following command to build from +LLVM source, and you will get the binary at `out/Release/tools/clang/third_party/llvm/build/bin/clangd`. ``` @@ -36,8 +42,9 @@ source file. tools/clang/scripts/generate_compdb.py -p out/Release > compile_commands.json ``` -Note: the compilation database is not re-generated automatically, you'd need to -regenerate it manually when you have new files checked in. +Note: the compilation database is not regenerated automatically. You need to +regenerate it manually whenever build rules change, e.g., when you have new files +checked in or when you sync to head. If using Windows PowerShell, use the following command instead to set the output's encoding to UTF-8 (otherwise Clangd will hit "YAML:1:4: error: Got diff --git a/chromium/docs/code_reviews.md b/chromium/docs/code_reviews.md index 1fc688d17d4..0c6df1b3802 100644 --- a/chromium/docs/code_reviews.md +++ b/chromium/docs/code_reviews.md @@ -3,9 +3,8 @@ Code reviews are a central part of developing high-quality code for Chromium. All changes must be reviewed. -The bigger patch-upload-and-land process is covered in more detail the -[contributing code](https://www.chromium.org/developers/contributing-code) -page. +The general patch, upload, and land process is covered in more detail in the +[contributing code](contributing.md) page. # Code review policies @@ -186,8 +185,8 @@ Otherwise the reviewer won't know to review the patch. * Add the reviewer's email address in the code review tool's reviewer field like normal. - * Add a line "Tbr:<reviewer's email>" to the bottom of the change list - description. e.g. `Tbr:reviewer1@chromium.org,reviewer2@chromium.org` + * Add a line "Tbr: <reviewer's email>" to the bottom of the change list + description. e.g. `Tbr: reviewer1@chromium.org,reviewer2@chromium.org` * Type a message so that the owners in the TBR list can understand who is responsible for reviewing what, as part of their post-commit review diff --git a/chromium/docs/commit_checklist.md b/chromium/docs/commit_checklist.md index ffa6149fd65..7827b2bcb0d 100644 --- a/chromium/docs/commit_checklist.md +++ b/chromium/docs/commit_checklist.md @@ -1,16 +1,21 @@ # Commit Checklist for Chromium Workflow Here is a helpful checklist to go through before uploading change lists (CLs) on -Gerrit, which is the code review platform for the Chromium project. This -checklist is designed to be streamlined. See +Gerrit and during the code review process. Gerrit is the code review platform +for the Chromium project. This checklist is designed to be streamlined. See [contributing to Chromium][contributing] for a more thorough reference. The intended audience is software engineers who are unfamiliar with contributing to the Chromium project. Feel free to skip steps that are not applicable to the patch set you're currently uploading. +According to the Checklist Manifesto by Atul Gawande, checklists are a marvelous +tool for ensuring consistent quality in the work you produce. Checklists also +help you work more efficiently by ensuring you never skip a step or waste brain +power figuring out the next step to take. + [TOC] -## 1. Create a new branch +## 1. Create a new branch or switch to the correct branch You should create a new branch before starting any development work. It's helpful to branch early and to branch often in Git. Use the command @@ -18,16 +23,36 @@ helpful to branch early and to branch often in Git. Use the command `git checkout -b <branch_name> --track origin/master`. You may also want to set another local branch as the upstream branch. You can do -that with `git checkout -b <branch_name> --track <upstream_branch>`. +that with `git checkout -b <branch_name> --track <upstream_branch>`. Do this if +you want to split your work across multiple CLs, but some CLs have dependencies +on others. Mark the associated crbug as "started" so that other people know that you have -started work on the bug. Doing this can avoid duplicated work. +started working on the bug. Taking this step can avoid duplicated work. + +If you have already created a branch, don't forget to `git checkout +<branch_name>` to the correct branch before resuming development work. There's +few things more frustrating than to finish implementing your ideas or feedback, +and to spend hours debugging some mysterious bug, only to discover that the bug +was caused by working on the wrong branch this whole time. + +## 2. If there's a local upstream branch, rebase the upstream changes + +Suppose you have a downstream branch chained to an upstream branch. If you +commit changes to the upstream branch, and you want the changes to appear in +your downstream branch, you need to: + +* `git checkout <branch_name>` to the downstream branch. +* Run `git rebase -i @{u}` to pull the upstream changes into the current + branch. +* Run `git rebase -i @{u}` again to rebase the downstream changes onto the + upstream branch. -## 2. Make your changes +## 3. Make your changes Do your thing. There's no further advice here about how to write or fix code. -## 3. Make sure the code builds correctly +## 4. Make sure the code builds correctly After making your changes, check that common targets build correctly: @@ -38,66 +63,74 @@ After making your changes, check that common targets build correctly: It's easy to inadvertently break one of the other builds you're not currently working on without realizing it. Even though the Commit Queue should catch any build errors, checking locally first can save you some time since the CQ Dry Run -can take a while. +can take a while to run, on the order of a few hours sometimes. -## 4. Test your changes +## 5. Test your changes -Make sure you hit every code path you changed. +Test your changes manually by running the X11 simulator or deploying your +changes to a test device. Follow the [Simple Chrome][simple-chrome] instructions +to deploy your changes to a test device. Make sure you hit every code path you +changed. -## 5. Write unit or browser tests for any new code +## 6. Write unit or browser tests for any new code Consider automating any manual testing you did in the previous step. -## 6. Ensure the code is formatted nicely +## 7. Ensure the code is formatted nicely Run `git cl format --js`. The `--js` option also formats JavaScript changes. -## 7. Review your changes +## 8. Review your changes Use `git diff` to review all of the changes you've made from the previous commit. Use `git upstream-diff` to review all of the changes you've made from the upstream branch. The output from `git upstream-diff` is what will be uploaded to Gerrit. -## 8. Stage relevant files for commit +## 9. Stage relevant files for commit Run `git add <path_to_file>` for all of the files you've modified that you want to include in the CL. Unlike other version-control systems such as svn, you have to specifically `git add` the files you want to commit before calling `git commit`. -## 9. Commit your changes +## 10. Commit your changes Run `git commit`. Be sure to write a useful commit message. Here are some [tips for writing good commit messages][uploading-a-change-for-review]. A -shortcut for combining steps 8 and 9 is `git commit -a -m <commit_message>`. +shortcut for combining steps the previous step and this one is `git commit -a -m +<commit_message>`. -## 10. Squash your commits +## 11. Squash your commits If you have many commits on your current branch, and you want to avoid some nasty commit-by-commit merge conflicts in the next step, consider collecting all your changes into one commit. Run `git rebase -i @{u}`. The `@{u}` is a -short-hand pointer for the upstream branch, which is usually origin/master. -After running the `git rebase` command, you should see a list of commits, with -each commit starting with the word "pick". Make sure the first commit says -"pick" and change the rest from "pick" to "squash". This will squash each commit -into the previous commit, which will continue until each commit is squashed into -the first commit. - -## 11. Rebase your local repository - -Rebasing is a neat way to resolve any merge conflict errors on your CL. Run -`git rebase-update`. This command updates all of your local branches with -remote changes that have landed since you started development work, which -could've been a while ago. It also deletes any branches that match the remote -repository, such as after the CL associated with that branch has been merged. -In summary, `git rebse-update` cleans up your local branches. +short-hand pointer for the upstream branch, which is usually origin/master, but +can also be one of your local branches. After running the `git rebase` command, +you should see a list of commits, with each commit starting with the word +"pick". Make sure the first commit says "pick" and change the rest from "pick" +to "squash". This will squash each commit into the previous commit, which will +continue until each commit is squashed into the first commit. + +An alternative way to squash your commits into a single commit is to do `git +commit --amend` in the previous step. + +## 12. Rebase your local repository + +Rebasing is a neat way to sync changes from the remote repository and resolve +any merge conflict errors on your CL. Run `git rebase-update`. This command +updates all of your local branches with remote changes that have landed since +you started development work, which could've been a while ago. It also deletes +any branches that match the remote repository, such as after the CL associated +with that branch has been merged. In summary, `git rebase-update` cleans up your +local branches. You may run into rebase conflicts. Fix them manually before proceeding with `git rebase --continue`. Note that rebasing has the potential to break your build, so you might want to try re-building afterwards. -## 12. Upload the CL to Gerrit +## 13. Upload the CL to Gerrit Run `git cl upload`. Some useful options include: @@ -105,21 +138,22 @@ Run `git cl upload`. Some useful options include: * `-r <chromium_username>` will add reviewers. * `-b <bug_number>` automatically populates the bug reference line of the commit message. +* `--edit-description` will let you update the commit message. -## 13. Check the CL again in Gerrit +## 14. Check the CL again in Gerrit Run `git cl web` to go to the Gerrit URL associated with the current branch. Open the latest patch set and verify that all of the uploaded files are correct. Click `Expand All` to check over all of the individual line-by-line changes -again. +again. Basically do a self-review before asking your reviewers for a review. -## 14. Make sure all auto-regression tests pass +## 15. Make sure all auto-regression tests pass Click `CQ Dry Run`. Fix any errors because otherwise the CL won't pass the commit queue (CQ) checks. Consider waiting for the CQ Dry Run to pass before notifying your reviewers, in case the results require major changes in your CL. -## 15. Add reviewers to review your code +## 16. Add reviewers to review your code Click `Find Owners` or run `git cl owners` to find file owners to review your code and instruct them about which parts you want them to focus on. Add anyone @@ -129,7 +163,7 @@ your CL touches. For your CL to land, you need an approval from an owner for each file you've changed, unless you are an owner of some files, in which case you don't need separate owner approval for those files. -## 16. Implement feedback from your reviewers +## 17. Implement feedback from your reviewers Then go through this commit checklist again. Reply to all comments from the reviewers on Gerrit and mark all resolved issues as resolved (clicking `Done` or @@ -138,7 +172,7 @@ receive a notification. Doing this signals that your CL is ready for review again, since the assumption is that your CL is not ready for review until you hit reply. -## 17. Land your CL +## 18. Land your CL Once you have obtained a Looks Good To Me (LGTM), which is reflected by a Code-Review+1 in Gerrit, from at least one owner for each file, then you have @@ -147,7 +181,7 @@ of your reviewers to approve your changes as well, even if they're not owners. Click `Submit to CQ` to try your change in the commit queue (CQ), which will land it if successful. -## 18. Cleanup +## 19. Cleanup After your CL is landed, you can use `git rebase-update` or `git cl archive` to clean up your local branches. These commands will automatically delete merged @@ -155,4 +189,5 @@ branches. Mark the associated crbug as "fixed". [//]: # (the reference link section should be alphabetically sorted) [contributing]: contributing.md +[simple-chrome]: https://chromium.googlesource.com/chromiumos/docs/+/master/simple_chrome_workflow.md [uploading-a-change-for-review]: contributing.md#Uploading-a-change-for-review diff --git a/chromium/docs/contributing.md b/chromium/docs/contributing.md index bbaaacd315d..753586fcc57 100644 --- a/chromium/docs/contributing.md +++ b/chromium/docs/contributing.md @@ -327,6 +327,10 @@ general rules of thumb can be helpful in navigating how to structure changes: find a product in the Chromium repositories that depends on that line of code or else the line of code should be removed. + Completely new additions to the project (for example, support for a new OS + or architecture, or a new top-level directory for a new sub-project) must + be approved by [//ENG_REVIEW_OWNERS](../ENG_REVIEW_OWNERS). + - **Code should only be moved to a central location (e.g., //base) when multiple consumers would benefit.** We should resist the temptation to build overly generic common libraries as that can lead to code bloat and diff --git a/chromium/docs/design/sandbox.md b/chromium/docs/design/sandbox.md index 47bf871bf41..a73501f8c00 100644 --- a/chromium/docs/design/sandbox.md +++ b/chromium/docs/design/sandbox.md @@ -463,6 +463,14 @@ Rules can only be added before each target process is spawned, and cannot be modified while a target is running, but different targets can have different rules. +### Diagnostics + +In Chromium, the policies associated with active processes can be viewed at +chrome://sandbox. Tracing of the `sandbox` category will output the policy used +when a process is launched. Tracing can be enabled using chrome://tracing or by +using the `--trace-startup=-*,disabled-by-default-sandbox` command line flag. +Trace output can be investigated with `//tools/win/trace-sandbox-viewer.py`. + ## Target bootstrapping Targets do not start executing with the restrictions specified by policy. They diff --git a/chromium/docs/enterprise/add_new_policy.md b/chromium/docs/enterprise/add_new_policy.md index d90cd69ff71..9c5f400f583 100644 --- a/chromium/docs/enterprise/add_new_policy.md +++ b/chromium/docs/enterprise/add_new_policy.md @@ -37,10 +37,12 @@ Usually you need a policy when sure you get the version and feature flags (such as dynamic_refresh and supported_on) right. - Here are the most used attributes. Please note that, all attributes - below other than `supported_on` and `default_for_enterprise_users` do - not change the code behavior. - - `supported_on`: It controls the platform and Chrome milestone the - policy supports. + below other than `supported_on`, `future_on' and + `default_for_enterprise_users` do not change the code behavior. + - `supported_on` and `future_on`: They control the platforms that the + policy supports. `supported_on` is used for released platforms with + milestone range while `future_on` is used for unreleased platforms. + See **Launch a policy** below for more information. - `default_for_enterprise_users`: Its value is applied as a mandatory policy for managed users on Chrome OS unless a different setting is explicitly set. @@ -51,9 +53,6 @@ Usually you need a policy when - `can_be_recommended`: It tells the admin whether they can mark the policy as recommended and allow the user to override it in the UI, using a command line switch or an extension. - - `future`: It hides the policy from auto-generated templates and - documentation. It's used when your policy needs multiple milestone - development. - The complete list of attributes and their expected values can be found in the [policy_templates.json](https://cs.chromium.org/chromium/src/components/policy/resources/policy_templates.json). @@ -152,6 +151,33 @@ Usually you need a policy when 10. If your policy has interactions with other policies, make sure to document, test and cover these by automated tests. +## Launch a policy +1. When adding a new policy, put the platforms it will be supported in the + `future_on` list. + - The policy is hidden from any auto-generated template or documentation on + those platforms. + - The policy will also be unavailable on Beta and Stable channel unless it's + enabled specifically by + [EnableExperimentalPolicies](https://cloud.google.com/docs/chrome-enterprise/policies/?policy=EnableExperimentalPolicies) + policy. +2. Implement the policy, get launch approval if necessary. +3. If the policy needs to be tested with small set of users first, keep the + platforms in the `future_on` list and running the tester program with the + [EnableExperimentalPolicies](https://cloud.google.com/docs/chrome-enterprise/policies/?policy=EnableExperimentalPolicies) + policy. +4. Move the launched platforms from `future_on` to `supported_on` and set the + 'since_version' of those platforms to the milestone for which the launch + approval was granted. +5. If the 'since_version' is set to a earlier milestone, you need to merge + back all necessary commits. + +**Do not use finch to control policy launch process.** + +Policies are inherently switches that admins will turn on if they need. Getting +inconsistent behavior based on factors outside of their control only causes +confusion and is source for support requests. Use the step 3 above if the policy +needs external testers before being officially announced. + ## Examples Here is an example based on the instructions above. It's a good, simple place to @@ -221,6 +247,7 @@ The presubmit checks perform the following verifications: before a new stable release has rolled out. Normally such a change should eventually be merged into the stable branch before the release. + 3. `supported_on` list is empty. 2. If the policy is considered **unreleased**, all changes to it are allowed. @@ -261,6 +288,15 @@ The presubmit checks perform the following verifications: 1. Dictionary policies can have some of their "required" fields removed in order to be less restrictive. +4. A policy is **partially released** if both `supported_on` and + `future_on` list are not empty. + +5. The **partially released** policy is considered as a **released** policy + and only the `future_on` list can be modified freely. However, any + platform in the `supported_on` list cannot be moved back to the `future_on` + list. + + ## Cloud Policy **For Googlers only**: The Cloud Policy will be maintained by the Admin console @@ -292,4 +328,7 @@ than regular consumer behavior. ### Additional Notes -1. policy_templates.json is actually a Python dictionary even though the file name contains *json*. +1. policy_templates.json is actually a Python dictionary even though the file + name contains *json*. +2. The `future_on` flag can disable policy on Beta of Stable channel only if the + policy value is copied to `PrefService` in Step 3 of **Adding a new policy**. diff --git a/chromium/docs/enterprise/extension_query.py b/chromium/docs/enterprise/extension_query.py index c1a23974efc..f1e6dd1c758 100755 --- a/chromium/docs/enterprise/extension_query.py +++ b/chromium/docs/enterprise/extension_query.py @@ -1,5 +1,5 @@ #!/usr/bin/env python -# Copyright (c) 2020 The Chromium Authors. All rights reserved. +# Copyright 2020 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. """Transform CBCM Takeout API Data (Python3).""" @@ -8,6 +8,7 @@ import argparse import csv import json import sys +import time import google_auth_httplib2 @@ -41,26 +42,20 @@ def ComputeExtensionsList(extensions_list, data): key = key + ' @ ' + extension['version'] if key not in extensions_list: current_extension = { - 'name': - extension['name'], - 'permissions': - extension['permissions'] - if 'permissions' in extension else '', - 'installed': - set(), - 'disabled': - set(), - 'forced': - set() + 'name': extension.get('name', ''), + 'permissions': extension.get('permissions', ''), + 'installed': set(), + 'disabled': set(), + 'forced': set() } else: current_extension = extensions_list[key] machine_name = device['machineName'] current_extension['installed'].add(machine_name) - if 'installType' in extension and extension['installType'] == 3: + if extension.get('installType', '') == 'ADMIN': current_extension['forced'].add(machine_name) - if 'disabled' in extension and extension['disabled']: + if extension.get('disabled', False): current_extension['disabled'].add(machine_name) extensions_list[key] = current_extension @@ -170,11 +165,21 @@ def main(args): browsers_processed = 0 while True: print('Making request to server ...') - response = http.request(base_request_url + '?' + request_parameters, - 'GET')[1] - if isinstance(response, bytes): - response = response.decode('utf-8') - data = json.loads(response) + + retrycount = 0 + while retrycount < 5: + response = http.request(base_request_url + '?' + request_parameters, + 'GET')[1] + + if isinstance(response, bytes): + response = response.decode('utf-8') + data = json.loads(response) + if 'browsers' not in data: + print('Response error, retrying...') + time.sleep(3) + retrycount += 1 + else: + break browsers_in_data = len(data['browsers']) print('Request returned %s results, analyzing ...' % (browsers_in_data)) diff --git a/chromium/docs/enterprise/extension_query_py2.py b/chromium/docs/enterprise/extension_query_py2.py index 73856278e65..2bf496b85e9 100755 --- a/chromium/docs/enterprise/extension_query_py2.py +++ b/chromium/docs/enterprise/extension_query_py2.py @@ -1,8 +1,7 @@ #!/usr/bin/env python -# Copyright (c) 2020 The Chromium Authors. All rights reserved. +# Copyright 2020 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. - """Transform CBCM Takeout API Data (Python2).""" from __future__ import print_function @@ -44,30 +43,25 @@ def ComputeExtensionsList(extensions_list, data): key = key + ' @ ' + extension['version'] if key not in extensions_list: current_extension = { - 'name': - extension['name'], - 'permissions': - extension['permissions'] - if 'permissions' in extension else '', - 'installed': - set(), - 'disabled': - set(), - 'forced': - set() + 'name': extension.get('name', ''), + 'permissions': extension.get('permissions', ''), + 'installed': set(), + 'disabled': set(), + 'forced': set() } else: current_extension = extensions_list[key] machine_name = device['machineName'] current_extension['installed'].add(machine_name) - if 'installType' in extension and extension['installType'] == 3: + if extension.get('installType', '') == 'ADMIN': current_extension['forced'].add(machine_name) - if 'disabled' in extension and extension['disabled']: + if extension.get('disabled', False): current_extension['disabled'].add(machine_name) extensions_list[key] = current_extension + def ToUtf8(data): """Ensures all the values in |data| are encoded as UTF-8. @@ -84,6 +78,7 @@ def ToUtf8(data): entry[prop] = unicode(value).encode('utf-8') yield entry + def DictToList(data, key_name='id'): """Converts a dict into a list. @@ -181,8 +176,17 @@ def main(args): browsers_processed = 0 while True: print('Making request to server ...') - data = json.loads( - http.request(base_request_url + '?' + request_parameters, 'GET')[1]) + retrycount = 0 + while retrycount < 5: + data = json.loads( + http.request(base_request_url + '?' + request_parameters, 'GET')[1]) + + if 'browsers' not in data: + print('Response error, retrying...') + time.sleep(3) + retrycount += 1 + else: + break browsers_in_data = len(data['browsers']) print('Request returned %s results, analyzing ...' % (browsers_in_data)) diff --git a/chromium/docs/fuchsia_build_instructions.md b/chromium/docs/fuchsia_build_instructions.md index 5c5d2ac39d4..4dee4bf448b 100644 --- a/chromium/docs/fuchsia_build_instructions.md +++ b/chromium/docs/fuchsia_build_instructions.md @@ -91,7 +91,7 @@ target_os = ['fuchsia'] You will then need to run: ```shell -$ gclient runhooks +$ gclient sync ``` This makes sure the Fuchsia SDK is available in third\_party and keeps it up to diff --git a/chromium/docs/fuchsia_gardening.md b/chromium/docs/fuchsia_gardening.md index d248c9f0da8..63c11245f04 100644 --- a/chromium/docs/fuchsia_gardening.md +++ b/chromium/docs/fuchsia_gardening.md @@ -34,5 +34,7 @@ While the Gardener takes primary responsibility for each of these areas during t The waterfall is green, the try-bots reliable, SDK is rolling and pigs soar majestically in the sky. Fear not, gentle Gardener, you still have a valuable role to play! +Googlers: The current gardener can be found at [go/cr-fuchsia-cop](http://go/cr-fuchsia-cop). + * Look for tests that have been filtered under Fuchsia, and diagnose them. * File a new bug, or upate the filter to refer to existing bugs, as appropriate. diff --git a/chromium/docs/gpu/gpu_testing.md b/chromium/docs/gpu/gpu_testing.md index a2d9b20470d..958801a0f14 100644 --- a/chromium/docs/gpu/gpu_testing.md +++ b/chromium/docs/gpu/gpu_testing.md @@ -278,11 +278,11 @@ being tested. This *should* mean that the pixel tests will automatically work when run locally. However, if the local run detection code fails for some reason, you can manually pass some flags to force the same behavior: -In order to get around the local run issues, simply pass the `--local-run=1` -flag to the tests. This will disable uploading, but otherwise go through the -same steps as a test normally would. Each test will also print out `file://` -URLs to the produced image, the closest image for the test known to Gold, and -the diff between the two. +In order to get around the local run issues, simply pass the +`--local-pixel-tests` flag to the tests. This will disable uploading, but +otherwise go through the same steps as a test normally would. Each test will +also print out `file://` URLs to the produced image, the closest image for the +test known to Gold, and the diff between the two. Because the image produced by the test locally is likely slightly different from any of the approved images in Gold, local test runs are likely to fail during @@ -292,7 +292,7 @@ comparison. When using `--no-skia-gold-failure`, you'll also need to pass the `--passthrough` flag in order to actually see the link output. Example usage: -`run_gpu_integration_test.py pixel --no-skia-gold-failure --local-run=1 +`run_gpu_integration_test.py pixel --no-skia-gold-failure --local-pixel-tests --passthrough` If, for some reason, the local run code is unable to determine what the git @@ -300,7 +300,7 @@ revision is, simply pass `--git-revision aabbccdd`. Note that `aabbccdd` must be replaced with an actual Chromium src revision (typically whatever revision origin/master is currently synced to) in order for the tests to work. This can be done automatically using: -``run_gpu_integration_test.py pixel --no-skia-gold-failure --local-run +``run_gpu_integration_test.py pixel --no-skia-gold-failure --local-pixel-tests --passthrough --git-revision `git rev-parse origin/master` `` ## Running Binaries from the Bots Locally @@ -628,6 +628,9 @@ Here are some examples: * A change to Blink's memory purging primitive which caused intermittent timeouts of WebGL conformance tests on all platforms ([Issue 840988](http://crbug.com/840988)). +* Screen DPI being inconsistent across seemingly identical Linux machines, + causing the Maps pixel test to flakily produce incorrectly sized images + ([Issue 1091410](https://crbug.com/1091410)). If you notice flaky test failures either on the GPU waterfalls or try servers, please file bugs right away with the component Internals>GPU>Testing and diff --git a/chromium/docs/gpu/gpu_testing_bot_details.md b/chromium/docs/gpu/gpu_testing_bot_details.md index 7255a9ea027..84f40dc0311 100644 --- a/chromium/docs/gpu/gpu_testing_bot_details.md +++ b/chromium/docs/gpu/gpu_testing_bot_details.md @@ -251,14 +251,11 @@ sorry): * [`pools.cfg`][pools.cfg] * Defines the Swarming pools for GCEs and Mac VMs used for manually triggered trybots. -* [`bot_config.py`][bot_config.py] - * Defines the stable GPU driver and OS versions in GPU Swarming pools. [infradata/config]: https://chrome-internal.googlesource.com/infradata/config [gpu.star]: https://chrome-internal.googlesource.com/infradata/config/+/master/configs/chromium-swarm/starlark/bots/chromium/gpu.star [chromium.star]: https://chrome-internal.googlesource.com/infradata/config/+/master/configs/chromium-swarm/starlark/bots/chromium/chromium.star [pools.cfg]: https://chrome-internal.googlesource.com/infradata/config/+/master/configs/chromium-swarm/pools.cfg -[bot_config.py]: https://chrome-internal.googlesource.com/infradata/config/+/master/configs/chromium-swarm/scripts/bot_config.py [main.star]: https://chrome-internal.googlesource.com/infradata/config/+/master/main.star [vms.cfg]: https://chrome-internal.googlesource.com/infradata/config/+/master/configs/gce-provider/vms.cfg @@ -666,10 +663,6 @@ or OS update. To do this: 1. Make sure that all of the current Swarming jobs for this OS and GPU configuration are targeted at the "stable" version of the driver and the OS in [`waterfalls.pyl`][waterfalls.pyl] and [`mixins.pyl`][mixins.pyl]. - Make sure that there are "named" stable versions of the driver and the OS - there, which target the `_TARGETED_DRIVER_VERSIONS` and - `_TARGETED_OS_VERSIONS` dictionaries in [`bot_config.py`][bot_config.py] - (Google internal). 1. File a `Build Infrastructure` bug, component `Infra>Labs`, to have ~4 of the physical machines already in the Swarming pool upgraded to the new version of the driver or the OS. @@ -684,51 +677,41 @@ or OS update. To do this: it'll be necessary to follow the instructions on [updating Gold baselines (step #4)][updating gold baselines]. 1. Watch the new machine for a day or two to make sure it's stable. -1. When it is, update [`bot_config.py`][bot_config.py] (Google internal) - to *add* a mapping between the new driver version and the "stable" version. - For example: +1. When it is, add the experimental driver/OS to the `_stable` mixin using the + swarming OR operator `|`. For example: ``` - _TARGETED_DRIVER_VERSIONS = { - # NVIDIA Quadro P400, Ubuntu Stable version - '10de:1cb3-384.90': 'nvidia-quadro-p400-ubuntu-stable', - # NVIDIA Quadro P400, new Ubuntu Stable version - '10de:1cb3-410.78': 'nvidia-quadro-p400-ubuntu-stable', - # ... - } - ``` - - And/or a mapping between the new OS version and the "stable" version. - For example: - - ``` - _TARGETED_OS_VERSIONS = { - # Linux NVIDIA Quadro P400 - '10de:1cb3': { - 'Ubuntu-14.04': 'linux-nvidia-stable', - 'Ubuntu-19.04': 'linux-nvidia-stable', + 'win10_intel_hd_630_stable': { + 'swarming': { + 'dimensions': { + 'gpu': '8086:5912-26.20.100.7870|8086:5912-26.20.100.8141', + 'os': 'Windows-10', + 'pool': 'chromium.tests.gpu', + }, }, - # ... } ``` - The new driver or OS version should match the one just added for the - experimental bot. Get this CL reviewed and landed. - [Sample CL (Google internal)][sample targeted version cl]. + This will cause tests triggered using the `_stable` mixin to run on either + the old stable dimension or the experimental/new stable dimension. + + **NOTE** There is a hard cap of 8 combinations in swarming, so you can only + use the OR operator in up to 3 dimensions if each dimension only has two + options. More than two options per dimension is allowed as long as the total + number of combinations is 8 or less. 1. After it lands, ask the Chrome Infrastructure Labs team to roll out the driver update across all of the similarly configured bots in the swarming pool. 1. If necessary, update pixel test expectations and remove the suppressions added above. -1. Remove the old driver or OS version from [`bot_config.py`][bot_config.py], - leaving the "stable" driver version pointing at the newly upgraded version. +1. Remove the old driver or OS version from the `_stable` mixin, leaving just + the new stable version. Note that we leave the experimental bot in place. We could reclaim it, but it seems worthwhile to continuously test the "next" version of graphics drivers as well as the current stable ones. [sample driver cl]: https://chromium-review.googlesource.com/c/chromium/src/+/1726875 -[sample targeted version cl]: https://chrome-internal-review.googlesource.com/c/infradata/config/+/1602377 [updating gold baselines]: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/gpu/pixel_wrangling.md#how-to-keep-the-bots-green ## Credentials for various servers diff --git a/chromium/docs/how_cc_works.md b/chromium/docs/how_cc_works.md index 2b1f897ce58..fa3b84eb1cf 100644 --- a/chromium/docs/how_cc_works.md +++ b/chromium/docs/how_cc_works.md @@ -131,13 +131,13 @@ There are a number of heuristics to determine when and how to change rasterizati These aren’t perfect, but change them at your own peril. 🐉🐉🐉 -### PictureImageLayer +#### Directly composited images -A subclass of PictureLayer. -This is a special case for composited images in Blink. -If an image gets a composited layer but has no borders or padding (i.e. the painted content is exactly equal to the image) then some work can be saved here. -It "rasters" the image at fixed scales such that scaling this image is performant. -This is really a savings for software raster and in a gpu raster world such layers should never be created. +A specialized raster mode of PictureLayerImpl. +If a PictureLayer's DisplayItemList consists of a single drawImageRect DrawOp, we use different logic to determine what the raster scale of the image should end up being. +The layer is rastered at fixed scales such that scaling this image is performant. +The default raster scale is chosen such that the image will raster at its intrinsic side, then that scale is adjusted, based on how close it is to the ideal scale. +See PictureLayerImpl::ShouldAdjustRasterScale and RecalculateRasterScales for more details. ### TextureLayer diff --git a/chromium/docs/ios/build_instructions.md b/chromium/docs/ios/build_instructions.md index 5695097a072..2a42d01be38 100644 --- a/chromium/docs/ios/build_instructions.md +++ b/chromium/docs/ios/build_instructions.md @@ -76,6 +76,11 @@ build directories under `out` for Release and Debug device and simulator builds, and generates an appropriate Xcode workspace (`out/build/all.xcworkspace`) as well. +Warning: *do not open `out/build/products.xcodeproj` as this file does not +configure the "Legacy Build System". If you do open this file, many things +won't work properly (clicking on an error won't open the corresponding file, +...).* + More information about [developing with Xcode](xcode_tips.md). You can customize the build by editing the file `$HOME/.setup-gn` (create it if diff --git a/chromium/docs/ios/xcode_tips.md b/chromium/docs/ios/xcode_tips.md index 0e643693dce..63b11367899 100644 --- a/chromium/docs/ios/xcode_tips.md +++ b/chromium/docs/ios/xcode_tips.md @@ -5,8 +5,9 @@ to develop Chrome for iOS. ## Background -### .xcworkspace and .xcproject +### .xcworkspace and .xcodeproj +* Open all.xcworkspace only. * Generated from BUILD.gn files. * Don't make changes to it as it is a "fake" representation of the project - changes will not be committed @@ -15,6 +16,10 @@ to develop Chrome for iOS. * Added BUILD.gn files/`source_sets` need to be referenced from other ones as a `dep` in order for it to be shown in xcode. +Warning: *do not open the `.xcodeproj` as this file does not configure the +"Legacy Build System". If you do open this file, many things won't work +properly (clicking on an error won't open the corresponding file, ...).* + ### Adding new files * Create new files with `tools/boilerplate.py` diff --git a/chromium/docs/linux/debugging.md b/chromium/docs/linux/debugging.md index fd96694d8ca..03b29e3dcb8 100644 --- a/chromium/docs/linux/debugging.md +++ b/chromium/docs/linux/debugging.md @@ -258,9 +258,12 @@ can also step or execute backwards. This works by first recording a trace and then debugging based on that. I recommend installing it by compiling [from source](https://github.com/mozilla/rr/wiki/Building-And-Installing). -Currently you must apply a patch adding -[support for recording the MADV_WIPEONFORK](https://bugs.chromium.org/p/chromium/issues/detail?id=1082304#c6) -syscall as upstream rr triggers an internal assert on this call. +As of May 2020, you must build from source for [`MADV_WIPEONFORK` +support](https://bugs.chromium.org/p/chromium/issues/detail?id=1082304). If you +get the following error, rr is too old: +``` +Expected EINVAL for 'madvise' but got result 0 (errno SUCCESS); unknown madvise(18) +``` Once installed, you can use it like this: ``` diff --git a/chromium/docs/mac/triage.md b/chromium/docs/mac/triage.md index 2ece59d32c1..7fc93c93eef 100644 --- a/chromium/docs/mac/triage.md +++ b/chromium/docs/mac/triage.md @@ -155,6 +155,6 @@ triage processes: [unconfirmed]: https://bugs.chromium.org/p/chromium/issues/list?q=OS%3DMac%20status%3AUnconfirmed%20-component%3ABlink%2CEnterprise%2CInternals%3ENetwork%2CPlatform%3EDevtools%2CServices%3ESyncs%2CUI%3EBrowser%3EPasswords&can=2 [untriaged-m]: https://bugs.chromium.org/p/chromium/issues/list?q=has%3AMac%20status%3AUntriaged&can=2 -[untriaged-c]: https://bugs.chromium.org/p/chromium/issues/list?q=OS%3DMac%20-OS%3DWindows%2CLinux%2CChrome%2CAndroid%2CiOS%20status%3AUntriaged%20-component%3AAdmin%2CBlink%2CInfra%2CInternals%3EHeadless%2CInternals%3ENetwork%2CInternals%3EPlugins%3EPDF%2CInternals%3EPrinting%2CInternals%3ESkia%2CInternals%3EUpdater%2CInternals%3EViews%2CIO%3EBluetooth%2CIO%3EUSB%2CPlatform%2CServices%3EChromoting%2CTest%3ETelemetry%2CUI%3EBrowser%3EWebUI&can=2 +[untriaged-c]: https://bugs.chromium.org/p/chromium/issues/list?q=OS%3DMac%20-OS%3DWindows%2CLinux%2CChrome%2CAndroid%2CiOS%20status%3AUntriaged%20-component%3AAdmin%2CBlink%2CInfra%2CInternals%3EHeadless%2CInternals%3ENetwork%2CInternals%3EPlugins%3EPDF%2CInternals%3EPrinting%2CInternals%3ESkia%2CInternals%3EUpdater%2CInternals%3EViews%2CIO%3EBluetooth%2CIO%3EUSB%2CPlatform%2CServices%3EChromoting%2CTest%3ETelemetry%2CUI%3EBrowser%3EWebAppInstalls%2CUI%3EBrowser%3EWebUI&can=2 [available]: https://bugs.chromium.org/p/chromium/issues/list?q=has%3AMac%20status%3AAvailable&can=2 [assigned]: https://bugs.chromium.org/p/chromium/issues/list?q=has%3AMac%20status%3AAssigned&can=2 diff --git a/chromium/docs/media/gpu/video_decoder_perf_test_usage.md b/chromium/docs/media/gpu/video_decoder_perf_test_usage.md index 26bf41c8537..f5d4c6690dd 100644 --- a/chromium/docs/media/gpu/video_decoder_perf_test_usage.md +++ b/chromium/docs/media/gpu/video_decoder_perf_test_usage.md @@ -92,6 +92,10 @@ Multiple command line arguments can be given to the command: will be stored in the current working directory. --use_vd use the new VD-based video decoders, instead of the default VDA-based video decoders. + --use_vd_vda use the new VD-based video decoders with a wrapper + that translates to the VDA interface, used to test + interaction with older components expecting the VDA + interface. --gtest_help display the gtest help and exit. --help display this help and exit. diff --git a/chromium/docs/media/gpu/video_decoder_test_usage.md b/chromium/docs/media/gpu/video_decoder_test_usage.md index 321e48f236e..9d4e1bd9512 100644 --- a/chromium/docs/media/gpu/video_decoder_test_usage.md +++ b/chromium/docs/media/gpu/video_decoder_test_usage.md @@ -65,6 +65,10 @@ Multiple command line arguments can be given to the command: --disable_validator disable frame validation. --use_vd use the new VD-based video decoders, instead of the default VDA-based video decoders. + --use_vd_vda use the new VD-based video decoders with a wrapper + that translates to the VDA interface, used to test + interaction with older components expecting the VDA + interface. --output_frames write the selected video frames to disk, possible values are "all|corrupt". diff --git a/chromium/docs/media/gpu/video_encoder_test_usage.md b/chromium/docs/media/gpu/video_encoder_test_usage.md new file mode 100644 index 00000000000..688d497cb5c --- /dev/null +++ b/chromium/docs/media/gpu/video_encoder_test_usage.md @@ -0,0 +1,73 @@ +# Video Encoder tests +The video encoder tests are a set of tests that validate various video encoding +scenarios. They are accompanied by the video encoder performance tests that can +be used to measure a video encoder's performance. + +These tests run directly on top of the video encoder implementation, and +don't require the full Chrome browser stack. They are built on top of the +[GoogleTest](https://github.com/google/googletest/blob/master/README.md) +framework. + +__Note:__ Currently these tests are under development and functionality is still +limited. + +[TOC] + +## Running from Tast +Running these tests from Tast is not supported yet. + +## Running manually +To run the video encoder tests manually the _video_encode_accelerator_tests_ +target needs to be built and deployed to the device being tested. Running +the video encoder tests can be done by executing: + + ./video_encode_accelerator_tests [<video path>] [<video metadata path>] + +e.g.: `./video_encode_accelerator_tests bear_320x192_40frames.yuv.webm` + +Running the video encoder performance tests can be done in a smilar way by +building, deploying and executing the _video_encode_accelerator_perf_tests_ +target. + + ./video_encode_accelerator_perf_tests [<video path>] [<video metadata path>] + +e.g.: `./video_encode_accelerator_perf_tests bear_320x192_40frames.yuv.webm` + +__Test videos:__ Various test videos are present in the +[_media/test/data_](https://cs.chromium.org/chromium/src/media/test/data/) +folder in Chromium's source tree (e.g. +[_bear_320x192_40frames.yuv.webm_](https://cs.chromium.org/chromium/src/media/test/data/bear_320x192_40frames.yuv.webm)). +These videos are stored in compressed format and extracted at the start of each +test run, as storing uncompressed videos requires a lot of disk space. Currently +only VP9 or uncompressed videos are supported as test input. If no video is +specified _bear_320x192_40frames.yuv.webm_ will be used. + +__Video Metadata:__ These videos also have an accompanying metadata _.json_ file +that needs to be deployed alongside the test video. The video metadata file is a +simple json file that contains info about the video such as its pixel format, +dimensions, framerate and number of frames. These can also be found in the +_media/test/data_ folder (e.g. +[_bear_320x192_40frames.yuv.webm.json_](https://cs.chromium.org/chromium/src/media/test/data/bear_320x192_40frames.yuv.webm.json)). +If no metadata file is specified _\<video path\>.json_ will be used. + +## Command line options +Multiple command line arguments can be given to the command: + + --codec codec profile to encode, "h264 (baseline)", + "h264main, "h264high", "vp8" and "vp9" + + -v enable verbose mode, e.g. -v=2. + --vmodule enable verbose mode for the specified module, + e.g. --vmodule=*media/gpu*=2. + + --gtest_help display the gtest help and exit. + --help display this help and exit. + +Non-performance tests only: + + --disable_validator disable validation of encoded bitstream. + +## Source code +See the video encoder tests [source code](https://cs.chromium.org/chromium/src/media/gpu/video_encode_accelerator_tests.cc). +See the video encoder performance tests [source code](https://cs.chromium.org/chromium/src/media/gpu/video_encode_accelerator_perf_tests.cc). + diff --git a/chromium/docs/mojo_and_services.md b/chromium/docs/mojo_and_services.md index 2aff980269f..46c2c5611b5 100644 --- a/chromium/docs/mojo_and_services.md +++ b/chromium/docs/mojo_and_services.md @@ -398,7 +398,6 @@ API: mojo::Remote<math::mojom::MathService> math_service = content::ServiceProcessHost::Launch<math::mojom::MathService>( content::ServiceProcessHost::LaunchOptions() - .WithSandboxType(content::SandboxType::kUtility) .WithDisplayName("Math!") .Pass()); ``` @@ -423,6 +422,19 @@ NOTE: To ensure the execution of the response callback, the and [this note from an earlier section](#sending-a-message)). *** +### Using a non-standard sandbox + +Ideally services will run inside the utility process sandbox, in which +case there is nothing else to do. For services that need a custom +sandbox, a new sandbox type must be defined in consultation with +security-dev@chromium.org. To launch with a custom sandbox a +specialization of `GetServiceSandboxType()` must be supplied in an +appropriate `service_sandbox_type.h` such as +[`//chrome/browser/service_sandbox_type.h`](https://cs.chromium.org/chromium/src/chrome/browser/service_sandbox_type.h) +or +[`//content/browser/service_sandbox_type.h`](https://cs.chromium.org/chromium/src/content/browser/service_sandbox_type.h) +and included where `ServiceProcessHost::Launch()` is called. + ## Content-Layer Services Overview ### Interface Brokers diff --git a/chromium/docs/navigation-request-navigation-state.gv b/chromium/docs/navigation-request-navigation-state.gv new file mode 100644 index 00000000000..30f40975477 --- /dev/null +++ b/chromium/docs/navigation-request-navigation-state.gv @@ -0,0 +1,17 @@ +// Generated with https://crrev.com/c/2220116 and: +// python3 tools/state_transitions/state_graph.py content/browser/frame_host/navigation_request.cc NavigationState +// +// See tools/state_transitions/README.md +digraph createflow { + NOT_STARTED -> {WAITING_FOR_RENDERER_RESPONSE, WILL_START_NAVIGATION, WILL_START_REQUEST}; + WAITING_FOR_RENDERER_RESPONSE -> {WILL_START_NAVIGATION}; + WILL_START_NAVIGATION -> {WILL_START_REQUEST, WILL_FAIL_REQUEST}; + WILL_START_REQUEST -> {WILL_REDIRECT_REQUEST, WILL_PROCESS_RESPONSE, READY_TO_COMMIT, DID_COMMIT, CANCELING, WILL_FAIL_REQUEST, DID_COMMIT_ERROR_PAGE}; + WILL_REDIRECT_REQUEST -> {WILL_REDIRECT_REQUEST, WILL_PROCESS_RESPONSE, CANCELING, WILL_FAIL_REQUEST}; + WILL_PROCESS_RESPONSE -> {READY_TO_COMMIT, CANCELING, WILL_FAIL_REQUEST}; + READY_TO_COMMIT -> {NOT_STARTED, DID_COMMIT, DID_COMMIT_ERROR_PAGE}; + DID_COMMIT -> {}; + CANCELING -> {READY_TO_COMMIT, WILL_FAIL_REQUEST}; + WILL_FAIL_REQUEST -> {READY_TO_COMMIT, CANCELING, WILL_FAIL_REQUEST}; + DID_COMMIT_ERROR_PAGE -> {}; +} diff --git a/chromium/docs/navigation-request-navigation-state.png b/chromium/docs/navigation-request-navigation-state.png Binary files differnew file mode 100644 index 00000000000..aa1ccb94e00 --- /dev/null +++ b/chromium/docs/navigation-request-navigation-state.png diff --git a/chromium/docs/ozone_overview.md b/chromium/docs/ozone_overview.md index a47a6dbfb0d..c04e78a5748 100644 --- a/chromium/docs/ozone_overview.md +++ b/chromium/docs/ozone_overview.md @@ -56,7 +56,6 @@ Ozone moves platform-specific code behind the following interfaces: access to IPC between the browser & GPU processes. Some platforms need this to provide additional services in the GPU process such as display configuration. -* `CursorFactoryOzone` is used to load & set platform cursors. * `OverlayManagerOzone` is used to manage overlays. * `InputController` allows to control input devices such as keyboard, mouse or touchpad. @@ -91,7 +90,7 @@ Users of the Ozone abstraction need to do the following, at minimum: invoke `PlatformWindowDelegate::DispatchEvent` to dispatch each event. * Write a subclass of `SurfaceFactoryOzone` that handles allocating accelerated surfaces. I'll call this `SurfaceFactoryOzoneImpl`. -* Write a subclass of `CursorFactoryOzone` to manage cursors, or use the +* Write a subclass of `CursorFactory` to manage cursors, or use the `BitmapCursorFactoryOzone` implementation if only bitmap cursors need to be supported. * Write a subclass of `OverlayManagerOzone` or just use `StubOverlayManager` if diff --git a/chromium/docs/privacy_budget_code_locations.md b/chromium/docs/privacy_budget_code_locations.md new file mode 100644 index 00000000000..49655c4334f --- /dev/null +++ b/chromium/docs/privacy_budget_code_locations.md @@ -0,0 +1,68 @@ +# Privacy Budget: Code Locations + +Following on from the high level [Privacy +Budget](https://github.com/bslassey/privacy-budget) explainer, the current +implementation focuses on measuring the identifiability of various web exposed +features. Hence the word `Identifiability` occurs often in the code and +documentation. + +This document focuses on code layout for Privacy Budget. Concepts and background +for the identifiability study are out of scope. + +TODO(asanka): Link to study documents once they are checked in. + +## Core Metrics and Aggregation + +Locations: + +* [`third_party/blink/public/common/privacy_budget`](../third_party/blink/public/common/privacy_budget) +* [`third_party/blink/common/privacy_budget`](../third_party/blink/common/privacy_budget) + +Includes: + +* Core logic and primitives for constructing identifiability metrics. + + This is what one would use when reporting identifiability study samples. + Centralized logic makes it easier to construct consistent and stable samples. + +* Per-process aggregation of metrics. + + Aggregation minimizes the amount of information being communicated across + process boundaries. + +The code in this directory is shared across `//content`, `//chrome`, and +`//third_party/blink`. Hence its placement in `blink/public/common`. + +In addition, this directory also contains logic for per-process aggregation of +metrics so that they can be efficiently communicated across process boundaries. + +## Static study settings + +Locations: + +* [`chrome/common/privacy_budget`](../chrome/common/privacy_budget) + +Logic for accessing per-session settings based on externally supplied field +trial configurations. The full set of externally controlled settings are +in +[`privacy_budget_features.h`](../chrome/common/privacy_budget/privacy_budget_features.h). + +At a high level, these settings control such things as: + +* Whether the study is active. +* Which identifiable surfaces should *not* be sampled. +* Parameters for how surfaces are selected for sampling. + +Both the browser and the renderer need to access these settings. The browser +needs them for filtering and reporting. The renderer needs them to avoid +sampling surfaces where sampling itself is harmful for performance or stability +reasons. + +## Persistent study state and reporting + +Locations: +* [`chrome/browser/privacy_budget`](../chrome/browser/privacy_budget) + +Per-client state is primarily used and exposed by `IdentifiabilityStudyState` +([Source](../chrome/browser/privacy_budget/identifiability_study_state.h)). + diff --git a/chromium/docs/profiling.md b/chromium/docs/profiling.md index 22508adf107..73fa46a071f 100644 --- a/chromium/docs/profiling.md +++ b/chromium/docs/profiling.md @@ -123,7 +123,7 @@ Open devtools, and in the console, use `chrome.gpuBenchmarking.startProfiling` a `chrome.gpuBenchmarking` has a number of useful methods for simulating user-gesture-initiated actions; for example, to profile scrolling: - > chrome.gpuBenchmarking.startProfiling('perf.data'); chrome.gpuBenchmarking.smoothScrollBy(1000, () => { chrome.gpuBenchmarking.stopProfiling() }); + > chrome.gpuBenchmarking.startProfiling('perf.data'); chrome.gpuBenchmarking.smoothScrollByXY(0, 1000, () => { chrome.gpuBenchmarking.stopProfiling() }); ### Profiling content_shell with callgrind diff --git a/chromium/docs/render-frame-host-lifecycle-state.gv b/chromium/docs/render-frame-host-lifecycle-state.gv new file mode 100644 index 00000000000..fffc7b5bee4 --- /dev/null +++ b/chromium/docs/render-frame-host-lifecycle-state.gv @@ -0,0 +1,11 @@ +// Generated with https://crrev.com/c/2220116 and: +// python3 tools/state_transitions/state_graph.py content/browser/frame_host/render_frame_host_impl.cc LifecycleState +// +// See tools/state_transitions/README.md +digraph createflow { + kSpeculative -> {kActive, kReadyToBeDeleted}; + kActive -> {kInBackForwardCache, kRunningUnloadHandlers, kReadyToBeDeleted}; + kInBackForwardCache -> {kActive, kReadyToBeDeleted}; + kRunningUnloadHandlers -> {kReadyToBeDeleted}; + kReadyToBeDeleted -> {}; +} diff --git a/chromium/docs/render-frame-host-lifecycle-state.png b/chromium/docs/render-frame-host-lifecycle-state.png Binary files differnew file mode 100644 index 00000000000..1cd25ff1456 --- /dev/null +++ b/chromium/docs/render-frame-host-lifecycle-state.png diff --git a/chromium/docs/security/OWNERS b/chromium/docs/security/OWNERS index e30cc23542f..1ea372401d2 100644 --- a/chromium/docs/security/OWNERS +++ b/chromium/docs/security/OWNERS @@ -6,6 +6,7 @@ felt@chromium.org mmoroz@chromium.org nasko@chromium.org meacer@chromium.org +mkwst@chromium.org palmer@chromium.org parisa@chromium.org rsesek@chromium.org diff --git a/chromium/docs/security/compromised-renderers.md b/chromium/docs/security/compromised-renderers.md index 6733e1384e6..5b21ec9ebb6 100644 --- a/chromium/docs/security/compromised-renderers.md +++ b/chromium/docs/security/compromised-renderers.md @@ -245,9 +245,13 @@ Compromised renderers shouldn't be able to poison the JavaScript code cache used by scripts executed in cross-site execution contexts. Protection techniques: -- Partitioning the code cache by Network Isolation Key (NIK). -- Using `CanAccessDataForOrigin` in +- Validating origins sent in IPCs from a renderer process by using + `CanAccessDataForOrigin` in `CodeCacheHostImpl::DidGenerateCacheableMetadataInCacheStorage`. +- Using trustworthy, browser-side origin lock while writing to and fetching from + the code cache by using `ChildProcessSecurityPolicyImpl::GetOriginLock` in + `GetSecondaryKeyForCodeCache` in + `//content/browser/renderer_host/code_cache_host_impl.cc` ## Cross-Origin-Resource-Policy response header @@ -384,3 +388,24 @@ below. processes on Android (for example affecting protections of CORB, CORP, Sec-Fetch-Site and in the future SameSite cookies and Origin protections). See also https://crbug.com/891872. + + +## Renderer processes hosting DevTools frontend + +If an attacker could take control over the DevTools frontend then the attacker +would gain access to all the cookies, storage, etc. of any origin within the +page and would be able to execute arbitrary scripts in any frame of the page. +This means that treating the DevTools renderer as untrustworthy wouldn't in +practice offer additional protection for the same-origin-policy. + +Because of the above: + +- Chrome ensures that the DevTools frontend is always hosted in a renderer + process separate from renderers hosting web origins. +- Chrome assumes that the DevTools frontend is always trustworthy + (i.e. never compromised, or under direct control of an attacker). + For example, when the DevTools process asks to initiate a HTTP request on + behalf of https://example.com, the browser process trusts the DevTools + renderer to claim authority to initiate requests of behalf of this origin + (e.g. attach SameSite cookies, send appropriate Sec-Fetch-Site request header, + etc.). diff --git a/chromium/docs/security/sheriff.md b/chromium/docs/security/sheriff.md index 897fa7a5cd9..c4418c980fe 100644 --- a/chromium/docs/security/sheriff.md +++ b/chromium/docs/security/sheriff.md @@ -248,11 +248,12 @@ the assessment? Be especially on the lookout for Highs that are really Criticals, and Lows that are really Mediums (make sure to account for process types and sandbox boundaries). -For V8 issues, it can be hard to identify the correct security severity. If -you're not sure, please take your best guess, and add the -`Security_Needs_Attention-Severity` label alongside the regular -`Security_Severity-*` label. If you do this, the V8 team will check the -severity later and change it if necessary. +For V8 issues, it can be hard to identify the correct security severity. +Always set the severity to High unless there's strong evidence of an obvious +mitigation. Please add the `Security_Needs_Attention-Severity` label alongside +the regular `Security_Severity-*` label. If the bug is not exploitable, or is +mitigated, the V8 team will reduce the security severity (to avoid unnecessary +risk of merging the bug into stable branches). #### Step 3. [Label, label, label](security-labels.md). diff --git a/chromium/docs/security/side-channel-threat-model.md b/chromium/docs/security/side-channel-threat-model.md index 05f34b035bf..888baad3ce9 100644 --- a/chromium/docs/security/side-channel-threat-model.md +++ b/chromium/docs/security/side-channel-threat-model.md @@ -341,12 +341,34 @@ clocks, and we want the OWP to be able to support them. ### Gating Access To APIs That Enable Exploitation +We want to support a powerful web, though we recognize that some kinds of APIs +a powerful web requires are more likely than others to facilitate exploitation, +either because they can be used as very high-resolution timers +(`SharedArrayBuffer`), or because they provide powerful introspection +capabilities (`performance.measureMemory`). We can mitigate the risks these APIs +pose by exposing them only in contexts that have opted-into a set of +restrictions that limits access to cross-origin data. + +In particular, [`Cross-Origin-Opener-Policy` (COOP) and +`Cross-Origin-Embedder-Policy` (COEP)](https://docs.google.com/document/d/1zDlfvfTJ_9e8Jdc8ehuV4zMEu9ySMCiTGMS9y0GU92k/edit) +seem promising. Together, these mechanisms change web-facing behavior to enable +origin-level process isolation, and ensure that cross-origin subresources will +flow into a given process only if they opt-into that usage. These properties +give us a higher degree of confidence that otherwise dangerous APIs can be +exposed safely, as any attack they enable would only gain access to same-origin +data, or data that explicitly asserted that it accepted the risk of exposure. + +Both COOP and COEP are enabled as of M83, and we [plan to require both before +enabling APIs like `SharedArrayBuffer`](https://groups.google.com/a/chromium.org/d/msg/blink-dev/_0MEXs6TJhg/F0UduPfpAQAJ). +Other browsers seem likely to do the same. + +#### Other Gating Mechanisms + **Note:** This section explores ideas but we are not currently planning on implementing anything along these lines. -Although we want to support applications that necessarily need access to -features that enable exploitation, such as `SharedArrayBuffer`, we don’t -necessarily need to make the features available unconditionally. For example, a +Looking beyond developer opt-ins such as COOP and COEP, we might be able to find +other ways of limiting the scope of APIs to reduce their risk. For example, a third-party `iframe` that is trying to exploit Spectre is very different than a WebAssembly game, in the top-level frame, that the person is actively playing (and issuing many gestures to). We could programmatically detect engagement and diff --git a/chromium/docs/session_history.md b/chromium/docs/session_history.md new file mode 100644 index 00000000000..5071a7899d2 --- /dev/null +++ b/chromium/docs/session_history.md @@ -0,0 +1,125 @@ +# Session History + +A browser's session history keeps track of the navigations in each tab, to +support back/forward navigations and session restore. This is in contrast to +"history" (e.g., `chrome://history`), which tracks the main frame URLs the user +has visited in any tab for the lifetime of a profile. + +Chromium tracks the session history of each tab in NavigationController, using a +list of NavigationEntry objects to represent the joint session history items. +Each frame creates _session history items_ as it navigates. A _joint session +history item_ contains the state of each frame of a page at a given point in +time, including things like URL, partially entered form data, scroll position, +etc. Each NavigationEntry uses a tree of FrameNavigationEntries to track this +state. + +[TOC] + + +## Pruning Forward Navigations + +If the user goes back and then commits a new navigation, this essentially forks +the joint session history. However, joint session history is tracked as a list +and not as a tree, so the previous forward history is "pruned" and forgotten. +This pruning is performed for all new navigations, unless they commit with +replacement. + + +## Subframe Navigations + +When the first commit occurs within a new subframe of a document, it becomes +part of the existing joint session history item (which we refer to as an "auto +subframe navigation"). The user can't go back to the state before the frame +committed. Any subsequent navigations in the subframe create new joint session +history items (which we refer to as "manual subframe navigations"), such that +clicking back goes back within the subframe. + + +## Navigating with Replacement + +Some types of navigations can replace the previously committed joint session +history item for a frame, rather than creating a new item. These include: + + * `location.replace` (which is usually cross-document, unless it is a fragment + navigation) + * `history.replaceState` (which is always same-document) + * Client redirects + * The first non-blank URL after the initial empty document (unless the frame + was explicitly created with `about:blank` as the URL). + + +## Identifying Same- and Cross-Document Navigations + +Each FrameNavigationEntry contains both an _item sequence number_ (ISN) and a +_document sequence number_ (DSN). Same-document navigations create a new session +history item without changing the document, and thus have a new ISN but the same +DSN. Cross-document navigations create a new ISN and DSN. NavigationController +uses these ISNs and DSNs when deciding which frames need to be navigated during +a session history navigation, using a recursive frame tree walk in +`FindFramesToNavigate`. + + +## Classifying Navigations + +Much of the complexity in NavigationController comes from the bookkeeping needed +to track the various types of navigations as they commit (e.g., same-document vs +cross-document, main frame vs subframe, with or without replacement, etc). These +types may lead to different outcomes for whether a new NavigationEntry is +created, whether an existing one is updated vs replaced, and what events are +exposed to observers. This is handled by `ClassifyNavigation`, which determines +which `RendererDidNavigate` helper methods are used when a navigation commits. + + +## Persistence + +The joint session history of a tab is persisted so that tabs can be restored +(e.g., between Chromium restarts, after closing a tab, or on another device). +This requires serializing the state in each NavigationEntry and its tree of +FrameNavigationEntries, using a PageState object and other metadata. + +Not everything in NavigationEntry is persisted. All data members of +NavigationEntryImpl and FrameNavigationEntry should be documented with whether +they are preserved after commit and whether they need to be persisted. + +Note that the session history of a tab can also be cloned when duplicating a +tab, or when doing a back/forward/reload navigation in a new tab (such as when +middle-clicking the back/forward/reload button). This involves direct clones of +NavigationEntries rather than persisting and restoring. + +## Invariants + + * The `pending_entry_index_` is either -1 or an index into `entries_`. If + `pending_entry_` is defined and `pending_entry_index_` is -1, then it is a + new navigation. If `pending_entry_index_` is a valid index into `entries_`, + then `pending_entry_` must point to that entry and it is a session history + navigation. + * Newly created tabs have NavigationControllers with `is_initial_navigation_` + set to true. They can have `last_committed_entry_index_` defined before the + first commit, however, when session history is cloned from another tab. (In + this case, `pending_entry_index_` indicates which entry is going to be + restored during the initial navigation.) + * Every FrameNavigationEntry that has committed in the current session (as + opposed to those that have been restored) must have a SiteInstance. + * A renderer process can only update FrameNavigationEntries belonging to a + SiteInstance in that process. This especially includes attacker-controlled + data like PageState, which could be dangerous to load into a different + site's process. + * Any cross-SiteInstance navigation should result in a new NavigationEntry + with replacement, rather than updating an existing NavigationEntry. + + +## Caveats + + * Not every NavigationRequest has a pending NavigationEntry. For example, + subframe navigations do not, and renderer-initiated main frame navigations + may clear an existing browser-initiated pending NavigationEntry (using + PendingEntryRef) without replacing it with a new one. + * Some main frame documents may not have a corresponding NavigationEntry + even after commit (e.g., the initial empty document, per + [issue 524208](https://crbug.com/524208)). + * Some subframe documents may not have a corresponding FrameNavigationEntry + after commit (e.g., see [issue 608402](https://crbug.com/608402)). + * FrameNavigationEntries should be shared between NavigationEntries when they + do not change (e.g., to support history.replaceState after subframe + navigations), but this is not yet supported. + See [issue 373041](https://crbug.com/373041). diff --git a/chromium/docs/sheriff.md b/chromium/docs/sheriff.md index 2a9b6801766..9dd6e746ab8 100644 --- a/chromium/docs/sheriff.md +++ b/chromium/docs/sheriff.md @@ -302,7 +302,10 @@ If yes: * Find the relevant Slack thread or start a new one * Investigate the bug and see what needs to happen - ping the owner, see if the priority & assignment are right, or try fixing it yourself. Check if it still - needs to be in the queue! + needs to be in the queue! If it doesn't need to be in the queue, remove the + `Sheriff-Chromium` label. Do this when + * the test has been disabled or marked flaky + * the problem is no longer impacting any of the builders ### 5. All's well? @@ -387,6 +390,27 @@ find the right owner for a test, feel free to TBR your CL to one of the other sheriffs on your rotation and kick it into the Blink triage queue (ie: mark the bug as Untriaged with component Blink). +#### Skia Gold Outage + +This is a special case of a failed test. The following test suites rely on the +Skia Gold image diffing service: + +* `*pixel_skia_gold_test` +* `*maps_pixel_test` +* `chrome_public_test_apk` +* `chrome_public_test_vr_apk` +* `pixel_browser_tests` + +In the unlikely event of a Gold outage, **all** of those suites will begin +failing with errors related to Gold/`goldctl`. If this occurs, the best way to +prevent failures from breaking bots is to uncomment the +`--bypass-skia-gold-functionality` argument in the `skia_gold_test` mixin in +[`//testing/buildbot/mixins.pyl`][mixins]. + +This should only be used when you are sure that the failures are caused by an +outage, as it will cause all pixel tests that use Skia Gold to no longer +perform any actual pixel testing. + ### Infra Breakage If a bot turns purple rather than red, that indicates an infra failure of some @@ -420,3 +444,4 @@ it out. [slack #sheriffing]: https://chromium.slack.com/messages/CGJ5WKRUH/ [slack]: https://chromium.slack.com [tree status page]: https://chromium-status.appspot.com/ +[mixins]: https://source.chromium.org/chromium/chromium/src/+/master:testing/buildbot/mixins.pyl diff --git a/chromium/docs/speed/bot_health_sheriffing/how_to_disable_a_story.md b/chromium/docs/speed/bot_health_sheriffing/how_to_disable_a_story.md index 496391e9c78..3eb10fc993d 100644 --- a/chromium/docs/speed/bot_health_sheriffing/how_to_disable_a_story.md +++ b/chromium/docs/speed/bot_health_sheriffing/how_to_disable_a_story.md @@ -57,10 +57,10 @@ whereas an entry disabling a benchmark on an entire platform might look like: Once you've committed your changes locally, your CL can be submitted with: -- `No-Try:True` -- `Tbr:`someone from [`tools/perf/OWNERS`](https://cs.chromium.org/chromium/src/tools/perf/OWNERS?q=tools/perf/owners&sq=package:chromium&dr) -- `CC:`benchmark owner found in [this spreadsheet](https://docs.google.com/spreadsheets/u/1/d/1xaAo0_SU3iDfGdqDJZX_jRV0QtkufwHUKH3kQKF3YQs/edit#gid=0) -- `Bug:`tracking bug +- `No-Try: True` +- `Tbr: `someone from [`tools/perf/OWNERS`](https://cs.chromium.org/chromium/src/tools/perf/OWNERS?q=tools/perf/owners&sq=package:chromium&dr) +- `CC: `benchmark owner found in [this spreadsheet](https://docs.google.com/spreadsheets/u/1/d/1xaAo0_SU3iDfGdqDJZX_jRV0QtkufwHUKH3kQKF3YQs/edit#gid=0) +- `Bug: `tracking bug *Please make sure to CC the benchmark owner so that they're aware that they've lost coverage.* diff --git a/chromium/docs/speed/good_toplevel_metrics.md b/chromium/docs/speed/good_toplevel_metrics.md index 4bf6a249da5..111c9e70b24 100644 --- a/chromium/docs/speed/good_toplevel_metrics.md +++ b/chromium/docs/speed/good_toplevel_metrics.md @@ -33,6 +33,13 @@ To initially evaluate accuracy of a quality of experience metric, we rely heavil * Use the metric implementation to sort the samples. * Use [Spearman's rank-order correlation](https://statistics.laerd.com/statistical-guides/spearmans-rank-order-correlation-statistical-guide.php) to evaluate how similar the metric implementation is to the hand ordering. +## Incentivizes the right optimizations + +Ideally developers optimize their sites' performance on metrics by improving the user experience. +But if developers can easily improve their performance on a metric without improving the actual user experience, the metric does not incentivize the right things. + +For example, if we use the onload event as the time at which we consider a web page to be fully loaded, developers will shift work after the onload event to improve their page load time. In many cases, this is the right thing to do. But since the onload event doesn't correspond to any real user-visible milestone in loading the page, it's easy to just keep shifting work after it, until eventually the entire page is loaded after onload. So instead we work to write metrics that capture user experience in a way that it's clearer to developers how they should optimize. + ## Stable A metric is stable if the result doesn’t vary much between successive runs on similar input. This can be quantitatively evaluated, ideally using Chrome Trace Processor and cluster telemetry on the top 10k sites. Eventually we hope to have a concrete threshold for a specific spread metric here, but for now, we gather the stability data, and analyze it by hand. @@ -63,9 +70,17 @@ This is frequently at odds with the interpretability requirement. For example, F If your metric involves thresholds (such as the 50ms task length threshold in TTI), or heuristics (looking at the largest jump in the number of layout objects in FMP), it’s likely to be non-elastic. -## Realtime +## Performant to compute + +If a metric is to be made available in a real-user monitoring context, it must be performant enough to compute that computing the metric does not slow down the user's browsing experience. Some metrics, like [Speed Index](https://web.dev/speed-index/), are very difficult to compute quickly enough for real-user monitoring. + +## Immediate + +Ideally we would know the metric's value *at the time it occurred*. For example, as soon as there is a contentful paint, we know that First Contentful Paint has occurred. But when the largest image or text paints to the screen, while we know it is the Largest Contentful Paint *so far*, we do not know if there will be another, larger contentful paint later on. So we can't know the value of Largest Contentful Paint until an input, scroll, or page unload. + +This isn’t always attainable, but when possible, it avoids some classes of [survivorship bias](https://en.wikipedia.org/wiki/Survivorship_bias), which makes metrics easier to analyze. -We’d like to have metrics which we can compute in realtime. For example, if we’re measuring First Meaningful Paint, we’d like to know when First Meaningful Paint occurred *at the time it occurred*. This isn’t always attainable, but when possible, it avoids some classes of [survivorship bias](https://en.wikipedia.org/wiki/Survivorship_bias), which makes metrics easier to analyze. +It also makes it easier for developers to reason about simple things like when to send a beacon to analytics, and more complex things like deferring work until after a metric representing a major milestone, like the main content being displayed. ## Orthogonal @@ -90,18 +105,22 @@ We'd like to have metrics that correlate well in the wild and in the lab, so tha * Summary [here](https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/edit#heading=h.iqlwzaf6lqrh), analysis [here](https://docs.google.com/document/d/1pZsTKqcBUb1pc49J89QbZDisCmHLpMyUqElOwYqTpSI/edit#bookmark=id.4euqu19nka18). Overall, based on manual investigation of 25 sites, our approach fired uncontroversially at the right time 64% of the time, and possibly too late the other 36% of time. We split TTI in two to allow this metric to be quite pessimistic about when TTI fires, so we’re happy with when this fires for all 25 sites. A few issues with this research: * Ideally someone less familiar with our approach would have performed the evaluation. * Ideally we’d have looked at more than 25 sites. +* Incentivizes the right optimizations + * In real-user monitoring, Time To Interactive often isn't fully measured before the page is loaded. If users leave the page before TTI is complete, the value isn't tracked. This means that sites could accidentally improve the metric if the slowest users leave the page earlier. This doesn't incentivize the right thing, which is part of the reason we recommend [First Input Delay](https://web.dev/fid) for real-user monitoring instead. * Stable * Analysis [here](https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/edit#heading=h.27s41u6tkfzj). * Interpretable * Time to Interactive is easy to explain. We report the first 5 second window where the network is roughly idle and no tasks are greater than 50ms long. * Elastic - * Time to Interactive is generally non-elastic. We’re investigating another metric which will quantify how busy the main thread is between FMP and TTI, which should be a nice elastic proxy metric for TTI. + * Time to Interactive is generally non-elastic. This is the reason we now recommend Total Blocking Time (TBT) for lab monitoring. Analysis [here](https://docs.google.com/document/d/1xCERB_X7PiP5RAZDwyIkODnIXoBk-Oo7Mi9266aEdGg/edit). * Simple * Time To Interactive has a reasonable amount of complexity, but is much simpler than Time to First Interactive. Time to Interactive has 3 parameters: * Number of allowable requests during network idle (currently 2). * Length of allowable tasks during main thread idle (currently 50ms). * Window length (currently 5 seconds). -* Realtime - * Time To Interactive is definitely not realtime, as it needs to wait until it’s seen 5 seconds of idle time before declaring that we became interactive at the start of the 5 second window. +* Immediate + * Time To Interactive is definitely not immediate, as it needs to wait until it’s seen 5 seconds of idle time before declaring that we became interactive at the start of the 5 second window. First Input Delay is an immediate alternative. +* Performant to Compute + * Time To Interactive is performant enough in Chrome that it can be used for real-user monitoring, but we recommend [First Input Delay](https://web.dev/fid) due to its issues with incentivizing the right optimizations and elasticity. * Orthogonal - * Time to Interactive aims to represent interactivity during page load, which is also what [First Input Delay](https://web.dev/fid/) aims to represent. The reason is that we haven't found a way to accurately represent this across the lab (TTI) and wild (FID) with a single metric. + * Time to Interactive aims to represent interactivity during page load, which is also what [First Input Delay](https://web.dev/fid/) aims to represent. The reason is that we haven't found a way to accurately represent this across the lab (TBT/TTI) and wild (FID) with a single metric. diff --git a/chromium/docs/speed/graphics_metrics_definitions.md b/chromium/docs/speed/graphics_metrics_definitions.md new file mode 100644 index 00000000000..b23e5774a5d --- /dev/null +++ b/chromium/docs/speed/graphics_metrics_definitions.md @@ -0,0 +1,121 @@ +# Graphics metrics: Definitions + +We need to have a metric to understand the smoothness of a particular +interaction (e.g. scroll, animation, etc.). We also need to understand the +latency of such interactions (e.g. touch-on-screen to +scroll-displayed-on-screen), and the throughput during the interaction. + +[TOC] + +We define these metrics as follows: + +## Responsiveness / Latency + +Responsiveness is a measure of how quickly the web-page responds to an event. +Latency is defined as the time between when an event happens, (e.g. moving a +touch-point on screen) and when the screen is updated directly in response to +that event [1]. For example, the event could be a moving touch-point on the +touchscreen, and the update would be scrolled content in response to that +(may only require the compositor frame update). If a rAF callback was +registered, the event would be the one that caused the current script execution +(e.g. a click event which triggered rAF), and the update would be the displayed +frame after the rAF callback is run and the content from the main-thread has +been presented on screen. + +## Throughput + +The ratio between the number of times the screen is updated for a particular +interaction (e.g. scroll, animation, etc.), and the number of times the screen +was expected to be updated (see examples below). On a 60Hz display, there would +ideally be 60 frames produced during a scroll or an animation. + +## DroppedFrames / SkippedFrames + +The ratio between the number of dropped/skipped frames for a particular +interaction, and the number of times the screen was expected to be updated. This +is the other part data of Throughput so it is a "lower-is-better" metric and +works better with current out perf tools. + +## Smoothness / Jank + +Smoothness is a measure of how consistent the throughput is. Jank during an +interaction is defined as a change in the throughput for consecutive frames. +To explain this further: + +Consider the following presented frames: + +**f1** +**f2** +**f3** +**f4** +**f5** +**f6** +**f7** +**f8** +**f9** + +Each highlighted **fn** indicates a frame that contained response from the +renderer[2]. So in the above example, there were no janks, and throughput was +100%: i.e. all the presented frames included updated content. + +Considering the following frames: + +**f1** +**f2** +f3 +**f4** +f5 +**f6** +**f7** +**f8** +**f9** + +In this case, frames `f3` and `f5` did not include any updates (either +display-compositor was unable to submit a new frame, or the frame submitted by +the display compositor did not include any updates from the renderer). +Therefore, the throughput during this interaction is 78%. + +To explain the jank, during the first two frames `[f1, f2]`, the throughput is +100%. Because of the subsequently missed frame `f3`, the throughput changes for +`[f2, f4]` drops to 67%. The throughput for `[f4, f6]` is also 67%. For +subsequent frames, the throughput goes back up to 100%. Therefore, there was a +single jank. + +Consider the following two sequences: + +**f1** +**f2** +**f3** +**f4** +f5 +f6 +f7 +f8 +**f9** + +**f1** +f2 +**f3** +f4 +**f5** +f6 +**f7** +f8 +**f9** + +In both cases, throughput is 55%, since only 5 out of 9 frames are displayed. +In the first sequence, there is a jank (`[f1, f2][f2, f3][f3, f4]` has 100% +throughput, but `[f4, f9]` has a throughput of 33%). However, in the second +sequence, there are no janks, since `[f1, f3]` `[f3, f5]` `[f5, f7]` `[f7, f9]` +all have 67% throughput. + +[1]: Indirect updates in response to an event, e.g. an update from a +setTimeout() callback from an event-handler would not be associated with that +event. + +[2]: Note that the response could be either an update to the content, or a +notification that no update is expected for that frame. For example, for a 30fps +animation in this frame-sequence, only frames `f1` `f3` `f5` `f7` `f9` will have +actual updates from the animation, and frames `f2` `f4` `f6` `f8` should still +have notification from the client that no update is expected. + diff --git a/chromium/docs/testing/android_test_instructions.md b/chromium/docs/testing/android_test_instructions.md index ec2786c0c04..6a51945bb9e 100644 --- a/chromium/docs/testing/android_test_instructions.md +++ b/chromium/docs/testing/android_test_instructions.md @@ -172,10 +172,10 @@ For example, adding a test to `chrome_junit_tests` requires to update ninja -C out/Default chrome_junit_tests # Run the test suite. -out/Default/run_chrome_junit_tests +out/Default/bin/run_chrome_junit_tests # Run a subset of tests. You might need to pass the package name for some tests. -out/Default/run_chrome_junit_tests -f "org.chromium.chrome.browser.media.*" +out/Default/bin/run_chrome_junit_tests -f "org.chromium.chrome.browser.media.*" ``` ### Debugging diff --git a/chromium/docs/testing/rendering_representative_perf_tests.md b/chromium/docs/testing/rendering_representative_perf_tests.md index c4b8c5a4b9f..da3600b0370 100644 --- a/chromium/docs/testing/rendering_representative_perf_tests.md +++ b/chromium/docs/testing/rendering_representative_perf_tests.md @@ -1,10 +1,14 @@ # Representative Performance Tests for Rendering Benchmark -`rendering_representative_perf_tests` runs a sub set of stories from rendering +`rendering_representative_perf_tests` run a subset of stories from rendering benchmark on CQ, to prevent performance regressions. For each platform there is a `story_tag` which describes the representative stories used in this test. -These stories will be tested using the [`run_benchmark`](../../tools/perf/run_benchmark) script. Then the recorded values for `frame_times` will be -compared with the historical upper limit described in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`](../../testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json). +These stories will be tested using the [`run_benchmark`](../../tools/perf/run_benchmark) script, and then the recorded values for the target metric (currently `frame_times`) will be +compared with the historical upper limit described in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`][rep_perf_json]. + +These tests are currently running on CQ on: +* mac-rel +* win10_chromium_x64_rel_ng [TOC] @@ -18,13 +22,60 @@ Example:`animometer_webgl_attrib_arrays higher average frame_times(21.095) compa This means that the animometer_webgl_attrib_arrays story has the average frame_times of 21 ms and the recorded upper limit for the story (in the tested platform) is 17 ms. In these cases the failed story will be ran three more times to make sure that this has not been a flake, and the new result (average of three runs) will be reported in the same format. -For deeper investigation of such cases you can find the traces of the runs in the isolated outputs of the test. In the isolated outputs directory look at output.json for the initial run and at re_run_failures/output.json for the three re-runs. +For deeper investigation of such cases you can find the traces of the runs in the isolated outputs of the test. In the isolated outputs directory look at `output.json` for the initial run and at `re_run_failures/output.json` for the three traces recorded from re-runs. +If the failure is as a result of an expected regression, please follow the instructions in the next section for the "Updating Expectations". In the `output.json` file, you can find the name of the story and under the trace.html field of the story a gs:// link to the trace ([Example](https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f8961d773cdf0bf121525f29024c0e6c19d42e61&as=output.json)). To download the trace run: `gsutil cp gs://link_from_output.json trace_name.html` Also if tests fail with no specific messages in the output, it will be useful to check the {benchmark}/benchmark_log.txt file in the isolated outputs directory for more detailed log of the failure. +### Running representative perf tests locally + +You can run the representative perf tests locally for more investigation, but it is important to note that the values may differ with the values reported on the bots as these tests can have different values for different hardware. + +``` +./testing/scripts/run_rendering_benchmark_with_gated_performance.py ./tools/perf/run_benchmark \ +--benchmark rendering.desktop --isolated-script-test-output /tmp/temp_dir/ \ +--isolated-script-test-perf-output /tmp/temp_dir +``` +*rendering.mobile for running mobile representatives* + + +For investigation of crashes (or when the comparison of the values is not the focus) it might be easier to directly run the benchmark for the representative story tags such as: +* `representative_win_desktop (benchmark: rendering.desktop)` +* `representative_mac_desktop (benchmark: rendering.desktop)` +* `representative_mobile (benchmark: rendering.mobile)` + +``` +./tools/perf/run_benchmark rendering.desktop --story-tag-filter representative_win_desktop +``` + +## Updating Expectations + +There might be multiple reasons to skip a story in representative perf tests such as: +* Tests results are flaky and the story needs to be skipped +* Adding a new change with expected regression, so we need to skip the test, so that the change would pass on CQ), and adjust the upper limit later. +* We want to add a new story to the representative set and the appropriate upper limit is not known yet + +In these cases the story should not cause a failure but it needs to record the values for later adjustments. +As a result the preferred method to skip a story of the representative perf test is to mark the specific story as experimental in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`][rep_perf_json] along with a bug referring to the cause of test suppression (flakiness, change with expected regression or experimenting with new representatives). This way the test will be run but the values will not be considered for failing the test. + +To do so find the story under the affected platform in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`][rep_perf_json] and add the "experimental" tag to it. + +``` +"platform": { + "story_name": { + "ci_095": 0.377, + "avg": 31.486, + "experimental": true, + "_comment": "crbug.com/bug_id" + }, +} +``` +*[Example Cl](https://chromium-review.googlesource.com/c/chromium/src/+/2208294)* + + ## Maintaining Representative Performance Tests ### Clustering the Benchmark and Choosing Representatives @@ -43,27 +94,15 @@ managed by adding and removing story tags above to stories in [rendering benchma ### Updating the Upper Limits The upper limits for averages and confidence interval (CI) ranges of -`frame_times` described in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`](../../testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json) -are used to passing or failing a test. These values are the 95 percentile of -the past 30 runs of the test on each platform (for both average and CI). +`frame_times` described in [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`][rep_perf_json] are used to passing or failing a test. These values are the 95 percentile of the past 30 runs of the test on each platform (for both average and CI). This helps with catching sudden regressions which results in a value higher than the upper limits. But in case of gradual regressions, the upper limits may not be useful in not updated frequently. Updating these upper limits also helps with adopting to improvements. -Updating these values can be done by running [`src/tools/perf/experimental/representative_perf_test_limit_adjuster/adjust_upper_limits.py`](../../tools/perf/experimental/representative_perf_test_limit_adjuster/adjust_upper_limits.py)and committing the changes. +Updating these values can be done by running [`src/tools/perf/experimental/representative_perf_test_limit_adjuster/adjust_upper_limits.py`](../../tools/perf/experimental/representative_perf_test_limit_adjuster/adjust_upper_limits.py) and committing the changes. The script will create a new JSON file using the values of recent runs in place -of [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`](../../testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json). - -### Updating Expectations - -To skip any of the tests, update -[`src/tools/perf/expectations.config`](../../tools/perf/expectations.config) and -add the story under rendering benchmark (Examples [1](https://chromium-review.googlesource.com/c/chromium/src/+/2055681), [2](https://chromium-review.googlesource.com/c/chromium/src/+/1901357)). -This expectations file disables the story on the rendering benchmark, which rendering_representative_perf_tests are part of. -So please add the a bug for each skipped test and link to `Internals>GPU>Metrics`. +of [`src/testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json`][rep_perf_json]. -If the test is part of representative perf tests on Windows or MacOS, this -should be done under rendering.desktop benchmark and if it's a test on Android -under rendering.mobile.
\ No newline at end of file +[rep_perf_json]: ../../testing/scripts/representative_perf_test_data/representatives_frame_times_upper_limit.json
\ No newline at end of file diff --git a/chromium/docs/testing/web_tests.md b/chromium/docs/testing/web_tests.md index 5e4d188e293..faa428ca938 100644 --- a/chromium/docs/testing/web_tests.md +++ b/chromium/docs/testing/web_tests.md @@ -55,23 +55,26 @@ strip ./xcodebuild/{Debug,Release}/content_shell.app/Contents/MacOS/content_shel TODO: mention `testing/xvfb.py` -The test runner script is in -`third_party/blink/tools/run_web_tests.py`. +The test runner script is in `third_party/blink/tools/run_web_tests.py`. To specify which build directory to use (e.g. out/Default, out/Release, out/Debug) you should pass the `-t` or `--target` parameter. For example, to use the build in `out/Default`, use: ```bash -python third_party/blink/tools/run_web_tests.py -t Default +third_party/blink/tools/run_web_tests.py -t Default ``` For Android (if your build directory is `out/android`): ```bash -python third_party/blink/tools/run_web_tests.py -t android --android +third_party/blink/tools/run_web_tests.py -t android --android ``` +*** promo +Windows users need to use `third_party/blink/tools/run_web_tests.bat` instead. +*** + Tests marked as `[ Skip ]` in [TestExpectations](../../third_party/blink/web_tests/TestExpectations) won't be run by default, generally because they cause some intractable tool error. @@ -97,13 +100,13 @@ arguments to `run_web_tests.py` relative to the web test directory use: ```bash -python third_party/blink/tools/run_web_tests.py fast/forms +third_party/blink/tools/run_web_tests.py fast/forms ``` Or you could use the following shorthand: ```bash -python third_party/blink/tools/run_web_tests.py fast/fo\* +third_party/blink/tools/run_web_tests.py fast/fo\* ``` *** promo @@ -111,7 +114,7 @@ Example: To run the web tests with a debug build of `content_shell`, but only test the SVG tests and run pixel tests, you would run: ```bash -[python] third_party/blink/tools/run_web_tests.py -t Default svg +third_party/blink/tools/run_web_tests.py -t Default svg ``` *** @@ -143,7 +146,7 @@ for more details of running `content_shell`. To see a complete list of arguments supported, run: ```bash -python third_party/blink/tools/run_web_tests.py --help +third_party/blink/tools/run_web_tests.py --help ``` *** note @@ -217,7 +220,7 @@ There are two ways to run web tests with additional command-line arguments: * Using `--additional-driver-flag`: ```bash - python run_web_tests.py --additional-driver-flag=--blocking-repaint + third_party/blink/tools/run_web_tests.py --additional-driver-flag=--blocking-repaint ``` This tells the test harness to pass `--blocking-repaint` to the @@ -382,10 +385,10 @@ tips for finding the problem. spacing or box sizes are often unimportant, especially around fonts and form controls. Differences in wording of JS error messages are also usually acceptable. - * `python run_web_tests.py path/to/your/test.html` produces a page listing - all test results. Those which fail their expectations will include links - to the expected result, actual result, and diff. These results are saved - to `$root_build_dir/layout-test-results`. + * `third_party/blink/tools/run_web_tests.py path/to/your/test.html` produces + a page listing all test results. Those which fail their expectations will + include links to the expected result, actual result, and diff. These + results are saved to `$root_build_dir/layout-test-results`. * Alternatively the `--results-directory=path/for/output/` option allows you to specify an alternative directory for the output to be saved to. * If you're still sure it's correct, rebaseline the test (see below). @@ -428,8 +431,7 @@ tips for finding the problem. To run the server manually to reproduce/debug a failure: ```bash -cd src/third_party/blink/tools -python run_blink_httpd.py +third_party/blink/tools/run_blink_httpd.py ``` The web tests are served from `http://127.0.0.1:8000/`. For example, to @@ -473,18 +475,12 @@ machine? ### Debugging DevTools Tests -* Add `debug_devtools=true` to `args.gn` and compile: `autoninja -C out/Default devtools_frontend_resources` - > Debug DevTools lets you avoid having to recompile after every change to the DevTools front-end. * Do one of the following: * Option A) Run from the `chromium/src` folder: - `third_party/blink/tools/run_web_tests.sh - --additional-driver-flag='--debug-devtools' - --additional-driver-flag='--remote-debugging-port=9222' - --time-out-ms=6000000` + `third_party/blink/tools/run_web_tests.py --additional-driver-flag='--remote-debugging-port=9222' --additional-driver-flag='--debug-devtools' --time-out-ms=6000000` * Option B) If you need to debug an http/tests/inspector test, start httpd as described above. Then, run content_shell: - `out/Default/content_shell --debug-devtools --remote-debugging-port=9222 --run-web-tests - http://127.0.0.1:8000/path/to/test.html` + `out/Default/content_shell --remote-debugging-port=9222 --additional-driver-flag='--debug-devtools' --run-web-tests http://127.0.0.1:8000/path/to/test.html` * Open `http://localhost:9222` in a stable/beta/canary Chrome, click the single link to open the devtools with the test loaded. * In the loaded devtools, set any required breakpoints and execute `test()` in @@ -546,8 +542,7 @@ read on. *** ```bash -cd src/third_party/blink -python tools/run_web_tests.py --reset-results foo/bar/test.html +third_party/blink/tools/run_web_tests.py --reset-results foo/bar/test.html ``` If there are current expectation files for `web_tests/foo/bar/test.html`, @@ -569,12 +564,18 @@ Though we prefer the Rebaseline Tool to local rebaselining, the Rebaseline Tool doesn't support rebaselining flag-specific expectations. ```bash -cd src/third_party/blink -python tools/run_web_tests.py --additional-driver-flag=--enable-flag --reset-results foo/bar/test.html +third_party/blink/tools/run_web_tests.py --additional-driver-flag=--enable-flag --reset-results foo/bar/test.html ``` +*** promo +You can use `--flag-specific=config` as a shorthand of +`--additional-driver-flag=--enable-flag` if `config` is defined in +`web_tests/FlagSpecificConfig`. +*** New baselines will be created in the flag-specific baselines directory, e.g. -`web_tests/flag-specific/enable-flag/foo/bar/test-expected.{txt,png}`. +`web_tests/flag-specific/enable-flag/foo/bar/test-expected.{txt,png}` +or +`web_tests/flag-specific/config/foo/bar/test-expected.{txt,png}` Then you can commit the new baselines and upload the patch for review. diff --git a/chromium/docs/testing/web_tests_in_content_shell.md b/chromium/docs/testing/web_tests_in_content_shell.md index 9b4c4229481..ecdb57aabfd 100644 --- a/chromium/docs/testing/web_tests_in_content_shell.md +++ b/chromium/docs/testing/web_tests_in_content_shell.md @@ -14,11 +14,15 @@ You can run web tests using `run_web_tests.py` (in `src/third_party/blink/tools`). ```bash -python third_party/blink/tools/run_web_tests.py storage/indexeddb +third_party/blink/tools/run_web_tests.py -t <build directory> storage/indexeddb ``` To see a complete list of arguments supported, run with `--help`. -***promo +*** promo +Windows users need to use `third_party/blink/tools/run_web_tests.bat` instead. +*** + +*** promo You can add `<path>/third_party/blink/tools` into `PATH` so that you can run it from anywhere without the full path. *** @@ -85,6 +89,8 @@ Then run the test with a localhost URL: out/Default/content_shell --run-web-tests http://localhost:8000/<test> ``` +It may be necessary specify [--enable-blink-features](https://source.chromium.org/search?q=%22--enable-blink-features%3D%22) explicitly for some tests. + #### Running WPT Tests in Content Shell Similar to HTTP tests, many WPT (a.k.a. web-platform-tests under @@ -92,8 +98,9 @@ Similar to HTTP tests, many WPT (a.k.a. web-platform-tests under tests require some setup before running in Content Shell: ```bash -python third_party/blink/tools/run_blink_wptserve.py +python third_party/blink/tools/run_blink_wptserve.py -t <build directory> ``` + Then run the test: ```bash diff --git a/chromium/docs/threading_and_tasks.md b/chromium/docs/threading_and_tasks.md index 60d4ddfed5a..3bfcd0377c1 100644 --- a/chromium/docs/threading_and_tasks.md +++ b/chromium/docs/threading_and_tasks.md @@ -826,3 +826,90 @@ that component since unit tests will use the leaf layer directly. See [Threading and Tasks FAQ](threading_and_tasks_faq.md) for more examples. [task APIs v3]: https://docs.google.com/document/d/1tssusPykvx3g0gvbvU4HxGyn3MjJlIylnsH13-Tv6s4/edit?ts=5de99a52#heading=h.ss4tw38hvh3s + +## Internals + +### SequenceManager + +[SequenceManager](https://cs.chromium.org/chromium/src/base/task/sequence_manager/sequence_manager.h) +manages TaskQueues which have different properties (e.g. priority, common task +type) multiplexing all posted tasks into a single backing sequence. This will +usually be a MessagePump. Depending on the type of message pump used other +events such as UI messages may be processed as well. On Windows APC calls (as +time permits) and signals sent to a registered set of HANDLEs may also be +processed. + +### MessagePump + +[MessagePumps](https://cs.chromium.org/chromium/src/base/message_loop/message_pump.h) +are responsible for processing native messages as well as for giving cycles to +their delegate (SequenceManager) periodically. MessagePumps take care to mixing +delegate callbacks with native message processing so neither type of event +starves the other of cycles. + +There are different [MessagePumpTypes](https://cs.chromium.org/chromium/src/base/message_loop/message_pump_type.h), +most common are: + +* DEFAULT: Supports tasks and timers only + +* UI: Supports native UI events (e.g. Windows messages) + +* IO: Supports asynchronous IO (not file I/O!) + +* CUSTOM: User provided implementation of MessagePump interface + +### RunLoop + +RunLoop is s helper class to run the RunLoop::Delegate associated with the +current thread (usually a SequenceManager). Create a RunLoop on the stack and +call Run/Quit to run a nested RunLoop but please avoid nested loops in +production code! + +### Task Reentrancy + +SequenceManager has task reentrancy protection. This means that if a +task is being processed, a second task cannot start until the first task is +finished. Reentrancy can happen when processing a task, and an inner +message pump is created. That inner pump then processes native messages +which could implicitly start an inner task. Inner message pumps are created +with dialogs (DialogBox), common dialogs (GetOpenFileName), OLE functions +(DoDragDrop), printer functions (StartDoc) and *many* others. + +```cpp +Sample workaround when inner task processing is needed: + HRESULT hr; + { + MessageLoopCurrent::ScopedNestableTaskAllower allow; + hr = DoDragDrop(...); // Implicitly runs a modal message loop. + } + // Process |hr| (the result returned by DoDragDrop()). +``` + +Please be SURE your task is reentrant (nestable) and all global variables +are stable and accessible before before using +MessageLoopCurrent::ScopedNestableTaskAllower. + +## APIs for general use + +User code should hardly ever need to access SequenceManager APIs directly as +these are meant for code that deals with scheduling. Instead you should use the +following: + +* base::RunLoop: Drive the SequenceManager from the thread it's bound to. + +* base::Thread/SequencedTaskRunnerHandle: Post back to the SequenceManager TaskQueues from a task running on it. + +* SequenceLocalStorageSlot : Bind external state to a sequence. + +* base::MessageLoopCurrent : Proxy to a subset of Task related APIs bound to the current thread + +* Embedders may provide their own static accessors to post tasks on specific loops (e.g. content::BrowserThreads). + +### SingleThreadTaskExecutor and TaskEnvironment + +Instead of having to deal with SequenceManager and TaskQueues code that needs a +simple task posting environment (one default task queue) can use a +[SingleThreadTaskExecutor](https://cs.chromium.org/chromium/src/base/task/single_thread_task_executor.h). + +Unit tests can use [TaskEnvironment](https://cs.chromium.org/chromium/src/base/test/task_environment.h) +which is highly configurable. diff --git a/chromium/docs/ui/index.md b/chromium/docs/ui/index.md index cdf7b44f158..d64a1e7974e 100644 --- a/chromium/docs/ui/index.md +++ b/chromium/docs/ui/index.md @@ -16,6 +16,13 @@ Recipes to help you work with Chrome UI. Details on Chrome UI. * [Best Practices](/docs/ui/learn/index.md#best-practices) +* [Product Excellence](/docs/ui/product_excellence/index.md) +* [UI Devtools](/docs/ui/ui_devtools/index.md) +* [Views](/docs/ui/views/overview.md) + +Archival Documentation on Chrome UI. +* [Aura](/docs/ui/aura/index.md) +* [Compositor](/docs/ui/compositor/index.md) Processes diff --git a/chromium/docs/ui/learn/bestpractices/colors.md b/chromium/docs/ui/learn/bestpractices/colors.md index cf8a0846c37..f9802fbfe0a 100644 --- a/chromium/docs/ui/learn/bestpractices/colors.md +++ b/chromium/docs/ui/learn/bestpractices/colors.md @@ -1,12 +1,11 @@ -# Best Practice: Colors +# Best practice: colors -Views best practices for color handling largely center around distinguishing how -colors are handled physically versus logically. Physical colors are defined -presentationally, usually as specific RGB or Material values, or occasionally -referentially (e.g. "#202124", "Google Grey 900", or "10% white atop toolbar -background"). Logical colors are defined functionally, and may be more or less -specific depending on context (e.g. "body text", "call to action", "hovered -button background", or "profile menu bottom stroke"). +Product colors can be defined physically or logically. Physical colors can be +specific RGB values (e.g. "#202124"), Material values (e.g. "Google Grey 900"), +or occasionally referential values (“10% white atop toolbar background”). +Logical colors are defined functionally, and may be more or less specific +depending on context (e.g. “body text”, “call to action”, “hovered button +background”, or “profile menu bottom stroke”). Distinguishing logical and physical use in code makes code amenable to systematic changes (Material Design, Harmony, MD Refresh, dark mode...). It @@ -14,26 +13,26 @@ increases behavioral consistency and improves readability. It usually results in greater UI accessibility. Finally, it helps avoid edge case bugs that are easy to miss, such as problems with custom themes or the GTK native appearance. -Since colors tend to get scattered across the codebase, mixed with the guidance -on distinguishing physical and logical color use are some rules for consistent -code organization. +The following best practices focus on distinguishing physical and logical color +use, along with some rules for consistent code organization. [TOC] -## Agree on Color Function +## Agree on color function -**Get UX/eng agreement on color function, not just presentation.** A mock with +**UX and engineering should agree on color function (logical colors) rather +than just presentation (physical colors).** A mock with only physical values isn't amenable to the separation described above; there should be a rationale that describes how the various colors relate to each other and to the relevant specs (e.g. [Desktop Design System Guidelines](https://goto.google.com/chrome-snowglobe) -(Internal Link)). Ideally, physical color values aren't even necessary, because +(Internal Link)). Ideally, *physical color values aren't even necessary*, because all values refer to pre-specified concepts; if new colors are needed, UX leadership and the toolkit team should agree on how to incorporate them into an updated system. -## Do Not Use Physical Values Directly In A View +## Do not use physical values directly in a View -**Do not use physical values directly in a View.** This means avoiding +**Use logical color values in a `View`.** This means avoiding `SkColorSet[A]RGB(...)`, `SK_ColorBLACK`, `gfx::kGoogleGrey900`, and the like. Instead, obtain colors by requesting them (by identifier) from an appropriate theming object. `View`s in `ui/` should call @@ -50,7 +49,7 @@ Old code in `tooltip_icon.cc` used hardcoded colors: ##### -**Best Practice** +**Best practice** The [current version](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/bubble/tooltip_icon.cc;l=78;drc=756282c5ac4947c7329bc8b68711c3898334018f) uses color IDs, allowing the icon colors to potentially be tied to other icon @@ -94,7 +93,7 @@ void TooltipIcon::SetDrawAsHovered(bool hovered) { |||---||| -## Set Colors in OnThemeChanged() +## Set colors in OnThemeChanged() **Set colors in an `OnThemeChanged()` override;** update elsewhere as needed. Theming objects are generally provided through the `Widget`, and so will be null @@ -118,7 +117,7 @@ constructor since it seemed reasonable to do: ##### -**Best Practice** +**Best practice** The [current version](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/native_file_system/native_file_system_usage_bubble_view.cc;l=196;drc=532834e1da3874a57dde3ed76511f53b8eb8ecdf) moves this to `OnThemeChanged()` and thus handles theme changes correctly: @@ -190,7 +189,7 @@ void CollapsibleListView::OnThemeChanged() { |||---||| -## Only Use Colors Locally +## Only use colors locally **Keep color use local.** `View`s should generally access color IDs only for themselves, and not get or set colors on other `View`s (e.g. children). Because @@ -211,7 +210,7 @@ its child: ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc;l=157;drc=ab2ca22fb75c61f7167e9b20ac88cc7a85c3be6b) queries a color API that is guaranteed to have an up-to-date value: @@ -252,7 +251,7 @@ class CustomTabBarTitleOriginView : public views::View { |||---||| -## Only Use Color IDs to Forward Color Information +## Only use color IDs to forward color information Where color information must be passed on, **use color IDs, not `SkColor` or `ImageSkia`**. For example, menu items with images should use @@ -295,7 +294,7 @@ RecoveryInstallGlobalError::MenuItemIcon() { ##### -**Best Practice** +**Best practice** ``` cpp ui::ImageModel @@ -311,7 +310,7 @@ RecoveryInstallGlobalError::MenuItemIcon() { |||---||| -## Create New Color Identifiers for New UI +## Create new color identifiers for new UI For new UI, **add new, specific color identifiers**. If a mock for a new `FrobberMenu` UI calls for the background color to be the same as the @@ -334,7 +333,7 @@ constant to paint the drop indicator, since they have the same physical color: ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/controls/menu/submenu_view.cc;l=229;drc=7910ceae672184033abc44a287e309f14e664b5e) defines these as distinct logical colors with the same default physical color, @@ -404,7 +403,7 @@ void SubmenuView::PaintChildren( |||---||| -## Define Identifiers Using Relationships +## Define identifiers using relationships **Define identifiers' physical values by their relationships to other colors**. In most cases, new UI elements are using the same underlying concepts as other @@ -429,7 +428,7 @@ Old code in `common_theme.cc` defines a disabled button color absolutely: ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:ui/native_theme/common_theme.cc;l=264;drc=21c19ce054e99a4361dff61877a4197831b80e6b) makes it clear that the disabled color is related to the normal color: @@ -484,7 +483,7 @@ SkColor GetAuraColor( |||---||| -## Keep Image Resources Theme Neutral +## Keep image resources theme neutral **Use image resources that are theme-neutral**. Most vector images should not encode color information at all; in some cases, [badging](https://source.chromium.org/chromium/chromium/src/+/master:ui/gfx/paint_vector_icon.h;l=70;drc=7a9b1b437ae847e90ef3ac10f73991796dfe5591) @@ -518,7 +517,7 @@ An improved version could split the image into a fixed color portion and an uncolored portion using alpha that is programmatically computed based on the desired foreground color. -## Do Not Use Colors Outside a View +## Use colors inside a View **Do not use colors outside a `View`**, since doing so correctly is difficult. Global access to theming objects is deprecated and will eventually be removed, @@ -537,7 +536,7 @@ Old code in `tab_strip_ui_handler.cc` uses global theming objects: ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler.cc;l=524;drc=cfa76e5827628eb2104df0e0b55d5d89f4a93eaf) gets colors from its embedding `View`; this will still require the caller to diff --git a/chromium/docs/ui/learn/bestpractices/layout.md b/chromium/docs/ui/learn/bestpractices/layout.md new file mode 100644 index 00000000000..5740c73dd03 --- /dev/null +++ b/chromium/docs/ui/learn/bestpractices/layout.md @@ -0,0 +1,1350 @@ +# Best practice: layout + +The most important principles when working with Views layout are abstraction +and encapsulation. The goal of the guidance here is to split the broad work of +"layout" into many discrete pieces, each of which can be easily understood, +tested, and changed in isolation. Compliant code should be more readable, +performant, and maintainable; more adaptable to future modifications; and more +stylistically consistent with other UI elements both now and in the future. + +[TOC] + +## Express layout values logically + +Both in mocks and code, **layout values should be described in functional terms +that conform to the relevant specs** (particularly the +[Material Refresh][] and older [Harmony][] specs). Rather than a mock saying +a dialog title is "15 pt high", it should say it is the "Title 1" or +"DIALOG\_TITLE" style; two non-interacting controls on the same line should not +be separated by "16 dip" but by [`DISTANCE_UNRELATED_CONTROL_HORIZONTAL`][]. +Designers and engineers can reference a [dictionary][] to agree on common +terminology. Don't simply back-figure the constant to use based on what +produces the same value as the mock, as future design system changes will +cause subtle and confusing bugs. Similarly, don't hardcode designer-provided +values that aren't currently present in Chrome, as the semantics of such +one-off values are unclear and will cause maintenance problems. +Work with the Toolkit and UX teams to modify the design and overall spec so +the desired results can be achieved in a centralized way. + +Note: the concept in this section is general, but the linked specs are +**Googlers Only**. + +[Material Refresh]: https://docs.google.com/presentation/d/1EO7TOpIMJ7QHjaTVw9St-q6naKwtXX2TwzMirG5EsKY/edit#slide=id.g3232c09376_6_794 +[Harmony]: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20%28MD%29/Secondary%20UI%20Previews%20and%20specs%20%28exports%29/Spec +[`DISTANCE_UNRELATED_CONTROL_HORIZONTAL`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;l=56;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524 +[dictionary]: http://go/DesktopDictionary + +## Obtain layout values from provider objects + +Once layout styles and values are expressed functionally, **the exact values +should be obtained from relevant provider objects, not computed directly.** +Code should not have any magic numbers for sizes, positions, elevations, and +the like; this includes transformations like scaling values up or down by a +fraction, or tweaking a provided value by a few DIPs. The most commonly used +provider object is the [`LayoutProvider`][] (or its `chrome`/-side extension, +[`ChromeLayoutProvider`][]); `View`s can obtain the global instance via a +relevant [`Get()`][] method and ask it for relevant [distances][], [insets][], +[corner radii][], [shadow elevations][], and the like. For text-setting +controls like [`Label`][], the [`TypographyProvider`][] (or its +`chrome`/-side extension, [`ChromeTypographyProvider`]), +usually accessed via [global helper functions][], can provide appropriate +[fonts][], [colors][], and [line heights][]. Most `View`s should not use these +directly, but use `Label` and other such controls, providing the appropriate +[context][] and [style][]. + +[`LayoutProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;l=129;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 +[`ChromeLayoutProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524;l=76 +[`Get()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=135 +[distances]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=148 +[insets]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=144 +[corner radii]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=168 +[shadow elevations]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=172 +[`Label`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/controls/label.h;l=30;drc=59135b4042aa469752899e8e4bf2a0a81d3d320c +[`TypographyProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=22;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10 +[`ChromeTypographyProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_typography_provider.h;l=13;drc=a7ee000c95842e2dce6397ca36926924f4cb322b +[global helper functions]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=109;drc=8f7db479018a99e5906876954de93ae6d23bee58 +[fonts]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=28;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10 +[colors]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=32;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10 +[line heights]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=37;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10 +[context]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=23;drc=8f7db479018a99e5906876954de93ae6d23bee58 +[style]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=67;drc=8f7db479018a99e5906876954de93ae6d23bee58 + +|||---||| + +##### + +**Avoid** + +[Current code][1] uses file scoped hard-coded padding values for its layout +constants. + +[1]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/subtle_notification_view.cc;l=142;drc=787d0aacc071674dc83f6059072d15f8cfffbf84 + +##### + +**Best practice** + +A better approach would be to use layout constants sourced from the +[`ChromeLayoutProvider`][]. + +|||---||| + +|||---||| + +##### + +``` cpp +namespace { +// Space between the site info label. +const int kMiddlePaddingPx = 30; + +const int kOuterPaddingHorizPx = 40; +const int kOuterPaddingVertPx = 8; +} // namespace + +SubtleNotificationView::SubtleNotificationView() + : instruction_view_(nullptr) { + ... + instruction_view_ = + new InstructionView(base::string16()); + + int outer_padding_horiz = kOuterPaddingHorizPx; + int outer_padding_vert = kOuterPaddingVertPx; + AddChildView(instruction_view_); + + SetLayoutManager(std::make_unique<views::BoxLayout>( + views::BoxLayout::Orientation::kHorizontal, + gfx::Insets(outer_padding_vert, + outer_padding_horiz), + kMiddlePaddingPx)); +} +``` + +##### + +``` cpp + + + + + + + + +SubtleNotificationView::SubtleNotificationView() + : instruction_view_(nullptr) { + ... + AddChildView(std::make_unique<InstructionView>( + base::string16())); + + const gfx::Insets kDialogInsets = + ChromeLayoutProvider::Get()->GetInsetsMetric( + views::INSETS_DIALOG); + const int kHorizontalPadding = + ChromeLayoutProvider::Get()->GetDistanceMetric( + views::DISTANCE_RELATED_LABEL_HORIZONTAL); + SetLayoutManager(std::make_unique<views::BoxLayout>( + views::BoxLayout::Orientation::kHorizontal, + kDialogInsets, kHorizontalPadding)); +} +``` + +|||---||| + + +## Use hierarchy liberally + +While not a layout-specific tactic, it simplifies many layout issues +**to break a high-level UI construct into a hierarchy of `View`s**, +with as many levels as necessary to make each `View` as simple as possible. +In such hierarchies, most non-leaf `View`s will be nameless "containers" that +simply size or group their immediate children, perhaps with padding between +them or a margin around the outside. Each such `View` is easy to lay out, +and you can later combine or factor out pieces of the hierarchy as appropriate, +including adding helpers for common Material Design idioms to the core toolkit. + + +## Use LayoutManagers + +**Avoid overriding [`Layout()`][] to programmatically lay out children.** +In nearly all cases, the built-in [`LayoutManager`][]s can achieve the desired +layout, and do so in a declarative rather than imperative fashion. The +resulting code is often simpler and easier to understand. Writing a [bespoke +`LayoutManager`][] is also possible, but less common. + +[`Layout()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=730;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 +[`LayoutManager`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_manager.h;l=33;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 +[bespoke `LayoutManager`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/try_chrome_dialog_win/button_layout.h;l=30;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 + +|||---||| + +##### + +**Avoid** + +The following old code used Layout() to have its label text fill the dialog. + +##### + +**Best practice** + +[Current code][2] uses a [FillLayout][] to achieve the same result. + +[2]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/relaunch_notification/relaunch_required_dialog_view.cc;l=91;drc=1ec33e7c19e2d63b3f918df115c12f77f419645b +[FillLayout]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/fill_layout.h + +|||---||| + +|||---||| + +##### + +``` cpp +int RelaunchRequiredDialogView::GetHeightForWidth( + int width) const { + const gfx::Insets insets = GetInsets(); + return body_label_->GetHeightForWidth( + width - insets.width()) + insets.height(); +} + +void RelaunchRequiredDialogView::Layout() { + body_label_->SetBoundsRect(GetContentsBounds()); +} +``` + +##### + +``` cpp +RelaunchRequiredDialogView::RelaunchRequiredDialogView( + base::Time deadline, + base::RepeatingClosure on_accept) + : ...{ + SetLayoutManager( + std::make_unique<views::FillLayout>()); + ... +} + + +``` + +|||---||| + + +## Prefer intrinsic constraints to extrinsic computation + +Where possible, **express the desired outcome of layout in terms of intrinsic +constraints for each `View`,** instead of trying to conditionally compute +the desired output metrics. For example, using a [`ClassProperty`][] +to set each child's [margins][] is less error-prone than trying to +conditionally add padding `View`s between children. When coupled with +[margin collapsing][] and [internal padding][], it's possible to do things +like use [different padding amounts between different children][] +or visually align elements without manually computing offsets. +Such manual computation is prone to bugs if someone changes a size, padding +value, or child order in one place without also updating related computations +elsewhere. + +[`ClassProperty`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/base/class_property.h;l=55;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 +[margins]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_class_properties.h;l=30;drc=1449b8c60358c4cdea1722e4c1e8079bd1b5f306 +[margin collapsing]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/flex_layout.h;l=87;drc=62bf27aca5418212ceadd8daf9188d2aa437bfcc +[internal padding]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_class_properties.h;l=40;drc=1449b8c60358c4cdea1722e4c1e8079bd1b5f306 +[different padding amounts between different children]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/toolbar/toolbar_view.cc;l=974;drc=34a8c4215229379ced3586125399c7ad3c65b87f + +|||---||| + +##### + +**Avoid** + +The following is old code that calculated bubble padding through calculations +involving the control insets. + +##### + +**Best practice** + +[Current code][3] uses a combination of margin and padding on the +ColorPickerView to ensure proper alignment. + +[3]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/tabs/tab_group_editor_bubble_view.cc;l=89;drc=542c4c6ac89bc665807351d3fb4aca5ebddc82f8 + +|||---||| + +|||---||| + +##### + +``` cpp +TabGroupEditorBubbleView::TabGroupEditorBubbleView( + const Browser* browser, + const tab_groups::TabGroupId& group, + TabGroupHeader* anchor_view, + base::Optional<gfx::Rect> anchor_rect, + bool stop_context_menu_propagation) + : ... { + + ... + const auto* layout_provider = + ChromeLayoutProvider::Get(); + const int horizontal_spacing = + layout_provider->GetDistanceMetric( + views::DISTANCE_RELATED_CONTROL_HORIZONTAL); + const int vertical_menu_spacing = + layout_provider->GetDistanceMetric( + views::DISTANCE_RELATED_CONTROL_VERTICAL); + + // The vertical spacing for the non menu items within + // the editor bubble. + const int vertical_dialog_content_spacing = 16; + + + + + + + + + + + views::View* group_modifier_container = + AddChildView(std::make_unique<views::View>()); + + gfx::Insets color_element_insets = + ChromeLayoutProvider::Get()->GetInsetsMetric( + views::INSETS_VECTOR_IMAGE_BUTTON); + group_modifier_container->SetBorder( + views::CreateEmptyBorder( + gfx::Insets(vertical_dialog_content_spacing, + horizontal_spacing - + color_element_insets.left(), + vertical_dialog_content_spacing, + horizontal_spacing - + color_element_insets.right()))); + ... + // Add the text field for editing the title. + views::View* title_field_container = + group_modifier_container->AddChildView( + std::make_unique<views::View>()); + title_field_container->SetBorder( + views::CreateEmptyBorder(gfx::Insets( + 0, color_element_insets.left(), + vertical_dialog_content_spacing, + color_element_insets.right())) + ... + color_selector_ = + group_modifier_container->AddChildView( + std::make_unique<ColorPickerView>( + colors_, background_color(), initial_color, + base::Bind( + &TabGroupEditorBubbleView::UpdateGroup, + base::Unretained(this)))); + ... +} + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +##### + +``` cpp +TabGroupEditorBubbleView::TabGroupEditorBubbleView( + const Browser* browser, + const tab_groups::TabGroupId& group, + views::View* anchor_view, + base::Optional<gfx::Rect> anchor_rect, + TabGroupHeader* header_view, + bool stop_context_menu_propagation) + : ... { + ... + const auto* layout_provider = + ChromeLayoutProvider::Get(); + const int horizontal_spacing = + layout_provider->GetDistanceMetric( + views::DISTANCE_RELATED_CONTROL_HORIZONTAL); + const int vertical_spacing = + layout_provider->GetDistanceMetric( + views::DISTANCE_RELATED_CONTROL_VERTICAL); + + // The padding of the editing controls is adaptive, + // to improve the hit target size and screen real + // estate usage on touch devices. + const int group_modifier_vertical_spacing = + ui::TouchUiController::Get()->touch_ui() ? + vertical_spacing / 2 : vertical_spacing; + const gfx::Insets control_insets = + ui::TouchUiController::Get()->touch_ui() + ? gfx::Insets(5 * vertical_spacing / 4, + horizontal_spacing) + : gfx::Insets(vertical_spacing, + horizontal_spacing); + + views::View* group_modifier_container = + AddChildView(std::make_unique<views::View>()); + group_modifier_container->SetBorder( + views::CreateEmptyBorder(gfx::Insets( + group_modifier_vertical_spacing, 0))); + + views::FlexLayout* group_modifier_container_layout = + group_modifier_container->SetLayoutManager( + std::make_unique<views::FlexLayout>()); + group_modifier_container_layout + ->SetOrientation( + views::LayoutOrientation::kVertical) + .SetIgnoreDefaultMainAxisMargins(true); + + + // Add the text field for editing the title. + views::View* title_field_container = + group_modifier_container->AddChildView( + std::make_unique<views::View>()); + title_field_container->SetBorder( + views::CreateEmptyBorder( + control_insets.top(), control_insets.left(), + group_modifier_vertical_spacing, + control_insets.right())); + + ... + const tab_groups::TabGroupColorId initial_color_id = + InitColorSet(); + color_selector_ = + group_modifier_container->AddChildView( + std::make_unique<ColorPickerView>( + this, colors_, initial_color_id, + base::Bind( + &TabGroupEditorBubbleView::UpdateGroup, + base::Unretained(this)))); + color_selector_->SetProperty( + views::kMarginsKey, + gfx::Insets(0, control_insets.left(), 0, + control_insets.right())); + ... +} + +ColorPickerView::ColorPickerView( + const views::BubbleDialogDelegateView* bubble_view, + const TabGroupEditorBubbleView::Colors& colors, + tab_groups::TabGroupColorId initial_color_id, + ColorSelectedCallback callback) + : callback_(std::move(callback)) { + ... + // Set the internal padding to be equal to the + // horizontal insets of a color picker element, + // since that is the amount by which the color + // picker's margins should be adjusted to make it + // visually align with other controls. + gfx::Insets child_insets = elements_[0]->GetInsets(); + SetProperty(views::kInternalPaddingKey, + gfx::Insets(0, child_insets.left(), 0, + child_insets.right())); +} +``` + +|||---||| + +## Use GridLayout with caution + +[`GridLayout`][] is a `LayoutManager` used for tabular layouts. Much like +table-based layout in HTML, it can achieve almost any desired effect, and in +some scenarios (e.g. creating an actual table) is the best tool. Used +indiscriminately, it can be cryptic, verbose, and error-prone. Accordingly, +**use `GridLayout` only when creating a true grid, not simply for selective +horizontal and vertical alignment.** For simple layouts, [`BoxLayout`][] and +[`FlexLayout`][] are better choices; for more complex layouts, representing +sections or groups hierarchically may result in simpler inner layouts that +can be nested within an overall layout. + +[`GridLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/grid_layout.h;l=79;drc=8cab3382ac9b70b7ecfe29ae03b1b7ee8f4e01fa +[`BoxLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/box_layout.h;l=28;drc=5b9e43d976aca377588875fc59c5348ede02a8b5 +[`FlexLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/flex_layout.h;l=73;drc=62bf27aca5418212ceadd8daf9188d2aa437bfcc + +|||---||| + +##### + +**Avoid** + +The following old code uses a [`GridLayout`][] to create a HoverButton with +a stacked title and subtitle flanked on by views on both sides. + +##### + +**Best practice** + +[Current code][4] uses [`FlexLayout`][] to achieve the desired result, resulting +in clearer code. + +[4]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/hover_button.cc;l=106;drc=888af74006ea1c4ee9907d18c8df2a7ca424eab9 + +|||---||| + +|||---||| + +##### + +``` cpp +// Used to create the following layout +// +-----------+---------------------+----------------+ +// | icon_view | title | secondary_view | +// +-----------+---------------------+----------------+ +// | | subtitle | | +// +-----------+---------------------+----------------+ +HoverButton::HoverButton( + views::ButtonListener* button_listener, + std::unique_ptr<views::View> icon_view, + const base::string16& title, + const base::string16& subtitle, + std::unique_ptr<views::View> secondary_view, + bool resize_row_for_secondary_view, + bool secondary_view_can_process_events) { + ... + views::GridLayout* grid_layout = + SetLayoutManager( + std::make_unique<views::GridLayout>()); + ... + constexpr int kColumnSetId = 0; + views::ColumnSet* columns = + grid_layout->AddColumnSet(kColumnSetId); + columns->AddColumn( + views::GridLayout::CENTER, + views::GridLayout::CENTER, + views::GridLayout::kFixedSize, + views::GridLayout::USE_PREF, 0, 0); + columns->AddPaddingColumn( + views::GridLayout::kFixedSize, + icon_label_spacing); + columns->AddColumn( + views::GridLayout::FILL, + views::GridLayout::FILL, + 1.0, views::GridLayout::USE_PREF, 0, 0); + ... + grid_layout->StartRow( + views::GridLayout::kFixedSize, kColumnSetId, + row_height); + icon_view_ = grid_layout->AddView( + std::move(icon_view), 1, num_labels); + ... + auto title_wrapper = + std::make_unique<SingleLineStyledLabelWrapper>( + title); + title_ = title_wrapper->label(); + grid_layout->AddView(std::move(title_wrapper)); + + if (secondary_view) { + columns->AddColumn( + views::GridLayout::CENTER, + views::GridLayout::CENTER, + views::GridLayout::kFixedSize, + views::GridLayout::USE_PREF, 0, 0); + ... + secondary_view_ = + grid_layout->AddView( + std::move(secondary_view), 1, num_labels); + ... + } + if (!subtitle.empty()) { + grid_layout->StartRow( + views::GridLayout::kFixedSize, kColumnSetId, + row_height); + auto subtitle_label = + std::make_unique<views::Label>( + subtitle, views::style::CONTEXT_BUTTON, + views::style::STYLE_SECONDARY); + ... + grid_layout->SkipColumns(1); + subtitle_ = + grid_layout->AddView(std::move(subtitle_label)); + } + ... +} +``` + +##### + +``` cpp +// Used to create the following layout +// +-----------+---------------------+----------------+ +// | | title | | +// | icon_view |---------------------| secondary_view | +// | | subtitle | | +// +-----------+---------------------+----------------+ +HoverButton::HoverButton( + views::ButtonListener* button_listener, + std::unique_ptr<views::View> icon_view, + const base::string16& title, + const base::string16& subtitle, + std::unique_ptr<views::View> secondary_view, + bool resize_row_for_secondary_view, + bool secondary_view_can_process_events) { + ... + SetLayoutManager( + std::make_unique<views::FlexLayout>()) + ->SetCrossAxisAlignment( + views::LayoutAlignment::kCenter) + .SetChildViewIgnoredByLayout( + ink_drop_container(), true); + ... + icon_view_ = + AddChildView(std::make_unique<IconWrapper>( + std::move(icon_view), vertical_spacing)) + ->icon(); + + // |label_wrapper| will hold both the title and + // subtitle if it exists. + auto label_wrapper = std::make_unique<views::View>(); + title_ = label_wrapper->AddChildView( + std::make_unique<views::StyledLabel>( + title, nullptr)); + + if (!subtitle.empty()) { + auto subtitle_label = + std::make_unique<views::Label>( + subtitle, views::style::CONTEXT_BUTTON, + views::style::STYLE_SECONDARY); + ... + subtitle_ = label_wrapper->AddChildView( + std::move(subtitle_label)); + } + + label_wrapper->SetLayoutManager( + std::make_unique<views::FlexLayout>()) + ->SetOrientation( + views::LayoutOrientation::kVertical) + .SetMainAxisAlignment( + views::LayoutAlignment::kCenter); + label_wrapper->SetProperty( + views::kFlexBehaviorKey, + views::FlexSpecification( + views::MinimumFlexSizeRule::kScaleToZero, + views::MaximumFlexSizeRule::kUnbounded)); + label_wrapper->SetProperty( + views::kMarginsKey, + gfx::Insets(vertical_spacing, 0)); + label_wrapper_ = + AddChildView(std::move(label_wrapper)); + ... + + if (secondary_view) { + ... + secondary_view->SetProperty( + views::kMarginsKey, + gfx::Insets(secondary_spacing, + icon_label_spacing, + secondary_spacing, 0)); + secondary_view_ = + AddChildView(std::move(secondary_view)); + } + ... +} +``` + +|||---||| + +## Compute preferred/minimum sizes recursively from children + +**Avoid hardcoding preferred or minimum sizes,** including via metrics like +[`DISTANCE_BUBBLE_PREFERRED_WIDTH`][]. +In many cases, `LayoutManager`s will provide reasonable values for these, +and common codepaths like [`BubbleFrameView::GetFrameWidthForClientWidth()`][] +can help ensure that the returned values are [conformed to spec][]. +When a `View` does need to calculate these manually, it should do so based on +the corresponding values returned by its children, not by returning specific +numbers (e.g. dialog preferred size is 300 by 150). In particular, assuming +fonts will be in a certain size, or that a given fixed area is sufficient to +display all necessary information, can cause hard-to-find localization and +accessibility bugs for users with verbose languages or unusually large fonts. + +[`DISTANCE_BUBBLE_PREFERRED_WIDTH`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;l=68;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524 +[`BubbleFrameView::GetFrameWidthForClientWidth()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/bubble/bubble_frame_view.cc;l=688;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 +[conformed to spec]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/bubble/bubble_frame_view.cc;l=698;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38 + +|||---||| + +##### + +**Avoid** + +Current code overloads CalculatePreferredSize() in the dialog view. + +##### + +**Best practice** + +A better approach would be to omit the overload completely and let leaf views +size the dialog appropriately, relying on the minimum size fallbacks +if necessary. + +|||---||| + +|||---||| + +##### + +``` cpp +... +gfx::Size +CastDialogView::CalculatePreferredSize() const { + const int width = + ChromeLayoutProvider::Get()->GetDistanceMetric( + DISTANCE_BUBBLE_PREFERRED_WIDTH); + return gfx::Size(width, GetHeightForWidth(width)); +} +``` + +##### + +``` cpp +... +``` + +|||---||| + + +## Handle events directly, not via Layout() + +In addition to using `LayoutManager`s in place of manual layout, +**avoid overriding `Layout()` to perform non-layout actions.** For example, +instead of updating properties tied to a `View`'s size in `Layout()`, +do so in [`OnBoundsChanged()`][]; +when the `View` in question is a child, make the child a `View` subclass +with an `OnBoundsChanged()` override instead of having the parent both lay +the child out and update its properties. Modify the +[hit-testing and event-handling functions][] directly instead of laying out +invisible `View`s to intercept events. Toggle child visibility directly in +response to external events rather than calculating it inside `Layout()`. + +[`OnBoundsChanged()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1377;drc=34a8c4215229379ced3586125399c7ad3c65b87f +[hit-testing and event-handling functions]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=919;drc=34a8c4215229379ced3586125399c7ad3c65b87f + +|||---||| + +##### + +**Avoid** + +Old code updated hit testing and button properties in the Layout() method. + +##### + +**Best practice** + +Current code wraps the buttons in a file scoped class with an +OnBoundsChanged() method and modifies the hit testing functions directly to +achieve the same result. + +|||---||| + +|||---||| + +##### + +``` cpp + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +FindBarView::FindBarView(FindBarHost* host) + : find_bar_host_(host) { + auto find_text = std::make_unique<views::Textfield>(); + find_text_ = AddChildView(std::move(find_text)); + ... + auto find_previous_button = + views::CreateVectorImageButton(this); + find_previous_button_ = + AddChildView(std::move(find_previous_button)); + ... + auto find_next_button = + views::CreateVectorImageButton(this); + find_next_button_ = + AddChildView(std::move(find_next_button)); + ... + auto close_button = + views::CreateVectorImageButton(this); + close_button_ = + AddChildView(std::move(close_button)); +} + +void FindBarView::Layout() { + views::View::Layout(); + // The focus forwarder view is a hidden view that + // should cover the area between the find text box + // and the find button so that when the user clicks + // in that area we focus on the find text box. + const int find_text_edge = + find_text_->x() + find_text_->width(); + focus_forwarder_view_->SetBounds( + find_text_edge, find_previous_button_->y(), + find_previous_button_->x() - find_text_edge, + find_previous_button_->height()); + + for (auto* button : + {find_previous_button_, find_next_button_, + close_button_}) { + constexpr int kCircleDiameterDp = 24; + auto highlight_path = std::make_unique<SkPath>(); + // Use a centered circular shape for inkdrops and + // focus rings. + gfx::Rect circle_rect(button->GetLocalBounds()); + circle_rect.ClampToCenteredSize( + gfx::Size(kCircleDiameterDp, + kCircleDiameterDp)); + highlight_path->addOval( + gfx::RectToSkRect(circle_rect)); + button->SetProperty(views::kHighlightPathKey, + highlight_path.release()); + } +} +``` + +##### + +``` cpp +// An ImageButton that has a centered circular +// highlight. +class FindBarView::FindBarButton + : public views::ImageButton { + public: + using ImageButton::ImageButton; + protected: + void OnBoundsChanged( + const gfx::Rect& previous_bounds) override { + const gfx::Rect bounds = GetLocalBounds(); + auto highlight_path = std::make_unique<SkPath>(); + const gfx::Point center = bounds.CenterPoint(); + const int radius = views::LayoutProvider::Get() + ->GetCornerRadiusMetric( + views::EMPHASIS_MAXIMUM, bounds.size()); + highlight_path->addCircle( + center.x(), center.y(), radius); + SetProperty(views::kHighlightPathKey, + highlight_path.release()); + } +}; + +bool FindBarView::OnMousePressed( + const ui::MouseEvent& event) { + // The find text box only extends to the match count + // label. However, users expect to be able to click + // anywhere inside what looks like the find text + // box (including on or around the match_count label) + // and have focus brought to the find box. Cause + // clicks between the textfield and the find previous + // button to focus the textfield. + const int find_text_edge = + find_text_->bounds().right(); + const gfx::Rect focus_area( + find_text_edge, find_previous_button_->y(), + find_previous_button_->x() - find_text_edge, + find_previous_button_->height()); + if (!GetMirroredRect(focus_area).Contains( + event.location())) + return false; + find_text_->RequestFocus(); + return true; + +FindBarView::FindBarView(FindBarHost* host) + : find_bar_host_(host) { + auto find_text = std::make_unique<views::Textfield>(); + find_text_ = AddChildView(std::move(find_text)); + ... + auto find_previous_button = + std::make_unique<FindBarButton>(this); + views::ConfigureVectorImageButton( + find_previous_button.get()); + ... + auto find_next_button = + std::make_unique<FindBarButton>(this); + views::ConfigureVectorImageButton( + find_next_button.get()); + ... + auto close_button = + std::make_unique<FindBarButton>(this); + views::ConfigureVectorImageButton(close_button.get()); +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +|||---||| + +## Don't invoke Layout() directly + +**Avoid direct calls to `Layout()`.** +These are typically used for three purposes: + +1. *Calling `Layout()` on `this`, when something that affects layout has +changed.* This forces a synchronous layout, which can lead to needless +work (e.g. if several sequential changes each trigger layout). Use +asynchronous layout\* instead. In many cases (such as +[the preferred size changing][] or +[a child needing layout][], +a `View` will automatically mark itself as needing layout; when necessary, call +[`InvalidateLayout()`][] to mark it manually. + +1. *Calling `Layout()` or `InvalidateLayout()` on some `View` to notify it +that something affecting its layout has changed.* Instead, ensure that +`View` is notified of the underlying change (via specific method overrides or +plumbing from a model object), and then invalidates its own layout when needed. + +1. *Calling `Layout()` on some `View` to "ensure it's up to date" before +reading some layout-related property off it.* Instead, plumb any relevant +events to the current object, then handle them directly (e.g. override +[`ChildPreferredSizeChanged()`][] or use a [`ViewObserver`][] +to monitor the target `View`; then update local state as necessary and trigger +handler methods). + +\* *How does asynchronous layout work?* In the browser, the compositor +periodically [requests a LayerTreeHost update][]. +This ultimately calls back to [`Widget::LayoutRootViewIfNecessary()`][], +recursively laying out invalidated `View`s within the `Widget`. In unittests, +this compositor-driven sequence never occurs, so it's necessary to +[call LayoutRootViewIfNecessary() manually][] when a test needs to ensure a +`View`'s layout is up-to-date. Many tests fail to do this, but currently pass +because something triggers Layout() directly; +accordingly, changing existing code from synchronous to asynchronous layout may +require adding `LayoutRootViewIfNecessary()` calls to (possibly many) tests, +and this is not a sign that the change is wrong. + +[the preferred size changing]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.cc;l=1673;drc=bc9a6d40468646be476c61b6637b51729bec7b6d +[a child needing layout]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.cc;l=777;drc=bc9a6d40468646be476c61b6637b51729bec7b6d +[`InvalidateLayout()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=735;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4 +[`ChildPreferredSizeChanged()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1381;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4 +[`ViewObserver`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_observer.h;l=17;drc=eb20fd77330dc4a89eecf17459263e5895e7f177 +[requests a LayerTreeHost update]: https://source.chromium.org/chromium/chromium/src/+/master:cc/trees/layer_tree_host.cc;l=304;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4 +[`Widget::LayoutRootViewIfNecessary()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget.h;l=946;drc=b1dcb398c454a576092d38d0d67db3709b2b2a9b +[call LayoutRootViewIfNecessary() manually]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget_unittest.cc;l=3110;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4 + +|||---||| + +##### + +**Avoid** + +[Current code][5] makes a direct and unnecessary call to Layout() + +[5]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/media_router/cast_dialog_view.cc;l=349;drc=18ca2de542bfa53802639dc5c85762b5e7b5bef6 + +##### + +**Best practice** + +A better approach would be to call InvalidateLayout() and update the necessary tests. + +|||---||| + +|||---||| + +##### + +``` cpp +void CastDialogView::PopulateScrollView( + const std::vector<UIMediaSink>& sinks) { + ... + Layout(); +} + +TEST_F(CastDialogViewTest, PopulateDialog) { + CastDialogModel model = + CreateModelWithSinks({CreateAvailableSink()}); + InitializeDialogWithModel(model); + + EXPECT_TRUE(dialog_->ShouldShowCloseButton()); + EXPECT_EQ(model.dialog_header(), + dialog_->GetWindowTitle()); + EXPECT_EQ(ui::DIALOG_BUTTON_NONE, + dialog_->GetDialogButtons()); +} + + +``` + +##### + +``` cpp +void CastDialogView::PopulateScrollView( + const std::vector<UIMediaSink>& sinks) { + ... + InvalidateLayout(); +} + +TEST_F(CastDialogViewTest, PopulateDialog) { + CastDialogModel model = + CreateModelWithSinks({CreateAvailableSink()}); + InitializeDialogWithModel(model); + CastDialogView::GetCurrentDialogWidget() + ->LayoutRootViewIfNecessary(); + + EXPECT_TRUE(dialog_->ShouldShowCloseButton()); + EXPECT_EQ(model.dialog_header(), + dialog_->GetWindowTitle()); + EXPECT_EQ(ui::DIALOG_BUTTON_NONE, + dialog_->GetDialogButtons()); +} +``` + +|||---||| + + +## Consider different objects for different layouts + +If a surface needs very different appearances in different states (e.g. a +dialog whose content changes at each of several steps, or a container whose +layout toggles between disparate orientations), **use different `View`s to +contain the distinct states** instead of manually adding and removing +children and changing layout properties at each step. It's easier to reason +about several distinct fixed-layout `View`s than a single object whose layout +and children vary over time, and often more performant as well. + +|||---||| + +##### + +**Avoid** + +[Current code][6] holds both horizontal and vertical time views and replaces +the children and LayoutManager on orientation change. + +[6]: https://source.chromium.org/chromium/chromium/src/+/master:ash/system/time/time_view.h;l=35;drc=7d8bc7f807a433e6a127806e991fe780aa27ce77;bpv=1;bpt=0?originalUrl=https:%2F%2Fcs.chromium.org%2F + +##### + +**Best practice** + +A better approach would encapsulate the horizontal and vertical time views +into separate views. + +|||---||| + +|||---||| + +##### + +``` cpp +class ASH_EXPORT TimeView : public ActionableView, + public ClockObserver { + ... + private: + ... + std::unique_ptr<views::Label> horizontal_label_; + std::unique_ptr<views::Label> vertical_label_hours_; + std::unique_ptr<views::Label> vertical_label_minutes_; + ... +}; + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +void TimeView::SetupLabels() { + horizontal_label_.reset(new views::Label()); + SetupLabel(horizontal_label_.get()); + vertical_label_hours_.reset(new views::Label()); + SetupLabel(vertical_label_hours_.get()); + vertical_label_minutes_.reset(new views::Label()); + SetupLabel(vertical_label_minutes_.get()); + ... +} + + + +void TimeView::UpdateClockLayout( + ClockLayout clock_layout) { + // Do nothing if the layout hasn't changed. + if (((clock_layout == ClockLayout::HORIZONTAL_CLOCK) ? + horizontal_label_ : vertical_label_hours_) + ->parent() == this) + return; + + SetBorder(views::NullBorder()); + if (clock_layout == ClockLayout::HORIZONTAL_CLOCK) { + RemoveChildView(vertical_label_hours_.get()); + RemoveChildView(vertical_label_minutes_.get()); + SetLayoutManager( + std::make_unique<views::FillLayout>()); + AddChildView(horizontal_label_.get()); + } else { + RemoveChildView(horizontal_label_.get()); + // Remove the current layout manager since it could + // be the FillLayout which only allows one child. + SetLayoutManager(nullptr); + // Pre-add the children since ownership is being + // retained by this. + AddChildView(vertical_label_hours_.get()); + AddChildView(vertical_label_minutes_.get()); + views::GridLayout* layout = + SetLayoutManager( + std::make_unique<views::GridLayout>()); + const int kColumnId = 0; + views::ColumnSet* columns = + layout->AddColumnSet(kColumnId); + columns->AddPaddingColumn( + 0, kVerticalClockLeftPadding); + columns->AddColumn(views::GridLayout::TRAILING, + views::GridLayout::CENTER, + 0, views::GridLayout::USE_PREF, + 0, 0); + layout->AddPaddingRow(0, kClockLeadingPadding); + layout->StartRow(0, kColumnId); + // Add the views as existing since ownership isn't + // being transferred. + layout->AddExistingView( + vertical_label_hours_.get()); + layout->StartRow(0, kColumnId); + layout->AddExistingView( + vertical_label_minutes_.get()); + layout->AddPaddingRow( + 0, kVerticalClockMinutesTopOffset); + } + Layout(); +} +``` + +##### + +``` cpp +class ASH_EXPORT TimeView : public ActionableView, + public ClockObserver { + ... + private: + class HorizontalLabelView; + class VerticalLabelView; + ... + HorizontalLabelView* horizontal_label_; + VerticalLabelView* vertical_label_; + ... +}; + +TimeView::HorizontalLabelView::HorizontalLabelView() { + SetLayoutManager( + std::make_unique<views::FillLayout>()); + views::Label* time_label = + AddChildView(std::make_unique<views::Label>()); + SetupLabels(time_label); + ... +} + +TimeView::VerticalLabelView::VerticalLabelView() { + views::GridLayout* layout = + SetLayoutManager( + std::make_unique<views::GridLayout>()); + views::Label* label_hours = + AddChildView(std::make_unique<views::Label>()); + views::Label* label_minutes = + AddChildView(std::make_unique<views::Label>()); + SetupLabel(label_hours); + SetupLabel(label_minutes); + const int kColumnId = 0; + views::ColumnSet* columns = + layout->AddColumnSet(kColumnId); + columns->AddPaddingColumn( + 0, kVerticalClockLeftPadding); + columns->AddColumn( + views::GridLayout::TRAILING, + views::GridLayout::CENTER, + 0, views::GridLayout::USE_PREF, 0, 0); + layout->AddPaddingRow(0, kClockLeadingPadding); + layout->StartRow(0, kColumnId); + // Add the views as existing since ownership isn't + // being transferred. + layout->AddExistingView(label_hours); + layout->StartRow(0, kColumnId); + layout->AddExistingView(label_minutes); + layout->AddPaddingRow( + 0, kVerticalClockMinutesTopOffset); + ... +} + +void TimeView::TimeView(ClockLayout clock_layout, + ClockModel* model) { + ... + horizontal_label_ = + AddChildView( + std::make_unique<HorizontalLabelView>()); + vertical_label_ = + AddChildView( + std::make_unique<VerticalLabelView()); + ... +} + +void TimeView::UpdateClockLayout( + ClockLayout clock_layout) { + ... + const bool is_horizontal = + clock_layout == ClockLayout::HORIZONTAL_CLOCK; + horizontal_label_->SetVisible(is_horizontal); + vertical_label_->SetVisible(!is_horizontal); + InvalidateLayout(); +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +|||---||| + diff --git a/chromium/docs/ui/learn/bestpractices/ownership.md b/chromium/docs/ui/learn/bestpractices/ownership.md index 6c0a340f943..12d038f40f9 100644 --- a/chromium/docs/ui/learn/bestpractices/ownership.md +++ b/chromium/docs/ui/learn/bestpractices/ownership.md @@ -1,4 +1,4 @@ -# Best Practice: Ownership +# Best practice: ownership In the common case, a `View` instance lives within a hierarchy of `View`s, up to a `RootView` object that is owned by a `Widget`. Calling `AddChildView<>()` @@ -8,7 +8,7 @@ ownership patterns exist, newly-added code should use this one. [TOC] -## Lifetime Basics +## Lifetime basics Accordingly, the best practices for ownership and lifetime management in Views code are similar to those in the rest of Chromium, but include some details @@ -33,7 +33,7 @@ using bare `new`: ##### -**Best Practice** +**Best practice** Rewriting using the `unique_ptr<>` version of `AddChildView<>()` is shorter and safer: @@ -110,7 +110,7 @@ layouts without recreating the children: ##### -**Best Practice** +**Best practice** Rewriting using subclasses to encapsulate layout allows the parent to merely adjust visibility: @@ -259,7 +259,7 @@ void TimeView::UpdateClockLayout( |||---||| -## Avoid Refcounting and WeakPtrs +## Avoid refcounting and WeakPtrs Refcounting and `WeakPtr`s may also be indicators that a `View` is doing more than merely displaying UI. Views objects should only handle UI. Refcounting and @@ -276,7 +276,7 @@ Old code in `cast_dialog_no_sinks_view.{h,cc}` that used weak pointers to ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view.h;l=20;drc=2a147d67e51e67144257d3a405e3aafec3827513) eliminates lifetime concerns by using a `OneShotTimer`, which is canceled when @@ -358,7 +358,7 @@ which manually deletes a child after removing it: ##### -**Best Practice** +**Best practice** Rewriting using `RemoveChildViewT<>()` is shorter and safer: @@ -394,7 +394,7 @@ Rewriting using `RemoveChildViewT<>()` is shorter and safer: |||---||| -## Prefer Scoping Objects +## Prefer scoping objects **Prefer scoping objects to paired Add/Remove-type calls**. For example, use a [`ScopedObserver<>`](https://source.chromium.org/chromium/chromium/src/+/master:base/scoped_observer.h;l=43;drc=6bd38d45dbd4144235318b607216e51a7f9dc60e) @@ -413,7 +413,7 @@ that uses paired add/remove calls: ##### -**Best Practice** +**Best practice** Rewriting using `ScopedObserver<>` eliminates the destructor body entirely: @@ -477,7 +477,7 @@ AvatarToolbarButton::~AvatarToolbarButton() = default; |||---||| -## Keep Lifetimes Fully Nested +## Keep lifetimes fully nested For objects you own, **destroy in the reverse order you created,** so lifetimes are nested rather than partially-overlapping. This can also reduce the @@ -495,7 +495,7 @@ creation: ##### -**Best Practice** +**Best practice** [Current version](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget_interactive_uitest.cc;l=492;drc=cbe5dca6cb1e1511c82776370a964d99cbf5205d) uses scoping objects that are simpler and safer: @@ -536,12 +536,12 @@ TEST_F(WidgetTestInteractive, |||---||| -## Only Use Views Objects on the UI Thread +## Only use Views objects on the UI thread **Always use `View`s on the main (UI) thread.** Like most Chromium code, the Views toolkit is [not thread-safe](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=170;drc=7cba41605a8489bace83f380760486638a2a8a4a). -## Add Child Views in a View's Constructor +## Add child Views in a View's constructor In most cases, **add child `View`s in a `View`'s constructor.** From an ownership perspective, doing so is safe even though the `View` is not yet in a @@ -566,7 +566,7 @@ that sets up a child `View` in `ViewHierarchyChanged()`: ##### -**Best Practice** +**Best practice** Rewriting to do this at construction/`Init()` makes |`web_view_`|'s lifetime easier to reason about: diff --git a/chromium/docs/ui/learn/index.md b/chromium/docs/ui/learn/index.md index 56088324791..8594fad0f1b 100644 --- a/chromium/docs/ui/learn/index.md +++ b/chromium/docs/ui/learn/index.md @@ -3,6 +3,7 @@ # Best Practices * [Colors](bestpractices/colors.md): How to work with Chromium colors. +* [Layout](bestpractices/layout.md): How to use Views layout. * [Ownership](bestpractices/ownership.md): How to manage Views object lifetimes. # Architecture Overview diff --git a/chromium/docs/ui/navbar.md b/chromium/docs/ui/navbar.md index 9d52a47fa01..b1d6a779ec6 100644 --- a/chromium/docs/ui/navbar.md +++ b/chromium/docs/ui/navbar.md @@ -2,11 +2,10 @@ * [Chromium Docs Home](/docs/README.md) * [Chromium UI](/docs/ui/index.md) -* [Aura](/docs/ui/aura/index.md) -* [Compositor](/docs/ui/compositor/index.md) -* [Views](/docs/ui/views/overview.md) +* [Create](/docs/ui/create/index.md) +* [Learn](/docs/ui/learn/index.md) +* [Ask](/docs/ui/ask/index.md) * [Android UI](/docs/ui/android/overview.md) -* [Product Excellence](/docs/ui/product_excellence/index.md) -* [UI Devtools](/docs/ui/ui_devtools/index.md) + [home]: /docs/ui/index.md diff --git a/chromium/docs/ui/views/overview.md b/chromium/docs/ui/views/overview.md index aa8e5e694b1..df801055fe4 100644 --- a/chromium/docs/ui/views/overview.md +++ b/chromium/docs/ui/views/overview.md @@ -72,9 +72,12 @@ by the border's insets. A View's **layout manager** defines how the View's child views should be laid out within the View's content bounds. There are a few layout managers supplied with Views. The simplest is [FillLayout], which lays out all a View's children -occupying the View's entire content bounds. Other commonly-used layouts managers -are [BoxLayout], which lays out all child views along an axis, and [GridLayout], -which provides a flexible row-and-column system. +occupying the View's entire content bounds. [FlexLayout] provides a CSS-like +layout for horizontal and vertical arrangements of views. + +Other commonly-used layouts managers are [BoxLayout], a predecessor of +FlexLayout, and [GridLayout], which provides a flexible row-and-column +system. ### Painting diff --git a/chromium/docs/updating_clang.md b/chromium/docs/updating_clang.md index 9210b5426ec..72fe848b9ce 100644 --- a/chromium/docs/updating_clang.md +++ b/chromium/docs/updating_clang.md @@ -9,50 +9,37 @@ An archive of all packages built so far is at https://is.gd/chromeclang 1. Check that https://ci.chromium.org/p/chromium/g/chromium.clang/console looks reasonably green. 1. Sync your Chromium tree to the latest revision to pick up any plugin - changes -1. Run `python tools/clang/scripts/upload_revision.py NNNN` - with the target LLVM SVN revision number. This creates a roll CL on a new - branch, uploads it and starts tryjobs that build the compiler binaries into - a staging bucket on Google Cloud Storage (GCS). -1. If the clang upload try bots succeed, copy the binaries from the staging - bucket to the production one. For example: + changes. +1. Run go/chrome-push-clang-to-goma. This takes a recent dry run CL to update + clang, and if the trybots were successful it will copy the binaries from + the staging bucket to the production one. Writing to this bucket requires + special permissions. File a bug at g.co/bugatrooper if you don't have these + already (e.g., https://crbug.com/1034081). Then it will push the packages + to goma. If you do not have the necessary credentials to do the upload, ask + clang@chromium.org to find someone who does. +1. Run an exhaustive set of try jobs to test the new compiler. The CL + description created previously by upload_revision.py includes + `Cq-Include-Trybots:` lines for all needed bots, so it's sufficient to just + run `git cl try` (or hit "CQ DRY RUN" on gerrit). +1. Commit the roll CL from the previous step. +1. The bots will now pull the prebuilt binary, and goma will have a matching + binary, too. - ```shell - $ export rev=n123456-abcd1234-1 - $ for x in Linux_x64 Mac Win ; do \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/clang-$rev.tgz \ - gs://chromium-browser-clang/$x/clang-$rev.tgz ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/clang-$rev-buildlog.txt \ - gs://chromium-browser-clang/$x/clang-$rev-buildlog.txt ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/clang-tidy-$rev.tgz \ - gs://chromium-browser-clang/$x/clang-tidy-$rev.tgz ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/llvmobjdump-$rev.tgz \ - gs://chromium-browser-clang/$x/llvmobjdump-$rev.tgz ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/translation_unit-$rev.tgz \ - gs://chromium-browser-clang/$x/translation_unit-$rev.tgz ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/llvm-code-coverage-$rev.tgz \ - gs://chromium-browser-clang/$x/llvm-code-coverage-$rev.tgz ; \ - gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/$x/libclang-$rev.tgz \ - gs://chromium-browser-clang/$x/libclang-$rev.tgz ; \ - done && gsutil.py cp -n -a public-read gs://chromium-browser-clang-staging/Mac/lld-$rev.tgz \ - gs://chromium-browser-clang/Mac/lld-$rev.tgz - ``` +## Performance regressions - **Note** that writing to this bucket requires special permissions. File a - bug at g.co/bugatrooper if you don't have these already (e.g., - https://crbug.com/1034081). +After doing a clang roll, you may get a performance bug assigned to you +([example](https://crbug.com/1094671)). Some performance noise is expected +while doing a clang roll. -1. Run the goma package update script to push these packages to goma. If you do - not have the necessary credentials to do the upload, ask clang@chromium.org - to find someone who does -1. Run an exhaustive set of try jobs to test the new compiler. The CL - description created by upload_revision.py includes `Cq-Include-Trybots:` - lines for all needed bots, so it's sufficient to just run `git cl try` - (or hit "CQ DRY RUN" on gerrit). +You can check all performance data for a clang roll via +`https://chromeperf.appspot.com/group_report?rev=XXXXXX`, where `XXXXXX` is the +revision number, e.g. `778090` for the example bug (look in the first message +of the performance bug to find this). Click the checkboxes to display graphs. +Hover over points in the graph to see the value and error. -1. Commit roll CL from the first step -1. The bots will now pull the prebuilt binary, and goma will have a matching - binary, too. +Serious regressions require bisecting upstream commits (TODO: how to repro?). +If the regressions look insignificant and there is green as well as red, you +can close the bug as "WontFix" with an explanation. ## Adding files to the clang package diff --git a/chromium/docs/vscode.md b/chromium/docs/vscode.md index 735e6a13dd4..0e59e055524 100644 --- a/chromium/docs/vscode.md +++ b/chromium/docs/vscode.md @@ -122,8 +122,8 @@ every day: enabling it to provide smarter autocomplete than C/C++ IntelliSense as well as allowing you to jump from functions to their definitions. See [clangd.md](clangd.md) for setup instructions. - If you need to debug, disable the vscode-clangd extension, enable C/C++ - Intellisense, and restart VSCode. + If you need to debug, enable C/C++ extension but set "C_Cpp: Intelli Sense Engine" to disabled, + and restart VSCode. * ***Rewrap*** - Wrap lines at 80 characters with `Alt+Q`. * ***Remote*** - @@ -274,8 +274,9 @@ Launch commands are the equivalent of `F5` in Visual Studio: They launch some program or a debugger. Optionally, they can run some task defined in `tasks.json`. Launch commands can be run from the debug view (`Ctrl+Shift+D`). Open the file at [//tools/vscode/launch.json5](/tools/vscode/launch.json5) and -adjust the example launch commands to your situation and needs. To use these -settings wholesale, enter the following command into your terminal: +adjust the example launch commands to your situation and needs (e.g., the value +of "type" needs adjustment for Windows). To use these settings wholesale, enter +the following command into your terminal: ``` $ cp tools/vscode/launch.json5 .vscode/launch.json ``` @@ -329,6 +330,12 @@ to the listed hostname. It has you choose a folder - use the 'src' folder root. This will open a new VSCode window in 'Remote' mode. ***Now you can install extensions specifically for your remote connection, like vscode-clangd, etc.*** +#### Chromebooks + +For Googlers, [here](http://go/vscode/remote_development_via_web) are +Google-specific instructions for setting up remote development on chromebooks +without using Crostini. + #### Windows & SSH This currently is difficult on Windows because VSCode remote tools assumes 'sshd' is installed, which isn't the case on Windows. If someone figures out diff --git a/chromium/docs/webui_explainer.md b/chromium/docs/webui_explainer.md index 75d46bf45a5..e3dc1a0a521 100644 --- a/chromium/docs/webui_explainer.md +++ b/chromium/docs/webui_explainer.md @@ -583,7 +583,7 @@ WebUI listeners are a convenient way for C++ to inform JavaScript of events. Older WebUI code exposed public methods for event notification, similar to how responses to [chrome.send()](#chrome_send) used to work. They both -resulted in global namespace polution, but it was additionally hard to stop +resulted in global namespace pollution, but it was additionally hard to stop listening for events in some cases. **cr.addWebUIListener** is preferred in new code. diff --git a/chromium/docs/windows_build_instructions.md b/chromium/docs/windows_build_instructions.md index 1e6628586f9..9017a59b51d 100644 --- a/chromium/docs/windows_build_instructions.md +++ b/chromium/docs/windows_build_instructions.md @@ -90,6 +90,11 @@ Also, add a DEPOT_TOOLS_WIN_TOOLCHAIN system variable in the same way, and set it to 0. This tells depot_tools to use your locally installed version of Visual Studio (by default, depot_tools will try to use a google-internal version). +You may also have to set variable `vs2017_install` or `vs2019_install` to your +installation path of Visual Studio 2017 or 19, like +`set vs2019_install=C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional` +for Visual Studio 2019. + From a cmd.exe shell, run the command gclient (without arguments). On first run, gclient will install all the Windows-specific bits needed to work with the code, including msysgit and python. diff --git a/chromium/docs/workflow/debugging-with-swarming.md b/chromium/docs/workflow/debugging-with-swarming.md index ddaf475cdae..553c54aac1c 100644 --- a/chromium/docs/workflow/debugging-with-swarming.md +++ b/chromium/docs/workflow/debugging-with-swarming.md @@ -70,11 +70,13 @@ $ tools/run-swarmed.py $outdir $target ``` See the `--help` option of `run-swarmed.py` for more details about that script. +Note you might need `--swarming-os Ubuntu-14.04` if you get an error like, +`UnboundLocalError: local variable 'dbus_pid' referenced before assignment`. ### mb.py run -Similar to `tools/run_swarmed.py`, `mb.py run` bundles much of the logic into a -single command line. Unlike `tools/run_swarmed.py`, `mb.py run` allows the user +Similar to `tools/run-swarmed.py`, `mb.py run` bundles much of the logic into a +single command line. Unlike `tools/run-swarmed.py`, `mb.py run` allows the user to specify extra arguments to pass to the test, but has a messier command line. To use it, run: |
