FSO: Omit changedHandle for "disappear", "error", and "unknown" events [chromium/src : main]

0 views
Skip to first unread message

Rahul Singh (Gerrit)

unread,
Nov 12, 2024, 2:22:38 PMNov 12
to Nathan Memmott, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
Attention needed from Nathan Memmott

Rahul Singh added 1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Rahul Singh . resolved

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!

Open in Gerrit

Related details

Attention is currently required from:
  • Nathan Memmott
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
Gerrit-Change-Number: 6012533
Gerrit-PatchSet: 1
Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
Gerrit-CC: Daseul Lee <ds...@chromium.org>
Gerrit-Attention: Nathan Memmott <mem...@chromium.org>
Gerrit-Comment-Date: Tue, 12 Nov 2024 19:22:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Nathan Memmott (Gerrit)

unread,
Nov 12, 2024, 2:24:57 PMNov 12
to Rahul Singh, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
Attention needed from Rahul Singh

Nathan Memmott added 1 comment

Patchset-level comments
Nathan Memmott . resolved

Perfect this looks good to me!

Open in Gerrit

Related details

Attention is currently required from:
  • Rahul Singh
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
Gerrit-Change-Number: 6012533
Gerrit-PatchSet: 1
Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
Gerrit-CC: Daseul Lee <ds...@chromium.org>
Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
Gerrit-Comment-Date: Tue, 12 Nov 2024 19:24:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Rahul Singh (Gerrit)

unread,
Nov 13, 2024, 12:12:39 AMNov 13
to AyeAye, Nathan Memmott, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org

Rahul Singh voted and added 1 comment

Votes added by Rahul Singh

Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Rahul Singh . resolved

Hi memmott@! I've updated tests and this change is ready for your review. Thanks!

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
Gerrit-Change-Number: 6012533
Gerrit-PatchSet: 3
Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
Gerrit-CC: Daseul Lee <ds...@chromium.org>
Gerrit-Comment-Date: Wed, 13 Nov 2024 05:12:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Daseul Lee (Gerrit)

unread,
Nov 13, 2024, 10:50:09 AMNov 13
to Rahul Singh, AyeAye, Nathan Memmott, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
Attention needed from Rahul Singh

Daseul Lee added 3 comments

File content/browser/file_system_access/file_system_access_observer_browsertest.cc
Line 954, Patchset 3 (Latest): " await record.type == 'disappeared');"
Daseul Lee . unresolved

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?

Line 954, Patchset 3 (Latest): " await record.type == 'disappeared');"
Daseul Lee . unresolved

`await` not needed for checking the type

File content/browser/file_system_access/file_system_access_observer_observation.cc
Line 378, Patchset 3 (Latest): // TODO(crbug.com/377903461): Don't send a changedHandle for errored events.
Daseul Lee . resolved

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. :)

Open in Gerrit

Related details

Attention is currently required from:
  • Rahul Singh
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
    Gerrit-Change-Number: 6012533
    Gerrit-PatchSet: 3
    Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
    Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
    Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
    Gerrit-CC: Daseul Lee <ds...@chromium.org>
    Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
    Gerrit-Comment-Date: Wed, 13 Nov 2024 15:49:59 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nathan Memmott (Gerrit)

    unread,
    Nov 13, 2024, 1:46:29 PMNov 13
    to Rahul Singh, AyeAye, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daseul Lee and Rahul Singh

    Nathan Memmott added 2 comments

    File content/browser/file_system_access/file_system_access_observer_browsertest.cc
    Line 953, Patchset 3 (Latest): " promiseResolve(await dir.isSameEntry(record.root) &&"
    Nathan Memmott . unresolved

    Should we also check that `changedHandle` is `null`?

    File content/browser/file_system_access/file_system_access_observer_observation.cc
    Line 378, Patchset 3 (Latest): // TODO(crbug.com/377903461): Don't send a changedHandle for errored events.
    Daseul Lee . resolved

    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. :)

    Nathan Memmott

    Yeah that's the plan, but adding more TODO comments is good.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Daseul Lee
    • Rahul Singh
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
    Gerrit-Change-Number: 6012533
    Gerrit-PatchSet: 3
    Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
    Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
    Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
    Gerrit-CC: Daseul Lee <ds...@chromium.org>
    Gerrit-Attention: Daseul Lee <ds...@chromium.org>
    Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
    Gerrit-Comment-Date: Wed, 13 Nov 2024 18:46:20 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Daseul Lee <ds...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nathan Memmott (Gerrit)

    unread,
    Nov 13, 2024, 2:54:58 PMNov 13
    to Rahul Singh, AyeAye, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daseul Lee and Rahul Singh

    Nathan Memmott added 1 comment

    File third_party/blink/renderer/modules/file_system_access/file_system_change_record.idl
    Line 24, Patchset 3 (Latest): // The handle affected by the file system change.
    Nathan Memmott . unresolved

    Nit: Update comment to explain that this is `null` if `type` is "disappeared", "unknown" or "errored".

    Gerrit-Comment-Date: Wed, 13 Nov 2024 19:54:50 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Rahul Singh (Gerrit)

    unread,
    Nov 14, 2024, 1:16:56 PMNov 14
    to AyeAye, Nathan Memmott, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daseul Lee and Nathan Memmott

    Rahul Singh voted and added 5 comments

    Votes added by Rahul Singh

    Commit-Queue+1

    5 comments

    Patchset-level comments
    File-level comment, Patchset 6 (Latest):
    Rahul Singh . resolved

    Thanks for the comments folks! I've addressed them all.

    File content/browser/file_system_access/file_system_access_observer_browsertest.cc
    Line 953, Patchset 3: " promiseResolve(await dir.isSameEntry(record.root) &&"
    Nathan Memmott . resolved

    Should we also check that `changedHandle` is `null`?

    Rahul Singh

    Done

    Line 954, Patchset 3: " await record.type == 'disappeared');"
    Daseul Lee . resolved

    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?

    Rahul Singh

    Done

    Line 954, Patchset 3: " await record.type == 'disappeared');"
    Daseul Lee . resolved

    `await` not needed for checking the type

    Rahul Singh

    Done

    File third_party/blink/renderer/modules/file_system_access/file_system_change_record.idl
    Line 24, Patchset 3: // The handle affected by the file system change.
    Nathan Memmott . resolved

    Nit: Update comment to explain that this is `null` if `type` is "disappeared", "unknown" or "errored".

    Rahul Singh

    Done

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Daseul Lee
    • Nathan Memmott
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
    Gerrit-Change-Number: 6012533
    Gerrit-PatchSet: 6
    Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
    Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
    Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
    Gerrit-CC: Daseul Lee <ds...@chromium.org>
    Gerrit-Attention: Nathan Memmott <mem...@chromium.org>
    Gerrit-Attention: Daseul Lee <ds...@chromium.org>
    Gerrit-Comment-Date: Thu, 14 Nov 2024 18:16:47 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Nathan Memmott <mem...@chromium.org>
    Comment-In-Reply-To: Daseul Lee <ds...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nathan Memmott (Gerrit)

    unread,
    Nov 14, 2024, 4:27:46 PMNov 14
    to Rahul Singh, AyeAye, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daseul Lee and Rahul Singh

    Nathan Memmott added 2 comments

    File content/browser/file_system_access/file_system_access_observer_browsertest.cc
    Line 947, Patchset 6 (Parent): SupportsReportingModifiedPath() ? "subDir" : "dir";
    Nathan Memmott . unresolved

    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?

    Line 960, Patchset 6 (Latest): "self.observer = observer;"
    "await new Promise(resolve => setTimeout(resolve, $1));"
    "})()";
    // clang-format on
    EXPECT_TRUE(ExecJs(shell(),
    JsReplace(script, base::Int64ToValue(kFSATestTimeoutMs))));
    Nathan Memmott . unresolved

    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?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Daseul Lee
    • Rahul Singh
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 6
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      Gerrit-CC: Daseul Lee <ds...@chromium.org>
      Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
      Gerrit-Attention: Daseul Lee <ds...@chromium.org>
      Gerrit-Comment-Date: Thu, 14 Nov 2024 21:27:37 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Rahul Singh (Gerrit)

      unread,
      Nov 14, 2024, 10:46:55 PMNov 14
      to AyeAye, Nathan Memmott, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
      Attention needed from Daseul Lee and Nathan Memmott

      Rahul Singh voted and added 3 comments

      Votes added by Rahul Singh

      Commit-Queue+1

      3 comments

      Patchset-level comments
      File-level comment, Patchset 7 (Latest):
      Rahul Singh . resolved

      Hi memmott@! I've addressed both your comments. PTAL when you get a chance!

      File content/browser/file_system_access/file_system_access_observer_browsertest.cc
      Line 947, Patchset 6 (Parent): SupportsReportingModifiedPath() ? "subDir" : "dir";
      Nathan Memmott . resolved

      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?

      Rahul Singh

      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

      Line 960, Patchset 6: "self.observer = observer;"

      "await new Promise(resolve => setTimeout(resolve, $1));"
      "})()";
      // clang-format on
      EXPECT_TRUE(ExecJs(shell(),
      JsReplace(script, base::Int64ToValue(kFSATestTimeoutMs))));
      Nathan Memmott . resolved

      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?

      Rahul Singh

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Daseul Lee
      • Nathan Memmott
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 7
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      Gerrit-CC: Daseul Lee <ds...@chromium.org>
      Gerrit-Attention: Nathan Memmott <mem...@chromium.org>
      Gerrit-Attention: Daseul Lee <ds...@chromium.org>
      Gerrit-Comment-Date: Fri, 15 Nov 2024 03:46:46 +0000
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Nathan Memmott (Gerrit)

      unread,
      Nov 15, 2024, 1:30:53 PMNov 15
      to Rahul Singh, AyeAye, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
      Attention needed from Daseul Lee and Rahul Singh

      Nathan Memmott voted and added 3 comments

      Votes added by Nathan Memmott

      Code-Review+1

      3 comments

      Patchset-level comments
      Nathan Memmott . resolved

      Looks good!

      File content/browser/file_system_access/file_system_access_observer_browsertest.cc
      Line 947, Patchset 6 (Parent): SupportsReportingModifiedPath() ? "subDir" : "dir";
      Nathan Memmott . resolved

      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?

      Rahul Singh

      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

      Nathan Memmott

      Awesome thanks!

      Line 959, Patchset 7 (Latest): // Check for and ignore at most, one appeared event.
      Nathan Memmott . unresolved

      Nit: Here and the next one add `TODO(crbug.com/343801378)`.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Daseul Lee
      • Rahul Singh
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 7
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      Gerrit-CC: Daseul Lee <ds...@chromium.org>
      Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
      Gerrit-Attention: Daseul Lee <ds...@chromium.org>
      Gerrit-Comment-Date: Fri, 15 Nov 2024 18:30:40 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Nathan Memmott <mem...@chromium.org>
      Comment-In-Reply-To: Rahul Singh <rah...@microsoft.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Rahul Singh (Gerrit)

      unread,
      Nov 15, 2024, 2:29:07 PMNov 15
      to Nathan Memmott, AyeAye, Daseul Lee, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org
      Attention needed from Daseul Lee

      Rahul Singh voted and added 2 comments

      Votes added by Rahul Singh

      Commit-Queue+2

      2 comments

      Patchset-level comments
      File-level comment, Patchset 8 (Latest):
      Rahul Singh . resolved

      Thanks for the review memmott@!

      File content/browser/file_system_access/file_system_access_observer_browsertest.cc
      Line 959, Patchset 7: // Check for and ignore at most, one appeared event.
      Nathan Memmott . resolved

      Nit: Here and the next one add `TODO(crbug.com/343801378)`.

      Rahul Singh

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Daseul Lee
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 8
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      Gerrit-CC: Daseul Lee <ds...@chromium.org>
      Gerrit-Attention: Daseul Lee <ds...@chromium.org>
      Gerrit-Comment-Date: Fri, 15 Nov 2024 19:28:55 +0000
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      Nov 15, 2024, 3:31:49 PMNov 15
      to Rahul Singh, Nathan Memmott, AyeAye, Daseul Lee, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org

      Chromium LUCI CQ submitted the change with unreviewed changes

      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

      Commit message:
      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.
      Bug: 377903461
      Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Reviewed-by: Nathan Memmott <mem...@chromium.org>
      Commit-Queue: Rahul Singh <rah...@microsoft.com>
      Cr-Commit-Position: refs/heads/main@{#1383791}
      Files:
      • 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
      Change size: M
      Delta: 6 files changed, 60 insertions(+), 28 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Nathan Memmott
      Open in Gerrit
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: merged
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 9
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      open
      diffy
      satisfied_requirement

      Blink W3C Test Autoroller (Gerrit)

      unread,
      Nov 15, 2024, 4:13:46 PMNov 15
      to Chromium LUCI CQ, Rahul Singh, Nathan Memmott, AyeAye, Daseul Lee, chromium...@chromium.org, blink-revie...@chromium.org, edgesto...@microsoft.com, edgecapab...@microsoft.com, blink-...@chromium.org, jmedle...@chromium.org

      Message from Blink W3C Test Autoroller

      The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/49200

      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ie6abe56d9032dbe42aba6f893837e6a43a027980
      Gerrit-Change-Number: 6012533
      Gerrit-PatchSet: 9
      Gerrit-Owner: Rahul Singh <rah...@microsoft.com>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Nathan Memmott <mem...@chromium.org>
      Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
      Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
      Gerrit-CC: Daseul Lee <ds...@chromium.org>
      Gerrit-Comment-Date: Fri, 15 Nov 2024 21:13:36 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: No
      satisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages