Backlight OCR: Informs the user if ScreenAI init failed or no OCR results [chromium/src : main]

1 view
Skip to first unread message

Kyungjun Lee (Gerrit)

unread,
11:47 AM (12 hours ago) 11:47 AM
to Nektarios Paisios, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Nektarios Paisios

Kyungjun Lee added 1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Kyungjun Lee . unresolved

Hi Nektarios, it seems that this CL failed on some of the ChromeOS and Linux builders.

Open in Gerrit

Related details

Attention is currently required from:
  • Nektarios Paisios
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 1
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Attention: Nektarios Paisios <nek...@chromium.org>
Gerrit-Comment-Date: Tue, 02 Jul 2024 15:47:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Nektarios Paisios (Gerrit)

unread,
6:22 PM (5 hours ago) 6:22 PM
to Kyungjun Lee, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org

Nektarios Paisios added 1 comment

Patchset-level comments
Nektarios Paisios . resolved

After our offline discussion today I simplified the implementation as you suggested.
Thanks!

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 1
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 22:22:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Kyungjun Lee (Gerrit)

unread,
6:55 PM (5 hours ago) 6:55 PM
to Nektarios Paisios, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Nektarios Paisios

Kyungjun Lee added 2 comments

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Kyungjun Lee . resolved

Thanks for this CL! Mostly looks good, but I wanted to check with you about the structure for a page without OCR results. Please find my comment below.

File chrome/browser/accessibility/media_app/ax_media_app_untrusted_handler.cc
Line 993, Patchset 2 (Latest): ui::AXNodeData root;
Kyungjun Lee . unresolved

What do you think about making it consistent with the existing structure of a tree built with OCR results, which has the page node as its root? Then, we can add the unlabeled_image node to this page node, which would look like: ```

  • Page node
  • - Image node
  • ```

Or we can follow how we create a page node with an unlabeled image node in `PdfAccessibilityTree` for Chrome PDF Viewer, which has the following structure: ```

  • Page node
  • - Paragraph node
  • -- Image node
  • ```

Let me know if it's better to keep the current implementation, which seems to mimic how we create a tree for a HTML page.

Open in Gerrit

Related details

Attention is currently required from:
  • Nektarios Paisios
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 2
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Attention: Nektarios Paisios <nek...@chromium.org>
Gerrit-Comment-Date: Tue, 02 Jul 2024 22:55:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Nektarios Paisios (Gerrit)

unread,
7:54 PM (4 hours ago) 7:54 PM
to Kyungjun Lee, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Kyungjun Lee

Nektarios Paisios added 1 comment

File chrome/browser/accessibility/media_app/ax_media_app_untrusted_handler.cc
Line 993, Patchset 2 (Latest): ui::AXNodeData root;
Kyungjun Lee . resolved

What do you think about making it consistent with the existing structure of a tree built with OCR results, which has the page node as its root? Then, we can add the unlabeled_image node to this page node, which would look like: ```

  • Page node
  • - Image node
  • ```

Or we can follow how we create a page node with an unlabeled image node in `PdfAccessibilityTree` for Chrome PDF Viewer, which has the following structure: ```

  • Page node
  • - Paragraph node
  • -- Image node
  • ```

Let me know if it's better to keep the current implementation, which seems to mimic how we create a tree for a HTML page.

Nektarios Paisios

You are perfectly right. I totally forgot that we are creating a page, not a new document. The page will be provided by GenerateDocumentTree so we don't need to include it. Only the paragraph and the unlabeled image needs to be created.

Open in Gerrit

Related details

Attention is currently required from:
  • Kyungjun Lee
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 2
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Attention: Kyungjun Lee <kyung...@google.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 23:54:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kyungjun Lee <kyung...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Nektarios Paisios (Gerrit)

unread,
7:55 PM (4 hours ago) 7:55 PM
to Kyungjun Lee, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Kyungjun Lee

Nektarios Paisios voted

Auto-Submit+1
Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Kyungjun Lee
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 3
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Attention: Kyungjun Lee <kyung...@google.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 23:55:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Kyungjun Lee (Gerrit)

unread,
8:31 PM (3 hours ago) 8:31 PM
to Nektarios Paisios, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Nektarios Paisios

Kyungjun Lee voted and added 4 comments

Votes added by Kyungjun Lee

Code-Review+1

4 comments

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Kyungjun Lee . resolved

Two comments that I think can be quickly fixed; so giving LGTM to unblock.

File chrome/browser/accessibility/media_app/ax_media_app_untrusted_handler.cc
Line 33, Patchset 3 (Latest):#include "chrome/grit/generated_resources.h"
Kyungjun Lee . unresolved

nit: I think this header is not needed.

Line 1003, Patchset 3 (Latest): unlabeled_image.relative_bounds.bounds = gfx::RectF(1, 1);
Kyungjun Lee . unresolved

Ideally, it should be the same as the image size. I think we can get that information from `page_metadata_.at(dirty_page_id).rect`? I believe that we just need to use the width and height info from it; that is, no need to set its origin (x,y).

File chrome/browser/accessibility/media_app/ax_media_app_untrusted_handler_browsertest.cc
Line 691, Patchset 3 (Latest): "offset_container_id=1 (0, 0)-(1, 1) restriction=readonly\n",
Kyungjun Lee . unresolved

This would need to be updated if we set the unlabeled image node to be the same as the image size.

Open in Gerrit

Related details

Attention is currently required from:
  • Nektarios Paisios
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Icdfe9b2cdcfac44b2551f19a774257db5f6886c7
Gerrit-Change-Number: 5671001
Gerrit-PatchSet: 3
Gerrit-Owner: Nektarios Paisios <nek...@chromium.org>
Gerrit-Reviewer: Kyungjun Lee <kyung...@google.com>
Gerrit-Reviewer: Nektarios Paisios <nek...@chromium.org>
Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
Gerrit-Attention: Nektarios Paisios <nek...@chromium.org>
Gerrit-Comment-Date: Wed, 03 Jul 2024 00:30:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Kyungjun Lee (Gerrit)

unread,
8:58 PM (3 hours ago) 8:58 PM
to Nektarios Paisios, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, abigailbk...@google.com, blundell+...@chromium.org, dtseng...@chromium.org, feature-me...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, yuzo+...@chromium.org
Attention needed from Nektarios Paisios

Kyungjun Lee added 1 comment

Patchset-level comments
Kyungjun Lee . unresolved

Hi Nektarios, it seems that this CL failed on some of the ChromeOS and Linux builders.

Kyungjun Lee

Looks like this CL failed on the linux-chromeos-rel builder. Please fix the issue.

Gerrit-Comment-Date: Wed, 03 Jul 2024 00:58:11 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages