Automate more Better Session Restore tests.

Automate more Better Session Restore tests.

This also makes libdom_storage a component instead of a static library.

BUG=153260
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161951

[marja, 2012-10-08 14:25:22]

michaeln, sky: please review.

michaeln: webkit/dom_storage/
sky: chrome/browser/sessions/ & chrome/test/data/

[sky, 2012-10-08 15:41:39]

LGTM

[marja, 2012-10-10 15:13:36]

Patch set 3 was what sky reviewed, but I needed to modify this heavily for the
test to pass.

The problem was that the test was calling
DomStorageArea::SetCommitDelayInSeconds directly, and that won't fly.

More information here: http://codereview.chromium.org/11028110/ (patch set 6 is
a small addition that demonstrates the problem, and patch set 7 fixes it by
exposing DomStorageArea when linking).

The problem is that browser_tests links with libwebkit_support, but
DomStorageArea is not exposed there, and it also links against libcontent, which
links against the static libwebkit_support. Now if I try use DomStorageArea in
the test prog, the address for the file-level static is *not* the same than the
DomStorageAreas accessed via libcontent see. (Making it a static member of
DomStorageArea doesn't make a difference.)

Whereas it works if setting the variable go through the libcontent, and thus the
ugly addition here. wdyt? Or should I unbreak this some other way?

[marja, 2012-10-10 15:21:38]

Oops. s/libwebkit_support/libdom_storage/g in the previous reply.

[sky, 2012-10-10 16:45:49]

You better run this by John (I added him as a reviewer). He'll have a better
idea how to handle it (John, see DisableCommitDelayForTesting).

http://codereview.chromium.org/11088005/diff/7011/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/11088005/diff/7011/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:133: void
DOMStorageContextImpl::DisableCommitDelayForTesting() {
Order doesn't match header.

[jam, 2012-10-10 19:38:58]

why make the changes to content instead of having the test call DomStorageArea
directly?

[marja, 2012-10-11 07:15:18]

On 2012/10/10 19:38:58, John Abd-El-Malek wrote:
> why make the changes to content instead of having the test call DomStorageArea
> directly?

The access needs to go via libcontent, otherwise it doesn't work. The address of
the variable modified is not the same when accessed via test and accessed via
libcontent, i.e.., the test and libcontent have two separate data areas for
libdom_storage. So, if I modify the variable in the test, the actual
DomStorageArea (which gets created when the test page accesses sessionStorage)
doesn't see the modified value.

Pls see http://codereview.chromium.org/11028110/ patch set 6 and the
corresponding tryjob output, demonstrating how the test sees a different address
for the variable than libcontent.

Afaics this is because of -fvisibility=hidden, DomStorageArea not being exposed,
and libdom_storage being a static library. libdom_storage is linked into
libcontent, and then again into the test, and this makes 2 separate copies of
the data area to occur.

Another solution would be to expose DomStorageArea (see
http://codereview.chromium.org/11028110/ patch set 7), but I guess that's not
wanted.

[marja, 2012-10-11 11:58:29]

http://codereview.chromium.org/11088005/diff/7011/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/11088005/diff/7011/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:133: void
DOMStorageContextImpl::DisableCommitDelayForTesting() {
On 2012/10/10 16:45:49, sky wrote:
> Order doesn't match header.

Done.

[jam, 2012-10-11 15:56:49]

On 2012/10/11 07:15:18, marja wrote:
> On 2012/10/10 19:38:58, John Abd-El-Malek wrote:
> > why make the changes to content instead of having the test call
DomStorageArea
> > directly?
> 
> The access needs to go via libcontent, otherwise it doesn't work. The address
of
> the variable modified is not the same when accessed via test and accessed via
> libcontent, i.e.., the test and libcontent have two separate data areas for
> libdom_storage. So, if I modify the variable in the test, the actual
> DomStorageArea (which gets created when the test page accesses sessionStorage)
> doesn't see the modified value.
> 
> Pls see http://codereview.chromium.org/11028110/ patch set 6 and the
> corresponding tryjob output, demonstrating how the test sees a different
address
> for the variable than libcontent.
> 
> Afaics this is because of -fvisibility=hidden, DomStorageArea not being
exposed,
> and libdom_storage being a static library. libdom_storage is linked into
> libcontent, and then again into the test, and this makes 2 separate copies of
> the data area to occur.

the issue here seems to be that dom storage is a static library and not a
component. i.e. if the gyp target's type was 'component' (see webkit_blob.gypi
as an example), this would work. as such, i'd prefer that this be made a
component so that we don't add wrapper functions in the content api (which we
generally try to keep to a minimum)

> 
> Another solution would be to expose DomStorageArea (see
> http://codereview.chromium.org/11028110/ patch set 7), but I guess that's not
> wanted.

[marja, 2012-10-11 16:31:14]

Ok, I changed libdom_storage to be '<(component)'. LGTY assuming the tryjobs
pass?

The test part of patch set 8 is essentially what sky reviewed as patch set 3.

michaeln; could you review the webkit/dom_storage/* parts?

[jam, 2012-10-11 16:35:01]

On 2012/10/11 16:31:14, marja wrote:
> Ok, I changed libdom_storage to be '<(component)'. LGTY assuming the tryjobs
> pass?

that's not enough, see blob_export.h, you'll need something similar for
dom_storage (i.e. dom_storage_export.h). then you'll need to use
DOM_STORAGE_EXPORT keyword by all the classes

[jam, 2012-10-11 16:35:24]

On 2012/10/11 16:35:01, John Abd-El-Malek wrote:
> On 2012/10/11 16:31:14, marja wrote:
> > Ok, I changed libdom_storage to be '<(component)'. LGTY assuming the tryjobs
> > pass?
> 
> that's not enough, see blob_export.h, you'll need something similar for
> dom_storage (i.e. dom_storage_export.h). then you'll need to use
> DOM_STORAGE_EXPORT keyword by all the classes

all the classes that are referenced externally

[michaeln, 2012-10-11 20:06:01]

The tests will need most of the classes in there exported, but the primary
consumer (contentlib+chrome) probably only needs Context, Host, Session,
CachedArea and TaskRunner.

[marja, 2012-10-12 13:47:29]

The latest patch set should now export everything. Can you have another look?

[jam, 2012-10-12 17:12:43]

On 2012/10/12 13:47:29, marja wrote:
> The latest patch set should now export everything. Can you have another look?

cool, thanks. removing myself as reviewer since you don't need my lgtm anymore

[michaeln, 2012-10-12 20:52:15]

lgtm!

[commit-bot: I haz the power, 2012-10-12 21:45:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/24001

[commit-bot: I haz the power, 2012-10-12 21:45:53]

Presubmit check for 11088005-24001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/support

Presubmit checks took 2.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[marja, 2012-10-12 21:56:43]

brettw, could you review the fix in webkit/support/webkit_support.gypi; it
didn't list the dependency, and that error was exposed when libdom_storage was
made a "component".

[brettw, 2012-10-12 23:23:33]

gypi lgtm

[commit-bot: I haz the power, 2012-10-13 13:27:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/24001

[commit-bot: I haz the power, 2012-10-13 13:41:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-13 14:41:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/24001

[commit-bot: I haz the power, 2012-10-13 14:51:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 08:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/42003

[commit-bot: I haz the power, 2012-10-15 08:49:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/36002

[commit-bot: I haz the power, 2012-10-15 09:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/36005

[commit-bot: I haz the power, 2012-10-15 09:45:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 09:48:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/37026

[commit-bot: I haz the power, 2012-10-15 09:54:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 10:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/35005

[commit-bot: I haz the power, 2012-10-15 10:30:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 10:55:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/42012

[commit-bot: I haz the power, 2012-10-15 11:40:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 12:51:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/35009

[commit-bot: I haz the power, 2012-10-15 13:13:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/45010

[commit-bot: I haz the power, 2012-10-15 14:00:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 17:45:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11088005/40006

[commit-bot: I haz the power, 2012-10-15 20:44:53]

Change committed as 161951