ben chin (Gerrit)

unread,

Jun 23, 2026, 3:27:17 AM (8 days ago) Jun 23

to devtools...@chromium.org, chromium...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

ben chin has uploaded the change for review

Commit message

Add "Inspect Element" to text selection dropdown menu

This CL adds the "Inspect Element" option to the text selection dropdown
menu on Android (triggered via mouse right-click on selected text). It
modifies ChromeSelectionDropdownMenuDelegate to evaluate if DevTools is
enabled and injects the inspect item into the ui::MenuModel, intercepting
the click to launch DevToolsWindow.

To make the label string available in C++ on Android, this CL also moves
IDS_CONTENT_CONTEXT_INSPECTELEMENT outside the `not is_android`
conditional block in generated_resources.grd.

Bug: 524045764

Change-Id: I967601ac053df3e45d9ed0e8c4855b4e81c11878

Change diff

Change information

Files:

M chrome/app/generated_resources.grd
M chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Change size: M

Delta: 2 files changed, 114 insertions(+), 9 deletions(-)

Open in Gerrit

Related details

Attention set is empty

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

ben chin (Gerrit)

unread,

Jun 23, 2026, 3:32:39 AM (8 days ago) Jun 23

to Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from Calder Kitagawa

ben chin voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Calder Kitagawa

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

ben chin (Gerrit)

unread,

Jun 23, 2026, 6:55:05 AM (8 days ago) Jun 23

to Chromium LUCI CQ, Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from Calder Kitagawa

ben chin voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Calder Kitagawa

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

Calder Kitagawa (Gerrit)

unread,

Jun 23, 2026, 12:05:17 PM (8 days ago) Jun 23

to ben chin, Jenna Himawan, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from Jenna Himawan and ben chin

Calder Kitagawa added 3 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Calder Kitagawa . resolved

Jenna could you also take a look since I'm not actually that familiar with this code?

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 74, Patchset 2 (Latest):class ChromeSelectionDropdownMenuModel : public ui::SimpleMenuModel,

Calder Kitagawa . unresolved

Should we override the other methods for this too?

Can we have just one implementation and share common logic and just wrap the extension related things in the buildflag?

Line 119, Patchset 2 (Latest):

  std::unique_ptr<ChromeSelectionDropdownMenuModel> model =
      std::make_unique<ChromeSelectionDropdownMenuModel>(render_frame_host,
                                                         params);
  model->PopulateModel();
  if (is_devtools_allowed) {
    if (model->GetItemCount() > 0) {
      model->AddSeparator(ui::NORMAL_SEPARATOR);
    }
    model->AddItemWithStringId(IDC_CONTENT_CONTEXT_INSPECTELEMENT,
                               IDS_CONTENT_CONTEXT_INSPECTELEMENT);
  }
  return std::move(model);
#else
  if (!is_devtools_allowed) {
    return nullptr;
  }
  std::unique_ptr<ChromeSelectionDropdownMenuModel> model =
      std::make_unique<ChromeSelectionDropdownMenuModel>(render_frame_host,
                                                         params);
  model->AddItemWithStringId(IDC_CONTENT_CONTEXT_INSPECTELEMENT,
                             IDS_CONTENT_CONTEXT_INSPECTELEMENT);
  return std::move(model);
#endif

Calder Kitagawa . unresolved

Again this is mostly duplicated. Combining it might be nicer?

Open in Gerrit

Related details

Attention is currently required from:

Jenna Himawan
ben chin

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

Jenna Himawan (Gerrit)

unread,

Jun 23, 2026, 4:19:47 PM (8 days ago) Jun 23

to ben chin, Chromium LUCI CQ, Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from ben chin

Jenna Himawan added 2 comments

File chrome/app/generated_resources.grd

Line 605, Patchset 2 (Latest):

        <message name="IDS_CONTENT_CONTEXT_INSPECTELEMENT" desc="The name of the Inspect Element command in the content area context menu">

Jenna Himawan . unresolved

I think we don't support & for accelerators in Android, and generally Clank has its own strings files instead of using strings in chrome/app.

Consider adding a dedicated Android string in chrome/browser/ui/android/strings/android_chrome_strings.grd. C++ can still access it by including the generated header.

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 70, Patchset 2 (Latest): raw_ptr<content::RenderFrameHost> rfh_ptr_;

Jenna Himawan . unresolved

ExtensionMenuModel already stores the RenderFrameHost and ContextMenuParams. While they are private in the base class, re-defining them here leads to redundant storage and potential confusion. If you unify the classes as suggested above, you might still need them for the non-extension case, but consider naming them consistently.

Open in Gerrit

Related details

Attention is currently required from:

ben chin

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

ben chin (Gerrit)

unread,

Jun 23, 2026, 11:00:36 PM (8 days ago) Jun 23

to Jenna Himawan, Chromium LUCI CQ, Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from Calder Kitagawa and Jenna Himawan

ben chin voted and added 4 comments

Votes added by ben chin

Commit-Queue

+1

4 comments

File chrome/app/generated_resources.grd

Line 605, Patchset 2:

        <message name="IDS_CONTENT_CONTEXT_INSPECTELEMENT" desc="The name of the Inspect Element command in the content area context menu">

Jenna Himawan . resolved

I think we don't support & for accelerators in Android, and generally Clank has its own strings files instead of using strings in chrome/app.
Consider adding a dedicated Android string in chrome/browser/ui/android/strings/android_chrome_strings.grd. C++ can still access it by including the generated header.

ben chin

I see, thanks!

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 70, Patchset 2: raw_ptr<content::RenderFrameHost> rfh_ptr_;

Jenna Himawan . resolved

ExtensionMenuModel already stores the RenderFrameHost and ContextMenuParams. While they are private in the base class, re-defining them here leads to redundant storage and potential confusion. If you unify the classes as suggested above, you might still need them for the non-extension case, but consider naming them consistently.

ben chin

sure, unified both implementations into a single `ChromeSelectionDropdownMenuModel` class and rename the parameters

Line 74, Patchset 2:class ChromeSelectionDropdownMenuModel : public ui::SimpleMenuModel,

Calder Kitagawa . resolved

Should we override the other methods for this too?
Can we have just one implementation and share common logic and just wrap the extension related things in the buildflag?

ben chin

sure, combined both model into a single `ChromeSelectionDropdownMenuModel` class and used a preprocessor guards to eliminate the duplicate structures.

Line 119, Patchset 2: std::unique_ptr<ChromeSelectionDropdownMenuModel> model =


      std::make_unique<ChromeSelectionDropdownMenuModel>(render_frame_host,
                                                         params);
  model->PopulateModel();
  if (is_devtools_allowed) {
    if (model->GetItemCount() > 0) {
      model->AddSeparator(ui::NORMAL_SEPARATOR);
    }
    model->AddItemWithStringId(IDC_CONTENT_CONTEXT_INSPECTELEMENT,
                               IDS_CONTENT_CONTEXT_INSPECTELEMENT);
  }
  return std::move(model);
#else
  if (!is_devtools_allowed) {
    return nullptr;
  }
  std::unique_ptr<ChromeSelectionDropdownMenuModel> model =
      std::make_unique<ChromeSelectionDropdownMenuModel>(render_frame_host,
                                                         params);
  model->AddItemWithStringId(IDC_CONTENT_CONTEXT_INSPECTELEMENT,
                             IDS_CONTENT_CONTEXT_INSPECTELEMENT);
  return std::move(model);
#endif

Calder Kitagawa . resolved

Again this is mostly duplicated. Combining it might be nicer?

ben chin

Done

Open in Gerrit

Related details

Attention is currently required from:

Calder Kitagawa
Jenna Himawan

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

Calder Kitagawa (Gerrit)

unread,

Jun 24, 2026, 7:58:19 AM (7 days ago) Jun 24

to ben chin, Jenna Himawan, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from Jenna Himawan and ben chin

Calder Kitagawa voted and added 2 comments

Votes added by Calder Kitagawa

Code-Review

+1

2 comments

Commit Message

Line 14, Patchset 3 (Latest):


To make the label string available in C++ on Android, this CL also moves
IDS_CONTENT_CONTEXT_INSPECTELEMENT outside the `not is_android`
conditional block in generated_resources.grd.

Calder Kitagawa . unresolved

I think this is stale?

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 123, Patchset 3 (Latest):


  if (!is_devtools_allowed
#if !BUILDFLAG(ENABLE_DESKTOP_ANDROID_EXTENSIONS)
  ) {
    return nullptr;
  }
#else
  ) {
  }
#endif

Calder Kitagawa . unresolved

I think this is more readable?

```suggestion

#if !BUILDFLAG(ENABLE_DESKTOP_ANDROID_EXTENSIONS)
  if (!is_devtools_allowed) {
    return nullptr;
  }
#endif
```

Open in Gerrit

Related details

Attention is currently required from:

Jenna Himawan
ben chin

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

Jenna Himawan (Gerrit)

unread,

Jun 24, 2026, 1:25:31 PM (7 days ago) Jun 24

to ben chin, Calder Kitagawa, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Attention needed from ben chin

Jenna Himawan voted and added 2 comments

Votes added by Jenna Himawan

Code-Review

+1

2 comments

File chrome/app/generated_resources.grd

Line 604, Patchset 3 (Latest):

      <message name="IDS_INSPECT_ELEMENT_ANDROID" desc="The name of the Inspect Element command in the context menu for Android.">

Jenna Himawan . unresolved

The commit message says this CL moves `IDS_CONTENT_CONTEXT_INSPECTELEMENT`, but it actually adds a new `IDS_INSPECT_ELEMENT_ANDROID`. Please update the commit message to be accurate.

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 85, Patchset 3 (Latest): return true;

Jenna Himawan . unresolved

Would it be safer to return false here? I'm not sure when we expect to hit this code block and what happens based on true / false returns.

Open in Gerrit

Related details

Attention is currently required from:

ben chin

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

ben chin (Gerrit)

unread,

Jun 24, 2026, 9:25:46 PM (7 days ago) Jun 24

to Jenna Himawan, Calder Kitagawa, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

ben chin voted and added 4 comments

Votes added by ben chin

Commit-Queue

+2

4 comments

Commit Message

Line 14, Patchset 3:


To make the label string available in C++ on Android, this CL also moves
IDS_CONTENT_CONTEXT_INSPECTELEMENT outside the `not is_android`
conditional block in generated_resources.grd.

Calder Kitagawa . resolved

I think this is stale?

ben chin

thanks! updated msg

File chrome/app/generated_resources.grd

Line 604, Patchset 3:

      <message name="IDS_INSPECT_ELEMENT_ANDROID" desc="The name of the Inspect Element command in the context menu for Android.">

Jenna Himawan . resolved

The commit message says this CL moves `IDS_CONTENT_CONTEXT_INSPECTELEMENT`, but it actually adds a new `IDS_INSPECT_ELEMENT_ANDROID`. Please update the commit message to be accurate.

ben chin

thanks! updated commit msg

File chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Line 85, Patchset 3: return true;

Jenna Himawan . resolved

Would it be safer to return false here? I'm not sure when we expect to hit this code block and what happens based on true / false returns.

ben chin

changed both fallbacks to return false for safety

Line 123, Patchset 3:


  if (!is_devtools_allowed
#if !BUILDFLAG(ENABLE_DESKTOP_ANDROID_EXTENSIONS)
  ) {
    return nullptr;
  }
#else
  ) {
  }
#endif

Calder Kitagawa . resolved

I think this is more readable?

```suggestion

#if !BUILDFLAG(ENABLE_DESKTOP_ANDROID_EXTENSIONS)
  if (!is_devtools_allowed) {
    return nullptr;
  }
#endif
```

ben chin

Done

Open in Gerrit

Related details

Attention set is empty

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

ben chin (Gerrit)

unread,

Jun 24, 2026, 9:29:11 PM (7 days ago) Jun 24

to Jenna Himawan, Calder Kitagawa, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

ben chin voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

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

Chromium LUCI CQ (Gerrit)

unread,

Jun 24, 2026, 10:24:10 PM (7 days ago) Jun 24

to ben chin, Jenna Himawan, Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, agriev...@chromium.org, srahim...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

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

```
The name of the file: chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc
Insertions: 9, Deletions: 14.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

Add "Inspect Element" to text selection dropdown menu

This CL adds the "Inspect Element" option to the text selection dropdown
menu on Android (triggered via mouse right-click on selected text). It
modifies ChromeSelectionDropdownMenuDelegate to evaluate if DevTools is
enabled and injects the inspect item into the ui::MenuModel, intercepting
the click to launch DevToolsWindow.

To make the label string available in C++ on Android, this CL also defines
a new IDS_INSPECT_ELEMENT_ANDROID string outside the is_android block in
generated_resources.grd.

Bug: 524045764

Change-Id: I967601ac053df3e45d9ed0e8c4855b4e81c11878

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

Reviewed-by: Jenna Himawan <jhim...@google.com>

Commit-Queue: ben chin <lu...@chromium.org>

Reviewed-by: Calder Kitagawa <ckit...@chromium.org>

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

Files:

M chrome/app/generated_resources.grd
A chrome/app/generated_resources_grd/IDS_INSPECT_ELEMENT_ANDROID.png.sha1
M chrome/browser/android/BUILD.gn
M chrome/browser/android/selection/chrome_selection_dropdown_menu_delegate.cc

Change size: M

Delta: 4 files changed, 126 insertions(+), 9 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Jenna Himawan, +1 by Calder Kitagawa

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

Add "Inspect Element" to text selection dropdown menu [chromium/src : main]

ben chin (Gerrit)

ben chin has uploaded the change for review

Commit message

Change diff

Change information

Related details

ben chin (Gerrit)

ben chin voted Commit-Queue+1

Related details

ben chin (Gerrit)

ben chin voted Commit-Queue+1

Related details

Calder Kitagawa (Gerrit)

Calder Kitagawa added 3 comments

Related details

Jenna Himawan (Gerrit)

Jenna Himawan added 2 comments

Related details

ben chin (Gerrit)

ben chin voted and added 4 comments

Votes added by ben chin

4 comments

Related details

Calder Kitagawa (Gerrit)

Calder Kitagawa voted and added 2 comments

Votes added by Calder Kitagawa

2 comments

Related details

Jenna Himawan (Gerrit)

Jenna Himawan voted and added 2 comments

Votes added by Jenna Himawan

2 comments

Related details

ben chin (Gerrit)

ben chin voted and added 4 comments

Votes added by ben chin

4 comments

Related details

ben chin (Gerrit)

ben chin voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information