I suspect that much of the failures are due to generic bindings issues, but I think not all of them, attention required.

storage_builtins.html is issue 510994

storage_setitem.html appears to pass in M53

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

raphael, maybe you can check to see if we're passing or what the remaining failures are?

I'm pretty sure we're still failing cases covered by issue 510994 but the others may be fixed with your CL?

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

commit 950f7f23ed5a060b0af65f914230f46eee858c01
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Oct 02 20:04:10 2017

Storage.idl: Drop [NotEnumerable], remove anonymous getter/setter/deleter.

Fix a few TODO items by making our IDL closer to the spec:

* There is nothing in the spec saying the operations declared in the Storage
  interface should not be enumerable. Since the default according to WebIDL
  is for operations to be enumerable, drop the non-standard [NotEnumerable]
  extended attribute.
  This is a user-visible behavior change that matches the spec, as well as
  Gecko (since Firefox 34) and WebKit (since r195760, Safari 10.1).

* Merge the anonymous getter, setter and deleter into the existing getItem,
  setItem and removeItem operations as mandated by the spec. One visible
  difference is that the anonymous getter returned a DOMString, whereas
  getItem() returns a DOMString? (i.e. null is valid return value).

Adjust a few tests in the process:
* external/wpt/webstorage/storage_enumerate.html: the first test did not
  make a lot of sense; now that Storage's operations are enumerable, looping
  with a for-in would add them to |enumeratedArray| unless we add a
  hasOwnProperty() check. Adding that check no longer adds
  "prototypeTestKey" to |enumeratedArray| though, but that is working as
  expected since it is not really a key but rather something inherited from
  Storage's prototype. Add a test for that as well.
* Drop a few Blink-only tests from storage/domstorage that were duplicating
  the same checks done by the test above. It makes sense to rely on WPT
  instead of fixing them as well.

Bug: 651774
Change-Id: Ibf90700eb3d472f66b4ff2dc6ab9b6e6a4b2f211
Reviewed-on: https://chromium-review.googlesource.com/691974
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505745}
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/external/wpt/webstorage/idlharness-expected.txt
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/LayoutTests/external/wpt/webstorage/storage_enumerate.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/platform/linux/virtual/mojo-localstorage/external/wpt/webstorage/idlharness-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/platform/mac/virtual/mojo-localstorage/external/wpt/webstorage/idlharness-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/localstorage/enumerate-storage-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/localstorage/enumerate-storage.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-storage-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-storage.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-with-length-and-key-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-with-length-and-key.html
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.cpp
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.h
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.idl

Sure; I don't have access to my primary build machine until Thursday to quickly build content_shell and run the webstorage tests, but I'll get to that as soon as I can.

Meanwhile, I've added a few chromestatus.com entries for the user-visible changes mentioned in comment #7:
- https://www.chromestatus.com/feature/5120599789666304 (Web Storage: Methods are now enumerable)
- https://www.chromestatus.com/feature/5700418495578112 (Web Storage: Anonymous getter may return null)

#9 - does that mean that one or more of the following snippets (for example) -
1. for (x in localStorage) {...}
2. Object.keys(localStorage)
Would now also include getItem and similar?
If so, it seems like a potential compatibility issue and probably needs an intent thread.

#9 - And for me localStorage.a returns undefined, not the empty string, so how can I trigger this anonymous getter?

Anyway, on the surface, those two changes sounds like compatibility breaks that (I think) merit intents, use counters, and/or a deprecation period.

> 1. for (x in localStorage) {...}
> 2. Object.keys(localStorage)
> Would now also include getItem and similar?

Yes for 1, no for 2: getItem() and friends are in Storage.prototype, so
they show up in for-in loops but not Object.keys().

> #9 - And for me localStorage.a returns undefined, not the empty
> string, so how can I trigger this anonymous getter?

Thanks for spotting that; I misread the old code and the bindings side
indeed just turns the empty strings returned by the old anonymous getter
code into undefined. I've updated the chromestatus.com entry.

> Anyway, on the surface, those two changes sounds like compatibility
> breaks that (I think) merit intents, use counters, and/or a
> deprecation period.

I asked the reviewers in Gerrit if they'd prefer I sent an I2I email
first, but jsbell seemed fine with the patch.

jsbell/michaeln: would you prefer that I reverted it (at least until M63
branches) and/or started an intent thread on blink-dev?

This seems below the bar for requiring an intent to me, but sending a note to blink-dev wouldn't hurt. Sites that relied on this would have been broken in Firefox/Safari, and it's unfortunately not the kind of change we can validate using HTTPArchive or UMA stats and we can't issue console warnings. It'll have to get feedback from anyone with an affected (Chrome-only) site.

But as noted, an FYI message to blink-dev wouldn't hurt.

FYI sent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/uptbsZKhUxg/xTZalM25BAAJ

> raphael, maybe you can check to see if we're passing or what
> the remaining failures are?
>
> I'm pretty sure we're still failing cases covered by issue 510994
> but the others may be fixed with your CL?

Right now, we're failing 3 tests:
* storage_builtins.html (bug 510994)
* storage_session_window_noopener.html (new test from Sept 2017, we need to adapt to a change in the HTML spec, see bug 771959)
* storage_string_conversion.html (started failing recently, blocked on  bug v8:4958 ).

The 3 bugs mentioned above are now marked as blocking this one; for now, there isn't much else to do here.

Thank you raphael - for the investigation, tracking bugs, and follow-up on the v8 issue!

\o/

I'm changing the dependency on  bug v8:4958  to a dependency on  issue 753073 , which tracks the LayoutTests side of that V8 bug more closely.

FWIW, storage_string_conversion.html started working after the rebaseline commit from the bug above.

https://wpt.fyi/results/webstorage/storage_builtins.html?sha=434ca47448 still shows Chrome failing and Edge and Firefox passing. Safari also fails though.

And https://wpt.fyi/results/webstorage/storage_setitem.html?sha=434ca47448 is reported as a crash for Chrome. Needs investigation.