Pi-Hsun Shih (Gerrit)

unread,

Jul 3, 2024, 5:44:15 AM (2 days ago) Jul 3

to Shik Chen, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Shik Chen

Pi-Hsun Shih voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Shik Chen

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Shik Chen (Gerrit)

unread,

Jul 3, 2024, 2:22:39 PM (2 days ago) Jul 3

to Pi-Hsun Shih, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Pi-Hsun Shih

Shik Chen added 4 comments

File ash/webui/recorder_app_ui/resources/core/platform_handler.ts

Line 101, Patchset 5 (Latest): abstract installSoda(): void;

Shik Chen . unresolved

nit: add comment for the expected way to handle errors.

File ash/webui/recorder_app_ui/resources/core/recording_session.ts

Line 177, Patchset 5 (Latest): let dispose: Dispose|null = null;

Shik Chen . unresolved

Do we want to set `dispose` to null after calling it...? What will happen if we call dispose() twice?

File ash/webui/recorder_app_ui/resources/pages/record-page.ts

Line 350, Patchset 5 (Latest):

    // Don't enable SODA if it's unavailable. All UI to enable transcription
    // are gated behind transcriptionAvailable.

Shik Chen . unresolved

Hmm if it's gated by UI already, do we still need the `&&` below?

File ash/webui/recorder_app_ui/resources/platforms/dev/handler.ts

Line 251, Patchset 5 (Latest): console.log('SODA installation requested');

Shik Chen . unresolved

Do we want to keep this console.log()?

Maybe we should have our own logger that will send event logs alone with the feedback report :P

Open in Gerrit

Related details

Attention is currently required from:

Pi-Hsun Shih

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Pi-Hsun Shih (Gerrit)

unread,

Jul 4, 2024, 2:06:09 AM (yesterday) Jul 4

to Chromium LUCI CQ, Shik Chen, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Shik Chen

Pi-Hsun Shih added 4 comments

File ash/webui/recorder_app_ui/resources/core/platform_handler.ts

Line 101, Patchset 5: abstract installSoda(): void;

Shik Chen . resolved

nit: add comment for the expected way to handle errors.

Pi-Hsun Shih

Done

File ash/webui/recorder_app_ui/resources/core/recording_session.ts

Line 177, Patchset 5: let dispose: Dispose|null = null;

Shik Chen . resolved

Do we want to set `dispose` to null after calling it...? What will happen if we call dispose() twice?

Pi-Hsun Shih

We wouldn't ever call dispose twice here since it's called in an effect block, and that got cancelled when dispose is called the first time. The second call to dispose is a no-op anyway.

The code is structued in this way due to the limitation that the effect callback is called synchronously on the first call, so the dispose won't be set at that time... Hmm so there's a bug here that the first call won't be able to stop the effect :P

Changed the effect interface to have a disposed function passed in to be used inside the effect callback.

File ash/webui/recorder_app_ui/resources/pages/record-page.ts

Line 350, Patchset 5: // Don't enable SODA if it's unavailable. All UI to enable transcription


    // are gated behind transcriptionAvailable.

Shik Chen . resolved

Hmm if it's gated by UI already, do we still need the `&&` below?

Pi-Hsun Shih

We still need this one since this one in particular is called in connectedCallback and not gated by UI.

File ash/webui/recorder_app_ui/resources/platforms/dev/handler.ts

Line 251, Patchset 5: console.log('SODA installation requested');

Shik Chen . resolved

Do we want to keep this console.log()?
Maybe we should have our own logger that will send event logs alone with the feedback report :P

Pi-Hsun Shih

This is for dev so we want to log to console for easier debugging.

(Note that we have eslint that disallow console.log for places other than dev)

Open in Gerrit

Related details

Attention is currently required from:

Shik Chen

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Shik Chen (Gerrit)

unread,

Jul 4, 2024, 3:11:10 AM (yesterday) Jul 4

to Pi-Hsun Shih, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Pi-Hsun Shih

Shik Chen voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Pi-Hsun Shih

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Pi-Hsun Shih (Gerrit)

unread,

