Introduced copy text from image feature on macOS #16508

simonhong · 2023-01-03T06:39:18Z

"Copy Text From Image" entry is added to render view's context menu.
It gets bitmap data from renderer via LocalFrame::GetImage() mojom interface,
and then passed it to TextRecognitionDialog to extract and show text from the image.

Newly added context meny entry

Dialog shows the copied text from image. Vertical scroll bar can be shown based on the contents.

When text recognition is in-progress:

When text recognition is failed:

Resolves

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

TextRecognitionBrowserTest.TextRecognitionTest

Search any image that contains text from search.brave.com
Choose Copy Text From Image item from context menu
Check dialog is shown with text
Check extracted text is copied to clipboard

github-actions · 2023-01-03T06:39:30Z

⚠️ PR head is an unsigned commit
commit: 6c7ad23
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

simonhong · 2023-01-05T00:56:57Z

@diracdeltas Should I have security review for this PR?
This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.
VNImageRequestHandler - https://developer.apple.com/documentation/vision/vnimagerequesthandler?language=objc

simonhong · 2023-01-05T01:03:36Z

@rmcfadden3 Can you review the newly added sentences?
As you can see in the above description, we have four new strings.
Copy Text From Image menu entry
Copying text from image... when text recognition is in progress.
Text copied from image when text recognition is completed.
Copy text failed when text recognition is failed.

diracdeltas · 2023-01-05T01:31:57Z

This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.

thanks for flagging. is this framework local-only? (AKA no network requests are generated in language detection and text extraction). if so it should be fine.

if not we should maybe disable in Tor windows

also do we need any licensing changes to use the framework? i assume not.

simonhong · 2023-01-05T01:48:58Z

This PR extracts text from bitmap image came from renderer by using Vision framework from macOS SDK.

thanks for flagging. is this framework local-only? (AKA no network requests are generated in language detection and text extraction). if so it should be fine.

According to the docs, processing happens on user's device. - https://developer.apple.com/documentation/vision/recognizing_text_in_images?language=objc

One of Vision’s many powerful features is its ability to detect and recognize multilanguage text in images.
You can use this functionality in your own apps to handle both real-time and offline use cases.
In all cases, all of Vision’s processing happens on the user’s device to enhance performance and user privacy.

if not we should maybe disable in Tor windows

also do we need any licensing changes to use the framework? i assume not.

I think we don't need another licensing for builtin SDK cc @fmarier

diracdeltas · 2023-01-05T02:11:46Z

no concerns from me then!

fmarier · 2023-01-05T04:57:53Z

I think we don't need another licensing for builtin SDK

If it's an OS feature provided by Apple, then I don't think so either.

rmcfadden3 · 2023-01-05T22:44:16Z

@rmcfadden3 Can you review the newly added sentences? As you can see in the above description, we have four new strings. Copy Text From Image menu entry Copying text from image... when text recognition is in progress. Text copied from image when text recognition is completed. Copy text failed when text recognition is failed.

@simonhong — all of your text looks great. However, I would slightly edit the last option to make it grammatically correct:

Text copy failed

simonhong · 2023-01-06T01:33:46Z

@rmcfadden3 Can you review the newly added sentences? As you can see in the above description, we have four new strings. Copy Text From Image menu entry Copying text from image... when text recognition is in progress. Text copied from image when text recognition is completed. Copy text failed when text recognition is failed.

@simonhong — all of your text looks great. However, I would slightly edit the last option to make it grammatically correct:

Text copy failed

Updated. Thanks!

bsclifton · 2023-01-08T09:13:19Z

Functionality works great! 😄 This is really cool - will check out the code next

app/brave_generated_resources.grd

bsclifton · 2023-01-09T07:30:29Z

browser/ui/views/text_recognition_dialog_view.cc

+  for (const auto& t : text) {
+    unified_string += t;
+    unified_string += '\n';
+  }


The above is fine 😄 But you could use std library:

const char* const delim = "\n"; std::ostringstream unified; std::copy(text.begin(), text.end(), std::ostream_iterator<std::string>(unified, delim)); std::string unified_string = unified.str();

Need to include <sstream> and maybe some others (lint should warn with include what you use)

Pushed a quick update with this - 1d611fe 😄

Inspired from your changes - I found base::JoinString() api!

Sometimes I don't understand why we don't have basic operation like join string in std 😭

Nice find! 😃 And yes - I completely agree, @sangwoo108 😄

bsclifton

LGTM++

github-actions · 2023-01-09T17:58:20Z

⚠️ PR head is an unsigned commit
commit: 1d611fe
reason: unsigned
Please follow the handbook to configure commit signing
cc: @bsclifton

github-actions · 2023-01-09T23:56:31Z

⚠️ PR head is an unsigned commit
commit: 09475f3
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-09T23:57:12Z

⚠️ PR head is an unsigned commit
commit: 09475f3
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-11T09:56:42Z

⚠️ PR head is an unsigned commit
commit: bcec82d
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-11T10:03:48Z

⚠️ PR head is an unsigned commit
commit: 5594934
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-11T10:03:50Z

⚠️ PR head is an unsigned commit
commit: 5594934
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-11T10:12:48Z

⚠️ PR head is an unsigned commit
commit: 5594934
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-12T04:27:22Z

⚠️ PR head is an unsigned commit
commit: 758dfb9
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-12T04:28:53Z

⚠️ PR head is an unsigned commit
commit: 9e19f2e
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

github-actions · 2023-01-12T04:30:27Z

⚠️ PR head is an unsigned commit
commit: 9e19f2e
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

simonhong · 2023-01-12T04:31:13Z

TextRecognitionBrowserTest.TextRecognitionTest browser test is added. PTAL again. Thanks.

goodov · 2023-01-12T15:20:35Z

components/text_recognition/browser/text_recognition_browser_export.h

+
+#define TEXT_RECOGNITION_BROWSER_EXPORT
+
+#endif


please use base/component_export.h instead along with COMPONENT_EXPORT(TEXT_RECOGNITION...) macro.

goodov · 2023-01-12T15:23:08Z

browser/ui/BUILD.gn

      "//brave/browser/ui",
      "//brave/components/constants",
      "//chrome/browser",
      "//chrome/browser/devtools:test_support",
      "//chrome/browser/profiles:profile",
      "//chrome/browser/ui",
      "//chrome/test:test_support_ui",
+      "//chrome/test:unit_tests",


with this line you effectively added browser_tests->unit_tests dependency.
you should definitely not do that. maybe you only need to depend on gtest?

I picked wrong target for chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h among below three candidates. //chrome/test:test_support is picked. Thanks!

ERROR at //brave/browser/ui/text_recognition_browsertest.cc:20:11: Include not allowed. #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h" ^------------------------------------------------------------------------ It is not in any dependency of //brave/browser/ui:browser_tests The include file is in the target(s): //chrome/test:interactive_ui_tests //chrome/test:test_support //chrome/test:unit_tests at least one of which should somehow be reachable.

github-actions · 2023-01-12T23:51:30Z

⚠️ PR head is an unsigned commit
commit: e6c43a1
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

goodov · 2023-01-13T07:52:56Z

components/text_recognition/browser/BUILD.gn

+    "//skia",
+  ]
+
+  defines = [ "IS_TEXT_RECOGNITION_BROWSER_IMPL" ]


nit: https://gn.googlesource.com/gn/+/main/docs/style_guide.md#ordering-within-a-target

Oh, good to know. Updated with verified commit :) !

github-actions · 2023-01-13T09:47:26Z

⚠️ PR head is an unsigned commit
commit: e6c43a1
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

fix brave/brave-browser#27513 "Copy Text From Image" entry is added to render view's context menu. It gets bitmap data from renderer via LocalFrame::GetImage() mojom interface, and then passed it to TextRecognitionDialog to extract and show text from that image.

If renderer is busy or on slow machine, copy image could take some time. In that situation, user could ask copy text menu multiple times. And then, browser would get multiple copied image from renderer. When it happens, use same dialog and show lastly recognized text in dialog instead of creating multiple dialogs.

goodov

lgtm with one more comment.

components/text_recognition/browser/text_recognition.h

This test extract "brave" text from test image.

simonhong added the CI/run-draft label Jan 3, 2023

simonhong self-assigned this Jan 3, 2023

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Jan 3, 2023

simonhong force-pushed the text_recognition branch 2 times, most recently from 8e10b4b to d951d34 Compare January 4, 2023 15:32

