Merge into trunk : container-print-help-when-no-appid : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
Olivier Tilloy		2016-06-13	Approve on 2016-06-16
system-apps-ci-bot	continuous-integration		Needs Fixing on 2016-06-16
Review via email: mp+297220@code.qastaging.launchpad.net

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-06-13:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/526/rebuild

FAILED: Continuous integration, rev:1477
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/526/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build/553
    FAILURE: https://jenkins.canonical.com/system-apps/job/test-0-autopkgtest/label=phone-armhf,release=vivid+overlay,testname=default/62/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-0-fetch/553
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=vivid+overlay/541
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=xenial/541
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/537/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/537/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/537/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/537/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/537/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/537
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/537/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/526/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-06-14:

#

BrowserApplication::isPrintHelpLaunch() doesn’t need to take an argument, it could be made non-static and read m_arguments.

But there’s a cleaner approach I think: the check for -h or --help and the call to printUsage() could be done in the constructor (BrowserApplication::BrowserApplication()), and this would set a flag (maybe called "m_shouldExitEarly", or something like that) that the main function would test before calling initialize().

On a slightly related note, in WebappContainer::initialize() the call to earlyEnvironment() should go after the check for the appId.

lp://qastaging/~abreu-alexandre/webbrowser-app/container-print-help-when-no-appid updated on 2016-06-14

1478. By Alexandre Abreu on 2016-06-14: delay the call to earlyEnvironment()

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-06-14:

#

Thank you for your comments,

> BrowserApplication::isPrintHelpLaunch() doesn’t need to take an argument, it
> could be made non-static and read m_arguments.

I know, but I tend to prefer environment agnostic function when possible,
hence my approach there, I dont like "introspective" function that dont have to be,

> But there’s a cleaner approach I think: the check for -h or --help and the
> call to printUsage() could be done in the constructor
> (BrowserApplication::BrowserApplication()), and this would set a flag (maybe
> called "m_shouldExitEarly", or something like that) that the main function
> would test before calling initialize().

I am not sure if it is a cleaner approach, constructor work + state flag seem weird
to me,
What bothers you is the double calls to isPrintHelpLaunch?

> On a slightly related note, in WebappContainer::initialize() the call to
> earlyEnvironment() should go after the check for the appId.

+1, done

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-06-14:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/530/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-06-16:

#

> I know, but I tend to prefer environment agnostic function when
> possible, hence my approach there, I dont like "introspective"
> function that dont have to be,

I’m not sure I’m following you here. isPrintHelpLaunch() is not used outside of the context of BrowserApplication (and indeed it’s a member function), so what’s wrong with making it non-static and accessing member variables?
It has a similar scope to BrowserApplication::inspectorPort() or BrowserApplication::urls(), which both read m_arguments.

> I am not sure if it is a cleaner approach, constructor work + state
> flag seem weird to me,

You’re right, it would really be cleaner if there was a way to bail out early in the constructor, but adding a flag sort of cancels the "elegance" of the solution.

> What bothers you is the double calls to isPrintHelpLaunch?

I guess the naming does :) Can it be renamed to e.g. "helpRequested", or maybe "shouldDisplayHelp"?

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-06-16:

#

> > I know, but I tend to prefer environment agnostic function when
> > possible, hence my approach there, I dont like "introspective"
> > function that dont have to be,
>
> I’m not sure I’m following you here. isPrintHelpLaunch() is not used outside
> of the context of BrowserApplication (and indeed it’s a member function), so
> what’s wrong with making it non-static and accessing member variables?
> It has a similar scope to BrowserApplication::inspectorPort() or
> BrowserApplication::urls(), which both read m_arguments.

yes but the key point is that it is a pure side effect free function
which I tend to set as static. Why make a function access its environment
where it does not need to be? and here it does not need to,

but I changed it to using m_arguments (even if I am not really for it) for
the sake of consistency w/ the rest, and it is a minor thing anyway,

>
> > What bothers you is the double calls to isPrintHelpLaunch?
>
> I guess the naming does :) Can it be renamed to e.g. "helpRequested", or maybe
> "shouldDisplayHelp"?

done to helpRequested,

lp://qastaging/~abreu-alexandre/webbrowser-app/container-print-help-when-no-appid updated on 2016-06-16

1479. By Alexandre Abreu on 2016-06-16: Renaming; remove static

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-06-16:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/541/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-06-16:

#

Looks good to me now, thanks!

review: Approve

webbrowser-app

Merge lp://qastaging/~abreu-alexandre/webbrowser-app/container-print-help-when-no-appid into lp://qastaging/webbrowser-app

Commit message

Description of the change

Preview Diff

Subscribers