Christian Xu (Gerrit)

unread,

Jul 2, 2024, 5:25:41 AM (3 days ago) Jul 2

to Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Gauthier Ambard and Stepan Khapugin

Christian Xu voted

Auto-Submit	+1
Commit-Queue	+1

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard
Stepan Khapugin

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

Christian Xu (Gerrit)

unread,

Jul 2, 2024, 5:25:51 AM (3 days ago) Jul 2

to Chromium LUCI CQ, Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Gauthier Ambard and Stepan Khapugin

Christian Xu voted Auto-Submit+0

Auto-Submit

+0

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard
Stepan Khapugin

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

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

unread,

Jul 2, 2024, 6:25:48 AM (3 days ago) Jul 2

to Christian Xu, Chromium LUCI CQ, Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu, Gauthier Ambard and Stepan Khapugin

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/lens_overlay/coordinator/lens_overlay_coordinator.mm
//ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.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:

Christian Xu
Gauthier Ambard
Stepan Khapugin

Submit Requirements:

Code-Coverage

Code-Review

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Stepan Khapugin (Gerrit)

unread,

Jul 2, 2024, 8:21:13 AM (3 days ago) Jul 2

to Christian Xu, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu and Gauthier Ambard

Stepan Khapugin added 3 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 81, Patchset 4 (Latest): !net::IsGoogleHostWithAlpnH3(base::SysNSStringToUTF8(request.URL.host))) {

Stepan Khapugin . unresolved

the comment on this method is "This should only be used for histograms and shouldn't be used to affect behavior."

Line 85, Patchset 4 (Latest): DCHECK(!URL.IsAboutBlank());

Stepan Khapugin . unresolved

should this be CHECK()? Or should we just add handling for this? (e.g. allow navigation?)

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_delegate.h

Line 11, Patchset 4 (Latest):

/// Called when the result page opens a new tab.
- (void)resultPageDidOpenNewTab;

Stepan Khapugin . unresolved

This should be unnecessary. Instead, I think we can rely on https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/lens_overlay/model/lens_overlay_tab_helper.mm;l=38?q=lens_overlay_tab_helper to hide the UI

Open in Gerrit

Related details

Attention is currently required from:

Christian Xu
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Stepan Khapugin (Gerrit)

unread,

Jul 2, 2024, 8:21:52 AM (3 days ago) Jul 2

to Christian Xu, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu and Gauthier Ambard

Stepan Khapugin added 1 comment

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 76, Patchset 4 (Latest):


- (void)shouldAllowRequest:(NSURLRequest*)request
               requestInfo:(web::WebStatePolicyDecider::RequestInfo)requestInfo
           decisionHandler:(PolicyDecisionHandler)decisionHandler {

Stepan Khapugin . unresolved

Can you add tests for this?

blocking_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

Jul 3, 2024, 10:12:59 AM (2 days ago) Jul 3

to Christian Xu, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu

Gauthier Ambard added 2 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.h

Line 25, Patchset 4 (Latest):@property(nonatomic, weak) id<ApplicationCommands> applicationCommandsHandler;

Gauthier Ambard . unresolved

Nit: remove "commands" go/bling-best-practices.

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 85, Patchset 4 (Latest): DCHECK(!URL.IsAboutBlank());

Stepan Khapugin . unresolved

should this be CHECK()? Or should we just add handling for this? (e.g. allow navigation?)

Gauthier Ambard

No new DCHECK should be added. Use CHECK or early return.

Open in Gerrit

Related details

Attention is currently required from:

Christian Xu

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 3, 2024, 11:45:57 AM (2 days ago) Jul 3

to findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Gauthier Ambard and Stepan Khapugin

Christian Xu voted and added 5 comments

Votes added by Christian Xu

Auto-Submit	+1
Commit-Queue	+1

5 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.h

Line 25, Patchset 4:@property(nonatomic, weak) id<ApplicationCommands> applicationCommandsHandler;

Gauthier Ambard . resolved

Nit: remove "commands" go/bling-best-practices.

Christian Xu

Done

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 76, Patchset 4:


- (void)shouldAllowRequest:(NSURLRequest*)request
               requestInfo:(web::WebStatePolicyDecider::RequestInfo)requestInfo
           decisionHandler:(PolicyDecisionHandler)decisionHandler {

Stepan Khapugin . resolved

Can you add tests for this?

Christian Xu

Done

Line 81, Patchset 4: !net::IsGoogleHostWithAlpnH3(base::SysNSStringToUTF8(request.URL.host))) {

Stepan Khapugin . resolved

the comment on this method is "This should only be used for histograms and shouldn't be used to affect behavior."

Christian Xu

Done

Line 85, Patchset 4: DCHECK(!URL.IsAboutBlank());

Stepan Khapugin . resolved

should this be CHECK()? Or should we just add handling for this? (e.g. allow navigation?)

Christian Xu

Done, I was using the wrong OpenNewTab command.

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_delegate.h

Line 11, Patchset 4:

/// Called when the result page opens a new tab.
- (void)resultPageDidOpenNewTab;

Stepan Khapugin . resolved

This should be unnecessary. Instead, I think we can rely on https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/lens_overlay/model/lens_overlay_tab_helper.mm;l=38?q=lens_overlay_tab_helper to hide the UI

Christian Xu

Done

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard
Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

Jul 3, 2024, 11:56:49 AM (2 days ago) Jul 3

to Christian Xu, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu and Stepan Khapugin

Gauthier Ambard added 2 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator+testing.h

Line 14, Patchset 7 (Latest):@interface LensResultPageMediator (Testing)

Gauthier Ambard . unresolved

Instead of that, I think I would rather:
1. Assert that the mediator is conforming to `CRWWebStatePolicyDecider` in a test
2. Cast it to `id<CRWWebStatePolicyDecider>` and then call the method on it.

And remove this file.

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 120, Patchset 7 (Latest): [mock_application_handler_ verify];

Gauthier Ambard . unresolved

Nit: Use EXPECT_OCMOCK_VERIFY. Otherwise it won't play nicely with gtest.

Open in Gerrit

Related details

Attention is currently required from:

Christian Xu
Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 3, 2024, 12:19:09 PM (2 days ago) Jul 3

to findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Gauthier Ambard and Stepan Khapugin

Christian Xu voted and added 2 comments

Votes added by Christian Xu

Auto-Submit	+1
Commit-Queue	+1

2 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator+testing.h

Line 14, Patchset 7:@interface LensResultPageMediator (Testing)

Gauthier Ambard . resolved

Instead of that, I think I would rather:
1. Assert that the mediator is conforming to `CRWWebStatePolicyDecider` in a test
2. Cast it to `id<CRWWebStatePolicyDecider>` and then call the method on it.
And remove this file.

Christian Xu

Done

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 120, Patchset 7: [mock_application_handler_ verify];

Gauthier Ambard . resolved

Nit: Use EXPECT_OCMOCK_VERIFY. Otherwise it won't play nicely with gtest.

Christian Xu

Done

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard
Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

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

unread,

Jul 3, 2024, 1:07:14 PM (2 days ago) Jul 3

to Christian Xu, Chromium LUCI CQ, Stepan Khapugin, Gauthier Ambard, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu, Gauthier Ambard and Stepan Khapugin

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/lens_overlay/coordinator/lens_overlay_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:

Christian Xu
Gauthier Ambard
Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

Jul 4, 2024, 4:22:02 AM (yesterday) Jul 4

to Christian Xu, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu and Stepan Khapugin

Gauthier Ambard voted and added 2 comments

Votes added by Gauthier Ambard

Code-Review

+1

2 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 82, Patchset 10 (Latest): EXPECT_TRUE(

Gauthier Ambard . unresolved

Nit: this should be an ASSERT_TRUE, not EXPECT. Assert will stop the execution of the test and will prevent a crash after if it doesn't conform.

Line 84, Patchset 10 (Latest): [(id<CRWWebStatePolicyDecider>)mediator_

Gauthier Ambard . unresolved

Nit: use a static_cast and an intermediate variable.

Open in Gerrit

Related details

Attention is currently required from:

Christian Xu
Stepan Khapugin

Submit Requirements:

Code-Coverage

Code-Review
No-Unresolved-Comments

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

Gerrit

blocking_requirement

satisfied_requirement

unsatisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 4, 2024, 5:22:18 AM (23 hours ago) Jul 4

to Gauthier Ambard, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Stepan Khapugin

Christian Xu voted and added 2 comments

Votes added by Christian Xu

Auto-Submit

+1

2 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 82, Patchset 10: EXPECT_TRUE(

Gauthier Ambard . resolved

Nit: this should be an ASSERT_TRUE, not EXPECT. Assert will stop the execution of the test and will prevent a crash after if it doesn't conform.

Christian Xu

ASSERT_TRUE will fails to void, which doesn't conform to the function's return type. Replaced with an if condition to avoid crash. The `EXPECT_TRUE(callback_called)` below should be sufficient.

Line 84, Patchset 10: [(id<CRWWebStatePolicyDecider>)mediator_

Gauthier Ambard . resolved

Nit: use a static_cast and an intermediate variable.

Christian Xu

Done

Open in Gerrit

Related details

Attention is currently required from:

Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

satisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 4, 2024, 5:22:36 AM (23 hours ago) Jul 4

to Gauthier Ambard, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Stepan Khapugin

Christian Xu voted

Auto-Submit	+1
Commit-Queue	+1

Open in Gerrit

Related details

Attention is currently required from:

Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

satisfied_requirement

open

diffy

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

unread,

Jul 4, 2024, 6:43:48 AM (22 hours ago) Jul 4

to Christian Xu, Gauthier Ambard, Chromium LUCI CQ, Stepan Khapugin, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu and Stepan Khapugin

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/lens_overlay/coordinator/lens_overlay_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:

Christian Xu
Stepan Khapugin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

satisfied_requirement

open

diffy

Stepan Khapugin (Gerrit)

unread,

Jul 4, 2024, 8:14:05 AM (20 hours ago) Jul 4

to Christian Xu, Gauthier Ambard, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu

Stepan Khapugin voted and added 5 comments

Votes added by Stepan Khapugin

Code-Review

+1

5 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_overlay_coordinator.mm

Line 209, Patchset 12 (Latest): isIncognito:browser->GetBrowserState()->IsOffTheRecord()];

Stepan Khapugin . resolved

I think we're not planning for the feature to be available in incognito, but let's keep this for now.

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 26, Patchset 12 (Latest):BOOL IsNavigationAllowed(const GURL& URL) {

Stepan Khapugin . unresolved

nit, naming: "allowed" sounds like the antonym is "blocked", not "open in new tab".
How about `IsValidURLToOpenInResultsPage` or we reverse the return arg and `URLShouldOpenInNewTab` ?

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 61, Patchset 12 (Latest): [[nodiscard]] bool TestShouldAllowRequest(

Stepan Khapugin . unresolved

nit: add comment

Line 64, Patchset 12 (Latest):

      ui::PageTransition transition_type =
          ui::PageTransition::PAGE_TRANSITION_LINK) {

Stepan Khapugin . unresolved

optional nit: do we need this, even if we never override this?

Line 127, Patchset 12 (Latest):// Tests that any navigation that's not on main frame is allowed.

Stepan Khapugin . unresolved

since it's "any" navigation, can we also test with google.com here?

Open in Gerrit

Related details

Attention is currently required from:

Christian Xu

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

blocking_requirement

satisfied_requirement

unsatisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 4, 2024, 9:01:34 AM (19 hours ago) Jul 4

to Stepan Khapugin, Gauthier Ambard, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Christian Xu voted and added 4 comments

Votes added by Christian Xu

Auto-Submit	+1
Commit-Queue	+2

4 comments

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm

Line 26, Patchset 12:BOOL IsNavigationAllowed(const GURL& URL) {

Stepan Khapugin . resolved

nit, naming: "allowed" sounds like the antonym is "blocked", not "open in new tab".
How about `IsValidURLToOpenInResultsPage` or we reverse the return arg and `URLShouldOpenInNewTab` ?

Christian Xu

Done

File ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Line 61, Patchset 12: [[nodiscard]] bool TestShouldAllowRequest(

Stepan Khapugin . resolved

nit: add comment

Christian Xu

Done

Line 64, Patchset 12:

      ui::PageTransition transition_type =
          ui::PageTransition::PAGE_TRANSITION_LINK) {

Stepan Khapugin . resolved

optional nit: do we need this, even if we never override this?

Christian Xu

Done

Line 127, Patchset 12:// Tests that any navigation that's not on main frame is allowed.

Stepan Khapugin . resolved

since it's "any" navigation, can we also test with google.com here?

Christian Xu

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

blocking_requirement

satisfied_requirement

open

diffy

Christian Xu (Gerrit)

unread,

Jul 4, 2024, 10:21:07 AM (18 hours ago) Jul 4

to Stepan Khapugin, Gauthier Ambard, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Christian Xu voted

Auto-Submit	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention set is empty

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

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

unread,

Jul 4, 2024, 11:27:36 AM (17 hours ago) Jul 4

to Christian Xu, Stepan Khapugin, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Attention needed from Christian Xu

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/lens_overlay/coordinator/lens_overlay_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:

Christian Xu

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

Chromium LUCI CQ (Gerrit)

unread,

Jul 4, 2024, 11:27:55 AM (17 hours ago) Jul 4

to Christian Xu, Stepan Khapugin, Gauthier Ambard, findit...@appspot.gserviceaccount.com, chromium...@chromium.org, AyeAye, feature-me...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-r...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm
Insertions: 2, Deletions: 2.

@@ -23,7 +23,7 @@
 namespace {
 
 /// Returns whether the navigation is allowed inside of the result page.
-BOOL IsNavigationAllowed(const GURL& URL) {
+BOOL IsValidURLToOpenInResultsPage(const GURL& URL) {
   std::string_view host = URL.host_piece();
   return base::EqualsCaseInsensitiveASCII(host, "google.com") ||
          base::EqualsCaseInsensitiveASCII(host, "www.google.com");
@@ -93,7 +93,7 @@


                requestInfo:(web::WebStatePolicyDecider::RequestInfo)requestInfo
            decisionHandler:(PolicyDecisionHandler)decisionHandler {

   GURL URL = net::GURLWithNSURL(request.URL);
-  if (requestInfo.target_frame_is_main && !IsNavigationAllowed(URL)) {
+  if (requestInfo.target_frame_is_main && !IsValidURLToOpenInResultsPage(URL)) {
     decisionHandler(web::WebStatePolicyDecider::PolicyDecision::Cancel());
     OpenNewTabCommand* command =
         [[OpenNewTabCommand alloc] initWithURL:URL
```
```
The name of the file: ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm
Insertions: 6, Deletions: 6.

@@ -58,14 +58,12 @@
     PlatformTest::TearDown();
   }
 
-  [[nodiscard]] bool TestShouldAllowRequest(
-      NSString* url_string,
-      bool target_frame_is_main,
-      ui::PageTransition transition_type =
-          ui::PageTransition::PAGE_TRANSITION_LINK) {
+  // Tests whether the navigation is allowed by the policy provider.
+  [[nodiscard]] bool TestShouldAllowRequest(NSString* url_string,
+                                            bool target_frame_is_main) {
     NSURL* url = [NSURL URLWithString:url_string];
     const web::WebStatePolicyDecider::RequestInfo request_info(
-        transition_type, target_frame_is_main,
+        ui::PageTransition::PAGE_TRANSITION_LINK, target_frame_is_main,
         /*target_frame_is_cross_origin=*/false,
         /*target_window_is_cross_origin=*/false,
         /*is_user_initiated=*/true,
@@ -128,6 +126,8 @@
 TEST_F(LensResultPageMediatorTest, ShouldAllowAnyNavigationNotInMainFrame) {
   EXPECT_TRUE(TestShouldAllowRequest(@"https://www.chromium.com",
                                      /*target_frame_is_main=*/false));
+  EXPECT_TRUE(TestShouldAllowRequest(@"https://www.google.com",
+                                     /*target_frame_is_main=*/false));
 }
 
 }  // namespace
```

Change information

Commit message:

[iOS][LO][LRP] Add web state policy decider

Add a web state policy decider in LensResultPageMediator to block
navigation to non-LRP links. These links are opened in a new tab.

Bug: 347232740

Change-Id: Ie8f432f9e2f0223107347e7ac8d91aa8a755b9fc

Low-Coverage-Reason: TESTS_IN_SEPARATE_CL cooordinator test in separate CL

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660424

Reviewed-by: Stepan Khapugin <stkha...@chromium.org>

Commit-Queue: Christian Xu <chris...@chromium.org>

Reviewed-by: Gauthier Ambard <gam...@chromium.org>

Auto-Submit: Christian Xu <chris...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1323368}

Files:

M ios/chrome/browser/lens_overlay/coordinator/BUILD.gn
M ios/chrome/browser/lens_overlay/coordinator/lens_overlay_coordinator.mm
M ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.h
M ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator.mm
M ios/chrome/browser/lens_overlay/coordinator/lens_result_page_mediator_unittest.mm

Change size: M

Delta: 5 files changed, 138 insertions(+), 8 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Coverage: -1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Gauthier Ambard, +1 by Stepan Khapugin

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[iOS][LO][LRP] Add web state policy decider [chromium/src : main]

Christian Xu (Gerrit)

Christian Xu voted

Related details

Christian Xu (Gerrit)

Christian Xu voted Auto-Submit+0

Related details

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

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

Related details

Stepan Khapugin (Gerrit)

Stepan Khapugin added 3 comments

Related details

Stepan Khapugin (Gerrit)

Stepan Khapugin added 1 comment

Gauthier Ambard (Gerrit)

Gauthier Ambard added 2 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted and added 5 comments

Votes added by Christian Xu

5 comments

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard added 2 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted and added 2 comments

Votes added by Christian Xu

2 comments

Related details

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

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

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard voted and added 2 comments

Votes added by Gauthier Ambard

2 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted and added 2 comments

Votes added by Christian Xu

2 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted

Related details

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

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

Related details

Stepan Khapugin (Gerrit)

Stepan Khapugin voted and added 5 comments

Votes added by Stepan Khapugin

5 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted and added 4 comments

Votes added by Christian Xu

4 comments

Related details

Christian Xu (Gerrit)

Christian Xu voted

Related details

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

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

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information