simonhong marked this pull request as ready for review January 5, 2023 00:46

simonhong requested review from a team as code owners January 5, 2023 00:46

simonhong force-pushed the text_recognition branch from d951d34 to 3062303 Compare January 5, 2023 00:46

simonhong removed the CI/run-draft label Jan 5, 2023

simonhong requested a review from rmcfadden3 January 5, 2023 01:01

simonhong requested a review from bridiver January 5, 2023 08:49

simonhong force-pushed the text_recognition branch from 3062303 to 1f145cb Compare January 6, 2023 01:33

simonhong requested a review from bsclifton January 6, 2023 21:27

bsclifton reviewed Jan 9, 2023

View reviewed changes

app/brave_generated_resources.grd Show resolved Hide resolved

bsclifton reviewed Jan 9, 2023

View reviewed changes

bsclifton approved these changes Jan 9, 2023

View reviewed changes

simonhong force-pushed the text_recognition branch from 758dfb9 to 9e19f2e Compare January 12, 2023 04:28

goodov reviewed Jan 12, 2023

View reviewed changes

simonhong force-pushed the text_recognition branch from fad8041 to e6c43a1 Compare January 12, 2023 23:51

goodov reviewed Jan 13, 2023

View reviewed changes

simonhong and others added 7 commits January 13, 2023 19:22

Updated using std::copy and stringstream

f3cccf6

Used base::JoinString() to make strings into one string

9e7c139

Use text_recognition namespace for text_recognition components

7cdd8b1

Moved compiler:enable_arc target into macOS only part

b4b3dd8

Make TextRecognitionDialogTracker track widget status more accurately

a055c91

simonhong force-pushed the text_recognition branch from e6c43a1 to 58374fa Compare January 13, 2023 10:23

goodov approved these changes Jan 13, 2023

View reviewed changes

components/text_recognition/browser/text_recognition.h Outdated Show resolved Hide resolved

simonhong added 3 commits January 13, 2023 20:00

Make text_recognition/browser target as shared lib

58f54ea

Added TextRecognitionBrowserTest browser test

8e18d4a

This test extract "brave" text from test image.

Reordering properties in text_recognition/browser target

6855228

simonhong force-pushed the text_recognition branch from 58374fa to 6855228 Compare January 13, 2023 11:02

simonhong merged commit 8704c6f into master Jan 13, 2023

simonhong deleted the text_recognition branch January 13, 2023 13:12

github-actions bot added this to the 1.49.x - Nightly milestone Jan 13, 2023


		#define TEXT_RECOGNITION_BROWSER_EXPORT

		#endif

Introduced copy text from image feature on macOS #16508

Introduced copy text from image feature on macOS #16508

Conversation

simonhong commented Jan 3, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Jan 3, 2023

simonhong commented Jan 5, 2023

simonhong commented Jan 5, 2023 • edited Loading

diracdeltas commented Jan 5, 2023

simonhong commented Jan 5, 2023 • edited Loading

diracdeltas commented Jan 5, 2023

fmarier commented Jan 5, 2023

rmcfadden3 commented Jan 5, 2023

simonhong commented Jan 6, 2023

bsclifton commented Jan 8, 2023

bsclifton Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

bsclifton Jan 9, 2023

Choose a reason for hiding this comment

simonhong Jan 9, 2023

Choose a reason for hiding this comment

sangwoo108 Jan 10, 2023

Choose a reason for hiding this comment

bsclifton Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2023

github-actions bot commented Jan 9, 2023

github-actions bot commented Jan 9, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

simonhong commented Jan 12, 2023

goodov Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

simonhong Jan 12, 2023

Choose a reason for hiding this comment

goodov Jan 12, 2023

Choose a reason for hiding this comment

simonhong Jan 12, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 12, 2023

goodov Jan 13, 2023

Choose a reason for hiding this comment

simonhong Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2023

goodov left a comment

Choose a reason for hiding this comment

simonhong commented Jan 3, 2023 •

edited

Loading

simonhong commented Jan 5, 2023 •

edited

Loading

simonhong commented Jan 5, 2023 •

edited

Loading

bsclifton Jan 9, 2023 •

edited

Loading

bsclifton Jan 10, 2023 •

edited

Loading

goodov Jan 12, 2023 •

edited

Loading

simonhong Jan 13, 2023 •

edited

Loading