[composebox] Enable pasting images in composebox [chromium/src : main]
Nihar Majmudar (Gerrit)
Nihar Majmudar added 4 comments![]( "Open in Gerrit")
Hey Wenhu left some initial comments. In general I think you should be delegating most of the file handling flow to the process that exists. Looks like you are creating a lot of logic here yourself. Also please add tests to [composebox_test.ts](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/webui/new_tab_page/composebox/composebox_test.ts?q=composebox_test.ts) for this
if (!fileToProcess.name) {
const type = fileToProcess.type.split('/')[1] || 'bin';
Object.defineProperty(fileToProcess, 'name', {
value: `Pasted_File_${Date.now()}.${type}`,
});
}Why are we doing this?
get currentFileCount(): number {
return this.files_.size;
}
get maxFileCount(): number {
return this.maxFileCount_;
}You don't use these anywhere, can get rid of this (also have access to these variable in this scope)
private validateFile_(file: File, showErrorUI: boolean): boolean {
if (file.size === 0 || file.size > this.maxFileSize_) {
const fileIsEmpty = file.size === 0;
if (showErrorUI) {
this.fire('on-file-validation-error', {
errorMessage: fileIsEmpty ?
this.i18n('composeboxFileUploadInvalidEmptySize') :
this.i18n('composeboxFileUploadInvalidTooLarge'),
});
}
this.recordFileValidationMetric_(
fileIsEmpty ? ComposeboxFileValidationError.FILE_EMPTY :
ComposeboxFileValidationError.FILE_SIZE_TOO_LARGE);
return false;
}
const isImage = file.type.startsWith('image/');
const allowedImageTypes = this.imageFileTypes_.split(',');
const allowedAttachmentTypes = this.attachmentFileTypes_.split(',');
let isValidType = false;
if (isImage) {
isValidType = allowedImageTypes.some(type => {
const trimmedType = type.trim();
if (trimmedType.endsWith('/*')) {
return file.type.startsWith(trimmedType.slice(0, -1));
} else {
return file.type === trimmedType;
}
});
} else {
isValidType = allowedAttachmentTypes.some(type => {
const trimmedType = type.trim();
if (trimmedType.startsWith('.')) {
return file.name.endsWith(trimmedType);
} else if (trimmedType.endsWith('/*')) {
return file.type.startsWith(trimmedType.slice(0, -1));
} else {
return file.type === trimmedType;
}
});
}
if (!isValidType) {
if (showErrorUI) {
this.fire('on-file-validation-error', {
errorMessage: this.i18n('composeboxFileUploadInvalidType'),
});
}
this.recordFileValidationMetric_(ComposeboxFileValidationError.INVALID_TYPE);
return false;
}Can't this all be handled by `addFileContext`? You might need to check if the max file count is already reached (but it looks like you already handle that in `composebox.ts`, but you shouldn't be handling validating the file and such
Related details
- Ananya Seelam
- WENHU CHENG
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. |
Ananya Seelam (Gerrit)
Ananya Seelam added 2 comments![]( "Open in Gerrit")
// Pasting multiple files is not supported. Do nothing.In realbox next, you should be able to upload multiple files at once so this case should not be skipped.
private fireAddFileContextEvent_(filesToUpload: File[], isImage: boolean) {Can you modify the `addFileContext_` method in this file and remove this method? That way we can reduce code duplication. Thanks!
Related details
- WENHU CHENG
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. |
WENHU CHENG (Gerrit)
WENHU CHENG added 5 comments![]( "Open in Gerrit")
if (!fileToProcess.name) {
const type = fileToProcess.type.split('/')[1] || 'bin';
Object.defineProperty(fileToProcess, 'name', {
value: `Pasted_File_${Date.now()}.${type}`,
});
}Why are we doing this?
deleted now , but do we need to handle the egde case if users uploads a nameless file?
// Pasting multiple files is not supported. Do nothing.In realbox next, you should be able to upload multiple files at once so this case should not be skipped.
Got it! now the maximum number of pasted files is controlled by maxFileCount_
get currentFileCount(): number {
return this.files_.size;
}
get maxFileCount(): number {
return this.maxFileCount_;
}You don't use these anywhere, can get rid of this (also have access to these variable in this scope)
Done
Can't this all be handled by `addFileContext`? You might need to check if the max file count is already reached (but it looks like you already handle that in `composebox.ts`, but you shouldn't be handling validating the file and such
Done
private fireAddFileContextEvent_(filesToUpload: File[], isImage: boolean) {Can you modify the `addFileContext_` method in this file and remove this method? That way we can reduce code duplication. Thanks!
- Ananya Seelam
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 5 comments![]( "Open in Gerrit")
event.preventDefault();
event.stopPropagation();Is this needed?
protected processFiles_(files: FileList|File[]|null, isImage: boolean = false) {You shouldn't be changing this data type, FileList is presumably already a wrapper for a `File[]`. Instead make the data you pass in a FileList.
addPastedFiles(files: File[]): void {
const isImage = files.some(f => f.type.startsWith('image/'));
this.processFiles_(files, isImage);
}Can we do something like what Dhruv did [here](https://chromium-review.googlesource.com/c/chromium/src/+/7108299/5/ui/webui/resources/cr_components/composebox/contextual_entrypoint_and_carousel.ts) and just take advtnage of the existing addFiles logic.
Related details
- Ananya Seelam
- WENHU CHENG
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. |
WENHU CHENG (Gerrit)
WENHU CHENG added 5 comments![]( "Open in Gerrit")
Done
event.preventDefault();
event.stopPropagation();WENHU CHENGIs this needed?
delete the event.stopPropagation();, but event.preventDefault() is still needed to prevent the name of the file is automatically pasted in the composebox?
INVALID_TYPE = 4,WENHU CHENGIs this used anywhere?
it was used when i try to handle the error by my own code, but now it is no longer needed because i would just use the existing function to handle it.
protected processFiles_(files: FileList|File[]|null, isImage: boolean = false) {You shouldn't be changing this data type, FileList is presumably already a wrapper for a `File[]`. Instead make the data you pass in a FileList.
Done
addPastedFiles(files: File[]): void {
const isImage = files.some(f => f.type.startsWith('image/'));
this.processFiles_(files, isImage);
}Can we do something like what Dhruv did [here](https://chromium-review.googlesource.com/c/chromium/src/+/7108299/5/ui/webui/resources/cr_components/composebox/contextual_entrypoint_and_carousel.ts) and just take advtnage of the existing addFiles logic.
- Ananya Seelam
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 7 comments![]( "Open in Gerrit")
Can you add a note in the CL that this `isImage` bug will be fixed in a subsequent CL
const errorEvent = await errorEventPromise;
img: assertEquals(Please run `git cl format --js`. The `img:` here seems out of place and same for the `H:` below.
Note: // Assert.Should this be in the comment?
const dataTransfer = new DataTransfer();
for (const file of pastedFiles) {
dataTransfer.items.add(file);
}
const fileList: FileList = dataTransfer.files;Can't this all be done in the for loop above, that way we don't have to loop through all the files twice. Then you can just check if the size of the fileList is > 0, you can call `addFiles`
if (pastedFiles.length > 0) {Isn't this already implicitly true from the check above `(pastedFiles.length === 0)`
const isImage =
!!files && Array.from(files).some(f => f.type.startsWith('image/'));Add TODO with bug number here to fix this.
Related details
- Ananya Seelam
- WENHU CHENG
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. |
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia added 1 comment![]( "Open in Gerrit")
if (!files || files.length === 0) {
return;
}
const incomingFileCount = files.length;
if ((this.files_.size + incomingFileCount) > this.maxFileCount_) {
this.recordFileValidationMetric_(
ComposeboxFileValidationError.TOO_MANY_FILES);
return;
}@nih...@google.com Should we change the File validation error metric here in the case of 0 files? I have filed a bug for it right now to add a separate metric category for no files. b:457494452
Related details
- Ananya Seelam
- WENHU CHENG
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. |
WENHU CHENG (Gerrit)
WENHU CHENG added 7 comments![]( "Open in Gerrit")
Can you add a note in the CL that this `isImage` bug will be fixed in a subsequent CL
Done
const errorEvent = await errorEventPromise;
img: assertEquals(Please run `git cl format --js`. The `img:` here seems out of place and same for the `H:` below.
Done
Should this be in the comment?
Done
const dataTransfer = new DataTransfer();
for (const file of pastedFiles) {
dataTransfer.items.add(file);
}
const fileList: FileList = dataTransfer.files;Can't this all be done in the for loop above, that way we don't have to loop through all the files twice. Then you can just check if the size of the fileList is > 0, you can call `addFiles`
Really good advice! Thank you !
Isn't this already implicitly true from the check above `(pastedFiles.length === 0)`
Done
const isImage =
!!files && Array.from(files).some(f => f.type.startsWith('image/'));Add TODO with bug number here to fix this.
Done
if (!files || files.length === 0) {
return;
}
const incomingFileCount = files.length;
if ((this.files_.size + incomingFileCount) > this.maxFileCount_) {
this.recordFileValidationMetric_(
ComposeboxFileValidationError.TOO_MANY_FILES);
return;
}@nih...@google.com Should we change the File validation error metric here in the case of 0 files? I have filed a bug for it right now to add a separate metric category for no files. b:457494452
- Ananya Seelam
- Dhruv Kathpalia
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar voted and added 7 comments![]( "Open in Gerrit")
Votes added by Nihar Majmudar
| Code-Review | +1 |
7 comments
loadTimeData.overrideValues({composeboxFileMaxCount: 5});Why is this needed here, you're not changing the test at all?
1 /* ComposeboxFileValidationError.TOO_MANY_FILES */));nit: can we use the enum here or no?
addFiles(files: FileList |null) {remove space
// TODO: crbug.com/457182498 - Update isImage logic to handle mixed file typesnit: TODO(crbug.com/457182498): Update isImage logic to handle mixed file types.
if (!files || files.length === 0) {
return;
}nit: format this correctly.
const incomingFileCount = files.length;nit: variable not needed since it's only used, can just use files.length below.
Related details
- Ananya Seelam
- Dhruv Kathpalia
- WENHU CHENG
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. |
WENHU CHENG (Gerrit)
WENHU CHENG added 6 comments![]( "Open in Gerrit")
loadTimeData.overrideValues({composeboxFileMaxCount: 5});Why is this needed here, you're not changing the test at all?
yeah i confirmed with Dhruv and Hanna, it turns out the original function of processed_files was flawed and I made it correct with my cl(which checking the incoming files too, originally it just checked the files that are already there), so we need to set composeboxFileMaxCount bigger than 5 here or it will fail the test.
1 /* ComposeboxFileValidationError.TOO_MANY_FILES */));nit: can we use the enum here or no?
const enum ComposeboxFileValidationError {
NONE = 0,
TOO_MANY_FILES = 1,
FILE_EMPTY = 2,
FILE_SIZE_TOO_LARGE = 3,
MAX_VALUE = FILE_SIZE_TOO_LARGE,
}from ui/webui/resources/cr_components/composebox/contextual_entrypoint_and_carousel.ts addFiles(files: FileList |null) {WENHU CHENGremove space
Done
// TODO: crbug.com/457182498 - Update isImage logic to handle mixed file typesnit: TODO(crbug.com/457182498): Update isImage logic to handle mixed file types.
Done
if (!files || files.length === 0) {
return;
}WENHU CHENGnit: format this correctly.
Done
nit: variable not needed since it's only used, can just use files.length below.
- Ananya Seelam
- Nihar Majmudar
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 2 comments![]( "Open in Gerrit")
Votes added by Ananya Seelam
| Code-Review | +1 |
2 comments
// TODO(crbug.com/457182498):update isImage logic to handle mixed file typesnit: add period to end of comments.
Related details
- Nihar Majmudar
- WENHU CHENG
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. |
WENHU CHENG (Gerrit)
WENHU CHENG added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/457182498):update isImage logic to handle mixed file typesnit: add period to end of comments.
- Ananya Seelam
- Nihar Majmudar
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 |
lgtm!
- Nihar Majmudar
- WENHU CHENG
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nihar Majmudar
| Code-Review | +1 |
1 comment
- WENHU CHENG
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[composebox] Enable pasting images in composebox
Pasting files (e.g., images, PDFs) into the composebox with Ctrl-V did
not work. This CL enables this feature.
A relevant CL to fix the bug about `processFiles` isImage parameter https://buganizer.corp.google.com/issues/457182498 will be uploaded subsequently
Screencasts:
Image paste -
https://screencast.googleplex.com/cast/NjQwNDM3MzI4MTI0MzEzNnw5ODY4NDFiMC1iZg
PDF paste -
https://screencast.googleplex.com/cast/NTM4NDg5OTgyODY0NTg4OHxmMzUxMDI5MS00NQ
- M chrome/test/data/webui/new_tab_page/composebox/composebox_test.ts
- M ui/webui/resources/cr_components/composebox/composebox.html.ts
- M ui/webui/resources/cr_components/composebox/composebox.ts
- M ui/webui/resources/cr_components/composebox/contextual_entrypoint_and_carousel.ts
Code-Review: +1 by Nihar Majmudar, +1 by Ananya Seelam
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |