New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 604292 link

Starred by 3 users
Status: WontFix
Owner:
█ mgiuca@chromium.org
Long OOO (go/where-is-mgiuca)
Closed: Apr 2016
Cc:
kinuko@chromium.org
nhiroki@chromium.org
jsb...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 603782



Sign in to add a comment

WorkerNavigator: webkitTemporaryStorage and webkitPersistentStorage are undefined

Project Member Reported by mgiuca@chromium.org, Apr 18 2016 
Version: 52
OS: All

What steps will reproduce the problem?
(1) Start a service worker.
(2) Open the inspector for the worker (chrome://serviceworker-internals).
(3) navigator.webkitTemporaryStorage or navigator.webkitPersistentStorage.

What is the expected output?
DeprecatedStorageQuota {}

What do you see instead?
undefined

These are deprecated so we may in fact not want to introduce them on WorkerNavigator. However, they are not undefined due to a policy decision. They are undefined due to a bug in the bindings generator:  Issue 603782  (in fact, they are erroneously gated on the geofencing flag --- as soon as geofencing becomes available in stable, these methods will also become stable).

See the IDL file which explicitly adds webkitTemporaryStorage and webkitPersistentStorage to the WorkerNavigator object:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/quota/WorkerNavigatorStorageQuota.idl

There is a test, but it is expecting the wrong behaviour:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt&l=659

When I fix  Issue 603782  (https://codereview.chromium.org/1884423002/), webkitTemporaryStorage and webkitPersistentStorage will automatically become available in WorkerNavigator as WorkerNavigatorStorageQuota. *IF WE DO ACTUALLY NOT WANT THESE METHODS* on WorkerNavigator, then we need to make an explicit policy decision and delete them from WorkerNavigator.idl. I am not the right person to make that call. Adding the authors of WorkerNavigator.idl.

In other words, is the test correct or the IDL?

Comment 1 by mgiuca@chromium.org, Apr 18 2016

Cc: alecflett@chromium.org
Labels: M-52
There is precedent for this decision:
https://codereview.chromium.org/1667213002

"Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers."

So maybe the correct answer is to delete these methods entirely from WorkerNavigatorStorageQuota.idl.

Note: There are actually lots of tests for this but they all run with the experimental flag turned on (so geofencing is enabled and therefore the quota APIs are also enabled). There is only one test with the flag turned off, and that test was auto-generated.

I did some archaeology to figure out whether this ever worked but it's hard because tests were only added recently.

1. Quota API added to WorkerNavigator (Mar 2013 / WebKit r146764). Presumably it worked at the time, though as I said above, there are *no* tests for stable.
2. Geofencing partial interface added (Sep 2014 / Blink r181755). Presumably this inadvertently disabled the Quota API from WorkerNavigator due to  Issue 603782 , but it's hard to tell (some other partial interface may have already disabled it before then).
3. webexposed tests for stable were added (July 2015 / Blink r198179). At this time, the methods were not exposed on WorkerNavigator due to this bug.

Because the tests were only added later, we can't figure out exactly when this was working / broken without going back and testing old versions of Chromium. But we can safely say this hasn't been working since at least M45.

Comment 2 by dgro...@chromium.org, Apr 18 2016

Cc: -alecflett@chromium.org -dgro...@chromium.org kinuko@chromium.org jsb...@chromium.org
jsbell@ probably knows the most about this space.

Comment 3 by jsb...@chromium.org, Apr 18 2016 

Thanks for digging, mguica@! 

I commented on the CL, but to recap here: if we've managed to "unship" anything webkit-prefixed in Workers, let's keep them unshipped. (I sent an I2S for a new Quota API)

So lets disable them in the IDL.

Comment 4 Deleted

Comment 5 by mgiuca@chromium.org, Apr 19 2016

Blockedon: -603782
Blocking: 603782
OK, on both yours and tkent's advice, it looks like we should be deleting this API rather than reinstating it. Therefore, I've mailed out a CL to remove it from WorkerNavigator (it will remain in Navigator as that is actually available on stable):
https://codereview.chromium.org/1896043002/

Once that is landed, fixing  Issue 603782  will have no immediate effect. Then I will close this issue as WontFix.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 19 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bb2926b2514ffec767b574df4fa1f4e339e3eab

commit 9bb2926b2514ffec767b574df4fa1f4e339e3eab
Author: mgiuca <mgiuca@chromium.org>
Date: Tue Apr 19 23:45:19 2016

WorkerNavigator: Remove deprecated Quota Management API.

Removes webkitTemporaryStorage and webkitPersistentStorage from
WorkerNavigator (which were available behind the
enable-experimental-web-platform-features flag). Does not affect stable.

These attributes were accidentally removed from stable in 2014 (due to
 https://crbug.com/604292 ) and nobody noticed. Since they are deprecated
and non-standard, the decision has been made to remove these rather than
reinstate them.

BUG= 604292 

Review URL: https://codereview.chromium.org/1896043002

Cr-Commit-Position: refs/heads/master@{#388361}

[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/resources/storagequota-query-usage.js
[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/resources/worker-storagequota-query-usage.js
[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/shared-worker-storagequota-query-usage-expected.txt
[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/shared-worker-storagequota-query-usage.html
[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/worker-storagequota-query-usage-expected.txt
[delete] https://crrev.com/ee3dffb579500fc4257369333a7f239620dd72c2/third_party/WebKit/LayoutTests/fast/workers/worker-storagequota-query-usage.html
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/Source/modules/quota/WorkerNavigatorStorageQuota.cpp
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/Source/modules/quota/WorkerNavigatorStorageQuota.h
[modify] https://crrev.com/9bb2926b2514ffec767b574df4fa1f4e339e3eab/third_party/WebKit/Source/modules/quota/WorkerNavigatorStorageQuota.idl

Comment 7 by mgiuca@chromium.org, Apr 20 2016

Status: WontFix (was: Started)
As discussed, WorkerNavigator.webkit{Temporary,Persistent}Storage will remain undefined, so closing as WontFix. (As of r388361, they are explicitly undefined as opposed to accidentally undefined.)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43

commit 4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43
Author: mgiuca <mgiuca@chromium.org>
Date: Wed Apr 20 02:48:56 2016

IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG= 603782 , 604292 

Review URL: https://codereview.chromium.org/1884423002

Cr-Commit-Position: refs/heads/master@{#388409}

[modify] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py
[add] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial.idl
[add] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial2.idl
[modify] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp
[modify] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.h
[add] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface2Partial.cpp
[add] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface2Partial.h
[modify] https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43/third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py
►

Sign in to add a comment