Apply local patch to chromium accessibility code #23110

chunhtai · 2020-12-16T17:46:50Z

Description

This PR rewire the dependency of these chromium code to be able to compile under the flutter.

Related Issues

flutter/flutter#23601

Tests

I added the following tests:

all the unittests are built and run in ci

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

dnfield · 2020-12-16T18:20:04Z

Trying to orient myself to this a bit - are there ways we could make this patch smaller and still have the same functionality?

For example, there's a large number of lines related to removing the mojom namespace. It seems like these are files that get generated in the Chromium repo but not ours - or perhaps they just live in a different component.

Is this something where we could just generate the files and check them in, or just add a namespace somewhere, or just create a new file with the expected members in the expected namespace?

I think that would make this patch easier to review, and make it a little more possible to compare this code to upstream as we go forward.

gw280 · 2020-12-16T18:49:20Z

There seems to be a lot of cosmetic changes in this which I'm not sure are necessary. For example, changing the header guards and namespaces. Is there a functional reason for these changes, or are they purely cosmetic?

chunhtai · 2020-12-16T18:56:04Z

@gw280 They are purely cosmetic. I feel keeping the ui namespace may be confusing because this does not related to ui.

Same thing for mojom namespace. are we ok with keeping the same namespace? Alternatively I can put up PR that is purely just namespace changes before this PR

gw280 · 2020-12-16T19:00:05Z

My vote would be to leave the namespaces as-is. The additional code churn will make it more difficult to cherry-pick changes from upstream.

dnfield · 2020-12-16T19:20:56Z

ci/licenses.sh

  local actualLicenseCount
  actualLicenseCount="$(tail -n 1 flutter/ci/licenses_golden/licenses_flutter | tr -dc '0-9')"
-  local expectedLicenseCount=15 # When changing this number: Update the error message below as well describing all expected license types.
+  local expectedLicenseCount=12 # When changing this number: Update the error message below as well describing all expected license types.


Is this simply a function of removing unused files?

not sure what you mean by removing unused files. This make sure the license count does not change unexpectedly

My assumption is that this is that changes made in thsi patch are reducing the number of licenses we're including, so we're lowering this number.

Is that accurate? We've simply deleted files with different copyright years or whatever, so we no longer have as many?

that is correct

chunhtai · 2020-12-16T19:44:04Z

Reverted namespace header guard changes. ready for review

dnfield · 2020-12-16T19:55:01Z

third_party/accessibility/ax/ax_relative_bounds.cc

+            std::to_string(static_cast<int>(round(bounds.y()))) + ")-(" +
+            std::to_string(static_cast<int>(round(bounds.width()))) + ", " +
+            std::to_string(static_cast<int>(round(bounds.height()))) + ")";
+  // result += " transform=" + transform->ToString();


Remove commented out code.

chunhtai · 2020-12-16T19:58:03Z

third_party/accessibility/ax/platform/ax_platform_node_mac.mm

@@ -2,26 +2,24 @@
 // Use of this source code is governed by a BSD-style license that can be


I have to completely rewrite this file because the upstream ax_platform_node_mac.mm is still WIP is not feature complete

third_party/accessibility/ax/ax_event_generator.cc

gw280 · 2020-12-16T19:40:43Z

third_party/accessibility/ax/ax_event_generator.h

  // Please make sure that this ScopedObserver is always declared last in order
  // to prevent any use-after-free.
-  ScopedObserver<AXTree, AXTreeObserver> tree_event_observer_{this};
+  // ScopedObserver<AXTree, AXTreeObserver> tree_event_observer_{this};


nit: should this be removed?

gw280 · 2020-12-16T19:42:42Z

third_party/accessibility/ax/ax_mode.cc

+      imploded << tokens[i] << " | ";
+    }
+  }
+  return imploded.str();


probably best to spin this out into a separate function

gw280 · 2020-12-16T19:42:53Z

third_party/accessibility/ax/ax_node.h


 class AXTableInfo;
-struct AXLanguageInfo;
+// struct AXLanguageInfo;


nit: remove

gw280 · 2020-12-16T19:43:42Z

third_party/accessibility/ax/ax_node.h

  //
  // Returns nullptr if the node has no language info.
-  AXLanguageInfo* GetLanguageInfo() const;
+  // AXLanguageInfo* GetLanguageInfo() const;


nit: remove

gw280 · 2020-12-16T19:43:48Z

third_party/accessibility/ax/ax_node.h

  // This should only be called by LabelLanguageForSubtree and is used as part
  // of the language detection feature.
-  void SetLanguageInfo(std::unique_ptr<AXLanguageInfo> lang_info);
+  // void SetLanguageInfo(std::unique_ptr<AXLanguageInfo> lang_info);


nit: remove

gw280 · 2020-12-16T20:10:17Z

third_party/accessibility/ax/ax_tree_id.cc

+      return "kToken";
  }

-  NOTREACHED();


FML_UNREACHABLE()

gw280 · 2020-12-16T20:10:24Z

third_party/accessibility/ax/ax_tree_id_registry.cc

                                             const AXTreeID& ax_tree_id) {
  auto it = frame_to_ax_tree_id_map_.find(frame_id);
  if (it != frame_to_ax_tree_id_map_.end()) {
-    NOTREACHED();


FML_UNREACHABLE()

Ah I didn't realize we have that, thanks for reminding

gw280 · 2020-12-16T20:10:31Z

third_party/accessibility/ax/platform/ax_platform_node_base.cc


-  NOTREACHED()


FML_UNREACHABLE()

gw280 · 2020-12-16T20:10:41Z

third_party/accessibility/ax/platform/ax_platform_node_base.cc

@@ -1640,32 +1607,32 @@ int AXPlatformNodeBase::GetHypertextOffsetFromEndpoint(
  if (endpoint_index_in_common_parent > index_in_common_parent)
    return static_cast<int32_t>(GetHypertext().size());

-  NOTREACHED();


FML_UNREACHABLE()

gw280 · 2020-12-16T20:10:53Z

third_party/accessibility/ax/platform/test_ax_node_wrapper.cc

  }

-  NOTREACHED();


FML_UNREACHABLE()

chinmaygarde

This library is going to be used by embedder implementations and is not part of the core engine (vended via the Embedder API in //flutter/platform/embedder). Because of this, it cannot depend on internal engine libraries. Neither FML nor Skia is used today by embedder only libraries.

chinmaygarde · 2020-12-16T20:31:07Z

third_party/accessibility/build.gn

+  }
+
+  public_deps = [
+    "//flutter/fml",


FML and Skia are engine internal libraries and may not be used by embedder implementations (or their dependencies).

chinmaygarde · 2020-12-16T20:33:22Z

third_party/accessibility/build.gn

+    deps = [
+      ":accessibility",
+      ":accessibility_fixtures",
+      "//flutter/testing",


This is also an internal engine dependency.

For unit test only target, do we need to worry about this?

chunhtai · 2020-12-16T20:41:22Z

@chinmaygarde I am not familiar with the internal library dependency. I thought the embedder library also uses FML library?

chinmaygarde · 2020-12-16T20:51:05Z

The public Embedder API is a single C header with no internal engine dependencies (see embedder.h). FML, Skia, and any of the libraries the engine uses internally are private implementation detail. Embedders are authored using just the stable interface defined in embedder.h. Imagine if the embedder were being authored out of tree, it would not have access to the engines Skia or FML libraries. The desktop embedders play by the same rules. You'll notice that none of the existing targets that are used by the embedder depend on Skia or FML.

chunhtai · 2020-12-22T21:32:24Z

third_party/accessibility/gfx/transform.cc

@@ -1,608 +1,134 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.


I have to completely rewrite this class because the chromium uses skia to implement this class.

chunhtai · 2020-12-22T22:05:51Z

@chinmaygarde @dnfield @gw280
I have rewritten the dependencies, This library does not depend on any non-std library now. I have also factored out a lot of string util so it should make the code a bit cleaner. This PR is ready for another look.

gw280 · 2020-12-29T00:50:12Z

A lot of the base types and classes seem to be mostly identical to std counterparts (e.g. base::Optional and std::optional). Would it be cleaner and reduce the amount of code churn if we just throw a bunch of using statements in a centralised header file somewhere? Not convinced whether it's the right direction to go down but would like to give it some thought.

chunhtai · 2020-12-29T17:54:28Z

A lot of the base types and classes seem to be mostly identical to std counterparts (e.g. base::Optional and std::optional). Would it be cleaner and reduce the amount of code churn if we just throw a bunch of using statements in a centralised header file somewhere? Not convinced whether it's the right direction to go down but would like to give it some thought.

the two examples are base::Optional and base::string16. I have thought about this and I am a bit lean toward just replace them instead of using ... ... for two reasons:

it is a simple enough change that is not too hard to cherry pick in the future
the latter will make the code slightly confusing for other people to read the code, and it feel quite unnecessary.

That being said, those are not strong arguments. I am willing to change it if there are other good reason to do so

gw280 · 2020-12-29T18:51:59Z

That's reasonable. If it's just those two then let's just leave it.

dnfield

I did not review every line of this patch, but RSLGTM.

I think @chinmaygarde 's concerns have been addressed.

gw280 · 2020-12-29T00:45:46Z

third_party/accessibility/ax/ax_event_generator.h

    DOCUMENT_SELECTION_CHANGED,
    DOCUMENT_TITLE_CHANGED,
    DROPEFFECT_CHANGED,
+    // TODO(nektar): Deprecate this event and replace it with


are these changes which have been pulled in from upstream? if so, is there a way we can separate those out as we can probably avoid reviewing them.

I reverted these change for now

gw280 · 2021-01-06T21:18:16Z

Sorry to be clear, I was mostly just curious where changes like that came from, rather than holding this up for merging. If Dan's ok with it, them it LGTM.

chunhtai · 2021-01-06T21:24:30Z

Since it won't affect the code and we can always pull it back if we need it in the future. The revert does make this pr less complex, i will continue merging with the revert.

fluttergithubbot · 2021-01-06T22:19:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ine#23110)

…ine#23110) (#73457)

google-cla bot added the cla: yes label Dec 16, 2020

chunhtai requested review from dnfield and gw280 December 16, 2020 17:53

dnfield reviewed Dec 16, 2020

View reviewed changes

chunhtai commented Dec 16, 2020

View reviewed changes

third_party/accessibility/ax/ax_event_generator.cc Show resolved Hide resolved

gw280 reviewed Dec 16, 2020

View reviewed changes

chinmaygarde suggested changes Dec 16, 2020

View reviewed changes

chunhtai force-pushed the third_party branch 3 times, most recently from b4f4ae9 to a6b08b6 Compare December 21, 2020 21:45

chunhtai commented Dec 22, 2020

View reviewed changes

gw280 requested a review from cbracken January 5, 2021 23:39

dnfield approved these changes Jan 6, 2021

View reviewed changes

gw280 reviewed Jan 6, 2021

View reviewed changes

chinmaygarde approved these changes Jan 6, 2021

View reviewed changes

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 6, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 6, 2021

chunhtai added 8 commits January 6, 2021 14:30

apply local modification

a70586f

addressing comments

fc7722f

update contain

80aaa4a

rest contains

6d6e209

finalized

15afceb

add more test

f7ca3b5

update license

7430c06

revert ax event generator code

74d1e57

chunhtai force-pushed the third_party branch from 051d4c4 to 74d1e57 Compare January 6, 2021 22:30

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 6, 2021

fluttergithubbot merged commit 2e5833b into flutter:master Jan 7, 2021

engine-flutter-autoroll mentioned this pull request Jan 7, 2021

Roll Engine from 1758eaa4edc3 to 2e5833b7c572 (1 revision) flutter/flutter#73457

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 7, 2021

2e5833b Apply local patch to chromium accessibility code (flutter/eng…

11ad1ce

…ine#23110)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Jan 7, 2021

2e5833b Apply local patch to chromium accessibility code (flutter/eng…

239f8ba

…ine#23110) (#73457)

chinmaygarde mentioned this pull request Jan 7, 2021

fix the flaky test AXPlatformUniqueIdTest.UnassignedIdsAreReused and re-enable the test. flutter/flutter#73512

Closed

HansMuller mentioned this pull request Jan 13, 2021

☂️ Desktop accessibility support flutter/flutter#73819

Closed

10 tasks

		@@ -2,26 +2,24 @@
		// Use of this source code is governed by a BSD-style license that can be

		@@ -1,608 +1,134 @@
		// Copyright (c) 2012 The Chromium Authors. All rights reserved.

Apply local patch to chromium accessibility code #23110

Apply local patch to chromium accessibility code #23110

Uh oh!

Conversation

chunhtai commented Dec 16, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

dnfield commented Dec 16, 2020

Uh oh!

gw280 commented Dec 16, 2020

Uh oh!

chunhtai commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gw280 commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 16, 2020

Uh oh!

chinmaygarde commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 22, 2020

Uh oh!

gw280 commented Dec 29, 2020

Uh oh!

chunhtai commented Dec 29, 2020

Uh oh!

gw280 commented Dec 29, 2020

chunhtai commented Dec 16, 2020 •

edited

Loading