[Extensions] chrome.storage: Add session namespace for Storage API [chromium/src : main]

Emilia Paz (Gerrit)

unread,

May 17, 2021, 11:41:13 AM5/17/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Devlin.

Emilia Paz uploaded patch set #33 to this change.

[Extensions] chrome.storage: Add session namespace for Storage API

[Don't submit until crrev.com/c/2895227 is landed]

This cl has 4 main changes:
a) Introduce `session` namespace in chrome.storage schema and set its
   channel to "canary" so we can test SettingsFunctions before
   `session` is on production.
b) Add `session` to renderer in SessionStorageArea class
c) Add RunInSession() in SettingsFunction for `session` namespace.
d) Implement Set and Get RunInSession() to test session namespace is
   properly introduced.
a) Update current browser tests. These will run in unknown channel
   while `session` namespace is running in canary.

Bug: 1185226
Change-Id: Id7b8d912adcd269e2a0703a2c1326165e690e211
---
M chrome/browser/extensions/api/storage/settings_apitest.cc
M chrome/browser/extensions/service_worker_apitest.cc
M chrome/test/data/extensions/api_test/native_bindings/extension/background.js
M chrome/test/data/extensions/api_test/service_worker/worker_based_background/storage/service_worker_background.js
M chrome/test/data/extensions/api_test/settings/simple_test/background.js
M extensions/browser/api/storage/storage_api.cc
M extensions/browser/api/storage/storage_api.h
M extensions/common/api/_api_features.json
M extensions/common/api/storage.json
M extensions/renderer/storage_area.cc
10 files changed, 456 insertions(+), 37 deletions(-)

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Devlin (Gerrit)

unread,

May 17, 2021, 4:59:36 PM5/17/21

to Emilia Paz, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tricium, Chromium Metrics Reviews, Devlin, Skia Gold Bot, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Emilia Paz.

View Change

9 comments:

Patchset:
- Patch Set #34:
  Thanks, Emilia! Just one last main comment on the test
File chrome/browser/extensions/api/storage/settings_apitest.cc:
- Patch Set #28, Line 36: #include "components/version_info/channel.h"
  Following our offline conversation: […]
  SG! The dependent CL is going in now, so should be unblocked.
- Patch Set #28, Line 275: ReplyWhenSatisfied(storage_area, "assertEmpty", "assertEmpty");
  A couple questions (more like statements that I wan't to make sure are correct): […]
  Ah, actually, this is a bit trickier. Basically, the extension is loaded (from LoadAndReplyWhenSatisfied()) and then runs commands until told to stop (in FinalReplyWhenSatisfied()). This worked easily when there was one storage area, because it was a simple flow. Now it's a bit trickier, because we want to test two storage areas. But we should only be loading the extension once, and only having a single FinalReply().
  What I'd lean towards would be pulling the Load and FinalReply calls to the outside of the for-loop. To avoid this becoming too much of a yak shave, we could just duplicate the assertEmpty() call. Something like:
  // Load the extension. To play nicely with our helper methods, we duplicate
  // one of the checks here (which is also in the for-loop in order to
  // exercise different storage areas).
  LoadAndReplyWhenSatisfied(kSync, "assertEmpty", "assertEmpty", "split_incognito");
```
for (auto storage_area : { kSync, kSession }) {
  ...  // All the checks
}
```
  // End the test. As above, we duplicate a check that already happened in
  // the for-loop in order to play nicely with our helper methods.
  FinalReplyWhenSatisfied(kSync, "assertEmpty", "assertEmpty");
  EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
  EXPECT_TRUE(catcher_incognito.GetNextResult()) << catcher.message();
File chrome/browser/extensions/api/storage/settings_apitest.cc:
- Patch Set #34, Line 236: ExtensionSettingsUnknownApiTest
  "UnknownApiTest" sounds a bit misleading (even though it matches the channel name). Maybe "Trunk"?
- Patch Set #34, Line 284: chrome.test.assertEq(undefined, chrome.storage.session);
  just as a sanity check, let's put in:
  chrome.test.assertTrue(!!chrome.storage.local);
  (Which double-checks that the rest of the storage API is being properly exposed)
File chrome/browser/extensions/service_worker_apitest.cc:
- Patch Set #34, Line 296: ServiceWorkerBasedBackgroundUnknownTest
  ditto re unknown
File extensions/browser/api/storage/storage_api.cc:
- Patch Set #28, Line 292: std::vector<std::string>(1, *input->GetIfString())
  {input->GetString()} doesn't create a vector because it's ambiguous, thus the call […]
  Thanks for the explanation; yeah, this is fine as-is
File extensions/browser/api/storage/storage_api.cc:
- Patch Set #34, Line 85: auto
  nit: const auto&
- Patch Set #34, Line 307: auto* value = values[default_value.first];
  nit: this results in an insertion if the item didn't exist in `values`. Prefer:
```
auto iter = values.find(default_value.first);
value_dict.SetKey(std::move(default_value.first()),
                  iter == values.end() ? std::move(default_value.second) : (*iter)->Clone());
```

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Emilia Paz (Gerrit)

unread,

May 18, 2021, 12:16:39 PM5/18/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tricium, Chromium Metrics Reviews, Devlin, Skia Gold Bot, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Devlin.

View Change

6 comments:

File chrome/browser/extensions/api/storage/settings_apitest.cc:
- Patch Set #28, Line 275: ReplyWhenSatisfied(storage_area, "assertEmpty", "assertEmpty");
  Ah, actually, this is a bit trickier. […]
  Makes sense. I agree with having Load and Final outside the loop. For now, I'll leave Final inside since we know `if (storage_area == StorageAreaNamespace::kSession)` will stop the loop. When all `session` functions are implemented, FinalReplyWhenSatisfied() will be moved outside the loop accordingly.
File chrome/browser/extensions/api/storage/settings_apitest.cc:

- Patch Set #34, Line 236: ExtensionSettingsUnknownApiTest

- "UnknownApiTest" sounds a bit misleading (even though it matches the channel name). […]
  Done

- Patch Set #34, Line 284: chrome.test.assertEq(undefined, chrome.storage.session);

- just as a sanity check, let's put in: […]
  Good idea, done.
File chrome/browser/extensions/service_worker_apitest.cc:

- Patch Set #34, Line 296: ServiceWorkerBasedBackgroundUnknownTest
  ditto re unknown

- Done
File extensions/browser/api/storage/storage_api.cc:

- Patch Set #34, Line 85: auto
  nit: const auto&

- Done

- Patch Set #34, Line 307: auto* value = values[default_value.first];

- nit: this results in an insertion if the item didn't exist in `values`. Prefer: […]
  Done

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Emilia Paz (Gerrit)

unread,

May 18, 2021, 5:07:33 PM5/18/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Devlin.

Emilia Paz uploaded patch set #36 to this change.

View Change

[Extensions] chrome.storage: Add session namespace for Storage API

This cl has 4 main changes:
a) Introduce `session` namespace in chrome.storage schema and set its
   channel to "canary" so we can test SettingsFunctions before
   `session` is on production.
b) Add `session` to renderer in SessionStorageArea class
c) Add RunInSession() in SettingsFunction for `session` namespace.
d) Implement Set and Get RunInSession() to test session namespace is
   properly introduced.
a) Update current browser tests. These will run in unknown channel
   while `session` namespace is running in canary.

Bug: 1185226
Change-Id: Id7b8d912adcd269e2a0703a2c1326165e690e211
---
M chrome/browser/extensions/api/storage/settings_apitest.cc
M chrome/browser/extensions/service_worker_apitest.cc
M chrome/test/data/extensions/api_test/native_bindings/extension/background.js
M chrome/test/data/extensions/api_test/service_worker/worker_based_background/storage/service_worker_background.js
M chrome/test/data/extensions/api_test/settings/simple_test/background.js
M extensions/browser/api/storage/storage_api.cc
M extensions/browser/api/storage/storage_api.h
M extensions/common/api/_api_features.json
M extensions/common/api/storage.json
M extensions/renderer/storage_area.cc

10 files changed, 458 insertions(+), 35 deletions(-)

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Devlin (Gerrit)

unread,

May 19, 2021, 1:14:46 PM5/19/21

to Emilia Paz, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Tricium, Chromium Metrics Reviews, Skia Gold Bot, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Emilia Paz.

Patch set 37:Code-Review +1

View Change

4 comments:

