Bug 28776 – [v8] Add support for newly added lowMemoryNotification() API in V8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andrei Popescu]

V8 added a new API for Android's benefit.

http://code.google.com/p/v8/source/detail?spec=svn2777&r=2725

We access V8 via the ScriptController class so the new V8 methods need to be proxied through this class, as well.

[Andrei Popescu]

Created attachment 38674 [details]
Add "LowMemoryNotification()" method on the ScriptController class.

[Mark Rowe (bdash)]

What is the point of adding ScriptController::lowMemoryNotification given that nothing calls it?

[Eric Seidel (no email)]

I assume this is called by some other Andriod code?  Why wouldn't that code just call the v8 API directly?

[David Levin]

Comment on attachment 38674 [details]
Add "LowMemoryNotification()" method on the ScriptController class.

Marking r- based on previous comments in bug that need an answer.

Also, note that chromium has moved past that revision, so there is no need for that if PLATFORM.
   http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?view=markup

[Andrei Popescu]

(In reply to comment #2)
> What is the point of adding ScriptController::lowMemoryNotification given that
> nothing calls it?

Android code calls it. We haven't yet sent the patches that call that code, but we plan to do so very soon.

[Andrei Popescu]

(In reply to comment #3)
> I assume this is called by some other Andriod code?  Why wouldn't that code
> just call the v8 API directly?

Android doesn't really know anything about V8, we just call methods on the script controller. I am not sure I understand why would we make an exception in this case?

[Andrei Popescu]

Created attachment 38859 [details]
Add "LowMemoryNotification()" method on the ScriptController class (take 2)

[Eric Seidel (no email)]

Comment on attachment 38859 [details]
Add "LowMemoryNotification()" method on the ScriptController class (take 2)

Rejecting patch 38859 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Ben Murdoch]

I will land.

[Andrei Popescu]

(In reply to comment #8)
> (From update of attachment 38859 [details])
> Rejecting patch 38859 from commit-queue.  This patch will require manual
> commit.
> 
> ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet',
> '--exit-after-n-failures=1'] failed with exit code 1

Does this mean my patch causes a layout test failure on OSX? That surely can't be since it only touches two files in bindings/v8... 

Anyway, Ben's looking at it.

[Adam Barth]

> Does this mean my patch causes a layout test failure on OSX? That surely can't
> be since it only touches two files in bindings/v8... 

The commit queue might be horked, or it could be flakiness.  I doubt it's a problem with your patch.

[Ben Murdoch]

Landed as r47942.

Sending        WebCore/ChangeLog
Sending        WebCore/bindings/v8/ScriptController.cpp
Sending        WebCore/bindings/v8/ScriptController.h
Transmitting file data ...
Committed revision 47942.

[Eric Seidel (no email)]

media/video-source-add-src.html -> timed out

Your patch was yet another victim of bug 28845. :(