[contextual tasks] Add voice metrics for contextual tasks [chromium/src : main]
James Leung (Gerrit)
James Leung added 1 comment![]( "Open in Gerrit")
Related details
- Ananya Seelam
- Duncan Mercer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Leung (Gerrit)
James Leung added 1 comment![]( "Open in Gerrit")
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->did i do the linting right?
Related details
- Ananya Seelam
- Duncan Mercer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Ananya Seelam (Gerrit)
Ananya Seelam added 12 comments![]( "Open in Gerrit")
Hi James! Great work! Left some comments below.
Please summarize metrics here for future readers of this CL.
// Safety return statement in rare case chrome metrics is not availablenit: add period.
```suggestion
// Safety return statement in rare case chrome metrics is not available.
```
/* Done with transcribing once there is one isFinal.nit: add backticks to code in comments.
```suggestion
/* Done with transcribing once there is one `isFinal`.
```
* take the specific result marked with resultIndex.nit: add backticks
```suggestion
* take the specific result marked with `resultIndex`.
```
<int value="0" label="VoiceSearchButtonClicked"/>
<int value="1" label="VoiceSearchTranscriptionSuccess"/>
<int value="2" label="VoiceSearchError"/>
<int value="3" label="VoiceSearchErrorAndCanceled"/>
<int value="4" label="VoiceSearchUserCanceled"/>The label values don't have to be CamelCase here. See enums above.
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->did i do the linting right?
I believe you need to make the names of both enums (in the constants file and in the enums file) the same. The lint directive is expecting `VoiceSearchState`.
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /chrome/...
Changes in the preceding block may need to be reflected in these files: /chrome/browser/contextual_tasks/constants.ts If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Tracks user interactions with the voice search feature on the NTP.Contextual Tasks? This is not for NTP right?
// If was an error that canceled voice search.nit:
```suggestion
// An error that canceled voice search.
```
// If was error that did not cancel voice search.nit:
```suggestion
// An error that did not cancel voice search.
```
this.fire('voice-search-cancel', /*user-canceled=*/ false);is there a difference between `user-canceled` and `canceled`? If not, keep the names consistent here and below.
Related details
- Duncan Mercer
- James Leung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Leung (Gerrit)
James Leung added 11 comments![]( "Open in Gerrit")
Please summarize metrics here for future readers of this CL.
Done
// Safety return statement in rare case chrome metrics is not availablenit: add period.
```suggestion
// Safety return statement in rare case chrome metrics is not available.
```
Done
nit: add backticks to code in comments.
```suggestion
/* Done with transcribing once there is one `isFinal`.
```
Done
nit: add backticks
```suggestion
* take the specific result marked with `resultIndex`.
```
Done
<int value="0" label="VoiceSearchButtonClicked"/>
<int value="1" label="VoiceSearchTranscriptionSuccess"/>
<int value="2" label="VoiceSearchError"/>
<int value="3" label="VoiceSearchErrorAndCanceled"/>
<int value="4" label="VoiceSearchUserCanceled"/>The label values don't have to be CamelCase here. See enums above.
Done
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /chrome/...
Changes in the preceding block may need to be reflected in these files: /chrome/browser/contextual_tasks/constants.ts If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Done
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->Ananya Seelamdid i do the linting right?
I believe you need to make the names of both enums (in the constants file and in the enums file) the same. The lint directive is expecting `VoiceSearchState`.
Done
Tracks user interactions with the voice search feature on the NTP.Contextual Tasks? This is not for NTP right?
Done
nit:
```suggestion
// An error that canceled voice search.
```
Done
nit:
```suggestion
// An error that did not cancel voice search.
```
Done
this.fire('voice-search-cancel', /*user-canceled=*/ false);is there a difference between `user-canceled` and `canceled`? If not, keep the names consistent here and below.
yes, the difference is that user canceled is canceled by user while canceled is canceled by error
Related details
- Ananya Seelam
- Duncan Mercer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ananya Seelam (Gerrit)
Ananya Seelam added 1 comment![]( "Open in Gerrit")
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->I would add the `NO_IFTTT=some reason...'` to your cl description you can say this is a false positive since VoiceSearchState name matches.
Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /chrome/...
Changes in the preceding block may need to be reflected in these files: /chrome/browser/contextual_tasks/constants.ts If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
- Duncan Mercer
- James Leung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Leung (Gerrit)
James Leung added 1 comment![]( "Open in Gerrit")
<!-- LINT.ThenChange(//chrome/browser/contextual_tasks/constants.ts:VoiceSearchState) -->I would add the `NO_IFTTT=some reason...'` to your cl description you can say this is a false positive since VoiceSearchState name matches.
Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /chrome/...Changes in the preceding block may need to be reflected in these files: /chrome/browser/contextual_tasks/constants.ts If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
- Ananya Seelam
- Duncan Mercer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ananya Seelam (Gerrit)
Ananya Seelam voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ananya Seelam
| Code-Review | +1 |
1 comment
Related details
- Duncan Mercer
- James Leung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist added 3 comments![]( "Open in Gerrit")
SUCCESFUL_TRANSCRIPT = 1,I think you want `SUCCESSFUL_TRANSCRIPT` here, not `SUCCESFUL_TRANSCRIPT` (and remember to update uses in case that doesn't fail compile automatically).
test('on focus out sets animation state as none otherwise', async () => {Nit: `otherwise`.
this.fire('voice-search-cancel', /*canceled-by-user=*/ false);Why do we emit both `-cancel` and `-error` again? Is it because `cancel` closes the modal, and `error` handles the logging?
Could we maybe update the comment above to say that "we therefore emit `voice-search-cancel` before the `voice-search-error`" or something along those lines if that's why we fire an event twice? The comments surrounding this seem to focus on whether metrics are recorded or not, which makes sense in line of the current CL, but for future readers I don't necessarily think they tell the whole story.
Related details
- Ananya Seelam
- Duncan Mercer
- James Leung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Leung (Gerrit)
James Leung added 3 comments![]( "Open in Gerrit")
SUCCESFUL_TRANSCRIPT = 1,I think you want `SUCCESSFUL_TRANSCRIPT` here, not `SUCCESFUL_TRANSCRIPT` (and remember to update uses in case that doesn't fail compile automatically).
Done
test('on focus out sets animation state as none otherwise', async () => {James LeungNit: `otherwise`.
Done
this.fire('voice-search-cancel', /*canceled-by-user=*/ false);Why do we emit both `-cancel` and `-error` again? Is it because `cancel` closes the modal, and `error` handles the logging?
Could we maybe update the comment above to say that "we therefore emit `voice-search-cancel` before the `voice-search-error`" or something along those lines if that's why we fire an event twice? The comments surrounding this seem to focus on whether metrics are recorded or not, which makes sense in line of the current CL, but for future readers I don't necessarily think they tell the whole story.
yes, cancel is an event that was previously here that closes voice search
done.
Related details
- Ananya Seelam
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |