Richard Chui (Gerrit)

unread,

Jun 24, 2024, 7:20:13 PM (8 days ago) Jun 24

to Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Sean Kau

Richard Chui voted and added 1 comment

Votes added by Richard Chui

Commit-Queue

+1

1 comment

File chrome/browser/ui/ash/ash_web_view_impl.h

Line 80, Patchset 3 (Latest): std::string GetTitleForMediaControls(

Richard Chui . unresolved

@sk...@chromium.org do you think this is fine to override here, or should we create our own `WebContentsDelegate` to override `GetTitleForMediaControls`?

Open in Gerrit

Related details

Attention is currently required from:

Sean Kau

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sean Kau (Gerrit)

unread,

Jun 24, 2024, 7:43:38 PM (8 days ago) Jun 24

to Richard Chui, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Richard Chui

Sean Kau added 2 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Sean Kau . resolved

Is it possible to plumb this via InitParams instead?

File chrome/browser/ui/ash/ash_web_view_impl.cc

Line 196, Patchset 3 (Latest):

  // If the url is for the Focus Mode media player, then we should display the
  // playlist information.
  if (web_contents->GetLastCommittedURL() ==
      GURL(chrome::kChromeUIFocusModeMediaURL)) {
    return ash::focus_mode_util::GetSourceTitleForMediaControls();
  }

  // Otherwise, preserve the default behavior.
  return {};

Sean Kau . unresolved

Does this need to be dynamic? i.e. if we know this when the web view is created, we could pass it in via params instead of trying to look it up like this. This is too specific to focus mode for this class.

Open in Gerrit

Related details

Attention is currently required from:

Richard Chui

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

Jun 25, 2024, 8:14:02 PM (7 days ago) Jun 25

to Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Sean Kau

Richard Chui added 1 comment

File chrome/browser/ui/ash/ash_web_view_impl.cc

Line 196, Patchset 3: // If the url is for the Focus Mode media player, then we should display the


  // playlist information.
  if (web_contents->GetLastCommittedURL() ==
      GURL(chrome::kChromeUIFocusModeMediaURL)) {
    return ash::focus_mode_util::GetSourceTitleForMediaControls();
  }

  // Otherwise, preserve the default behavior.
  return {};

Sean Kau . unresolved

Does this need to be dynamic? i.e. if we know this when the web view is created, we could pass it in via params instead of trying to look it up like this. This is too specific to focus mode for this class.

Richard Chui

Done, how does this look?

Open in Gerrit

Related details

Attention is currently required from:

Sean Kau

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sean Kau (Gerrit)

unread,

Jun 27, 2024, 1:37:05 PM (5 days ago) Jun 27

to Richard Chui, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Richard Chui

Sean Kau added 4 comments

File ash/system/focus_mode/focus_mode_util.cc

Line 104, Patchset 6 (Latest):std::string GetSourceTitleForMediaControls() {

Sean Kau . unresolved

Can this take the playlist as a parameter? Then you can test it without mocking everything.

File chrome/browser/ui/ash/ash_web_view_impl.h

Line 80, Patchset 3: std::string GetTitleForMediaControls(

Richard Chui . unresolved

@sk...@chromium.org do you think this is fine to override here, or should we create our own `WebContentsDelegate` to override `GetTitleForMediaControls`?

Sean Kau

This is probably fine.

File chrome/browser/ui/ash/ash_web_view_impl.cc

Line 193, Patchset 6 (Latest): return params_.source_title;

Sean Kau . unresolved

I'm pretty sure this needs a fallback if its unset.

Line 196, Patchset 3:

  // If the url is for the Focus Mode media player, then we should display the
  // playlist information.
  if (web_contents->GetLastCommittedURL() ==
      GURL(chrome::kChromeUIFocusModeMediaURL)) {
    return ash::focus_mode_util::GetSourceTitleForMediaControls();
  }

  // Otherwise, preserve the default behavior.
  return {};

Sean Kau . resolved

Does this need to be dynamic? i.e. if we know this when the web view is created, we could pass it in via params instead of trying to look it up like this. This is too specific to focus mode for this class.

Richard Chui

Done, how does this look?

Sean Kau

Done

Open in Gerrit

Related details

Attention is currently required from:

Richard Chui

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

Jun 27, 2024, 5:19:19 PM (5 days ago) Jun 27

to Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Sean Kau

Richard Chui added 2 comments

File chrome/browser/ui/ash/ash_web_view_impl.h

Line 80, Patchset 3: std::string GetTitleForMediaControls(

Richard Chui . resolved

@sk...@chromium.org do you think this is fine to override here, or should we create our own `WebContentsDelegate` to override `GetTitleForMediaControls`?

Sean Kau

This is probably fine.

Richard Chui

Acknowledged

File chrome/browser/ui/ash/ash_web_view_impl.cc

Line 193, Patchset 6 (Latest): return params_.source_title;

Sean Kau . unresolved

I'm pretty sure this needs a fallback if its unset.

Richard Chui

I don't think it does? The default behavior currently returns `{}`, and that should be what `source_title` is if unset, correct?

Open in Gerrit

Related details

Attention is currently required from:

Sean Kau

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

Jun 28, 2024, 3:45:02 AM (5 days ago) Jun 28

to Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Sean Kau

Richard Chui added 2 comments

File ash/system/focus_mode/focus_mode_util.cc

Line 104, Patchset 6:std::string GetSourceTitleForMediaControls() {

Sean Kau . resolved

Can this take the playlist as a parameter? Then you can test it without mocking everything.

Richard Chui

Done

File chrome/browser/ui/ash/ash_web_view_impl.cc

Line 193, Patchset 6: return params_.source_title;

Sean Kau . resolved

I'm pretty sure this needs a fallback if its unset.

Richard Chui

I don't think it does? The default behavior currently returns `{}`, and that should be what `source_title` is if unset, correct?

Richard Chui

Discussed offline, just for futureproofing we will return the parent behavior.

Open in Gerrit

Related details

Attention is currently required from:

Sean Kau

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

Sean Kau (Gerrit)

unread,

Jun 28, 2024, 5:31:06 PM (4 days ago) Jun 28

to Richard Chui, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Richard Chui

Sean Kau voted and added 2 comments

Votes added by Sean Kau

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Sean Kau . resolved

lgtm % test. Thanks for addressing the comments.

File ash/system/focus_mode/focus_mode_util_unittest.cc

Line 11, Patchset 9 (Latest):

TEST(FocusModeUtilTests, VerifySourceTitle) {
  // Verify that missing `id` or invalid playlist type results in an empty
  // string.
  SelectedPlaylist selected_playlist;
  EXPECT_TRUE(GetSourceTitleForMediaControls(selected_playlist).empty());
  selected_playlist.id = "id0";
  EXPECT_TRUE(GetSourceTitleForMediaControls(selected_playlist).empty());

  // Verify that having a missing playlist title will still return the playlist
  // type as a string.
  selected_playlist.type = SoundType::kYouTubeMusic;
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist), "YouTube Music");

  // Verify a fully formed YTM string.
  selected_playlist.title = "Playlist Title";
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist),
            "YouTube Music ᐧ Playlist Title");

  // Verify a fully formed Soundscape string.
  selected_playlist.type = SoundType::kSoundscape;
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist),
            "Focus sounds ᐧ Playlist Title");
}

