[webkit-reviews] review denied: [Bug 33857] [Qt] Need a public API for suspending/resuming DOM Objects on a page : [Attachment 46941] Proposed API for QWebFrame and QWebPage, and manual-tests

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Jesus
Sanchez-Palencia <jesus.palencia at openbossa.org>'s request for review:
Bug 33857: [Qt] Need a public API for suspending/resuming DOM Objects on a page
https://bugs.webkit.org/show_bug.cgi?id=33857

Attachment 46941: Proposed API for QWebFrame and QWebPage, and manual-tests
https://bugs.webkit.org/attachment.cgi?id=46941&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>

> 
> The API from QWebPage will call suspend/resume DOM Objects of the main frame
> and of all its children.


Add API to QWebPage so that it is possible to suspend/resume...


> * manual-tests/frames-multiple-active-dom-objects.html: Added.
> * manual-tests/resources/Elvis_with_parachute.gif: Added.

What is the license of this file?


> +// FIXME: add support to suspending/resuming of animated GIFs and plugins.
> +// Suspend DOM objects (JS timers, CSS animation, SVG animation) in this
frame.
> +bool QWebFramePrivate::suspendDOMObjects()
> +{
> +    if (!frame)
> +	   return false;
> +
> +    AnimationController* controller = frame->animation();
> +    if (!controller)
> +	   return false;

This seems wrong... what if you dont have a controller, can't you still suspend
the active DOM objects included by
doc->suspendActiveDOMObjects(); ?

> +
> +    Document* doc = frame->document();
> +    if (!doc)
> +	   return false;
> +
> +    // JavaScript
> +    doc->suspendActiveDOMObjects();

> +bool QWebFramePrivate::resumeDOMObjects()

same thing here

> +    QList<QWebFrame *> subFrames = frame->childFrames();

Should be  QList<QWebFrame*>. childFrames is probably a better name than
subFrames :-) and it is already used

> +    QList<QWebFrame *>::const_iterator i;
> +    for (i = subFrames.constBegin(); i != subFrames.constEnd() ; ++i) {
> +	   b = (b && suspendChildFrames((*i)));
> +    }
> +

Don't use b, use a descriptive name.

> +bool QWebPagePrivate::resumeChildFrames(QWebFrame *frame)

Wrong coding style, QWebFrame* frame

> +{
> +    if (!frame)
> +	   return false;
> +
> +    bool b = frame->resumeDOMObjects();
> +

Don't use b

> +    QList<QWebFrame *> subFrames = frame->childFrames();
> +    QList<QWebFrame *>::const_iterator i;

Coding style

> +/*!
> +    Resume all frames, starting at the main frame.
> +    DOM Objects (JS timers, CSS and SVG animation) will be resumed.

DOM objects that were suspended, will be resumed.

> +    bool suspendChildFrames(QWebFrame *frame);
> +    bool resumeChildFrames(QWebFrame *frame);

Coding style