Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(477)

Issue 2793033002: [Extensions Bindings] Add an end-to-end test for the fileSystem API (Closed)

Created:
3 years, 8 months ago by Devlin
Modified:
3 years, 8 months ago
Reviewers:
lazyboy, jbroman
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Extensions Bindings] Add an end-to-end test for the fileSystem API Add an end-to-end test for the fileSystem API, which exercises a few different niche properties: a) It has an unusual isInstanceOf property (Entry) b) It hacks up the arguments with a setUpdateArgumentsPostValidate hook c) It uses some craziness to bind file entries to the background page a) is already implemented, but this adds an end-to-end test for it. b) required updating logic to no longer parse and validate arguments after a post-update hook. This is unfortunate (see comments for why), but a number of APIs do this, so for now, we have to support it. Add a few comments lamenting the state, and a test to ensure it doesn't break. c) required updating the util js module to not require the method from the API module's custom bindings, since an API instantiated through a require() won't have the apiBridge passed in. APIs should be instatiated through the lazy get; utils should be require()d. BUG=653596 Review-Url: https://codereview.chromium.org/2793033002 Cr-Commit-Position: refs/heads/master@{#461976} Committed: https://chromium.googlesource.com/chromium/src/+/577b8f369864082950e5922472780e54317ea2ac

Patch Set 1 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -23 lines) Patch
M chrome/browser/extensions/native_bindings_apitest.cc View 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/renderer/resources/extensions/file_entry_binding_util.js View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/renderer/resources/extensions/file_system_custom_bindings.js View 4 chunks +10 lines, -9 lines 0 comments Download
A chrome/test/data/extensions/api_test/native_bindings/instance_of/background.js View 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/native_bindings/instance_of/manifest.json View 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html View 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/native_bindings/instance_of/test.js View 1 chunk +17 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/native_bindings/text.txt View 1 chunk +1 line, -0 lines 0 comments Download
M extensions/renderer/api_binding.cc View 3 chunks +26 lines, -3 lines 0 comments Download
M extensions/renderer/api_binding_hooks.h View 1 chunk +1 line, -0 lines 0 comments Download
M extensions/renderer/api_binding_hooks.cc View 2 chunks +8 lines, -2 lines 0 comments Download
M extensions/renderer/api_binding_unittest.cc View 1 chunk +34 lines, -0 lines 0 comments Download
M extensions/renderer/api_signature.h View 1 chunk +9 lines, -0 lines 0 comments Download
M extensions/renderer/api_signature.cc View 8 chunks +59 lines, -7 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 26 (21 generated)
Devlin
Folks, mind taking a look? Uncovered another ugly feature of our bindings system that I'd ...
3 years, 8 months ago (2017-04-04 01:40:45 UTC) #19
jbroman
lgtm That is indeed unfortunate.
3 years, 8 months ago (2017-04-04 20:25:21 UTC) #20
lazyboy
lgtm
3 years, 8 months ago (2017-04-04 22:49:36 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2793033002/80001
3 years, 8 months ago (2017-04-05 00:52:24 UTC) #23
commit-bot: I haz the power
3 years, 8 months ago (2017-04-05 04:21:33 UTC) #26
Message was sent while issue was closed.
Committed patchset #1 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/577b8f369864082950e592247278...

Powered by Google App Engine
This is Rietveld 408576698