Sean Kau . unresolved

This should be 4 separate tests

Open in Gerrit

Related details

Attention is currently required from:

Richard Chui

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

Jul 1, 2024, 7:01:53 PM (yesterday) Jul 1

to James Cook, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from James Cook and Polina Bondarenko

Richard Chui added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Richard Chui . resolved

Hi all, can I get an owners review?

jamescook@

a/p/c/ash_web_view
c/b/u/a/ash_web_view_impl

pbond@

a/c/remote_maintenance_curtain_view

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Polina Bondarenko

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

Jul 1, 2024, 7:07:53 PM (yesterday) Jul 1

to James Cook, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from James Cook and Polina Bondarenko

Richard Chui added 1 comment

File ash/system/focus_mode/focus_mode_util_unittest.cc

Line 11, Patchset 9:TEST(FocusModeUtilTests, VerifySourceTitle) {


  // Verify that missing `id` or invalid playlist type results in an empty
  // string.
  SelectedPlaylist selected_playlist;
  EXPECT_TRUE(GetSourceTitleForMediaControls(selected_playlist).empty());
  selected_playlist.id = "id0";
  EXPECT_TRUE(GetSourceTitleForMediaControls(selected_playlist).empty());

  // Verify that having a missing playlist title will still return the playlist
  // type as a string.
  selected_playlist.type = SoundType::kYouTubeMusic;
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist), "YouTube Music");

  // Verify a fully formed YTM string.
  selected_playlist.title = "Playlist Title";
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist),
            "YouTube Music ᐧ Playlist Title");

  // Verify a fully formed Soundscape string.
  selected_playlist.type = SoundType::kSoundscape;
  EXPECT_EQ(GetSourceTitleForMediaControls(selected_playlist),
            "Focus sounds ᐧ Playlist Title");
}

Sean Kau . resolved

This should be 4 separate tests

Richard Chui

Done

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Polina Bondarenko

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

Polina Bondarenko (Gerrit)

unread,

7:15 AM (16 hours ago) 7:15 AM

to Richard Chui, James Cook, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from James Cook and Richard Chui

Polina Bondarenko added 1 comment

File ash/curtain/remote_maintenance_curtain_view.cc

Line 77, Patchset 10 (Latest):

  AshWebView::InitParams web_view_params;
  web_view_params.rounded_corners =
      gfx::RoundedCornersF(util().GetCornerRadius());

  curtain_view_ =
      AddChildView(AshWebViewFactory::Get()->Create(web_view_params));