Patchset:
- Patch Set #37:
  Thanks, Emilia! LGTM % a few last nits.
File chrome/test/data/extensions/api_test/settings/simple_test/background.js:
- Patch Set #37, Line 298: )
  nit: extra paren
File extensions/browser/api/storage/storage_api.cc:
- Patch Set #37, Line 397: "Session storage quota bytes exceeded. Values were not stored.");
  For a follow-up: Let's add an API test that triggers this
File extensions/common/api/_api_features.json:
- Patch Set #37, Line 559: "channel": "canary"
  We should also restrict this to Manifest V3. This is going to be a little bit tricky because it means the tests will have to be updated, as well. We can tackle that in a followup, but maybe add a TODO here?

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Emilia Paz (Gerrit)

unread,

May 19, 2021, 2:03:25 PM5/19/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Tricium, Chromium Metrics Reviews, Skia Gold Bot, Chromium LUCI CQ, chromium...@chromium.org

View Change

3 comments:

File chrome/test/data/extensions/api_test/settings/simple_test/background.js:

- Patch Set #37, Line 298: )
  nit: extra paren

- Done
File extensions/browser/api/storage/storage_api.cc:

- Patch Set #37, Line 397: "Session storage quota bytes exceeded. Values were not stored.");
  For a follow-up: Let's add an API test that triggers this

- Added a TODO.
File extensions/common/api/_api_features.json:

- Patch Set #37, Line 559: "channel": "canary"

- We should also restrict this to Manifest V3. […]
  Added a TODO.

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Emilia Paz (Gerrit)

unread,

May 19, 2021, 2:05:24 PM5/19/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Tricium, Chromium Metrics Reviews, Skia Gold Bot, Chromium LUCI CQ, chromium...@chromium.org

Patch set 38:Commit-Queue +2

View Change

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

May 19, 2021, 4:17:06 PM5/19/21

to Emilia Paz, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Tricium, Chromium Metrics Reviews, Skia Gold Bot, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Devlin: Looks good to me
  Emilia Paz: Commit

[Extensions] chrome.storage: Add session namespace for Storage API

This cl has 4 main changes:
a) Introduce `session` namespace in chrome.storage schema and set its
   channel to "canary" so we can test SettingsFunctions before
   `session` is on production.
b) Add `session` to renderer in SessionStorageArea class
c) Add RunInSession() in SettingsFunction for `session` namespace.
d) Implement Set and Get RunInSession() to test session namespace is
   properly introduced.
a) Update current browser tests. These will run in unknown channel
   while `session` namespace is running in canary.

Bug: 1185226
Change-Id: Id7b8d912adcd269e2a0703a2c1326165e690e211

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2760538
Commit-Queue: Emilia Paz <emil...@chromium.org>
Reviewed-by: Devlin <rdevlin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#884655}


---
M chrome/browser/extensions/api/storage/settings_apitest.cc
M chrome/browser/extensions/service_worker_apitest.cc
M chrome/test/data/extensions/api_test/native_bindings/extension/background.js
M chrome/test/data/extensions/api_test/service_worker/worker_based_background/storage/service_worker_background.js
M chrome/test/data/extensions/api_test/settings/simple_test/background.js
M extensions/browser/api/storage/storage_api.cc
M extensions/browser/api/storage/storage_api.h
M extensions/common/api/_api_features.json
M extensions/common/api/storage.json
M extensions/renderer/storage_area.cc

10 files changed, 460 insertions(+), 35 deletions(-)

37 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

The name of the file: chrome/test/data/extensions/api_test/settings/simple_test/background.js
Insertions: 1, Deletions: 1.

```
@@ -297:298, +297:298 @@
-    // TODO(crbug.com/1185226)): Temporary function for `session` to test default
+    // TODO(crbug.com/1185226): Temporary function for `session` to test default

```
The name of the file: extensions/browser/api/storage/storage_api.cc
Insertions: 1, Deletions: 0.

```
@@ +395:396 @@
+      // TODO(crbug.com/1185226): Add API test that triggers this behavior.

```
The name of the file: extensions/common/api/_api_features.json
Insertions: 1, Deletions: 0.

```
@@ +559:560 @@
+      // TODO(crbug.com/1185226): Restrict to Manifest V3.

```

To view, visit change 2760538. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward