[Webkit-unassigned] [Bug 177993] Add UI for A/B testing on owned commits.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 15 00:58:13 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=177993
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #328791|review? |review-
Flags| |
--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 328791
--> https://bugs.webkit.org/attachment.cgi?id=328791
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=328791&action=review
I think we need to split the changes to CommitLog and the addition of IntermediateCommitSet into a separate patch to make this patch manageable.
> Websites/perf.webkit.org/ChangeLog:8
> + Customizable test group form should support specifying and A/B testing owned commits.
You should add a high description of what you're adding & what you're changing.
> Websites/perf.webkit.org/ChangeLog:23
> + (CustomizableTestGroupForm):
You should really explain why you're replacing dictionaries with map.
In theory, there should be a change log description for each code change you're making.
> Websites/perf.webkit.org/ChangeLog:61
> + (IntermediateCommitSet): In order to support owned commits, commit set should support mutation as we may add/remove commits for a repository.
> + This variant of commit set supports remove/update commits for repositories.
You should explain the difference between IntermediateCommitSet and CustomCommitSet.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:3
> + constructor(defaultValue, didUpdateValue, candidates)
This argument order seems backwards. It should take the list of candidates first and then the default value.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:7
> + this._didUpdateValue = didUpdateValue;
Don't use a callback in new code. Use dispatchAction & listenToAction combo instead.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:13
> + this._selectedItemValue = '';
Why isn't this initialized to null instead?
> Websites/perf.webkit.org/public/v3/components/combo-box.js:18
> + _createCandidateList(candidates, keyword)
I would call "keyword" just "key".
> Websites/perf.webkit.org/public/v3/components/combo-box.js:22
> + const startsWithKeywordList = [];
"startsWithKeyword" list is an awkward name. How about candidatesStartingWithKey?
> Websites/perf.webkit.org/public/v3/components/combo-box.js:23
> + const containsWithKeywordList = [];
candidatesContainingKey? contains with keyword doesn't make much sense as a phrase.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:24
> + for (const candidate of candidates) {
You should add some sort of a cap so that we don't end up showing too many results (might be slow to build a large DOM like that).
> Websites/perf.webkit.org/public/v3/components/combo-box.js:29
> + if (keyword && !candidateInLowerCase.includes(keyword.toLowerCase()))
> + continue;
> +
> + if (candidateInLowerCase.startsWith(keyword.toLowerCase()))
Instead of calling includes & startsWith and thereby searching each string twice, just call indexOf.
If the result is -1, it doesn't contain the keyword. If it's 0, then it starts with the keyword.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:38
> + item.addEventListener('mousedown', () => {
Why are you creating link and then manually attaching an event listener?
Just use link(candidate, () => ~).
> Websites/perf.webkit.org/public/v3/components/combo-box.js:43
> + this._elementByCandidateName.set(candidate, item);
Why do we need a map from the candidate to its item like this?
Since each candidate generates exactly one element in your design,
you can just create an array and use _currentCandidateIndex to access the right element.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:73
> + if(event.key === 'Tab' || event.key === 'Enter') {
Nit: Need a space between if and (. Why tab? Why is that special?
> Websites/perf.webkit.org/public/v3/components/combo-box.js:95
> + if (!this._inputValue.length)
This function should check the value of _currentCandidateIndex.
If it's not null, then the user had already picked a candidate so there is no need to do the work here.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:97
> + const matchingCandidates = this._candidates.filter((candidate) => candidate.toLowerCase().includes(this._inputValue.toLowerCase()));
Why don't we find the first two matching candidates instead of finding all of them?
> Websites/perf.webkit.org/public/v3/components/combo-box.js:119
> + if (candidateElement) {
> + if (previousElement)
> + previousElement.className = null;
> + candidateElement.className = 'selected';
This should be a enqueueToRender call instead.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:130
> + textField.value = this._inputValue;
You shouldn't always re-set the input element's value like this.
That would reset the caret position each time, which would be annoying user experience.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:140
> + <input id='text-field'/>
Don't use self-closing syntax in HTML5.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:153
> + }
> + ul:empty {
Please add a blank line between each CSS rule.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:161
> + padding-left: 0.3rem;
> + padding-right: 0.3rem;
> + padding-bottom: 0.2rem;
Use the short hand: padding: 0.1rem 0.3rem.
This would expand to padding-left/right: 0.3rem and padding-top/bottom: 0.1rem.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:168
> + margin-top: -0.1rem;
Use 0.1rem padding for top-bottom instead of using a negative margin like this.
> Websites/perf.webkit.org/public/v3/components/combo-box.js:185
> + li a:hover, a.selected {
You're missing li for the second selector.
li a:hover, a.selected expands to "li a:hover" and "a.selected", not "li a.selected".
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
> +function ensureSetForMap(map, key) {
I don't think we need this helper. It's only used once.
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:23
> + for (const label in map)
Do: for (const label of map.keys()) instead. It's more canonical & descriptive way of iterating over keys of a map.
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:74
> - this._renderCustomRevisionTableLazily.evaluate(this._commitSetMap, this._isCustomized);
> + this._renderCustomRevisionTable(this._commitSetMap, this._isCustomized);
This would force the reconstruction of table in each render call. That's not okay.
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:118
> + const ownsCommits = [...commitSetMap.values()].map((commitSet) => commitSet.ownsCommitsForRepository(repository)).every((value) => value);
Why isn't this just a call to every? map(f).every((x) => x) is equivalent to every(f).
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:125
> + const lastRow = index === ownedRepositories.size - 1 && incompleteRowCount === 0;
Don't compare against 0: https://webkit.org/code-style-guidelines/#zero-comparison
> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:129
> + if (incompleteRowCount === 0)
Ditto.
> Websites/perf.webkit.org/public/v3/models/commit-log.js:59
> - if (this == previousCommit)
> + if (this === previousCommit)
Why ===? This change should be explained in the change log.
> Websites/perf.webkit.org/public/v3/models/commit-log.js:71
> + else if (to.length === 40) // e.g. git hash
What's the point of using ===? There's no difference.
> Websites/perf.webkit.org/public/v3/models/commit-log.js:113
> + this._ownedCommits.forEach((ownedCommit) => {
> + ownedCommit.setOwnerCommits(this);
> + this._ownedCommitByOwnedRepository.set(ownedCommit.repository(), ownedCommit);
> + });
We should add tests to verify these changes are working as intended.
> Websites/perf.webkit.org/public/v3/models/commit-log.js:136
> + const ownedCommitRepositories = new Set([].concat(
> + ...ownedCommitMapList.map((ownedCommitMap) => Array.from(ownedCommitMap.keys()))));
All these constructs to concat arrays with spread just to flatten nested arrays is really hard to read.
Just write two nested loops.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:80
> +
Nit: Trailing whitespace.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:256
> +function ensureSetForMap(map, key) {
I don't think we need this helper. It's only used once and it's a simple code.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:267
> +class IntermediateCommitSet {
> +
> + constructor(commitSet)
> + {
> + this._commitByRepository = new Map;
> + this._ownedRepositoriesByOwnerRepository = new Map;
Not only do IntermediateCommitSet store commits instead of revision number, it also stores owned repositories for each commit?
That's a significant difference worth mentioning in the change log.
Also, IntermediateCommitSet is too vague of a name.
Someone who isn't familiar with this code can't tell the difference between CustomCommitSet and IntermediateCommitSet.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171215/8b1adc6a/attachment-0001.html>
More information about the webkit-unassigned
mailing list