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!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
Commit-Queue | +1 |
Hi memmott@! I've updated tests and this change is ready for your review. Thanks!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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. :)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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".
Commit-Queue | +1 |
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".
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
Commit-Queue | +1 |
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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
Code-Review | +1 |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
Commit-Queue | +2 |
Nit: Here and the next one add `TODO(crbug.com/343801378)`.
Done
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
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) {"
```
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/49200
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |