Remove NOTIMPLEMENTED directives that are being hit [chromium/src : main]

Bruce Dawson (Gerrit)

unread,

Jun 8, 2023, 7:03:43 PM6/8/23

to Charlie Reis, Kenichi Ishibashi, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org

Attention is currently required from: Charlie Reis, Kenichi Ishibashi.

Bruce Dawson would like Charlie Reis and Kenichi Ishibashi to review this change.

View Change

Remove NOTIMPLEMENTED directives that are being hit

While trying to understand 1453198 several "Not implemented reached"
errors were seen. Searching crbug found crbug.com/1433003 which suggests
that at least one of these should be disabled, so this change does that,
to avoid giving misleading information.

Bug: 1453198, 1433003
Change-Id: I9cb73c3fcf21702d018cebf2cdbb5d345cbdc343
---
M content/public/browser/web_contents_delegate.cc
M third_party/blink/renderer/core/workers/worklet_global_scope.cc
2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/content/public/browser/web_contents_delegate.cc b/content/public/browser/web_contents_delegate.cc
index 885899b..0e348f7c 100644
--- a/content/public/browser/web_contents_delegate.cc
+++ b/content/public/browser/web_contents_delegate.cc
@@ -161,7 +161,6 @@
 
 FullscreenState WebContentsDelegate::GetFullscreenState(
     const WebContents* web_contents) const {
-  NOTIMPLEMENTED_LOG_ONCE();
   FullscreenState state;
   state.target_mode =
       const_cast<WebContentsDelegate*>(this)->IsFullscreenForTabOrPending(
diff --git a/third_party/blink/renderer/core/workers/worklet_global_scope.cc b/third_party/blink/renderer/core/workers/worklet_global_scope.cc
index 9dfd614..80e2f2e 100644
--- a/third_party/blink/renderer/core/workers/worklet_global_scope.cc
+++ b/third_party/blink/renderer/core/workers/worklet_global_scope.cc
@@ -156,7 +156,6 @@
 
 const BrowserInterfaceBrokerProxy&
 WorkletGlobalScope::GetBrowserInterfaceBroker() const {
-  NOTIMPLEMENTED();
   return GetEmptyBrowserInterfaceBroker();
 }

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Bruce Dawson (Gerrit)

unread,

Jun 8, 2023, 7:03:48 PM6/8/23

to blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Charlie Reis, Kenichi Ishibashi, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Charlie Reis, Kenichi Ishibashi.

View Change

1 comment:

Patchset:
- Patch Set #1:
  Charlie, can you look at web_contents_delegate.cc
  Kenichi, can you look at worklet_global_scope.cc

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Kenichi Ishibashi (Gerrit)

unread,

Jun 8, 2023, 8:22:46 PM6/8/23

to Bruce Dawson, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Charlie Reis, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Bruce Dawson, Charlie Reis.

Patch set 1:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #1:
  lgtm

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Charlie Reis (Gerrit)

unread,

Jun 9, 2023, 11:37:32 AM6/9/23

to Mike Wasserman, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Bruce Dawson, Kenichi Ishibashi

Attention is currently required from: Bruce Dawson, Mike Wasserman.

Charlie Reis would like Mike Wasserman to review this change authored by Bruce Dawson.

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Charlie Reis (Gerrit)

unread,

Jun 9, 2023, 11:37:37 AM6/9/23

to Bruce Dawson, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Mike Wasserman, Kenichi Ishibashi, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Bruce Dawson, Mike Wasserman.

Patch set 1:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #1:
  Thanks! content/ LGTM to reflect reality, though it's probably worth following up with msw@ about the change.
File content/public/browser/web_contents_delegate.cc:
- Patch Set #1, Line 164: NOTIMPLEMENTED_LOG_ONCE();
  msw@: Does the fact that we're reaching here change any assumptions from your https://chromium-review.googlesource.com/c/chromium/src/+/4255184 CL earlier this year?

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Mike Wasserman (Gerrit)

unread,

Jun 9, 2023, 1:32:43 PM6/9/23

to Bruce Dawson, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Mike Wasserman, Charlie Reis, Kenichi Ishibashi, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Bruce Dawson, Mike Wasserman.

Patch set 1:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #1:
  web_contents_delegate.cc lgtm
File content/public/browser/web_contents_delegate.cc:

- Patch Set #1, Line 164: NOTIMPLEMENTED_LOG_ONCE();

- msw@: Does the fact that we're reaching here change any assumptions from your https://chromium-revie […]
  Right, content_shell and many other non-Browser WebContentsDelegate subclasses will use this naive, but workable impl. FWIW, my ideal code health followup would be to remove IsFullscreenForTabOrPending, so impls report (and callers check) the more detailed FullscreenState.
  Simply removing this log seems fine. Thanks for reducing log spam.

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Mike Wasserman (Gerrit)

unread,

Jun 9, 2023, 1:32:58 PM6/9/23

to Bruce Dawson, Mike Wasserman, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Charlie Reis, Kenichi Ishibashi, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Bruce Dawson.

Mike Wasserman removed Mike Wasserman from this change.

View Change

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Bruce Dawson (Gerrit)

unread,

Jun 9, 2023, 2:44:51 PM6/9/23

to blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Mike Wasserman, Charlie Reis, Kenichi Ishibashi, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Bruce Dawson.

Patch set 1:Commit-Queue +2

View Change

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jun 9, 2023, 2:49:44 PM6/9/23

to Bruce Dawson, blink-...@chromium.org, blink-work...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, shimazu...@chromium.org, Mike Wasserman, Charlie Reis, Kenichi Ishibashi, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Bruce Dawson: Commit
  Mike Wasserman: Looks good to me
  Kenichi Ishibashi: Looks good to me
  Charlie Reis: Looks good to me

Remove NOTIMPLEMENTED directives that are being hit

While trying to understand 1453198 several "Not implemented reached"
errors were seen. Searching crbug found crbug.com/1433003 which suggests
that at least one of these should be disabled, so this change does that,
to avoid giving misleading information.

Bug: 1453198, 1433003
Change-Id: I9cb73c3fcf21702d018cebf2cdbb5d345cbdc343

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599673
Reviewed-by: Kenichi Ishibashi <ba...@chromium.org>
Reviewed-by: Charlie Reis <cr...@chromium.org>
Reviewed-by: Mike Wasserman <m...@chromium.org>
Commit-Queue: Bruce Dawson <bruce...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155663}


---
M content/public/browser/web_contents_delegate.cc
M third_party/blink/renderer/core/workers/worklet_global_scope.cc
2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/content/public/browser/web_contents_delegate.cc b/content/public/browser/web_contents_delegate.cc
index 885899b..0e348f7c 100644
--- a/content/public/browser/web_contents_delegate.cc
+++ b/content/public/browser/web_contents_delegate.cc
@@ -161,7 +161,6 @@
 
 FullscreenState WebContentsDelegate::GetFullscreenState(
     const WebContents* web_contents) const {
-  NOTIMPLEMENTED_LOG_ONCE();
   FullscreenState state;
   state.target_mode =
       const_cast<WebContentsDelegate*>(this)->IsFullscreenForTabOrPending(
diff --git a/third_party/blink/renderer/core/workers/worklet_global_scope.cc b/third_party/blink/renderer/core/workers/worklet_global_scope.cc
index 9dfd614..80e2f2e 100644
--- a/third_party/blink/renderer/core/workers/worklet_global_scope.cc
+++ b/third_party/blink/renderer/core/workers/worklet_global_scope.cc
@@ -156,7 +156,6 @@
 
 const BrowserInterfaceBrokerProxy&
 WorkletGlobalScope::GetBrowserInterfaceBroker() const {
-  NOTIMPLEMENTED();
   return GetEmptyBrowserInterfaceBroker();
 }

To view, visit change 4599673. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward