webbrowser-app

Code review comment for lp:~abreu-alexandre/webbrowser-app/support-oxide-and-qtwebkit

  1. support-oxide-and-qtwebkit
  2. Merge into with-oxide
Revision history for this message
Olivier Tilloy (osomon) wrote :

A couple of general remarks before I dive in the code more seriously:

 - could you please get rid of the ComponentCreation parameter, thus making the instantiation of the component always a 2-step process? I don’t think we’re gaining anything from having two separate code paths there. See https://code.launchpad.net/~mardy/webbrowser-app/add-onlineaccount-support-for-container2/+merge/211701 where Alberto did that

 - the runtime dep on libqt5webkit5-qmlwebkitplugin should be added to webapp-container, not webbrowser-app

 - detecting the architecture would probably be more reliable by testing the definition of Q_PROCESSOR_X86_32, Q_PROCESSOR_X86_64, and Q_PROCESSOR_ARM (include <QtCore/QtGlobal> to make use of them)

 - out of curiosity, wouldn’t using the QFile API work to test whether we can access the oxide-renderer binary?

review: Needs Fixing

« Back to merge proposal