Fenced frames: Fixed CHECK in HTMLFencedFrameElement::Navigate. [chromium/src : main]

Xiaochen Zhou (Gerrit)

unread,

Mar 21, 2023, 12:49:46 PM3/21/23

to Garrett Tanzer, blink-rev...@chromium.org, blink-...@chromium.org

Attention is currently required from: Garrett Tanzer, Xiaochen Zhou.

Xiaochen Zhou would like Garrett Tanzer to review this change.

Fenced frames: Fixed CHECK in HTMLFencedFrameElement::Navigate.

The `deprecated_should_freeze_initial_size_` field in
`FencedFrameConfig` (html/fenced_frame/fenced_frame_config.h) is not
optional.

However, the parameter in HTMLFencedFrameElement::Navigate is
accepting an optional value for this flag.

The intention of this CHECK is to verify
`deprecated_should_freeze_initial_size_` should be false when there
exists `content_size`. But the current CHECK is verifying that it is
an `absl::nullopt`.

This change is not covered by test as it is currently unreachable.
It will soon be covered in my follow-up CL which stores the size
from winning ad to fenced frame config.

Change-Id: I52f205998f50369b12209a76541814d8fb096af8
---
M third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc b/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
index 446a0fa..a733f55 100644
--- a/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
+++ b/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
@@ -575,7 +575,8 @@
     // should freeze to that size rather than check the current size.
     // It is nonsensical to ask for the old size freezing behavior (freeze the
     // initial size) while also specifying a content size.
-    CHECK(!deprecated_should_freeze_initial_size);
+    CHECK(deprecated_should_freeze_initial_size.has_value() &&
+          !deprecated_should_freeze_initial_size.value());
     PhysicalSize converted_size(LayoutUnit(content_size->width()),
                                 LayoutUnit(content_size->height()));
     FreezeFrameSize(converted_size, /*should_coerce_size=*/false);

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

Xiaochen Zhou (Gerrit)

unread,

Mar 21, 2023, 12:49:48 PM3/21/23

to blink-rev...@chromium.org, blink-...@chromium.org, Garrett Tanzer, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Garrett Tanzer, Xiaochen Zhou.

Patch set 1:Commit-Queue +1

View Change

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

Garrett Tanzer (Gerrit)

unread,

Mar 21, 2023, 12:50:17 PM3/21/23

to Xiaochen Zhou, blink-rev...@chromium.org, blink-...@chromium.org, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Xiaochen Zhou.

Patch set 1:Code-Review +1

View Change

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

Xiaochen Zhou (Gerrit)

unread,

Mar 21, 2023, 12:51:35 PM3/21/23

to blink-rev...@chromium.org, blink-...@chromium.org

Attention is currently required from: Xiaochen Zhou.

Xiaochen Zhou uploaded patch set #2 to this change.

View Change

Fenced frames: Fixed CHECK in HTMLFencedFrameElement::Navigate.

The `deprecated_should_freeze_initial_size_` field in
`FencedFrameConfig` (html/fenced_frame/fenced_frame_config.h) is not
optional.

However, the parameter in HTMLFencedFrameElement::Navigate is
accepting an optional value for this flag.

The intention of this CHECK is to verify
`deprecated_should_freeze_initial_size_` should be false when there
exists `content_size`. But the current CHECK is verifying that it is
an `absl::nullopt`.

This change is not covered by test as it is currently unreachable.
It will soon be covered in my follow-up CL which stores the size
from winning ad to fenced frame config.

Low-Coverage-Reason: Not reachable for now, will be covered in an
follow-up CL by WPTs and browser tests.


Change-Id: I52f205998f50369b12209a76541814d8fb096af8
---
M third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
1 file changed, 2 insertions(+), 1 deletion(-)

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

Xiaochen Zhou (Gerrit)

unread,

Mar 21, 2023, 12:51:58 PM3/21/23

to blink-rev...@chromium.org, blink-...@chromium.org, Garrett Tanzer, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Xiaochen Zhou.

Patch set 2:Commit-Queue +2

View Change

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

Chromium LUCI CQ (Gerrit)

unread,

Mar 21, 2023, 12:54:38 PM3/21/23

to Xiaochen Zhou, blink-rev...@chromium.org, blink-...@chromium.org, Garrett Tanzer, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change



1 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.

Approvals:
  Xiaochen Zhou: Commit
  Garrett Tanzer: Looks good to me

Fenced frames: Fixed CHECK in HTMLFencedFrameElement::Navigate.

The `deprecated_should_freeze_initial_size_` field in
`FencedFrameConfig` (html/fenced_frame/fenced_frame_config.h) is not
optional.

However, the parameter in HTMLFencedFrameElement::Navigate is
accepting an optional value for this flag.

The intention of this CHECK is to verify
`deprecated_should_freeze_initial_size_` should be false when there
exists `content_size`. But the current CHECK is verifying that it is
an `absl::nullopt`.

This change is not covered by test as it is currently unreachable.
It will soon be covered in my follow-up CL which stores the size
from winning ad to fenced frame config.

follow-up CL by WPTs and browser tests.

Low-Coverage-Reason: Not reachable for now, will be covered in an

Change-Id: I52f205998f50369b12209a76541814d8fb096af8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4357083
Commit-Queue: Xiaochen Zhou <xiaoc...@chromium.org>
Reviewed-by: Garrett Tanzer <gta...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1119996}


---
M third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc b/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
index 0fd4a7e..c2ccb02 100644
--- a/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
+++ b/third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
@@ -546,7 +546,8 @@


     // should freeze to that size rather than check the current size.
     // It is nonsensical to ask for the old size freezing behavior (freeze the
     // initial size) while also specifying a content size.
-    CHECK(!deprecated_should_freeze_initial_size);
+    CHECK(deprecated_should_freeze_initial_size.has_value() &&
+          !deprecated_should_freeze_initial_size.value());
     PhysicalSize converted_size(LayoutUnit(content_size->width()),
                                 LayoutUnit(content_size->height()));
     FreezeFrameSize(converted_size, /*should_coerce_size=*/false);

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

Reply all

Reply to author

Forward