Bug 32724 – [Qt] It should be possible to disable inspector

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Ismail Donmez]

If you disable inspector in Platform.h via

#define ENABLE_INSPECTOR 0

build will fail ultimately, mostly because the build system does not handle this case. Disabling such features are important for embedded systems.

[Ismail Donmez]

Btw fixing the sources to handle !ENABLE(INSPECTOR) case is easy but the build system has inspector support hardcoded.

[Laszlo Gombos]

Created attachment 45636 [details]
proposed patch

To turn off inspector support built QtWebKit with the following command

build-webkit --no-javascript-debugger DEFINES+="ENABLE_INSPECTOR=0"

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45636 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45636 [details]
proposed patch

Should InspectElement even be defined?
20372045         case InspectElement: {
Seems we might as well remove that when ENABLE(INSPECTOR) isn't defined (unless it's part of some public API header and difficutl to remove?)

In general this looks OK.  Why would we even compile InspectorClientQt when ENABLE(INSPECTOR) was not defined?

[Ismail Donmez]

Also --minimal should disable inspector too.

[Simon Hausmann]

Comment on attachment 45636 [details]
proposed patch

r=me. I wonder what the gain is for embedded builds, given that we exclude the bulk of data files anyway.

[Ismail Donmez]

Well even 10k matters for me on Windows CE 5.0. Thanks for the patch Laszlo, its appreciated.

[WebKit Commit Bot]

Comment on attachment 45636 [details]
proposed patch

Clearing flags on attachment: 45636

Committed r52662: <http://trac.webkit.org/changeset/52662>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ismail Donmez]

The patch misses some ifdefs it seems, I get:

..\..\..\WebCore\rendering\RenderLayerBacking.cpp(1028) : error C2039: 'inspectorTimelineAgent' : is not a member of 'WebCore::Page' see declaration of 'WebCore::Page'

[Ismail Donmez]

Created attachment 48238 [details]
Patch to add missing ifdef for the disabled inspector case

[Ismail Donmez]

Created attachment 48239 [details]
Patch to add missing ifdef for the disabled inspector case (add the semicolon to make it compile!)

[Ismail Donmez]

Ping?

[Ismail Donmez]

Created attachment 48795 [details]
Patch against latest SVN, fix new compilation failures

[Pavel Feldman]

Comment on attachment 48795 [details]
Patch against latest SVN, fix new compilation failures

>  
>  static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
>  {
> +#if !ENABLE(INSPECTOR)
> +    return 0;
> +#else
>      Frame* frame = renderer->document()->frame();
>      if (!frame)
>          return 0;
> @@ -1026,6 +1029,7 @@ static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
>      if (!page)
>          return 0;
>      return page->inspectorTimelineAgent();
> +#endif
>  }


You should just surround this entire method with if ENABLE(INSPECTOR). Please fix this before landing.

>  
>  // Up-call from compositing layer drawing callback.
> diff --git a/WebKit/qt/Api/qwebpage.cpp b/WebKit/qt/Api/qwebpage.cpp
> index 185a2ee..256d6c4 100644
> --- a/WebKit/qt/Api/qwebpage.cpp
> +++ b/WebKit/qt/Api/qwebpage.cpp
> @@ -163,27 +163,42 @@ QString QWEBKIT_EXPORT qt_webpage_groupName(QWebPage* page)
>  
>  void QWEBKIT_EXPORT qt_drt_webinspector_executeScript(QWebPage* page, long callId, const QString& script)
>  {

Nit: In case these are only used from within DRT, should you hide these methods as well and use guards in layout test controller instead?

[Ismail Donmez]

(In reply to comment #15)
> (From update of attachment 48795 [details])
> >  
> >  static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> >  {
> > +#if !ENABLE(INSPECTOR)
> > +    return 0;
> > +#else
> >      Frame* frame = renderer->document()->frame();
> >      if (!frame)
> >          return 0;
> > @@ -1026,6 +1029,7 @@ static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> >      if (!page)
> >          return 0;
> >      return page->inspectorTimelineAgent();
> > +#endif
> >  }
> 
> 
> You should just surround this entire method with if ENABLE(INSPECTOR). Please
> fix this before landing.

Since I cannot land myself anyway, if I just surround with if ENABLE(INSPECTOR) it would give a compiler warning/error saying the function should return a value.

[Pavel Feldman]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 48795 [details] [details])
> > >  
> > >  static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> > >  {
> > > +#if !ENABLE(INSPECTOR)
> > > +    return 0;
> > > +#else
> > >      Frame* frame = renderer->document()->frame();
> > >      if (!frame)
> > >          return 0;
> > > @@ -1026,6 +1029,7 @@ static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> > >      if (!page)
> > >          return 0;
> > >      return page->inspectorTimelineAgent();
> > > +#endif
> > >  }
> > 
> > 
> > You should just surround this entire method with if ENABLE(INSPECTOR). Please
> > fix this before landing.
> 
> Since I cannot land myself anyway, if I just surround with if ENABLE(INSPECTOR)
> it would give a compiler warning/error saying the function should return a
> value.

I mean surround the function, not its body. It should never be referenced if there is no inspector.

[Ismail Donmez]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 48795 [details] [details] [details])
> > > >  
> > > >  static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> > > >  {
> > > > +#if !ENABLE(INSPECTOR)
> > > > +    return 0;
> > > > +#else
> > > >      Frame* frame = renderer->document()->frame();
> > > >      if (!frame)
> > > >          return 0;
> > > > @@ -1026,6 +1029,7 @@ static InspectorTimelineAgent* inspectorTimelineAgent(RenderObject* renderer)
> > > >      if (!page)
> > > >          return 0;
> > > >      return page->inspectorTimelineAgent();
> > > > +#endif
> > > >  }
> > > 
> > > 
> > > You should just surround this entire method with if ENABLE(INSPECTOR). Please
> > > fix this before landing.
> > 
> > Since I cannot land myself anyway, if I just surround with if ENABLE(INSPECTOR)
> > it would give a compiler warning/error saying the function should return a
> > value.
> 
> I mean surround the function, not its body. It should never be referenced if
> there is no inspector.

That would require patching the header too, isn't that kind of ugly? :-)

[Pavel Feldman]

> That would require patching the header too, isn't that kind of ugly? :-)

There is no (should be no) header entry for it. It is just a static helper function defined for private use in this cpp.

[Ismail Donmez]

(In reply to comment #19)
> > That would require patching the header too, isn't that kind of ugly? :-)
> 
> There is no (should be no) header entry for it. It is just a static helper
> function defined for private use in this cpp.

I forgot the fact that the function was static, was lame of me. Let me upload a fixed patch.

[Ismail Donmez]

Created attachment 48797 [details]
Surround the functions with a ifdef block, this is simpler

[Pavel Feldman]

Comment on attachment 48797 [details]
Surround the functions with a ifdef block, this is simpler

What about

static InspectorTimelineAgent* inspectorTimelineAgent

It is still not hidden, while it was the only one I really cared about.

[Ismail Donmez]

Created attachment 48798 [details]
Simplify the patch

[WebKit Commit Bot]

Comment on attachment 48798 [details]
Simplify the patch

Clearing flags on attachment: 48798

Committed r54815: <http://trac.webkit.org/changeset/54815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.