[webkit-reviews] review granted: [Bug 32369] Support for storage and databases in sandboxed iframes : [Attachment 50700] Patch that builds with Chromium

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Patrik Persson
<patrik.j.persson at ericsson.com>'s request for review:
Bug 32369: Support for storage and databases in sandboxed iframes
https://bugs.webkit.org/show_bug.cgi?id=32369

Attachment 50700: Patch that builds with Chromium
https://bugs.webkit.org/attachment.cgi?id=50700&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    bool isLocalStorage = (frame->domWindow()->localStorage() == storage);
> +    ExceptionCode ec = 0;
> +    bool isLocalStorage = (frame->domWindow()->localStorage(ec) == storage
&& !ec);'

> -    bool isLocalStorage = storage->frame()->domWindow()->localStorage() ==
storage;
> +    ExceptionCode ec = 0;
> +    bool isLocalStorage = (storage->frame()->domWindow()->localStorage(ec)
== storage && !ec);

Since the localStorage function returns 0 when there is an error, the !ec check
seems unnecessary.

> -	   for (unsigned i = 0; i < frames.size(); ++i)
> -	      
frames[i]->document()->enqueueStorageEvent(StorageEvent::create(eventNames().st
orageEvent, key, oldValue, newValue, sourceFrame->document()->url(),
frames[i]->domWindow()->localStorage()));
> +	   for (unsigned i = 0; i < frames.size(); ++i) {
> +	       ExceptionCode ec = 0;
> +	       Storage* storage = frames[i]->domWindow()->localStorage(ec);
> +	       if (!ec)
> +		  
frames[i]->document()->enqueueStorageEvent(StorageEvent::create(eventNames().st
orageEvent, key, oldValue, newValue, sourceFrame->document()->url(), storage));

> +	   }

I think it would be cleaner to just check the returned storage object to see if
it's 0 rather than checking ec. We do need the ExceptionCode to communicate
with the JavaScript binding, but I don't think we need to use it inside the
engine itself.

Minor issues.

r=me based on my own reading of the code, the EWS results, and Adam Barth’s
earlier review