Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693503002:
Add documentation for Origin Trials integration (Closed)
DescriptionAdd documentation for Origin Trials integration
This CL adds documentation for feature authors to integrate with the
origin trials framework.
This moves content that was previously hosted in GitHub:
https://github.com/jpchase/OriginTrials/blob/gh-pages/developer-guide.md
Also updated to reflect recent code changes.
Review-Url: https://codereview.chromium.org/2693503002
Cr-Commit-Position: refs/heads/master@{#449722}
Committed: https://chromium.googlesource.com/chromium/src/+/c74a4c9c6bbef16a70750cbf2662b4ad5a9541d9
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address nits #Messages
Total messages: 14 (9 generated)
The CQ bit was checked by chasej@chromium.org to run a CQ dry run
chasej@chromium.org changed reviewers: + iclelland@chromium.org
iclelland, please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits, thanks for moving this! https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... File docs/origin_trials_integration.md (right): https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:24: platforms will allow the trial to be enabled. Can we list the valid values for that list here? "android" and "linux" are easy (modulo case-sensitivity), but I can never remember whether its "mac" or "macos" or "osx". Is "win7" a thing, or just "windows"? etc. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:84: - Find the relevant methods by doing a code search for `installMyFeature`: I didn't realise this was best practise :) I suppose you could do a more targeted search if you include 'f:gen/blink/bindings' as a term as well. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:131: [/tools/origin_trials/generate_token.py]. You can generate signed tokens for Why is this reference using the complete path as its text, when the others are all just the filename? https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:141: this key will **not** work in the browser by default; see the [Developer Guide] We could mention that they *do* work in content shell, just not in Chrome. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:149: This is the public key associated with `eftest.key`. If it doesn't work, see ...is the base64 encoding of the public key...
Thanks, addressed the nits. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... File docs/origin_trials_integration.md (right): https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:24: platforms will allow the trial to be enabled. On 2017/02/10 19:41:28, iclelland wrote: > Can we list the valid values for that list here? "android" and "linux" are easy > (modulo case-sensitivity), but I can never remember whether its "mac" or "macos" > or "osx". Is "win7" a thing, or just "windows"? etc. I intentionally omitted the list of valid values. If the supported values change, the odds of someone updating this doc are approximately zero. Instead, I added a link to the file where the corresponding macros are defined. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:84: - Find the relevant methods by doing a code search for `installMyFeature`: On 2017/02/10 19:41:27, iclelland wrote: > I didn't realise this was best practise :) I suppose you could do a more > targeted search if you include 'f:gen/blink/bindings' as a term as well. You could do that. I had tested with a few examples (e.g. installWebUSB) and the results were just the generated bindings. I suppose it's always possible that could be other, hand-written code for "installMyFeature". Added more explicit guidance. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:131: [/tools/origin_trials/generate_token.py]. You can generate signed tokens for On 2017/02/10 19:41:27, iclelland wrote: > Why is this reference using the complete path as its text, when the others are > all just the filename? The source document had the complete path as its text, so I kept it the same. That, and I thought it made sense to highlight that it's a tool, not some other source file. That said, the following example has the full path, so probably don't need it in the text too. Removed the path. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:141: this key will **not** work in the browser by default; see the [Developer Guide] On 2017/02/10 19:41:27, iclelland wrote: > We could mention that they *do* work in content shell, just not in Chrome. Done. https://codereview.chromium.org/2693503002/diff/1/docs/origin_trials_integrat... docs/origin_trials_integration.md:149: This is the public key associated with `eftest.key`. If it doesn't work, see On 2017/02/10 19:41:28, iclelland wrote: > ...is the base64 encoding of the public key... Done.
The CQ bit was checked by chasej@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2693503002/#ps20001 (title: "Address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486758296497040,
"parent_rev": "b52e3c3e96944729c7f6a02b42f5c1b6d01dee24", "commit_rev":
"c74a4c9c6bbef16a70750cbf2662b4ad5a9541d9"}
Message was sent while issue was closed.
Description was changed from ========== Add documentation for Origin Trials integration This CL adds documentation for feature authors to integrate with the origin trials framework. This moves content that was previously hosted in GitHub: https://github.com/jpchase/OriginTrials/blob/gh-pages/developer-guide.md Also updated to reflect recent code changes. ========== to ========== Add documentation for Origin Trials integration This CL adds documentation for feature authors to integrate with the origin trials framework. This moves content that was previously hosted in GitHub: https://github.com/jpchase/OriginTrials/blob/gh-pages/developer-guide.md Also updated to reflect recent code changes. Review-Url: https://codereview.chromium.org/2693503002 Cr-Commit-Position: refs/heads/master@{#449722} Committed: https://chromium.googlesource.com/chromium/src/+/c74a4c9c6bbef16a70750cbf2662... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c74a4c9c6bbef16a70750cbf2662... |
