FSO: Omit changedHandle for "disappear", "error", and "unknown" events [chromium/src : main]
Rahul Singh (Gerrit)
Rahul Singh added 1 comment![]( "Open in Gerrit")
Hi memmott@! This change is ready for a first look. If we are on the right track here, I'll start updating expectations for the failing tests. Thanks!
Related details
- Nathan Memmott
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott added 1 comment![]( "Open in Gerrit")
- Rahul Singh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
1 comment
Hi memmott@! I've updated tests and this change is ready for your review. Thanks!
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daseul Lee (Gerrit)
Daseul Lee added 3 comments![]( "Open in Gerrit")
" await record.type == 'disappeared');"Seems to be flaky on Mac, and it's possible that an additional event is sent (maybe the historic create event?). You could just check or wait until the matched "disappeared" event is seen on the `relativePathComponents`? If it's not possible, we could consider disabling this on Mac with a note on flakiness. Wdyt?
" await record.type == 'disappeared');"`await` not needed for checking the type
// TODO(crbug.com/377903461): Don't send a changedHandle for errored events.Just wondering if we plan on making this field optional from mojo and the browser side, not just from the renderer-side? CL looks okay as is, but would be good to do this eventually as a clean-up, in which case it'd be nice to mention this in the CL description or `FileSystemAccessObserverObservation::OnChanges()` where `changed_entry` is created. :)
Related details
- Rahul Singh
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott added 2 comments![]( "Open in Gerrit")
" promiseResolve(await dir.isSameEntry(record.root) &&"Should we also check that `changedHandle` is `null`?
// TODO(crbug.com/377903461): Don't send a changedHandle for errored events.Just wondering if we plan on making this field optional from mojo and the browser side, not just from the renderer-side? CL looks okay as is, but would be good to do this eventually as a clean-up, in which case it'd be nice to mention this in the CL description or `FileSystemAccessObserverObservation::OnChanges()` where `changed_entry` is created. :)
Yeah that's the plan, but adding more TODO comments is good.
Related details
- Daseul Lee
- Rahul Singh
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott added 1 comment![]( "Open in Gerrit")
// The handle affected by the file system change.Nit: Update comment to explain that this is `null` if `type` is "disappeared", "unknown" or "errored".
Rahul Singh (Gerrit)
Rahul Singh voted and added 5 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
5 comments
Thanks for the comments folks! I've addressed them all.
" promiseResolve(await dir.isSameEntry(record.root) &&"Should we also check that `changedHandle` is `null`?
Done
Seems to be flaky on Mac, and it's possible that an additional event is sent (maybe the historic create event?). You could just check or wait until the matched "disappeared" event is seen on the `relativePathComponents`? If it's not possible, we could consider disabling this on Mac with a note on flakiness. Wdyt?
Done
`await` not needed for checking the type
Done
Nit: Update comment to explain that this is `null` if `type` is "disappeared", "unknown" or "errored".
- Daseul Lee
- Nathan Memmott
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott added 2 comments![]( "Open in Gerrit")
SupportsReportingModifiedPath() ? "subDir" : "dir";Optional: Is there any platform for which this should not be true? It looks like Fuchsia isn't included in the function, but is that still the case?
Should we get rid of `SupportsReportingModifiedPath` altogether in these tests? Maybe we could keep this for now and do it in a separate CL?
"self.observer = observer;"
"await new Promise(resolve => setTimeout(resolve, $1));"
"})()";
// clang-format on
EXPECT_TRUE(ExecJs(shell(),
JsReplace(script, base::Int64ToValue(kFSATestTimeoutMs))));
So I'm guessing we're waiting a few seconds before observing so that we miss the creation event on Mac?
If so, what you could do is keep what you originally had, but in the `onChange` function, ignore any creation events. It could be Mac only as well. Maybe even a counter to make sure we at most see one creation event?
Related details
- Daseul Lee
- Rahul Singh
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 3 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
3 comments
Hi memmott@! I've addressed both your comments. PTAL when you get a chance!
SupportsReportingModifiedPath() ? "subDir" : "dir";Optional: Is there any platform for which this should not be true? It looks like Fuchsia isn't included in the function, but is that still the case?
Should we get rid of `SupportsReportingModifiedPath` altogether in these tests? Maybe we could keep this for now and do it in a separate CL?
Let's do it in a follow up change. I've created a CrBug and added a TODO in the file.
https://issues.chromium.org/379052298 [FSO] [Clean up] Look into removing SupportsReportingModifiedPath() test helper
"self.observer = observer;"
"await new Promise(resolve => setTimeout(resolve, $1));"
"})()";
// clang-format on
EXPECT_TRUE(ExecJs(shell(),
JsReplace(script, base::Int64ToValue(kFSATestTimeoutMs))));
So I'm guessing we're waiting a few seconds before observing so that we miss the creation event on Mac?
If so, what you could do is keep what you originally had, but in the `onChange` function, ignore any creation events. It could be Mac only as well. Maybe even a counter to make sure we at most see one creation event?
- Daseul Lee
- Nathan Memmott
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nathan Memmott
| Code-Review | +1 |
3 comments
Looks good!
SupportsReportingModifiedPath() ? "subDir" : "dir";Rahul SinghOptional: Is there any platform for which this should not be true? It looks like Fuchsia isn't included in the function, but is that still the case?
Should we get rid of `SupportsReportingModifiedPath` altogether in these tests? Maybe we could keep this for now and do it in a separate CL?
Let's do it in a follow up change. I've created a CrBug and added a TODO in the file.
https://issues.chromium.org/379052298 [FSO] [Clean up] Look into removing SupportsReportingModifiedPath() test helper
Awesome thanks!
// Check for and ignore at most, one appeared event.Related details
- Daseul Lee
- Rahul Singh
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +2 |
2 comments
Nit: Here and the next one add `TODO(crbug.com/343801378)`.
Done
Related details
- Daseul Lee
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/file_system_access/file_system_access_observer_browsertest.cc
Insertions: 4, Deletions: 2.
@@ -956,7 +956,8 @@
"async function onChange(records, observer) {"
" const record = records[0];"
#if BUILDFLAG(IS_MAC)
- // Check for and ignore at most, one appeared event.
+ // TODO(crbug.com/343801378): Check for and ignore at most, one
+ // appeared event.
" if (record.type === 'appeared') {"
" appearedEventCount += 1;"
" if (appearedEventCount > 1) {"
@@ -995,7 +996,8 @@
"async function onChange(records, observer) {"
" const record = records[0];"
#if BUILDFLAG(IS_MAC)
- // Check for and ignore at most, one appeared event.
+ // TODO(crbug.com/343801378): Check for and ignore at most, one
+ // appeared event.
" if (record.type === 'appeared') {"
" appearedEventCount += 1;"
" if (appearedEventCount > 1) {"
```
Change information
FSO: Omit changedHandle for "disappear", "error", and "unknown" events
This updates the `changedHandle` field in FileSystemChangeRecord to be
an optional. It also updates the changedHandle() getter to return a
nullptr for disappeared, errored, and unknown change types.
- M content/browser/file_system_access/file_system_access_observer_browsertest.cc
- M content/browser/file_system_access/file_system_access_observer_observation.cc
- M third_party/blink/renderer/modules/file_system_access/file_system_change_record.h
- M third_party/blink/renderer/modules/file_system_access/file_system_change_record.idl
- M third_party/blink/web_tests/external/wpt/fs/resources/collecting-file-system-observer.js
- M third_party/blink/web_tests/external/wpt/fs/script-tests/FileSystemObserver.js
Code-Review: +1 by Nathan Memmott
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/49200
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |