Code review comment for lp:~abreu-alexandre/webbrowser-app/container-extension

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> Thanks for the explanations. I've added a few more inline comments

see my comments inline (I updated most)

> , and here
> are two generic remarks:
>
> 1) You should document somewhere the list of hooks that can be intercepted by
> the webapp script. Some king of a readme where we document the hook names,
> their parameters and return values.

agree, done (not as a readme though)

> 2) I really don't like using a Loader in order to load the script: a Loader is
> used to use a QQuick item, while what we really need is a JS object (also,
> using a Loader makes the loading of the webview even more complex). I haven't
> researched this in depth, and indeed it may be that QML doesn't offer much in
> terms of dynamically loading .js files. At a first sight, it looks like
> QJSEngine::evaluate() could be a good option: in webcontainer.cpp you could
> check if the webapp ships a .js file, load it, let the QQmlEngine (which is a
> subclass of QJSEngine) evaluate it and then set the returned QJSValue as a
> property of the main QML file we load:
> http://doc.qt.io/qt-5/qjsengine.html#evaluate
> I never did anything similar, but I believe that it should work.

yes it would work this is what I do for URI filtering code (see comment) in scheme-filter.cpp.

But for this extension mechanism part, I wanted something simpler maybe and more
flexible than plain QJSEngine ... If one wanted to put a bit more logic in
the 'extension', and possibly have more than one file which is not possible with
QJSEngine (import does not work).

WDYT?

« Back to merge proposal