Modify some gc related layout tests to work with Ignition.

Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N

Committed: https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12
Cr-Commit-Position: refs/heads/master@{#395327}

[mythria, 2016-05-20 08:59:05]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 08:59:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999013002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1999013002/1

[commit-bot: I haz the power, 2016-05-20 10:03:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-20 10:03:19]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-05-20 10:24:15]

mythria@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[mythria, 2016-05-20 10:24:16]

The earlier fix () was not correct because when we call a function, we store the
parameters in a register. So, even if we access the object in an inner function
it will still end up in a temporary register when we pass it as a parameter to
internals.observeGC. So I just used the same strategy as I did for the other gc
related tests. I added a function that performs all initializations. 

Last time when I tested I passed the wrong flags :(. This time hopefully I did
correct. They work locally.

[rmcilroy, 2016-05-20 14:53:40]

LGTM with some suggestions.

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html (right):

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html:36: callback =
null;
nit - could you move childConnection and callback into the initialization
function and avoid (it could just be a function literal passed straight to
addEventListener) and avoid having global variables for them which you need to
null.

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html (right):

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html:52: cursor =
null;
Ditto with cursorRequest and cursor

[mythria, 2016-05-23 10:35:45]

Hi Ross, I fixed one of them. For the cursor and cursorRequest, we can't do it I
think. More detailed comment inline.

Thanks,
Mythri.

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html (right):

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html:36: callback =
null;
On 2016/05/20 14:53:39, rmcilroy wrote:
> nit - could you move childConnection and callback into the initialization
> function and avoid (it could just be a function literal passed straight to
> addEventListener) and avoid having global variables for them which you need to
> null.

Done.

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html (right):

https://codereview.chromium.org/1999013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html:52: cursor =
null;
On 2016/05/20 14:53:39, rmcilroy wrote:
> Ditto with cursorRequest and cursor

We can't do this with cursorRequest or cursor. The initialize function is
executed on transaction complete. However we need to open cursor earlier. The
initialize function and the initialization of variables happens at different
times, so I can't move them. I can't make them local variables either. So
leaving this as is.

[rmcilroy, 2016-05-23 10:46:16]

Description was changed from

==========
Modify few gc related layout tests to work with ignition.

In ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

to

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

[rmcilroy, 2016-05-23 10:46:27]

Description was changed from

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

to

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

[rmcilroy, 2016-05-23 10:51:38]

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
(right):

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:9:
textInputController.setMarkedText("hello", 0, 5);
nit - move this initialization into the initialization function too.

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:15:
function initialize() {
nit - you could just make these immediately exectuted function literals, which
might be more consise and avoid name class - i.e.,:

var markedRangeResultGC, selectedRangeResultGC, firstRectForCharacterRangeGC;
(function() {
  ...
})()

[mythria, 2016-05-23 12:27:03]

Thanks, I fixed all tests to call the function immediately.

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
(right):

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:9:
textInputController.setMarkedText("hello", 0, 5);
On 2016/05/23 10:51:38, rmcilroy wrote:
> nit - move this initialization into the initialization function too.

Done.

https://codereview.chromium.org/1999013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:15:
function initialize() {
On 2016/05/23 10:51:38, rmcilroy wrote:
> nit - you could just make these immediately exectuted function literals, which
> might be more consise and avoid name class - i.e.,:
> 
> var markedRangeResultGC, selectedRangeResultGC, firstRectForCharacterRangeGC;
> (function() {
>   ...
> })()

I fixed it in all the tests. Done.

[rmcilroy, 2016-05-23 12:31:17]

LGTM

https://codereview.chromium.org/1999013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
(right):

https://codereview.chromium.org/1999013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:15:
textInputController.setMarkedText("hello", 0, 5);
I was meaning all the initialization code (e.g., getting the input element and
focusing on it too

[mythria, 2016-05-23 13:38:10]

Thanks, Done.

https://codereview.chromium.org/1999013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
(right):

https://codereview.chromium.org/1999013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html:15:
textInputController.setMarkedText("hello", 0, 5);
On 2016/05/23 12:31:17, rmcilroy wrote:
> I was meaning all the initialization code (e.g., getting the input element and
> focusing on it too

Done.

[mythria, 2016-05-23 13:38:17]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 13:38:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999013002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1999013002/80001

[commit-bot: I haz the power, 2016-05-23 14:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 14:46:57]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-05-23 15:18:35]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-05-23 15:18:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1999013002/#ps80001
(title: "Fixed more comments.")

[commit-bot: I haz the power, 2016-05-23 15:18:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999013002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1999013002/80001

[commit-bot: I haz the power, 2016-05-23 15:22:02]

Description was changed from

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

to

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

[commit-bot: I haz the power, 2016-05-23 15:22:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-23 15:23:24]

Description was changed from

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N
==========

to

==========
Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG=chromium:595672
LOG=N

Committed: https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12
Cr-Commit-Position: refs/heads/master@{#395327}
==========

[commit-bot: I haz the power, 2016-05-23 15:23:25]

Patchset 5 (id:??) landed as
https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12
Cr-Commit-Position: refs/heads/master@{#395327}