Darryl James (Gerrit)

unread,

6:44 PM (2 hours ago) 6:44 PM

to Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising

Darryl James added 2 comments

File chrome/browser/ui/views/side_panel/side_panel_interactive_uitest.cc

Line 299, Patchset 1 (Latest):

    if (layout_manager && layout_manager->is_animating()) {
      views::test::WaitForAnimatingLayoutManager(layout_manager);
    }

Darryl James . resolved

This is to ensure no animations are running before the test actually starts 👍

Line 539, Patchset 1 (Latest):

  SidePanelUI* const side_panel_ui =
      browser()->browser_window_features()->side_panel_ui();
  side_panel_ui->SetNoDelaysForTesting(true);

Darryl James . resolved

This is to help reduce test timeouts (I was experiencing this quite a bit locally).

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kristi Saney (Gerrit)

unread,

6:58 PM (2 hours ago) 6:58 PM

to Darryl James, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Darryl James

Kristi Saney added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1 (Latest):

    if (side_panel_trigger >=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . unresolved

why is this extra check needed? shouldn't this always be true?

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Darryl James

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

7:03 PM (2 hours ago) 7:03 PM

to Kristi Saney, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Kristi Saney

Darryl James added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1: if (side_panel_trigger >=


            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . unresolved

why is this extra check needed? shouldn't this always be true?

Darryl James

Good point - it shouldn't be needed and now that I look closer maybe the top path is also not needed?
Updating this or at least can be consolidated

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Kristi Saney

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kristi Saney (Gerrit)

unread,

7:05 PM (2 hours ago) 7:05 PM

to Darryl James, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Darryl James

Kristi Saney added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1:

    if (side_panel_trigger >=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . unresolved

why is this extra check needed? shouldn't this always be true?

Darryl James

Good point - it shouldn't be needed and now that I look closer maybe the top path is also not needed?
Updating this or at least can be consolidated

Kristi Saney

Nice. Can we still return early though if the trigger doesn't map to anything?

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Darryl James

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

7:07 PM (2 hours ago) 7:07 PM

to Kristi Saney, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Kristi Saney

Darryl James added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1:

    if (side_panel_trigger >=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . unresolved

why is this extra check needed? shouldn't this always be true?

Darryl James

Good point - it shouldn't be needed and now that I look closer maybe the top path is also not needed?
Updating this or at least can be consolidated

Kristi Saney

Nice. Can we still return early though if the trigger doesn't map to anything?

Darryl James

Yes! Before that I am double checking the implemetation of `SidePanelToReadAnythingOpenTrigger` since it seems to be missing some values?

Not clear if that is intentional yet - need to run the tests locally first 👍

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Kristi Saney

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

7:13 PM (2 hours ago) 7:13 PM

to Kristi Saney, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Kristi Saney

Darryl James added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1:

    if (side_panel_trigger >=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . unresolved

why is this extra check needed? shouldn't this always be true?

Darryl James

Good point - it shouldn't be needed and now that I look closer maybe the top path is also not needed?
Updating this or at least can be consolidated

Kristi Saney

Nice. Can we still return early though if the trigger doesn't map to anything?

Darryl James

Yes! Before that I am double checking the implemetation of `SidePanelToReadAnythingOpenTrigger` since it seems to be missing some values?
Not clear if that is intentional yet - need to run the tests locally first 👍

Darryl James

Okay so I think as part of a separate CL we can update the logic in the `SidePanelToReadAnythingOpenTrigger` to return unknown for unhandled cases, and remove the std::optional<> return type. This way all of the values in the enum must be handled otherwise we get a compile error.

I'll do that in a followup since that will likely affect other usages of the function, let me know if that is okay / not okay 🙏

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Kristi Saney

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

7:16 PM (2 hours ago) 7:16 PM

to Kristi Saney, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Caroline Rising and Kristi Saney

Darryl James added 1 comment

File chrome/browser/ui/read_anything/read_anything_entry_point_controller.cc

Line 258, Patchset 1:

    if (side_panel_trigger >=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMinValue) &&
        side_panel_trigger <=
            static_cast<std::underlying_type_t<SidePanelOpenTrigger>>(
                SidePanelOpenTrigger::kMaxValue)) {
      mapped_trigger = SidePanelToReadAnythingOpenTrigger(
          static_cast<SidePanelOpenTrigger>(side_panel_trigger));
    }

Kristi Saney . resolved

why is this extra check needed? shouldn't this always be true?

Darryl James

Good point - it shouldn't be needed and now that I look closer maybe the top path is also not needed?
Updating this or at least can be consolidated

Kristi Saney

Nice. Can we still return early though if the trigger doesn't map to anything?

Darryl James

Yes! Before that I am double checking the implemetation of `SidePanelToReadAnythingOpenTrigger` since it seems to be missing some values?
Not clear if that is intentional yet - need to run the tests locally first 👍

Darryl James

Okay so I think as part of a separate CL we can update the logic in the `SidePanelToReadAnythingOpenTrigger` to return unknown for unhandled cases, and remove the std::optional<> return type. This way all of the values in the enum must be handled otherwise we get a compile error.
I'll do that in a followup since that will likely affect other usages of the function, let me know if that is okay / not okay 🙏

Darryl James

I'll go ahead and spin that up actually just in case, thanks for catching this!

Open in Gerrit

Related details

Attention is currently required from:

Caroline Rising
Kristi Saney

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Caroline Rising (Gerrit)

unread,

8:07 PM (1 hour ago) 8:07 PM

to Darryl James, Kristi Saney, Chromium LUCI CQ, chromium...@chromium.org, jatapiaro+wat...@google.com, lwinston+watc...@google.com, trewin...@google.com

Attention needed from Darryl James and Kristi Saney

Caroline Rising voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Darryl James
Kristi Saney

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

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

[BAM] Support toolbar button trigger in ReadAnythingEntryPointController [chromium/src : main]

Darryl James (Gerrit)

Darryl James added 2 comments

Related details

Kristi Saney (Gerrit)

Kristi Saney added 1 comment

Related details

Darryl James (Gerrit)

Darryl James added 1 comment

Related details

Kristi Saney (Gerrit)

Kristi Saney added 1 comment

Related details

Darryl James (Gerrit)

Darryl James added 1 comment

Related details

Darryl James (Gerrit)

Darryl James added 1 comment

Related details

Darryl James (Gerrit)

Darryl James added 1 comment

Related details

Caroline Rising (Gerrit)

Caroline Rising voted Code-Review+1

Related details