Bug 62863 – [Qt] Orientation sensor is always on, which has negative impact on battery life.

• Component: Platform
• Status: RESOLVED INVALID

[Fabrizio]

In QWebFrame constructor, m_orientation.start() is called to listen for changes in device orientation via the readingChanged signal. 

The sensor is never stopped, but this should be handled with greater responsibility since power consumption due to accelerometer may remain elevated even when sensor readings are not needed.

This behavior is observed on Nokia Symbian devices where battery consumption is severely impacted by running the qtwebkit-based browser.  The accelerometer remains "on" even when browser is sent to background or phone goes to idle mode and screen locks, thus draining battery rapidly.

[Fabrizio]

Created attachment 97600 [details]
Call stop() on the m_orientation sensor object when window is deactivated to prevent battery consumption

Address power consumption by calling stop on m_orientation sensor object when QEvent::WindowDeactivate is received.

Also resume sensor when QEvent::WindowActivate is received by calling start().

[Fabrizio]

Created attachment 97603 [details]
remove unnecessary #include from original patch

[Andreas Kling]

Comment on attachment 97603 [details]
remove unnecessary #include from original patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97603&action=review

What about QGraphicsWebView?
And this feels very hackish, I don't think all users of Q*WebView would want the orientation events to disappear when the app is in the background.
Perhaps we could have an explicit API (on QWebFrame) to enable/disable the accelerometer? I'd probably \internal that kind of API though.

> Source/WebKit/qt/Api/qwebview.cpp:830
> +        QWebFrame* frame = page()->mainFrame();

You should use d->page instead of page() here.

> Source/WebKit/qt/Api/qwebview.cpp:834
> +        if (e->type() == QEvent::WindowDeactivate)
> +              frame->d->m_orientation.stop();
> +        if (e->type() == QEvent::WindowActivate)
> +              frame->d->m_orientation.start();

Coding style, tabs are 4 spaces. Also, second condition should be 'else if'.

[Fabrizio]

Created attachment 97638 [details]
provide API in qwebframe to give qtwebkit client option to start/stop sensors

Reworked patch based on last comments.  Thanks.

[Andreas Kling]

Comment on attachment 97638 [details]
provide API in qwebframe to give qtwebkit client option to start/stop sensors

View in context: https://bugs.webkit.org/attachment.cgi?id=97638&action=review

> Source/WebKit/qt/Api/qwebframe.h:203
> +    void stopOrientationSensor() const;
> +    void startOrientationSensor() const;

'const' does not make semantic sense for these methods, as they alter the state of the object.
Also, for internal methods, we typically do something like:

void QWEBKIT_EXPORT qtwebkit_webframe_setOrientationSensorEnabled(QWebFrame* frame, bool enabled)

(This goes in the cpp file to avoid polluting public headers with internal API.)

[Fabrizio]

Created attachment 97965 [details]
rework based on last comments, introduce new internal APIs according to qtwebkit coding conventions.

Is this ok?  I also created a new static function in QWebFramePrivate to manage the private sensor object from the internal API.

[WebKit Review Bot]

Attachment 97965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebframe.cpp', u'Sou..." exit_code: 1

Source/WebKit/qt/Api/qwebframe.cpp:1637:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Fabrizio]

Created attachment 97968 [details]
Add space between "if" and "(".

Fix style.

[Kenneth Rohde Christiansen]

Comment on attachment 97968 [details]
Add space between "if" and "(".

I find this quite broken. WebKit handles when things has to be suspended and resumed and that is quite tricky, so now you are letting the browser handle it without knowing the right state of the web engine. Why not just fix the suspend/resume instead?

[Andreas Kling]

(In reply to comment #9)
> (From update of attachment 97968 [details])
> I find this quite broken. WebKit handles when things has to be suspended and resumed and that is quite tricky, so now you are letting the browser handle it without knowing the right state of the web engine. Why not just fix the suspend/resume instead?

Excellent point. How can we integrate this with the existing suspend/resume functionality?

[Fabrizio]

Is the resume/suspend functionality contained within the DumpRenderTreeSupportQt::resumeActiveDOMObjects() and DumpRenderTreeSupportQt::suspendActiveDOMObjects() methods?

[Fabrizio]

Don't we need the internal API in QWebFrame even if this behavior is tied to suspend/resume?

Our browser is already suspending/resuming the web engine based on the state of the browser window (active/inactive).

So we will know the state of the web engine when we attempt to start/stop the sensor and thus in our browser code, we will call start/stop in the same event handler that suspends/resumes web engine on receipt of WindowActivated/deactivated event.

Is this what was meant by fixing suspend/resume?   

Or do we wish to stop/start the sensor within the webkit code that actually performs suspend/resume?

If the latter is the case, won't we face the same problem of forcing all clients of webkit (whether they want to or not) to start/stop the sensor when they perform suspend/resume?

[Fabrizio]

Created attachment 98906 [details]
Rework patch to prevent calling start() when sensor is already active and stop() when sensor is inactive.

[Alexis Menard (darktears)]

Comment on attachment 98906 [details]
Rework patch to prevent calling start() when sensor is already active and stop() when sensor is inactive.

View in context: https://bugs.webkit.org/attachment.cgi?id=98906&action=review

> Source/WebKit/qt/ChangeLog:9
> + 

How this API works? Could you tell me how the client code will use it? Is it in the case of there is an orientation sensor but you don't want to use it?

> Source/WebKit/qt/Api/qwebframe.cpp:1615
> +    Give application some control of orientation sensor for power management purposes.

This is a private API why?

[Chang Shu]

Comment on attachment 98906 [details]
Rework patch to prevent calling start() when sensor is already active and stop() when sensor is inactive.

I agree with Kenneth and kling. The webkit should take care of turning on/off the sensor by itself rather than relying on external applications, especially through hacking a private API.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.