Polina Bondarenko . unresolved

Hi Richard,

Why do you need this code?

Also web_view_params are not being used afterwards, please make them either local or use std::move() to explicitly forbid further usage in the AddCurtainWebView() function.

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Richard Chui

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

1:50 PM (10 hours ago) 1:50 PM

to James Cook, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from James Cook and Polina Bondarenko

Richard Chui added 1 comment

File ash/curtain/remote_maintenance_curtain_view.cc

Line 77, Patchset 10: AshWebView::InitParams web_view_params;


  web_view_params.rounded_corners =
      gfx::RoundedCornersF(util().GetCornerRadius());

  curtain_view_ =
      AddChildView(AshWebViewFactory::Get()->Create(web_view_params));

Polina Bondarenko . unresolved

Hi Richard,
Why do you need this code?
Also web_view_params are not being used afterwards, please make them either local or use std::move() to explicitly forbid further usage in the AddCurtainWebView() function.

Richard Chui

This is because because updates to AshWebView now require it to have a defined constructor/destructor (since it’s now considered complex), so it is no longer an aggregate type.

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Polina Bondarenko

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

6:36 PM (5 hours ago) 6:36 PM

to Xiyuan Xia, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Polina Bondarenko and Xiyuan Xia

Richard Chui added 1 comment

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Richard Chui . resolved

Hi Xiyuan, can you do an owners review for:

a/p/c/ash_web_view
c/b/u/a/ash_web_view_impl

Open in Gerrit

Related details

Attention is currently required from:

Polina Bondarenko
Xiyuan Xia

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Xiyuan Xia (Gerrit)

unread,

7:23 PM (4 hours ago) 7:23 PM

to Richard Chui, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Polina Bondarenko and Richard Chui

Xiyuan Xia voted and added 2 comments

Votes added by Xiyuan Xia

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Xiyuan Xia . resolved

lgtm

File ash/public/cpp/ash_web_view.h

Line 34, Patchset 11 (Latest): InitParams(const InitParams&);

Xiyuan Xia . unresolved

nit: Also add an assignment op. And devs might want it to be movable too.

Open in Gerrit

Related details

Attention is currently required from:

Polina Bondarenko
Richard Chui

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Richard Chui (Gerrit)

unread,

8:04 PM (3 hours ago) 8:04 PM

to Xiyuan Xia, Polina Bondarenko, Sean Kau, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Polina Bondarenko

Richard Chui voted and added 1 comment

Votes added by Richard Chui

Commit-Queue

+1

1 comment

File ash/public/cpp/ash_web_view.h

Line 34, Patchset 11: InitParams(const InitParams&);

Xiyuan Xia . resolved

nit: Also add an assignment op. And devs might want it to be movable too.

Richard Chui

Thanks for pointing this out, added!

Open in Gerrit

Related details

Attention is currently required from:

Polina Bondarenko

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sean Kau (Gerrit)

unread,

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

to Richard Chui, Xiyuan Xia, Polina Bondarenko, Chromium LUCI CQ, chromium...@chromium.org, blundell+...@chromium.org

Attention needed from Polina Bondarenko, Richard Chui and Xiyuan Xia

Sean Kau voted and added 1 comment

Votes added by Sean Kau

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Sean Kau . resolved

lgtm

Open in Gerrit

Related details

Attention is currently required from:

Polina Bondarenko
Richard Chui
Xiyuan Xia

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

focus-mode: Update media controls source title [chromium/src : main]

Richard Chui (Gerrit)

Richard Chui voted and added 1 comment

Votes added by Richard Chui

1 comment

Related details

Sean Kau (Gerrit)

Sean Kau added 2 comments

Related details

Richard Chui (Gerrit)

Richard Chui added 1 comment

Related details

Sean Kau (Gerrit)

Sean Kau added 4 comments

Related details

Richard Chui (Gerrit)

Richard Chui added 2 comments

Related details

Richard Chui (Gerrit)

Richard Chui added 2 comments

Related details

Sean Kau (Gerrit)

Sean Kau voted and added 2 comments

Votes added by Sean Kau

2 comments

Related details

Richard Chui (Gerrit)

Richard Chui added 1 comment

Related details

Richard Chui (Gerrit)

Richard Chui added 1 comment

Related details

Polina Bondarenko (Gerrit)

Polina Bondarenko added 1 comment

Related details

Richard Chui (Gerrit)

Richard Chui added 1 comment

Related details

Richard Chui (Gerrit)

Richard Chui added 1 comment

Related details

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 2 comments

Votes added by Xiyuan Xia

2 comments

Related details

Richard Chui (Gerrit)

Richard Chui voted and added 1 comment

Votes added by Richard Chui

1 comment

Related details

Sean Kau (Gerrit)

Sean Kau voted and added 1 comment

Votes added by Sean Kau

1 comment

Related details