Bug 45149 - [Qt] V8 port for Qt platform: Qt WebCoreSupport changes
Summary: [Qt] V8 port for Qt platform: Qt WebCoreSupport changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 45136
  Show dependency treegraph
 
Reported: 2010-09-02 16:14 PDT by Vlad
Modified: 2010-09-11 16:07 PDT (History)
6 users (show)

See Also:


Attachments
QT WebCoreSupport changes (deleted)
2010-09-02 17:50 PDT, Vlad
kling: review-
Details | Formatted Diff | Diff
Review comments changes (15.86 KB, patch)
2010-09-03 18:26 PDT, Vlad
kling: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vlad 2010-09-02 16:14:53 PDT
V8 changes in
WebCoreSupport/FrameLoaderClientQt.cpp
WebCoreSupport/FrameLoaderClientQt.h
WebCoreSupport/DumpRenderTreeSupportQt.cpp
Comment 1 Vlad 2010-09-02 17:50:41 PDT
Created attachment 66444 [details]
QT WebCoreSupport changes
Comment 2 Andreas Kling 2010-09-03 03:56:15 PDT
Comment on attachment 66444 [details]
QT WebCoreSupport changes

> +#if USE(JSC)
>      return JSDOMWindowBase::commonJSGlobalData()->heap.globalObjectCount();
> +#else
> +    // TBD
> +    return 1;
> +#endif

What's the plan for this?

> +#if 0

For disabled code blocks (and workarounds like the above), please include a comment saying why it's disabled (and if applicable, a bug# where we track any dependencies of the disabled code.)

This patch then digresses into style cleanups. While nice, please do that in a separate patch.
Comment 3 Vlad 2010-09-03 11:56:32 PDT
(In reply to comment #2)
There is no such API in v8 AFAIK
> (From update of attachment 66444 [details])
> > +#if USE(JSC)
> >      return JSDOMWindowBase::commonJSGlobalData()->heap.globalObjectCount();
> > +#else
> > +    // TBD
> > +    return 1;
> > +#endif
> 
> What's the plan for this?
> 
Has to be enabled. #if 0 removed.
> > +#if 0
> 
> For disabled code blocks (and workarounds like the above), please include a comment saying why it's disabled (and if applicable, a bug# where we track any dependencies of the disabled code.)
> 
> This patch then digresses into style cleanups. While nice, please do that in a separate patch.
Comment 4 Vlad 2010-09-03 18:26:41 PDT
Created attachment 66578 [details]
Review comments changes

Changed 
#if USE(JSC)
...
#elif USE(V8)
...
#endif
Enabled garbageCollectorCollect() v8 implementation.
Comment 5 Andreas Kling 2010-09-05 17:10:05 PDT
Comment on attachment 66578 [details]
Review comments changes

> +        Comment out references to JSC.

This isn't a very accurate summary of the changes below.

Patch contents are looking pretty good though like I've said before I'd personally prefer it without all the unrelated style fixups.
FIXME's are acceptable at this stage IMO, but for longer-term things there should be bugs opened and referenced in the code.

r- for lack of relevant ChangeLog.
Comment 6 Kent Hansen 2010-09-08 08:14:31 PDT
Comment on attachment 66578 [details]
Review comments changes

>  void DumpRenderTreeSupportQt::garbageCollectorCollect()
>  {
> +#if USE(JSC)
>      gcController().garbageCollectNow();
> +#elif USE(V8)
> +    v8::Local<v8::Context> context = v8::Context::GetCurrent();
> +    Frame* frame = V8Proxy::retrieveFrame(context);
> +    V8Proxy* proxy = V8Proxy::retrieve(frame);
> +    if (proxy)
> +        proxy->evaluate(ScriptSourceCode("if (window.gc) void(gc());"), 0);
> +#endif
>  }

You can call v8::LowMemoryNotification() instead (it calls an internal CollectAllGarbage() function).

And please please get rid of all the style changes; you can disregard anything the style checker complains about for lines that _you_ didn't touch.
Comment 7 Andreas Kling 2010-09-11 10:44:37 PDT
Committed r67304: <http://trac.webkit.org/changeset/67304>
Comment 8 WebKit Review Bot 2010-09-11 16:07:25 PDT
http://trac.webkit.org/changeset/67304 might have broken Leopard Intel Debug (Tests)