Jul 4, 2024, 5:46:33 AM (23 hours ago) Jul 4

to Chromium IPC Reviews, Shik Chen, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Chromium IPC Reviews

Pi-Hsun Shih voted Auto-Submit+1

Auto-Submit

+1

Open in Gerrit

Related details

Attention is currently required from:

Chromium IPC Reviews

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

Jul 4, 2024, 5:49:54 AM (23 hours ago) Jul 4

to Pi-Hsun Shih, Chromium IPC Reviews, Takashi Toyoshima, Shik Chen, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Takashi Toyoshima

Message from gwsq

From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org

📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).

IPC reviewer(s): toyo...@chromium.org

Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)

Open in Gerrit

Related details

Attention is currently required from:

Takashi Toyoshima

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Takashi Toyoshima (Gerrit)

unread,

1:55 AM (3 hours ago) 1:55 AM

to Pi-Hsun Shih, Chromium IPC Reviews, Shik Chen, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Pi-Hsun Shih

Takashi Toyoshima added 1 comment

File ash/webui/recorder_app_ui/mojom/recorder_app.mojom

Line 38, Patchset 6 (Latest): int32? progress;

Takashi Toyoshima . unresolved

uint8?

Open in Gerrit

Related details

Attention is currently required from:

Pi-Hsun Shih

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Pi-Hsun Shih (Gerrit)

unread,

2:26 AM (2 hours ago) 2:26 AM

to Chromium IPC Reviews, Takashi Toyoshima, Shik Chen, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Takashi Toyoshima

Pi-Hsun Shih voted and added 1 comment

Votes added by Pi-Hsun Shih

Auto-Submit

+1

1 comment

File ash/webui/recorder_app_ui/mojom/recorder_app.mojom

Line 38, Patchset 6: int32? progress;

Takashi Toyoshima . resolved

uint8?

Pi-Hsun Shih

Done

Open in Gerrit

Related details

Attention is currently required from:

Takashi Toyoshima

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Takashi Toyoshima (Gerrit)

unread,

4:10 AM (23 minutes ago) 4:10 AM

to Pi-Hsun Shih, Chromium IPC Reviews, Shik Chen, Chromium LUCI CQ, Kentaro Hara, Sadrul Chowdhury, Zijie He, chromium...@chromium.org, ipc-securi...@chromium.org, agriev...@chromium.org, asvitki...@chromium.org, bartek...@chromium.org, chromiumme...@microsoft.com, fdoray...@chromium.org, feature-me...@chromium.org, fuchsia...@chromium.org, fuzzin...@chromium.org, gab+...@chromium.org, gavinp...@chromium.org, grt+...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, jshin...@chromium.org, lize...@chromium.org, lizeb...@chromium.org, mac-r...@chromium.org, nyquis...@chromium.org, pasko...@chromium.org, roblia...@chromium.org, scheduler...@chromium.org, security-...@chromium.org, spang...@chromium.org, torne...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org, wittma...@chromium.org, yfriedm...@chromium.org, oshima...@chromium.org

Attention needed from Pi-Hsun Shih

Takashi Toyoshima voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Pi-Hsun Shih

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

recorder: Add SODA installation progress [chromium/src : main]

Pi-Hsun Shih (Gerrit)

Pi-Hsun Shih voted Commit-Queue+1

Related details

Shik Chen (Gerrit)

Shik Chen added 4 comments

Related details

Pi-Hsun Shih (Gerrit)

Pi-Hsun Shih added 4 comments

Related details

Shik Chen (Gerrit)

Shik Chen voted Code-Review+1

Related details

Pi-Hsun Shih (Gerrit)

Pi-Hsun Shih voted Auto-Submit+1

Related details

gwsq (Gerrit)

Message from gwsq

Related details

Takashi Toyoshima (Gerrit)

Takashi Toyoshima added 1 comment

Related details

Pi-Hsun Shih (Gerrit)

Pi-Hsun Shih voted and added 1 comment

Votes added by Pi-Hsun Shih

1 comment

Related details

Takashi Toyoshima (Gerrit)

Takashi Toyoshima voted

Related details