Replace accessibility-developer-tools submodule with accessibility-audit directory containing just …

Replace accessibility-developer-tools submodule with accessibility-audit directory containing just the generated axs_testing.js file

Follow-up CL will remove accessibility-developer-tools from DEPS.

BUG=165901
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224794

[dmazzoni, 2013-09-05 15:27:57]

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
File third_party/accessibility-audit/README.chromium (right):

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:1: Name: Accessibility Developer
Tools
Maybe this should be "Accessibility Audit Library from Accessibility Developer
Tools"?

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:2: Short Name:
accessibility-developer-tools
accessibility-audit?

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process).
Add build instructions, or point to what README to follow?

[szager, 2013-09-05 19:27:42]

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
File third_party/accessibility-audit/axs_testing.js (right):

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/axs_testing.js:20: // AUTO-GENERATED CONTENT
BELOW: DO NOT EDIT! See above for details.
Might want a blank line before and after // AUTO-GENERATED

[aboxhall, 2013-09-06 18:26:30]

+ phajdan.jr for changes in chrome/test, jhawkins/maruel for changes in
*.isolate (jhawkins was on original review
https://chromiumcodereview.appspot.com/11363170), darin for third_party

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
File third_party/accessibility-audit/README.chromium (right):

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:1: Name: Accessibility Developer
Tools
On 2013/09/05 15:27:57, Dominic Mazzoni wrote:
> Maybe this should be "Accessibility Audit Library from Accessibility Developer
> Tools"?

Done.

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:2: Short Name:
accessibility-developer-tools
On 2013/09/05 15:27:57, Dominic Mazzoni wrote:
> accessibility-audit?

Ambiguous. The template says "Short Name: Name the package is distributed under
(ex. libxml, openssl, etc)" which to me suggests accessibility-developer-tools
(although "short" isn't really correct) is what we want. But it is checked in
here as accessibility-audit, so maybe I should go with that.

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process).
On 2013/09/05 15:27:57, Dominic Mazzoni wrote:
> Add build instructions, or point to what README to follow?

Done.

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
File third_party/accessibility-audit/axs_testing.js (right):

https://codereview.chromium.org/23830003/diff/1/third_party/accessibility-aud...
third_party/accessibility-audit/axs_testing.js:20: // AUTO-GENERATED CONTENT
BELOW: DO NOT EDIT! See above for details.
On 2013/09/05 19:27:42, szager wrote:
> Might want a blank line before and after // AUTO-GENERATED

Done.

[szager, 2013-09-06 18:28:47]

lgtm

[Paweł Hajdan Jr., 2013-09-06 18:31:31]

LGTM

[aboxhall, 2013-09-09 17:06:24]

maruel, jhawkins/darin: ping?

[M-A Ruel, 2013-09-09 18:24:34]

https://codereview.chromium.org/23830003/diff/9001/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/23830003/diff/9001/chrome/browser_tests.isola...
chrome/browser_tests.isolate:67: '../third_party/accessibility-audit/',
If a single file is needed, I'd prefer you to reference it directly. Otherwise
the README and LICENSE files will be archived for no good reason. If you ever
plan to get more than a trivial number of files, I'm fine with keeping the whole
directory. It's a trade off of .isolate configuration complexity vs
over-archival.

[aboxhall, 2013-09-09 18:30:39]

https://codereview.chromium.org/23830003/diff/9001/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/23830003/diff/9001/chrome/browser_tests.isola...
chrome/browser_tests.isolate:67: '../third_party/accessibility-audit/',
On 2013/09/09 18:24:34, M-A Ruel wrote:
> If a single file is needed, I'd prefer you to reference it directly. Otherwise
> the README and LICENSE files will be archived for no good reason. If you ever
> plan to get more than a trivial number of files, I'm fine with keeping the
whole
> directory. It's a trade off of .isolate configuration complexity vs
> over-archival.

Should be a single file for the foreseeable future. Done.

[M-A Ruel, 2013-09-09 18:36:39]

https://codereview.chromium.org/23830003/diff/24001/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/23830003/diff/24001/chrome/browser_tests.isol...
chrome/browser_tests.isolate:67:
'../third_party/accessibility-audit/axs_testing.js',
Ok, then it has to move into isolate_dependency_tracked, that's where files go
(contrary to directories).

Sorry I should have noted this difference in my previous message.

[aboxhall, 2013-09-09 19:54:52]

https://codereview.chromium.org/23830003/diff/24001/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/23830003/diff/24001/chrome/browser_tests.isol...
chrome/browser_tests.isolate:67:
'../third_party/accessibility-audit/axs_testing.js',
On 2013/09/09 18:36:40, M-A Ruel wrote:
> Ok, then it has to move into isolate_dependency_tracked, that's where files go
> (contrary to directories).
> 
> Sorry I should have noted this difference in my previous message.

Ah makes sense, thanks.

[M-A Ruel, 2013-09-09 19:56:02]

.isolate lgtm

[aboxhall, 2013-09-12 18:16:10]

darin: ping re third_party?

[aboxhall, 2013-09-12 20:26:23]

-darin +cpu

cpu: could you take a look at the changes in third_party?

[darin (slow to review), 2013-09-13 07:18:23]

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
File third_party/accessibility-audit/README.chromium (right):

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process). See
README.md in original project for build instructions.
nit: maybe some 80-column line breaking would be nice here.

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process). See
README.md in original project for build instructions.
Note: The security team previously raised concerns about third-party JS
libraries being checked into Chrome that are the result of a build procedure.
The code can be difficult to audit in that form.

You might take a look at how this was handled for Polymer (see
src/third_party/polymer). It has a build.log file that provides information that
would be helpful to someone trying to re-create the bits that are committed into
the Chromium tree.

That said, this seems like test-only code, so there may be much less of an
issue.

[aboxhall, 2013-09-13 22:47:22]

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
File third_party/accessibility-audit/README.chromium (right):

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process). See
README.md in original project for build instructions.
On 2013/09/13 07:18:23, darin wrote:
> nit: maybe some 80-column line breaking would be nice here.

Done.

https://codereview.chromium.org/23830003/diff/28001/third_party/accessibility...
third_party/accessibility-audit/README.chromium:13: This version has been built
from the revision linked in the comment at the top of the axs_testing.js file
(the revision information is added automatically by the build process). See
README.md in original project for build instructions.
On 2013/09/13 07:18:23, darin wrote:
> Note: The security team previously raised concerns about third-party JS
> libraries being checked into Chrome that are the result of a build procedure.
> The code can be difficult to audit in that form.
> 
> You might take a look at how this was handled for Polymer (see
> src/third_party/polymer). It has a build.log file that provides information
that
> would be helpful to someone trying to re-create the bits that are committed
into
> the Chromium tree.
> 
> That said, this seems like test-only code, so there may be much less of an
> issue.

Thanks - I did see the polymer example. The version link in the generated file
is my "poor man's" version of that - if you check out the repository at that
version and follow the build instructions, you should end up with the same
thing.

Do you think that will suffice? If not, it should be fairly straightforward to
add a build.log file in the same way.

[darin (slow to review), 2013-09-13 22:48:47]

LGTM

[aboxhall, 2013-09-14 00:28:43]

phajdan.jr: I had to make some more changes to
accessibility_audit_browsertest.js; could you take a look?

[Paweł Hajdan Jr., 2013-09-14 00:38:47]

LGTM

[cpu_(ooo_6.6-7.5), 2013-09-15 19:11:50]

lgtm

[commit-bot: I haz the power, 2013-09-16 21:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/23830003/37002

[commit-bot: I haz the power, 2013-09-17 00:24:12]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-18 00:27:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/23830003/72001

[commit-bot: I haz the power, 2013-09-18 03:08:02]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-20 19:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/23830003/100001

[commit-bot: I haz the power, 2013-09-20 23:52:47]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-21 00:28:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/23830003/100001

[commit-bot: I haz the power, 2013-09-21 02:56:07]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-23 18:41:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/23830003/100001

[commit-bot: I haz the power, 2013-09-23 21:36:55]

Change committed as 224794