[iOS Cont. Panel] Switch to rich IPH for entrypoint [chromium/src : main]

[findit-for-me@appspot.gserviceaccount.com (Gerrit)]

Attention needed from Nicolas MacBeth

findit...@appspot.gserviceaccount.com voted Code-Coverage-1

This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.

• //ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe

Code-Coverage

-1

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 1

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Tue, 18 Jun 2024 16:26:25 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Nicolas MacBeth (Gerrit)]

Attention needed from Robbie Gibson

Nicolas MacBeth added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Nicolas MacBeth . resolved

PTAL! thanks

Open in Gerrit

Related details

Attention is currently required from:

• Robbie Gibson

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 2

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Comment-Date: Thu, 27 Jun 2024 18:10:21 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Robbie Gibson (Gerrit)]

Attention needed from Nicolas MacBeth

Robbie Gibson added 2 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Robbie Gibson . resolved

one question, as this giant `&&` is getting pretty large

File ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm

Line 369, Patchset 2 (Latest): !config->iph_title.empty() && !config->iph_image_name.empty() &&

Robbie Gibson . unresolved

I wonder if we want to start factoring out some of these into helper functions (maybe in `contextual_panel_item_configuration.h`) like "allows IPH" or something like that.

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth

Submit Requirements:

• Code-Coverage
• Code-Review

• No-Unresolved-Comments

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 2

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Thu, 27 Jun 2024 19:03:52 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Nicolas MacBeth (Gerrit)]

Attention needed from Robbie Gibson

Nicolas MacBeth added 3 comments

Patchset-level comments

File-level comment, Patchset 2:

Robbie Gibson . resolved

one question, as this giant `&&` is getting pretty large

Nicolas MacBeth

yeah that's fair

File-level comment, Patchset 4 (Latest):

Nicolas MacBeth . resolved

thanks Robbie

File ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm

Line 369, Patchset 2: !config->iph_title.empty() && !config->iph_image_name.empty() &&

Robbie Gibson . resolved

I wonder if we want to start factoring out some of these into helper functions (maybe in `contextual_panel_item_configuration.h`) like "allows IPH" or something like that.

Nicolas MacBeth

Done!

Open in Gerrit

Related details

Attention is currently required from:

• Robbie Gibson

Submit Requirements:

• Code-Coverage

• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 4

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Comment-Date: Mon, 01 Jul 2024 19:43:43 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Robbie Gibson <rkgi...@google.com>

[findit-for-me@appspot.gserviceaccount.com (Gerrit)]

Attention needed from Nicolas MacBeth and Robbie Gibson

findit...@appspot.gserviceaccount.com voted Code-Coverage-1

This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.

• //ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe

Code-Coverage

-1

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth
• Robbie Gibson

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 4

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Mon, 01 Jul 2024 19:46:12 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Nicolas MacBeth (Gerrit)]

Attention needed from Robbie Gibson and Sylvain Defresne

Nicolas MacBeth added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Nicolas MacBeth . resolved

PTAL as `i/c/b/browser_view/ui_bundled` and `i/c/b/s/public/commands` owner. thanks!

Open in Gerrit

Related details

Attention is currently required from:

• Robbie Gibson
• Sylvain Defresne

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 5

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Comment-Date: Wed, 03 Jul 2024 15:28:00 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[findit-for-me@appspot.gserviceaccount.com (Gerrit)]

Attention needed from Nicolas MacBeth, Robbie Gibson and Sylvain Defresne

findit...@appspot.gserviceaccount.com voted Code-Coverage-1

This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.

• //ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe

Code-Coverage

-1

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth
• Robbie Gibson
• Sylvain Defresne

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 6

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Wed, 03 Jul 2024 16:17:28 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Sylvain Defresne (Gerrit)]

Attention needed from Nicolas MacBeth and Robbie Gibson

Sylvain Defresne added 2 comments

File ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Line 2152, Patchset 6 (Latest): contextualPanelEntrypointIPHDidDismissWithFeature:*config->iph_feature

Sylvain Defresne . unresolved

This will crash if `config.get()` returns null. Need to handle that case.

Line 2158, Patchset 6 (Latest): [UIImage imageNamed:base::SysUTF8ToNSString(config->iph_image_name)];

Sylvain Defresne . unresolved

Probably `CHECK(config)` at the beginning of the method. And then store a ref to it.

Maybe using

    ContextualPanelItemConfiguration& config_ref = CHECK_DEREF(config.get());

Because calling `.get()` or `operator ->()` on a WeakPtr is more expensive than just a deref (WeakPtr is implemented with multiple pointers, a refcounted object, ...).

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth
• Robbie Gibson

Submit Requirements:

• Code-Coverage
• Code-Review

• No-Unresolved-Comments

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 6

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Thu, 04 Jul 2024 15:10:34 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Nicolas MacBeth (Gerrit)]

Attention needed from Robbie Gibson and Sylvain Defresne

Nicolas MacBeth added 3 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Nicolas MacBeth . resolved

thanks Sylvain

File ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Line 2152, Patchset 6: contextualPanelEntrypointIPHDidDismissWithFeature:*config->iph_feature

Sylvain Defresne . resolved

This will crash if `config.get()` returns null. Need to handle that case.

Nicolas MacBeth

yeah that's fair. we check it before calling this method but you're right technically it can change between the two. Thanks Sylvain! Done.

Line 2158, Patchset 6: [UIImage imageNamed:base::SysUTF8ToNSString(config->iph_image_name)];

Sylvain Defresne . resolved

Probably `CHECK(config)` at the beginning of the method. And then store a ref to it.

Maybe using

    ContextualPanelItemConfiguration& config_ref = CHECK_DEREF(config.get());

Because calling `.get()` or `operator ->()` on a WeakPtr is more expensive than just a deref (WeakPtr is implemented with multiple pointers, a refcounted object, ...).

Nicolas MacBeth

TIL about `CHECK_DEREF`, thanks!

Open in Gerrit

Related details

Attention is currently required from:

• Robbie Gibson
• Sylvain Defresne

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 7

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Comment-Date: Thu, 04 Jul 2024 17:30:41 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Sylvain Defresne <sdef...@chromium.org>

[findit-for-me@appspot.gserviceaccount.com (Gerrit)]

Attention needed from Nicolas MacBeth, Robbie Gibson and Sylvain Defresne

findit...@appspot.gserviceaccount.com voted Code-Coverage-1

This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.

• //ios/chrome/browser/browser_view/ui_bundled/browser_coordinator.mm

Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe

Code-Coverage

-1

Open in Gerrit

Related details

Attention is currently required from:

• Nicolas MacBeth
• Robbie Gibson
• Sylvain Defresne

Submit Requirements:

• Code-Coverage
• Code-Review

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I4f605da8262bc66263c5e43c753c9a2fa0e9d951

Gerrit-Change-Number: 5639433

Gerrit-PatchSet: 7

Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Reviewer: Robbie Gibson <rkgi...@google.com>

Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Reviewer: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>

Gerrit-Attention: Robbie Gibson <rkgi...@google.com>

Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>

Gerrit-Comment-Date: Thu, 04 Jul 2024 18:20:15 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes