Bug 85221 - [chromium] DomStorage events handling needs TLC (2)
Summary: [chromium] DomStorage events handling needs TLC (2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on: 86484
Blocks: 85215
  Show dependency treegraph
 
Reported: 2012-04-30 11:37 PDT by Michael Nordman
Modified: 2012-05-21 10:19 PDT (History)
6 users (show)

See Also:


Attachments
events3 (40.18 KB, patch)
2012-04-30 11:40 PDT, Michael Nordman
michaeln: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.48 MB, application/zip)
2012-04-30 13:53 PDT, WebKit Review Bot
no flags Details
events3 (40.18 KB, patch)
2012-04-30 15:25 PDT, Michael Nordman
michaeln: commit-queue-
Details | Formatted Diff | Diff
events3 (40.18 KB, patch)
2012-04-30 17:37 PDT, Michael Nordman
abarth: review+
michaeln: commit-queue-
Details | Formatted Diff | Diff
events3-again (39.99 KB, patch)
2012-05-18 17:17 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
events3-again (41.41 KB, patch)
2012-05-18 17:23 PDT, Michael Nordman
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2012-04-30 11:37:30 PDT
See https://bugs.webkit.org/show_bug.cgi?id=85215 for more info.

Alter the WebKit::WebStorageArea and WebCore::StorageArea virtual interfaces
such that the mutators no longer return old values. This is to allow implementations
of the interface to operate more asynchronously.

Also clean up from the last patch, remove support for the DEPRECATED event
dispatching API.
Comment 1 Michael Nordman 2012-04-30 11:40:37 PDT
Created attachment 139494 [details]
events3
Comment 2 WebKit Review Bot 2012-04-30 11:43:07 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Michael Nordman 2012-04-30 12:09:21 PDT
This is the third in the multi-sided sequence of changes.
wkpatch1 - https://bugs.webkit.org/show_bug.cgi?id=84387
crpatch2 - http://codereview.chromium.org/10201010/
Comment 4 WebKit Review Bot 2012-04-30 13:53:14 PDT
Comment on attachment 139494 [details]
events3

Attachment 139494 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12586367

New failing tests:
storage/domstorage/events/documentURI.html
storage/domstorage/events/basic-setattribute.html
storage/domstorage/events/basic-body-attribute.html
storage/domstorage/events/basic.html
storage/domstorage/events/case-sensitive.html
Comment 5 WebKit Review Bot 2012-04-30 13:53:21 PDT
Created attachment 139510 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Michael Nordman 2012-04-30 14:11:33 PDT
(In reply to comment #5)
> Created an attachment (id=139510) [details]
> Archive of layout-test-results from ec2-cr-linux-01
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

crpatch two is r134037
https://src.chromium.org/viewvc/chrome?view=rev&revision=134037

From the bot build log...
48>________ running 'svn update /mnt/git/webkit-chromium-ews/Source/WebKit/chromium/webkit --revision 133673 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-chromium-ews/Source/WebKit/chromium'
48>At revision 133673.

133673 < 134037... looks like i have to inverse rolldeps
Comment 7 Michael Nordman 2012-04-30 14:26:56 PDT
> 133673 < 134037... looks like i have to inverse rolldeps

ah... gavin already did it today
Comment 8 Michael Nordman 2012-04-30 15:25:24 PDT
Created attachment 139530 [details]
events3

re-upping the same patch to get new crlinux try job to run (idk, maybe there's a better way to do that)
Comment 9 WebKit Review Bot 2012-04-30 16:46:35 PDT
Comment on attachment 139530 [details]
events3

Attachment 139530 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12598222
Comment 10 Michael Nordman 2012-04-30 17:37:19 PDT
Created attachment 139551 [details]
events3

fixed the gyp file to delete the 3 files listed in the changelog instead of 2 out of those 3 and then one that wasn't supposed to be deleted :/
Comment 11 Michael Nordman 2012-04-30 19:11:26 PDT
hooray for green bots!
Comment 12 Adam Barth 2012-05-01 16:12:48 PDT
Comment on attachment 139551 [details]
events3

View in context: https://bugs.webkit.org/attachment.cgi?id=139551&action=review

> Source/WebKit/chromium/src/WebViewImpl.h:304
> +    // A pageGroup identifies a namespace of pages. Page groups are used on OSX

OSX -> PLATFORM(MAC)

Specifically, it's used by the Apple port on OSX, not by other ports (like Chromium) on OSX.
Comment 13 Michael Nordman 2012-05-01 17:09:22 PDT
thank you!

i'll fixup the comment prior to committing... like we chatted about, i'll be waiting till after the chromium branch date to land
Comment 14 Michael Nordman 2012-05-09 16:30:27 PDT
> i'll fixup the comment prior to committing... like we chatted about, i'll be waiting till after the chromium branch date to land

just a heads up, i'm still waiting to land this till branch is safely behind us (which apparently is not quite yet)
Comment 15 Michael Nordman 2012-05-10 17:42:36 PDT
Committed r116712: <http://trac.webkit.org/changeset/116712>
Comment 16 WebKit Review Bot 2012-05-15 07:43:33 PDT
Re-opened since this is blocked by 86484
Comment 17 Michael Nordman 2012-05-18 17:17:51 PDT
Created attachment 142825 [details]
events3-again

Let's try this one again. The original patch got reverted because it made for spectacular crash fest, see http://code.google.com/p/chromium/issues/detail?id=127919 for those details.

This patch very close to the original with minor of differences to avoid crashing the crash described in the crbug. The critical difference is to not create a page's sessionStorage object while iterating in the 'find' method.
Comment 18 Michael Nordman 2012-05-18 17:23:16 PDT
Created attachment 142828 [details]
events3-again

added the ChangeLog to the patch
Comment 19 Adam Barth 2012-05-19 09:24:20 PDT
Comment on attachment 142828 [details]
events3-again

View in context: https://bugs.webkit.org/attachment.cgi?id=142828&action=review

> Source/WebKit/chromium/src/WebViewImpl.h:323
> +    // A pageGroup identifies a namespace of pages. Page groups are used on OSX

OSX -> PLATFORM(MAC)
Comment 20 Michael Nordman 2012-05-21 10:19:35 PDT
Committed r117797: <http://trac.webkit.org/changeset/117797>