Skip to content

Conversation

@yoshisatoyanagisawa
Copy link
Collaborator

@yoshisatoyanagisawa yoshisatoyanagisawa commented Apr 3, 2023

This is for stop calling Get for detecting an empty fetch listener. Benefit of the change is that it won't execute the Javascript, and drawback is that it just ignores an empty fetch listener defined as a handleEvent property. Since it is rare to configure the fetch event listener with the handleEvent property, not detecting empty function body for it should be negligible coverage reduction.

+@domenic


Preview | Diff

@jakearchibald
Copy link
Contributor

This looks good, but can you add a note that makes it clear that we're deliberately returning false for handleEvent?

@jakearchibald
Copy link
Contributor

@annevk @youennf @asutherland This is a change to the "optimise for empty fetch listeners" feature (#1671).

Previously, we were looking at both regular listeners, and { handleEvent } listeners. However, running getters during the check adds a bunch of complexity. With this change, the optimisation doesn't apply if there's a non-callable listener object (as in, { handleEvent }).

@yoshisatoyanagisawa
Copy link
Collaborator Author

This looks good, but can you add a note that makes it clear that we're deliberately returning false for handleEvent?

Sure. I have added the note.
I am not confident that the explanation makes sense to you.

aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 5, 2023
Make the code aligned with the following specification update:
w3c/ServiceWorker#1676

With the previous specification and code, event listener vector
can be modified during the GetEffectiveFunction execution, which may
bring unexpected vector state.

Change-Id: I732c4c9ab2caebc49a7f4ef52640df7b8476d838
Bug: 1429201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4394402
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1126483}
@yoshisatoyanagisawa
Copy link
Collaborator Author

Hi, are there any updates on this?
This narrows down the area, which is treated as an empty fetch handler to exclude something rarely used for the PWA purpose.

Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jakearchibald jakearchibald merged commit fb1e711 into w3c:main Apr 11, 2023
github-actions bot added a commit that referenced this pull request Apr 11, 2023
…hm. (#1676)

SHA: fb1e711
Reason: push, by jakearchibald

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to asleekgeek/ServiceWorker that referenced this pull request Apr 11, 2023
…hm. (w3c#1676)

SHA: fb1e711
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
trflynn89 pushed a commit to RebelBrowser/rebel that referenced this pull request Apr 20, 2023
Make the code aligned with the following specification update:
w3c/ServiceWorker#1676

With the previous specification and code, event listener vector
can be modified during the GetEffectiveFunction execution, which may
bring unexpected vector state.

(cherry picked from commit 5105ce3)

Change-Id: I732c4c9ab2caebc49a7f4ef52640df7b8476d838
Bug: 1429201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4394402
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1126483}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4408837
Reviewed-by: Shunya Shishido <[email protected]>
Reviewed-by: Minoru Chikamune <[email protected]>
Cr-Commit-Position: refs/branch-heads/5615@{#1203}
Cr-Branched-From: 9c6408e-refs/heads/main@{#1109224}
qtprojectorg pushed a commit to qt/qtwebengine-chromium that referenced this pull request May 2, 2023
…er API

Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4406580:
Stop supporting { handleEvent }.

M108 merge issues:
  content_unittests_bundle_data.filelist:
    Not present in 108, skipped; Only used in iOS tests on main

Make the code aligned with the following specification update:
w3c/ServiceWorker#1676

With the previous specification and code, event listener vector
can be modified during the GetEffectiveFunction execution, which may
bring unexpected vector state.

(cherry picked from commit 5105ce37a6853d52ec97894bf6969b3c29a23afd)

Change-Id: I732c4c9ab2caebc49a7f4ef52640df7b8476d838
Bug: 1429201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4394402
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1126483}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4406580
Commit-Queue: Roger Felipe Zanoni da Silva <[email protected]>
Reviewed-by: Yoshisato Yanagisawa <[email protected]>
Cr-Commit-Position: refs/branch-heads/5359@{#1449}
Cr-Branched-From: 27d3765d341b09369006d030f83f582a29eb57ae-refs/heads/main@{#1058933}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/474621
Reviewed-by: Allan Sandfeld Jensen <[email protected]>
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request May 9, 2023
Make the code aligned with the following specification update:
w3c/ServiceWorker#1676

With the previous specification and code, event listener vector
can be modified during the GetEffectiveFunction execution, which may
bring unexpected vector state.

(cherry picked from commit 5105ce3)

Change-Id: I732c4c9ab2caebc49a7f4ef52640df7b8476d838
Bug: 1429201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4394402
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1126483}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4411225
Reviewed-by: Minoru Chikamune <[email protected]>
Cr-Commit-Position: refs/branch-heads/5672@{#442}
Cr-Branched-From: 5f2a724-refs/heads/main@{#1121455}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants