[PDF Ink Signatures] Add state management for text annotations [chromium/src : main]
Andy Phan (Gerrit)
Andy Phan added 5 comments![]( "Open in Gerrit")
export interface TextStyles {Will this be used by multiple files? I think it's only being used in one file and can be moved there.
// TODO: Replace this with a real call to the plugin, once the backend hasNit: add a bug number.
chrome.test.assertEq(expected.type, brushUpdates[index]!.type);Store as a variable and assert this so we don't have to use `!` everytime? Same with line 109.
// Test doesn't currently use eraser, so assert the brush has a color.Should we test eraser?
manager.setTextColor({r: 0, b: 100, g: 0});Nit: create a `const` to be consistent with the test above. Same with line 159.
Related details
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 5 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Commit-Queue | +1 |
5 comments
export interface TextStyles {Will this be used by multiple files? I think it's only being used in one file and can be moved there.
We can't move only this interface because AnnotationText uses it, and importing the interface back into this file from the manager would create a circular dependency.
AnnotationText itself is indeed only used in the manager in this CL, but will be needed in the next CL in the chain in the new text side panel element. Seems like a lot of churn to put all of these 3 types in the manager for 1 CL only to revert back in its child. Do you want me to combine the 2 CLs instead so that it's clear where this is used (along with the new manager APIs, which are also unused in prod code in this CL)?
// TODO: Replace this with a real call to the plugin, once the backend hasNit: add a bug number.
Done
chrome.test.assertEq(expected.type, brushUpdates[index]!.type);Store as a variable and assert this so we don't have to use `!` everytime? Same with line 109.
`!` is fine to use for array elements as long as the length of the array has previously been asserted, which is done on line 52. This is just an issue with TS compiler not realizing that the assertion on line 52 means that this can't be undefined.
Asserting the array length is at least `N` and then using `!` for `array[index]` for `index < N` is standard practice for WebUI tests.
// Test doesn't currently use eraser, so assert the brush has a color.Should we test eraser?
From the perspective of the manager these two are handled the same way, no? It's testing the setBrushType() API and that it correctly requests the brush from the plugin and then fires an event with that updated brush. There's no difference in handling in Ink2Manager for eraser vs highlighter. Obviously there is a difference in handling in the UI elements, which handle the eraser case differently, but this is a unit test for the manager itself; UI element behavior should be tested in dedicated tests for those UI elements.
More broadly I don't think we need to test every value that can be set using these APIs. Testing each once is fine, since they handle all values in the same way.
Updated this to use an if() so that we avoid the comment, and if you'd prefer to test the eraser case instead of the highlighter we can do that easily. LMK if that's your preference.
Nit: create a `const` to be consistent with the test above. Same with line 159.
- Andy Phan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan voted Code-Review+1![]( "Open in Gerrit")
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 3 comments![]( "Open in Gerrit")
export interface TextStyles {Rebekah PotterWill this be used by multiple files? I think it's only being used in one file and can be moved there.
We can't move only this interface because AnnotationText uses it, and importing the interface back into this file from the manager would create a circular dependency.
AnnotationText itself is indeed only used in the manager in this CL, but will be needed in the next CL in the chain in the new text side panel element. Seems like a lot of churn to put all of these 3 types in the manager for 1 CL only to revert back in its child. Do you want me to combine the 2 CLs instead so that it's clear where this is used (along with the new manager APIs, which are also unused in prod code in this CL)?
AnnotationText uses it, ...
Ahh, right. It's fine to leave as-is.
chrome.test.assertEq(expected.type, brushUpdates[index]!.type);Rebekah PotterStore as a variable and assert this so we don't have to use `!` everytime? Same with line 109.
`!` is fine to use for array elements as long as the length of the array has previously been asserted, which is done on line 52. This is just an issue with TS compiler not realizing that the assertion on line 52 means that this can't be undefined.
Asserting the array length is at least `N` and then using `!` for `array[index]` for `index < N` is standard practice for WebUI tests.
Ack, thanks.
// Test doesn't currently use eraser, so assert the brush has a color.Rebekah PotterShould we test eraser?
From the perspective of the manager these two are handled the same way, no? It's testing the setBrushType() API and that it correctly requests the brush from the plugin and then fires an event with that updated brush. There's no difference in handling in Ink2Manager for eraser vs highlighter. Obviously there is a difference in handling in the UI elements, which handle the eraser case differently, but this is a unit test for the manager itself; UI element behavior should be tested in dedicated tests for those UI elements.
More broadly I don't think we need to test every value that can be set using these APIs. Testing each once is fine, since they handle all values in the same way.
Updated this to use an if() so that we avoid the comment, and if you'd prefer to test the eraser case instead of the highlighter we can do that easily. LMK if that's your preference.
Ack. That makes sense. How it is now is fine.
Related details
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PDF Ink Signatures] Add state management for text annotations
Add methods to set and manage text annotations to the Ink2Manager.
Add a test suite for the class for both existing code and the new
additions.
Note that since backend/frontend communication is still TBD for text
annotations, the manager currently just logs to console.info, instead
of notifying the plugin about changes to text properties.
- M chrome/browser/pdf/pdf_extension_js_test.cc
- M chrome/browser/resources/pdf/constants.ts
- M chrome/browser/resources/pdf/ink2_manager.ts
- M chrome/browser/resources/pdf/pdf_viewer_wrapper.ts
- M chrome/test/data/pdf/BUILD.gn
- A chrome/test/data/pdf/ink2_manager_test.ts
Code-Review: +1 by Andy Phan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
const blue = {r: 0, b: 100, g: 0};I have a CL that fixes this. RBG is confusing.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |