Bug 64297 – [Qt][WK2] Add the Web Inspector to WebKit2

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Genisim]

Get things showing on Linux in QtWebKit2.

[Genisim]

Looks that MiniBrowser based on QtWebKit2 does not creates INspectorClientQt and InspectorServerQt objects. It maybe one of possible reasons for Web Inspector does not showing in QtWebKit2

[Genisim]

Created attachment 103018 [details]
Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser. 

This patch for WebKit2 Qt 4.7 version. Does not compiled with Qt 5.

[Benjamin Poulain]

Comment on attachment 103018 [details]
Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser. 

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

Nice idea, it would be nice to have the inspector back.
This should be implemented through private C++ apis though, not via the C APIs. Talk with Alexis to see what his plans are for private APIs.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:54
> +    QDesktopWebView(WKContextRef, WKPageGroupRef);
> +    WKPageRef pageRef() const;
> +

This should really not be public. Do not forget this Class is in the public API.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:85
> +    m_view->setWindowTitle(QString::fromAscii("Web Inspector - ")+QString::fromAscii(url.utf8().data()));

QString::fromAscii() is never a good idea in the library. The first should probably be tr().
The second is definitely wrong. There is constructor for that.
You need to add spaces on both side of the + sign.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:105
> +    return ("qrc:/webkit/inspector/inspector.html");

Not sure Whey there are parentesis here :)

[Benjamin Poulain]

> This patch for WebKit2 Qt 4.7 version. Does not compiled with Qt 5.

This should be done for Qt 5. There is not plan to support WebKit2 on Qt 4.

[Genisim]

Created attachment 103333 [details]
Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser. (works with Qt 4.7, does not support Qt 5)

Thanks, for fast response.

Patch updated according to reviewer's comments attached. 

Please review.

[Benjamin Poulain]

Alexis, this will require private APIs. Did you start working on those? I think we should we make a sprint to have them this week. Are you interested?

[Benjamin Poulain]

Comment on attachment 103333 [details]
Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser. (works with Qt 4.7, does not support Qt 5)

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

You should update to Qt 5 first, and look how to expose the inspector in the cleanest way possible.

> ChangeLog:3
> +        Add WebInspector to WebKit2 Qt4.7 (revision 91860) MiniBrowser

You should update the title. The task title is now "[Qt][WK2] Add the Web Inspector to WebKit2"

> Source/WebKit.pro:12
> -    lessThan(QT_MAJOR_VERSION, 5) {
> +    lessThan(QT_MAJOR_VERSION, 4) {

Just no :)
WebKit2 trunk does not even build with Qt 4 anymore.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:47
> -
> +    

Junk change

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:94
> +    m_inspectorView = 0;

This is defined only for platform Qt, this will not compile on the other ports.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:52
> +#include <WKView.h>
> +#include <QGraphicsView>
> +#include <QGraphicsScene>

You should use forward declaration.
Those are not the right types.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:35
> -    : q(q)
> +    : q(q ? q : new QDesktopWebView(contextRef, pageGroupRef))

This is a really bad idea. You create implicitely a view that nobody own.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:197
> +void QDesktopWebView::toggleWebInspector()
> +{
> +    WKPageGroupRef pageGroupRef = WKPageGetPageGroup(pageRef());
> +    WKPreferencesRef preferences = WKPageGroupGetPreferences(pageGroupRef);
> +    if (WKInspectorIsVisible(WKPageGetInspector(pageRef()))) { 
> +        WKPreferencesSetDeveloperExtrasEnabled(preferences, false);
> +        WKInspectorClose(WKPageGetInspector(pageRef()));
> +    } else {
> +        WKPreferencesSetDeveloperExtrasEnabled(preferences, true);
> +        WKInspectorShow(WKPageGetInspector(pageRef()));
> +    }
> +}

The interface should be in the view, but the implementation should not. This is typically code for QtWebPageProxy.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:51
> +    void toggleWebInspector();

We do not want to expose that in QDesktopWebView.
The API should also be property based so it is accessible easily in QML2.

[Genisim]

Follow team's recommendation, I move toggleWebInspector function

> +void QDesktopWebView::toggleWebInspector()
> +{
> +    WKPageGroupRef pageGroupRef = WKPageGetPageGroup(pageRef());
> +    WKPreferencesRef preferences = WKPageGroupGetPreferences(pageGroupRef);
> +    if (WKInspectorIsVisible(WKPageGetInspector(pageRef()))) { 
> +        WKPreferencesSetDeveloperExtrasEnabled(preferences, false);
> +        WKInspectorClose(WKPageGetInspector(pageRef()));
> +    } else {
> +        WKPreferencesSetDeveloperExtrasEnabled(preferences, true);
> +        WKInspectorShow(WKPageGetInspector(pageRef()));
> +    }
> +}

to application MiniBrowser / BrowserWindow.cpp

and got problem with "pageRef"

pageRef() is private member of qdesktopwebview class
and will be available by friend class qdesktopwebviewprivate
but in app (MiniBrowser) one using qdesktopwebview

Question: how one will get pageRef. (WKPageRef() does not work. Any suggestions
recommendations ?

[Genisim]

Benjamin,

"
> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:35
> -    : q(q)
> +    : q(q ? q : new QDesktopWebView(contextRef, pageGroupRef))

This is a really bad idea. You create implicitely a view that nobody own."

It is only way I make WebInspector working (showing). With other solution rendering happened on one view and showing other one (just white blank window)

Please suggest other solution. 

Remember - we need create relation between q (QDesktopWebView*) and inspected page and, I think, it must be just one QGraphicsWidget. 
This is what we have with proposed solution

thanks a lot for fast responses

P.S. I fixed other issues according your comments, except :

 1. described in this comment

 2. private pageRef(). Can't get pageRef from MiniBrowser app

Some apps / developers continue use Qt 4.7 and this is a main reason for 
probably two patches based on Qt 4.7 and Qt 5

[Genisim]

Benjamin,

one more thing related to your comment below:

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:35
> -    : q(q)
> +    : q(q ? q : new QDesktopWebView(contextRef, pageGroupRef))

This is a really bad idea. You create implicitely a view that nobody own.


In the method platformOpen created view added as an item to m_scene
Please review 

thanks

[Genisim]

Created attachment 103428 [details]
Patch to add Web Inspector to WebKit2

Patch is still for WebKit2 based on Qt 4. This patch is for developers who currently continues use Qt 4
I prepare patch for WebKit2 based on Qt 5

Benjamin, thanks for fast responses and good suggestions
Please check three previous comments related to this patch.

I fixed problem with pageRef (hope it will be accepted. All changes in MiniBrowser / BrowserView)

[Alexis Menard (darktears)]

(In reply to comment #11)
> Created an attachment (id=103428) [details]
> Patch to add Web Inspector to WebKit2
> 
> Patch is still for WebKit2 based on Qt 4. This patch is for developers who currently continues use Qt 4
> I prepare patch for WebKit2 based on Qt 5

But Qt4 + WebKit2 will never be a supported path anymore I believe so I'm not sure we want to integrate the changes the way they are. For me we should integrate the Qt5 version when you will upload it.

> 
> Benjamin, thanks for fast responses and good suggestions
> Please check three previous comments related to this patch.
> 
> I fixed problem with pageRef (hope it will be accepted. All changes in MiniBrowser / BrowserView)

[Benjamin Poulain]

> > Patch is still for WebKit2 based on Qt 4. This patch is for developers who currently continues use Qt 4
> > I prepare patch for WebKit2 based on Qt 5
> 
> But Qt4 + WebKit2 will never be a supported path anymore I believe so I'm not sure we want to integrate the changes the way they are. For me we should integrate the Qt5 version when you will upload it.

Exactly :)
Patches for old version of WebKit are of no interest for WebKit trunk.

You got the change backward with BrowserView::toggleWebInspector(). Each layer abstract the complexity of the one under it. You are not supposed to expose everything to the user code.

You can use the keyword "friend" to make the inspector and the QDesktopWebView work together.

[Genisim]

Benjamin,

fast comment regarding: "You can use the keyword "friend" to make the inspector and the QDesktopWebView work together."

I think it is bad idea - ask developers modify core sources to make new application "friend" for one of core classes. Extra API looks much better.

Probably must be opened bug for this issue ?

[Benjamin Poulain]

(In reply to comment #14)
> fast comment regarding: "You can use the keyword "friend" to make the inspector and the QDesktopWebView work together."
> 
> I think it is bad idea - ask developers modify core sources to make new application "friend" for one of core classes. Extra API looks much better.

I have no clue what you are talking about.
Both the inspector widget and the desktop web view are in WebKit.

[Genisim]

Where is inspector widget ? Are you talking about QGraphicsWidget ?

[Genisim]

question not related to the bug.

I'm installing Qt5 follow instructions from: 

   http://developer.qt.nokia.com/wiki/Building_Qt_5_from_Git

and got follow error:

From git://gitorious.org/webkit/qtwebkit
   883ca7e..66b76cd  qtwebkit-2.2 -> origin/qtwebkit-2.2
 * [new tag]         qtwebkit-2.2-week31 -> qtwebkit-2.2-week31
Fetching gerrit
From git://gitorious.org/webkit/qtwebkit
   883ca7e..66b76cd  qtwebkit-2.2 -> gerrit/qtwebkit-2.2
### [qtwebkit] git branch -D qt-modularization-base
error: branch 'qt-modularization-base' not found.
git failed 256:Bad file descriptor at ./qtrepotools/bin/qt5_tool line 143.

below part of install script

134 # -- Locate an utility (grep, scp, etc) in MSYS git. This is specifically
135 #    for the setup case in which only git.cmd and not the utilities are in
136 #    the path. We then look at the git.cmd and return ..\bin\<utility>.exe.
137 sub msysGitUtility
138 {
139 #   -- Look for 'git.cmd' and cd ..\bin
140     my ($git, $utility) = @_;
141     if ($git =~ /.cmd$/i) {
142         my $msysGitBinFolder = File::Spec->catfile(dirname(dirname($git)), 'bin');
143         return File::Spec->catfile($msysGitBinFolder, $utility . '.exe');
144     }
145     return $utility;
146 }


Any comments, suggestions

thanks

[Genisim]

Finally got qt5 build works. Below URL one will use to checkout and build qt5.

   http://developer.qt.nokia.com/wiki/Building_Qt_5_Documentation

[Genisim]

Benjamin,

qt5 installed, pure webkit2 build fail:

In file included from /scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:21:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:97: error: ISO C++ forbids declaration of ‘TextureMapperVideoLayer’ with no type
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:97: error: expected ‘;’ before ‘*’ token
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h: In constructor ‘WebCore::TextureMapperNode::ContentData::ContentData()’:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:103: error: class ‘WebCore::TextureMapperNode::ContentData’ does not have any field named ‘media’
In file included from /scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:23:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h: At global scope:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:84: error: ‘NativeLayer’ does not name a type
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp: In member function ‘void WebCore::TextureMapperNode::uploadTextureFromContent(WebCore::TextureMapper*, const WebCore::IntRect&, WebCore::GraphicsLayer*)’:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:431: error: ‘struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:436: error: ‘struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
In file included from /scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:27,
                 from /scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:21:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:97: error: ISO C++ forbids declaration of ‘TextureMapperVideoLayer’ with no type
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:97: error: expected ‘;’ before ‘*’ token
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h: In constructor ‘WebCore::TextureMapperNode::ContentData::ContentData()’:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:103: error: class ‘WebCore::TextureMapperNode::ContentData’ does not have any field named ‘media’
In file included from /scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:21:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h: At global scope:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:84: error: ‘NativeLayer’ does not name a type
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp: In member function ‘void WebCore::TextureMapperNode::syncCompositingStateSelf(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*)’:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:837: error: ‘struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:837: error: ‘const struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp: In member function ‘virtual void WebCore::GraphicsLayerTextureMapper::setContentsToMedia(WebCore::PlatformLayer*)’:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: ‘struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: expected type-specifier before ‘TextureMapperVideoLayer’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: expected ‘>’ before ‘TextureMapperVideoLayer’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: expected ‘(’ before ‘TextureMapperVideoLayer’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: ‘TextureMapperVideoLayer’ was not declared in this scope
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: expected primary-expression before ‘>’ token
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:339: error: expected ‘)’ before ‘;’ token
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:341: error: ‘struct WebCore::TextureMapperNode::ContentData’ has no member named ‘media’
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp: At global scope:
/scratchbox/users/genisim/home/genisim/swork/qt5/qtwebkit/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:370: error: ‘NativeLayer’ does not name a type
make[1]: *** [obj/debug/TextureMapperNode.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: *** [obj/debug/GraphicsLayerTextureMapper.o] Error 1
make: *** [sub-WebCore-make_default-ordered] Error 2


I'm using follow command line for webkit2 build:

 WEBKITOUTPUTDIR=`pwd`/qtbuild_x86 Tools/Scripts/build-webkit --qt --makeargs="-s -j20" --no-video --debug --qmakearg="CONFIG+=texmap"  --qmakearg="CONFIG+=webkit2" --qmakearg="DEFINES+=MOZ_PLATFORM_MAEMO=6" --qmakearg="DEFINES+=ENABLE_INSPECTOR=1"


Please comments, suggestions

thanks

[Benjamin Poulain]

Please use the mailing list webkit-qt. Bugzilla is not the best place to get help on building WebKit.

Try building with the default flags unless you are willing to fix the flags you use.

[Genisim]

qt5 is installed.
pure webkit2 from qt5 package build and runs

Patch for WebInspector for WebKit2 based on Qt4.7 ported for 
WebKit2 based on Qt 5

Major changes:
   QGraphicsWidget replaced by QSGPaintedItem
   QGraphicsView replaced by QSGView
   QGraphicsScene not using

And got follow problems:
   for WebInspector created new QSGView (instead of QGraphicsScene and 
   QGraphivsView, and tried add / append WebInspector QSGPaintedItem 
   (new qdesktopwebview) to this QSGView. No success

In previous Qt 4.7 version it was easy - using QGraphicsScene, QGraphicsView
add created WebInspectors QGraphicsItem on QGraphicsScene.

I tried (just for test) use WebInspector URL when created QSGView and
(was expected) QSGView works with .qml not .html pages, but WebInspector UI is 
.html page

I think we have two options:
  1. convert QSGPaintedItem to QGraphicsItem and use QGraphicsView and
     QGraphicsScene and .html WebInspector UI (use WebInspector patch 
     for Qt4.7)

  2. or start BIG port includes .html to .qml , and ... (not sure what else
     must be port)

I prefer #1. Please comments, suggestions. 
  Any solution to get from QSGPaitingItem QGraphicsItem ?

[Benjamin Poulain]

(In reply to comment #21)
> I think we have two options:
>   1. convert QSGPaintedItem to QGraphicsItem and use QGraphicsView and
>      QGraphicsScene and .html WebInspector UI (use WebInspector patch 
>      for Qt4.7)
> 
>   2. or start BIG port includes .html to .qml , and ... (not sure what else
>      must be port)
> 
> I prefer #1. Please comments, suggestions. 
>   Any solution to get from QSGPaitingItem QGraphicsItem ?

Using graphics view is most definitely not an option.

For now, I guess the web inspector should just spawn a new canvas as top level window and use the QDesktopWebView for rendering.

[Genisim]

Created attachment 104146 [details]
Patch to add Web Inspector to WebKit2

Patch to add Web Inspector feature to the WebKit2 based on the qt5. Updating MiniBrowser to support Web Inspector feature.

[Alexis Menard (darktears)]

Comment on attachment 104146 [details]
Patch to add Web Inspector to WebKit2

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

> Source/WebKit2/UIProcess/WebInspectorProxy.h:179
> +    QDesktopWebViewPrivate* m_inspectorView;

Not sure here if it makes sense to make it work with the touchview. Yes the inpesctor is not touch friendly but despite that it could be nice to have it.

[Alexis Menard (darktears)]

Comment on attachment 104146 [details]
Patch to add Web Inspector to WebKit2

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

> Source/WebKit2/UIProcess/WebInspectorProxy.h:179
> +    QDesktopWebViewPrivate* m_inspectorView;

Not sure here if it makes sense to make it work with the touchview. Yes the inpesctor is not touch friendly but despite that it could be nice to have it.

[Genisim]

Lets do step by step. 

First, review and if patch accepted, add patch to webkit2 (developers groups waiting this patch to continue work on qt webkit2 based products - for exm. simulators, testing and debugging Javascripts, ...).

Second, prepare and add patch with Web Inspector "attach" / "detach" features implementations.

Third, work on new suggestions / possible bugs

P.S. I think that still make sense keep both patches : for Qt 4.7 and for Qt 5
     Still exist or in the developing process products based on Qt 4.7.

[Genisim]

Dear Benjamin,

did you start review Qt5 based patch ?

Few groups of developers waiting this patch in upstream !

[Genisim]

Created attachment 104931 [details]
Updated Web Inspector patch for one of latest WebKit2 rev.

Hi,

Please review patch as soon as possible, don't wait until patch start be too old for new webkit2 revs !

thanks

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 12 files


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

[Genisim]

Created attachment 104938 [details]
Updated Web Inspector patch for one of latest WebKit2 rev. Fixed Alphabetical sorting problem

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 12 files


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

[Genisim]

Created attachment 104944 [details]
Updated Web Inspector patch for one of latest WebKit2 rev. Fixed Alphabetical sorting problem

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 12 files


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

[Genisim]

Created attachment 104947 [details]
Updated Web Inspector patch for one of latest WebKit2 rev. Fixed Alphabetical sorting problem

[Alexis Menard (darktears)]

Comment on attachment 104947 [details]
Updated Web Inspector patch for one of latest WebKit2 rev. Fixed Alphabetical sorting problem

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

> Source/WebKit2/ChangeLog:17
> +        (WebKit::WebInspectorProxy::didLoadInspectorPage): RequestAttachWindow() not exist. Commented.

Regenerate your changelog after removing commented code.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:223
> +    inspectorPage->loadURL(inspectorPageURL());

Why this change?

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:235
> +//        m_page->process()->send(Messages::WebInspector::RequestAttachWindow(), m_page->pageID());

You can't commit that.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:183
> +    QSGView* m_view;

I think you can use OwnPtr here.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:184
> +    QDesktopWebViewPrivate* m_inspectorView;

I don't understand why it needs to be a QDesktopWebViewPrivate unless I miss something.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:38
> +class QObject;

You don't forward declare class in a cpp object.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:58
> +    m_view = new QSGView();

This seems to leak.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:68
> +        delete m_inspectorView->q;

What is that? This is very wrong programming methods.

> Tools/MiniBrowser/qt/BrowserView.cpp:35
> +#include <qdesktopwebview_p.h>

This is not acceptable. You can't use private API.

> Tools/MiniBrowser/qt/BrowserView.cpp:100
> +        QDesktopWebViewPrivate* desktopWebViewPrivate = new QDesktopWebViewPrivate(desktopWebView()); 

This is wrong. You can't create private object like this.

> Tools/MiniBrowser/qt/BrowserView.cpp:112
> +        delete desktopWebViewPrivate;

This is very wrong to me. You shouldn't do that at all. Have you look how the API looks like in WebKit1, we should aim for something easy, this is WAY too complicated to use and requires private API and some hacks.

> Tools/MiniBrowser/qt/MiniBrowser.pro:23
> +include(../../../Source/WebKit2/WebKit2.pri)

Why this?

[Alexis Menard (darktears)]

Comment on attachment 104947 [details]
Updated Web Inspector patch for one of latest WebKit2 rev. Fixed Alphabetical sorting problem

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:41
> +    : q(q ? q : new QDesktopWebView(contextRef, pageGroupRef))

Very wrong change. This case should never happen, Qt coding practices to create private object should be respected, you create the private object from the public and it's linked to it, you can't create private object itself alone.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:46
> +    m_inspectorView = new QDesktopWebViewPrivate(0, toAPI(page()->process()->context()), toAPI(inspectorPageGroup()));

See comment below, you can't create the private without its public object.

[Genisim]

Created attachment 104975 [details]
Updated Web Inspector diff

Alexis,

first of all thanks for fast and detail review.

Attached updated patch for Web Inspector for Qt5 MiniBrowser.

All updated according to your comments except follow:

"
  > Source/WebKit2/UIProcess/WebInspectorProxy.h:183
  > +    QSGView* m_view;

  I think you can use OwnPtr here.
"

And yes, you miss one point - Proposed (reviewed patch) solve problem with MiniBrowser -> BrowserView.cpp ->

+void BrowserView::toggleWebInspector()
+{
+    if (desktopWebView()) {
+        WKPageRef m_pageRef = desktopWebView()->pageRef();
+        WKPageGroupRef m_pageGroupRef = WKPageGetPageGroup(m_pageRef);
+        WKPreferencesRef m_preferences = WKPageGroupGetPreferences(m_pageGroupRef);
+
+        if (WKInspectorIsVisible(WKPageGetInspector(m_pageRef))) {
+            WKPreferencesSetDeveloperExtrasEnabled(m_preferences, false);
+            WKInspectorClose(WKPageGetInspector(m_pageRef));
+        } else {
+            WKPreferencesSetDeveloperExtrasEnabled(m_preferences, true);
+            WKInspectorShow(WKPageGetInspector(m_pageRef));
+        }
+    }
+}

One need to use "WKPageRef m_pageRef" for WK... functions and
"pageRef" is a private method of DesktopWebView class.

For WebInspectorProxy similar problem was fixed by "friend"

But one can't use "friend" for all external classes like MiniBrowser, BrowserView class

In one of previous proposed patches all WK... functions was moved to qdesktopwebview but

this patch was declined by Benjamin (I agree with him). 

Now I updated all according your comments and back to "pageRef" issue.

Any solution, suggestion for this problem ???

thanks

[Benjamin Poulain]

Please read all the comments that have already been made when you update.
I feel every time I read an update of this patch, I see the some basic problems that were commented on before.

Please, first, define what a public API that is nice and easy to use, and solve all the layering problems that have already been discussed.
Once you have that, you can build what is needed in WebKit to expose that API.

As mentioned before, this should probably be exposed through private API. So please, go on IRC, discuss with Alexis the best way to expose those private API, then implement it.


The get you started on the API:
The full code on the MiniBrowser should probably be something like this:
::enableWebInspector(bool toggled) 
{
    if (desktopWebView())
        desktopWebView()->experimentalFeatures()->setWebInspectorEnabled(toggled);
    else
        touchWebView()->experimentatlFeatures()->setWebInspectorEnabled(toggled);
}
This should enable the inspector and the menu action "inspect" should work as expected.

You can add another action showWebInspector() if needed (for the touch view essentially because the context menu support is not upstreamed yet.
::showWebInspector()
{
    if (desktopWebView())
        desktopWebView()->experimentalFeatures()->showWebInspector();
    else
        touchWebView()->experimentatlFeatures()->showWebInspector();
}

[Genisim]

Thanks for comment.

I'm waiting more then 2 weeks these API. Start work around to get working
Web Inspector and unblock developers who needs working Web Inspector for Qt

I hope 
           void BrowserView::toggleWebInspector()

shows what exactly one expects from these API

[Alexis Menard (darktears)]

(In reply to comment #39)
> Thanks for comment.
> 
> I'm waiting more then 2 weeks these API. Start work around to get working
> Web Inspector and unblock developers who needs working Web Inspector for Qt
> 
> I hope 
>            void BrowserView::toggleWebInspector()
> 
> shows what exactly one expects from these API

I think you really need to come to IRC to have a chat with us. You can reach us on freenode #qtwebkit and my nick is darktears.


"I'm waiting more then 2 weeks these API."

-> well I don't know what you mean but in WebKit2 and its Qt port is a moving target, making the WebInspector working is not our first priority when for example cookies are not working, downloads are not working, comboboxes are not working. The fact that you come and try to help on it is very welcome for sure.

In the other hand we can't accept workarounds in WebKit trunk, the code quality is meant to be the best possible. The patches you proposed are fine to keep them locally just to get the inspector working BUT the way they are today will be a no go for us. The trunk accepts clean stuff.

The last attachment you posted 104975 is a work in progress, I'm not even sure that thing compile at all.

I think you need to calm down, sit down, look at the design and rethink the entire solution. The fact that lot of people need the inspector (which we all agree is nice to have) doesn't mean you can rush the feature. They can live with a workaround in the meantime.

Now show up on IRC that will help.

[Genisim]

Hi,

I'm on IRC. #qtwebkit, nickname genisim

Yes, it is work in progress

All is compiled, no problem, except MiniBrowser, BrowserView.cpp

genisim/home/genisim/swork/webkit_upstream/webkit_trunk/trunk_qt5/Tools/MiniBrowser/qt/BrowserView.cpp
/scratchbox/users/genisim/home/genisim/swork/webkit_upstream/webkit_trunk/trunk_qt5/Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h: In member function ‘void BrowserView::toggleWebInspector()’:
/scratchbox/users/genisim/home/genisim/swork/webkit_upstream/webkit_trunk/trunk_qt5/Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:117: error: ‘const OpaqueWKPage* QDesktopWebView::pageRef() const’ is private
/scratchbox/users/genisim/home/genisim/swork/webkit_upstream/webkit_trunk/trunk_qt5/Tools/MiniBrowser/qt/BrowserView.cpp:99: error: within this context

As you can see the problem is in API. Current API does not provides access to 
pageRef

We can start discussion directly on IRC

thanks

[Genisim]

Created attachment 105103 [details]
Updated Web Inspector patch for  Qt5 WebKit2. Patch modified according Alexis comments

Using instead of WK... APIs in MiniBrowser toggleInspector API of qdesktopwebview and qtouchwebview
Probably toggleInspector method can be the same for qdesktopwebview and for qtouchwebview.
In current patch toggleInspector of qtouchwebview is empty.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[Genisim]

Created attachment 105107 [details]
Updated Web Inspector patch for Qt5 WebKit2. Patch modified according Alexis comments. Fixed Alphabetical sorting problem

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[Genisim]

Created attachment 105110 [details]
Updated Web Inspector patch for Qt5 WebKit2. Patch modified according Alexis comments. Fixed Alphabetical sorting problem

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 13 files


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

[Noam Rosenthal]

Comment on attachment 105110 [details]
Updated Web Inspector patch for Qt5 WebKit2. Patch modified according Alexis comments. Fixed Alphabetical sorting problem

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

In general it's going in a good direction. See comments.

>> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:42
>> +#include "WebInspectorProxy.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Please fix.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:319
> +    if (toImpl(m_inspector)->isVisible()) {
> +        toImpl(m_preferences)->setDeveloperExtrasEnabled(false);
> +        toImpl(m_inspector)->close();
> +    } else {
> +        toImpl(m_preferences)->setDeveloperExtrasEnabled(true);
> +        toImpl(m_inspector)->show();
> +    }

Wouldn't work. You need to separate the developer-extras setting from showing the inspector; otherwise inspector would only collect information from the page while it's shown.

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:116
> +void QTouchWebView::toggleWebInspector()
> +{
> +}

Maybe add notImplemented() or a FIXME comment.

[Genisim]

Noam,

thanks for comments. Two fixes are easy:

  alphabetical and add notImplemented

fix #3 - split toggleWebInspector on two :

   - enable / disable DeveloperExtras 
   - toggleWebInspector

is not simple as it looks.

 1. At least two features (Web Inspector and WebGL) depends from DeveloperExtras preference. And only for this reason toggleWebInspector must be split.

 2. If user disables DeveloperExtras, and dependent features (WebInspector for exm) are on, we have three choices:
    - note user - "Can't disable DeveloperExtras" because of ...
    - keep DeveloperExtras enable without notification
    - turn of features dependent from DeveloperExtras and after disable DeveloperExtras

 3. If DeveloperExtras features menu is visible (independently on DeveloperExtras preference), QDesktopWebView::toggleWebInspector() before
call inspector functions (isVisible, close, show) must check DeveloperExtras status.
    Otherwise if DeveloperExtras features menu start be visible only after DeveloperExtras enable, we can skip DeveloperExtras status check

 4. QDesktopWebView::enableDeveloperExtras(bool enable) must include additional tests (read point #2) if we decide check and turn off features dependent on DeveloperExtras preference

So, I propose for now split QDesktopWebView::toggleWebInspector on two
 - enableDeveloperExtras(bool)
 - toggleWebInspector
no extra tests. I think we can commit this one and after continue to work on patches related to point #2, #3, #4 and new APIs like toggleWebGL

thanks

[Genisim]

So far seems to me that 
      void enableDeveloperExtras(bool enable);
      void toggleWebInspector();
and probably in near future
      void toggleWebGL()
maybe more must be common for QDesktopWebView and QTouchWebView ?

[Genisim]

Created attachment 105255 [details]
Updated Web Inspector patch for Qt5 WebKit2. Patch modified according latest comments.

[Genisim]

Created attachment 105540 [details]
Patch to add Web Inspector to WebKit2. Implemented 2 methods for qtouchwebview. 

Patch supports both views qdesktopwebview and qtouchwebview

[Genisim]

Created attachment 105562 [details]
Patch to add Web Inspector to WebKit2. Second variant - enableDeveloperExtras and toggleWebInspector implementation moves to the QtWebPageProxy

Hi Alexis,

hope all your requirements are done. You can choose between Patch 105540, and this last one.

Please review and let me know if you have extra requirements.


P.S. unfortunately I don't have device (tablet or laptop) with touch screen. Can't test touchscreen version
     But hope all works well, from both views (desktop and touch) one calls same function in QtWebPageProxy.

[Noam Rosenthal]

Comment on attachment 105255 [details]
Updated Web Inspector patch for Qt5 WebKit2. Patch modified according latest comments.

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

> Source/WebKit2/ChangeLog:5
> +        Add WebInspector to WebKit2 MiniBrowser
> +        https://bugs.webkit.org/show_bug.cgi?id=64297
> +

Insufficient changelog.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:79
> +#if PLATFORM(QT)
> +    , m_view(0)
> +    , m_inspectorView(0)
> +#endif

OwnPtr would make this unnecessary.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:185
> +#elif PLATFORM(QT)
> +    QSGView* m_view;
> +    QDesktopWebView* m_inspectorView;

OwnPtr

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:309
> +    WKPageGroupRef m_pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> +    WKPreferencesRef m_preferences = toAPI(toImpl(m_pageGroupRef)->preferences());

What does the m_ stand for? Those are not members.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:45
> +    ASSERT(m_inspectorView);

This assert doesn't add anything.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:57
> +    ASSERT(m_view);

ditto.

> Tools/ChangeLog:5
> +        Add WebInspector to WebKit2 MiniBrowser
> +        https://bugs.webkit.org/show_bug.cgi?id=64297
> +

Insufficient changelog.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:119
> +    toggleWebInspector->connect(this, SIGNAL(enteredDeveloperExtrasMode(bool)), SLOT(setEnabled(bool)));
> +    toggleWebInspector->connect(this, SIGNAL(enteredWebInspectorMode(bool)), SLOT(setChecked(bool)));
> +    enableDeveloperExtras->connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(setEnabledInvert(bool)));
> +    connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(toggleWebInspectorMode(bool)));

Hard to understand from this code how this UI is going to behave. There has to be a cleaner way of doing this.
I'd be happy to help (on IRC / in person).

> Tools/MiniBrowser/qt/BrowserWindow.cpp:172
> +void BrowserWindow::setEnabledInvert(bool enable)
> +{
> +    enableDeveloperExtras->setEnabled(!enable);
> +}
> +

What does setEnabledInvert mean? It seems like logic that can be done in a cleaner way with better understanding of Qt desktop.

[Noam Rosenthal]

Comment on attachment 105540 [details]
Patch to add Web Inspector to WebKit2. Implemented 2 methods for qtouchwebview. 

This seems like a duplicate of the previous patch. Please obsolete old patches when uploading new ones.

[Noam Rosenthal]

Comment on attachment 105562 [details]
Patch to add Web Inspector to WebKit2. Second variant - enableDeveloperExtras and toggleWebInspector implementation moves to the QtWebPageProxy

See previous comments, not sure how much was fixed. The "obsolete previous patches" button is your friend.

[Genisim]

(In reply to comment #54)
> (From update of attachment 105255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105255&action=review
> 
> > Source/WebKit2/ChangeLog:5
> > +        Add WebInspector to WebKit2 MiniBrowser
> > +        https://bugs.webkit.org/show_bug.cgi?id=64297
> > +
> 
> Insufficient changelog.
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:79
> > +#if PLATFORM(QT)
> > +    , m_view(0)
> > +    , m_inspectorView(0)
> > +#endif
> 
> OwnPtr would make this unnecessary.
Maybe. Can you please check code with m_view and m_inspector and give example how replace both of them with OwnPtr ?
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.h:185
> > +#elif PLATFORM(QT)
> > +    QSGView* m_view;
> > +    QDesktopWebView* m_inspectorView;
> 
> OwnPtr
Check please previous comment
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:309
> > +    WKPageGroupRef m_pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> > +    WKPreferencesRef m_preferences = toAPI(toImpl(m_pageGroupRef)->preferences());
> 
> What does the m_ stand for? Those are not members.
No problem I can switch to local vars(no m_)
> 
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:45
> > +    ASSERT(m_inspectorView);
> 
> This assert doesn't add anything.
assert checks if m_inspector got value
> 
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:57
> > +    ASSERT(m_view);
> 
> ditto.
checks m_view got value or not
Please check Apple variant. I guess it will be necessary
> 
> > Tools/ChangeLog:5
> > +        Add WebInspector to WebKit2 MiniBrowser
> > +        https://bugs.webkit.org/show_bug.cgi?id=64297
> > +
> 
> Insufficient changelog.
> 
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:119
> > +    toggleWebInspector->connect(this, SIGNAL(enteredDeveloperExtrasMode(bool)), SLOT(setEnabled(bool)));
> > +    toggleWebInspector->connect(this, SIGNAL(enteredWebInspectorMode(bool)), SLOT(setChecked(bool)));
> > +    enableDeveloperExtras->connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(setEnabledInvert(bool)));
> > +    connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(toggleWebInspectorMode(bool)));
> 
> Hard to understand from this code how this UI is going to behave. There has to be a cleaner way of doing this.
> I'd be happy to help (on IRC / in person).
> 
Please read one of previous comments related to combination of 
"enable developers extras" and "show web inspector" features

With proposed solution are possible follow combinations:

1.  - "enable developers extras" available (not selected)
    - "show web inspector" unavailable (not selected)

2.  - "enable developers extras" available (selected)
    - "show web inspector" available (not selected)

3.  - "enable developers extras" unavailable (selected)
    - "show web inspector" available (selected)

The idea is protect MiniBrowser against unexpected operations, like disable developers extras, when web inspector is running

> > Tools/MiniBrowser/qt/BrowserWindow.cpp:172
> > +void BrowserWindow::setEnabledInvert(bool enable)
> > +{
> > +    enableDeveloperExtras->setEnabled(!enable);
> > +}
> > +
> 
> What does setEnabledInvert mean? It seems like logic that can be done in a cleaner way with better understanding of Qt desktop.

Read previous comments. One need convert selected "enable developers extras" to the unavailable mode when it is selected and web inspector is running
If is not clear please let me know and I'll submit snapshots from running Web Inspector

[Genisim]

(In reply to comment #55)
> (From update of attachment 105540 [details])
> This seems like a duplicate of the previous patch. Please obsolete old patches when uploading new ones.

Please read patch description:

Patch to add Web Inspector to WebKit2. Implemented 2 methods for qtouchwebview.

Is not detail description, but - "Implemented 2 methods for qtouchwebview"

Was implemented enableDeveloperExtras and toggleWebInspector for qtouchwebview

[Genisim]

(In reply to comment #56)
> (From update of attachment 105562 [details])
> See previous comments, not sure how much was fixed. The "obsolete previous patches" button is your friend.

Please read patch description:
Patch to add Web Inspector to WebKit2. 

Second variant - enableDeveloperExtras and toggleWebInspector implementation moves to the QtWebPageProxy

Methods enableDeveloperExtras and toggleWebInspector implementation moves from qdesktopwebview and qtouchwebview to the common for both views QtWebPageProxy

[Genisim]

Noam,

Please review my comments.

I'll replace vars (remove m_ prefix)

Please explain why is so principal to use OwnPtr, what benefits one will get from this change

Please review again submitted patch and try send me requests to ,hope, all unacceptable elements in patch

thanks a lot

[Alexis Menard (darktears)]

(In reply to comment #60)
> Noam,
> 
> Please review my comments.
> 
> I'll replace vars (remove m_ prefix)
> 
> Please explain why is so principal to use OwnPtr, what benefits one will get from this change

OwnPtr will delete the object it holds for you whenever the pointer dies. It's a nice practice to use it, just as the WebKit code base is doing.

> 
> Please review again submitted patch and try send me requests to ,hope, all unacceptable elements in patch
> 
> thanks a lot

[Genisim]

(In reply to comment #61)
> (In reply to comment #60)
> > Noam,
> > 
> > Please review my comments.
> > 
> > I'll replace vars (remove m_ prefix)
> > 
> > Please explain why is so principal to use OwnPtr, what benefits one will get from this change
> 
> OwnPtr will delete the object it holds for you whenever the pointer dies. It's a nice practice to use it, just as the WebKit code base is doing.
> 
> > 
> > Please review again submitted patch and try send me requests to ,hope, all unacceptable elements in patch
> > 
> > thanks a lot

Hi Alexis,

using smart pointer OwnPtr is a good practice. Agreed. I'm not familiar good enough with OwnPtr. Can you please provide simple example for OwnPtr usage.

According to WebInspector code I'll set follow requirements for this simple example:
  1. OwnPtr must be initialize by NULL (no value) in WebInspectorProxy constructor
  2. One will assign value at WebInspector creating time
  3. OwnPtr will be released at WebInspector close time  

thanks

[Alexis Menard (darktears)]

(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > Noam,
> > > 
> > > Please review my comments.
> > > 
> > > I'll replace vars (remove m_ prefix)
> > > 
> > > Please explain why is so principal to use OwnPtr, what benefits one will get from this change
> > 
> > OwnPtr will delete the object it holds for you whenever the pointer dies. It's a nice practice to use it, just as the WebKit code base is doing.
> > 
> > > 
> > > Please review again submitted patch and try send me requests to ,hope, all unacceptable elements in patch
> > > 
> > > thanks a lot
> 
> Hi Alexis,
> 
> using smart pointer OwnPtr is a good practice. Agreed. I'm not familiar good enough with OwnPtr. Can you please provide simple example for OwnPtr usage.
> 
> According to WebInspector code I'll set follow requirements for this simple example:
>   1. OwnPtr must be initialize by NULL (no value) in WebInspectorProxy constructor
>   2. One will assign value at WebInspector creating time
>   3. OwnPtr will be released at WebInspector close time  
> 
> thanks

Please look at existing code, a simple search in WebKit code and you have gazilions of examples.

http://www.webkit.org/coding/RefPtr.html is also a good starting point.

You can take initiatives :D

[Genisim]

Created attachment 105709 [details]
Patch to add Web Inspector to WebKit2 updated. using smart pointers OwnPtr, removed m_ prefix for local variable, added more info to ChangeLog's

Hi Alexis, Benjamin, Noam

hope this patch response to your requirements.

Please review, thanks

[Andreas Kling]

Comment on attachment 105709 [details]
Patch to add Web Inspector to WebKit2 updated. using smart pointers OwnPtr, removed m_ prefix for local variable, added more info to ChangeLog's

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> +    void enableDeveloperExtras(bool enable);
> +    void toggleWebInspector(bool enable);

Inconsistent API. Let's call both of them either "enableBlahBlah" or "toggleBlahBlah".
Or a more "WebKitty" option would be setBlahBlahEnabled(bool);

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:744
> +    WKPageGroupRef pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> +    WKPreferencesRef preferences = toAPI(toImpl(pageGroupRef)->preferences());
> +    toImpl(preferences)->setDeveloperExtrasEnabled(enable);

Pointless toAPI/toImpl churn.

m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:753
> +    WKInspectorRef inspector = toAPI(toImpl(pageRef())->inspector());
> +    if (enable)
> +        toImpl(inspector)->show();
> +    else
> +        toImpl(inspector)->close();

Pointless toAPI/toImpl churn.

if (enable)
    m_webPageProxy->inspector()->show();
else
    m_webPageProxy->inspector()->close();

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:66
> +    if (m_inspectorView)
> +        m_inspectorView.clear();

No need for the null check here, clear() is always safe.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:81
> +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));

Why on earth do we need to translate the URL?
On a related note, I don't think it makes sense to translate the " - " part of the string.
Either "Web Inspector" or "Web Inspector - %1" (so the translator is free to localize the string any way she/he wants.)

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:107
> +    return ("qrc:/webkit/inspector/inspector.html");

No need for () around the string.

> Tools/ChangeLog:18
> +        * MiniBrowser/qt/BrowserView.cpp:

A newline before this would be nice on the eyes.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:107
> +    enableDeveloperExtras = toolsMenu->addAction("Enable Developer Extras", this, SIGNAL(enteredDeveloperExtrasMode(bool)));

The QObject/member pair passed to addAction() is typically supposed to be a receiver/slot, rather than a sender/signal.
The fact that it isn't makes the following code particularly hard to follow.

It would be much more readable if the addAction() call would connect to a SLOT(onDeveloperExtrasModeChanged(bool)) (or some similar name.)
Same thing for the web inspector action.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:171
> +void BrowserWindow::setEnabledInvert(bool enable)
> +{
> +    enableDeveloperExtras->setEnabled(!enable);
> +}

This function needs a new name, badly.
To clarify, it is not obvious what a function called "setEnabledInvert" will do if I call it.
In fact, it would be much nicer to fold this logic into the onWebInspectorModeChanged() slot suggested above.

> Tools/MiniBrowser/qt/BrowserWindow.h:87
> +    QAction* enableDeveloperExtras;

Style, missing m_ prefix on member variable.

[Genisim]

(In reply to comment #65)
> (From update of attachment 105709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105709&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> > +    void enableDeveloperExtras(bool enable);
> > +    void toggleWebInspector(bool enable);
> 
> Inconsistent API. Let's call both of them either "enableBlahBlah" or "toggleBlahBlah".
> Or a more "WebKitty" option would be setBlahBlahEnabled(bool);
> 
DONE
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:744
> > +    WKPageGroupRef pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> > +    WKPreferencesRef preferences = toAPI(toImpl(pageGroupRef)->preferences());
> > +    toImpl(preferences)->setDeveloperExtrasEnabled(enable);
> 
> Pointless toAPI/toImpl churn.
> 
> m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);
> 
DONE
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:753
> > +    WKInspectorRef inspector = toAPI(toImpl(pageRef())->inspector());
> > +    if (enable)
> > +        toImpl(inspector)->show();
> > +    else
> > +        toImpl(inspector)->close();
> 
> Pointless toAPI/toImpl churn.
> 
> if (enable)
>     m_webPageProxy->inspector()->show();
> else
>     m_webPageProxy->inspector()->close();
> 
DONE
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:66
> > +    if (m_inspectorView)
> > +        m_inspectorView.clear();
> 
> No need for the null check here, clear() is always safe.
> 
DONE
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:81
> > +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));
> 
> Why on earth do we need to translate the URL?
> On a related note, I don't think it makes sense to translate the " - " part of the string.
> Either "Web Inspector" or "Web Inspector - %1" (so the translator is free to localize the string any way she/he wants.)
> 
NO CHANGES
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:107
> > +    return ("qrc:/webkit/inspector/inspector.html");
> 
> No need for () around the string.
> 
DONE
> > Tools/ChangeLog:18
> > +        * MiniBrowser/qt/BrowserView.cpp:
> 
> A newline before this would be nice on the eyes.
> 
DONE
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:107
> > +    enableDeveloperExtras = toolsMenu->addAction("Enable Developer Extras", this, SIGNAL(enteredDeveloperExtrasMode(bool)));
> 
> The QObject/member pair passed to addAction() is typically supposed to be a receiver/slot, rather than a sender/signal.
> The fact that it isn't makes the following code particularly hard to follow.
> 
> It would be much more readable if the addAction() call would connect to a SLOT(onDeveloperExtrasModeChanged(bool)) (or some similar name.)
> Same thing for the web inspector action.
> 
DONE. If you look BrowserWindow.cpp and find same issues with:

  QAction* toggleFullScreen = windowMenu->addAction("Toggle FullScreen", this, SIGNAL(enteredFullScreenMode(bool)));

JUST REMEMBER it is not Genisim's code.

> > Tools/MiniBrowser/qt/BrowserWindow.cpp:171
> > +void BrowserWindow::setEnabledInvert(bool enable)
> > +{
> > +    enableDeveloperExtras->setEnabled(!enable);
> > +}
> 
> This function needs a new name, badly.
> To clarify, it is not obvious what a function called "setEnabledInvert" will do if I call it.
> In fact, it would be much nicer to fold this logic into the onWebInspectorModeChanged() slot suggested above.
>
DONE partially (name changed). Logic did not moved to the onWebInspector... 
> > Tools/MiniBrowser/qt/BrowserWindow.h:87
> > +    QAction* enableDeveloperExtras;
> 
> Style, missing m_ prefix on member variable.

DONE

[Genisim]

Created attachment 105726 [details]
Patch to add Web Inspector to WebKit2 updated.

Thanks for comments.

Please review attached patch. Hope you will find a progress.

Waiting your comments ...

[Alexis Menard (darktears)]

Comment on attachment 105726 [details]
Patch to add Web Inspector to WebKit2 updated.

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

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:80
> +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));

Problem here as we said earlier, you don't translate URLs.

[Gustavo Noronha (kov)]

Comment on attachment 105726 [details]
Patch to add Web Inspector to WebKit2 updated.

Attachment 105726 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9558943

[Genisim]

(In reply to comment #68)
> (From update of attachment 105726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105726&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:80
> > +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));
> 
> Problem here as we said earlier, you don't translate URLs.

Can you, please, explain how translate URL. 

I guess you check my comments.
This methods updates Web Inspector title. And if you compare results one can see
in Web Inspector safari and Web Inspector qt, results are the same.

[Alexis Menard (darktears)]

(In reply to comment #70)
> (In reply to comment #68)
> > (From update of attachment 105726 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=105726&action=review
> > 
> > > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:80
> > > +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));
> > 
> > Problem here as we said earlier, you don't translate URLs.
> 
> Can you, please, explain how translate URL. 

QObject::tr will trigger the translation system, you don't need to do that as there is no need to translate an URL, just like Andreas said. It didn't bug/warn/surprised you when you typed on the keyboard "Can you, please, explain how translate URL."? -> this sentence is totally odd.

No translation of URLs, or should I say : "please remove the QObject::tr() call of QObject::tr(url.utf8().data())) and replace it by QObject::tr(Web Inspector - %1).arg(url.utf8().data()) as Andreas suggested".

Seriously...

> 
> I guess you check my comments.
> This methods updates Web Inspector title. And if you compare results one can see
> in Web Inspector safari and Web Inspector qt, results are the same.

[Alexis Menard (darktears)]

(In reply to comment #66)
> (In reply to comment #65)
> > (From update of attachment 105709 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=105709&action=review
> > 
> > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> > > +    void enableDeveloperExtras(bool enable);
> > > +    void toggleWebInspector(bool enable);
> > 
> > Inconsistent API. Let's call both of them either "enableBlahBlah" or "toggleBlahBlah".
> > Or a more "WebKitty" option would be setBlahBlahEnabled(bool);
> > 
> DONE
> > > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:744
> > > +    WKPageGroupRef pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> > > +    WKPreferencesRef preferences = toAPI(toImpl(pageGroupRef)->preferences());
> > > +    toImpl(preferences)->setDeveloperExtrasEnabled(enable);
> > 
> > Pointless toAPI/toImpl churn.
> > 
> > m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);
> > 
> DONE
> > > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:753
> > > +    WKInspectorRef inspector = toAPI(toImpl(pageRef())->inspector());
> > > +    if (enable)
> > > +        toImpl(inspector)->show();
> > > +    else
> > > +        toImpl(inspector)->close();
> > 
> > Pointless toAPI/toImpl churn.
> > 
> > if (enable)
> >     m_webPageProxy->inspector()->show();
> > else
> >     m_webPageProxy->inspector()->close();
> > 
> DONE
> > > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:66
> > > +    if (m_inspectorView)
> > > +        m_inspectorView.clear();
> > 
> > No need for the null check here, clear() is always safe.
> > 
> DONE
> > > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:81
> > > +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));
> > 
> > Why on earth do we need to translate the URL?
> > On a related note, I don't think it makes sense to translate the " - " part of the string.
> > Either "Web Inspector" or "Web Inspector - %1" (so the translator is free to localize the string any way she/he wants.)
> > 
> NO CHANGES
> > > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:107
> > > +    return ("qrc:/webkit/inspector/inspector.html");
> > 
> > No need for () around the string.
> > 
> DONE
> > > Tools/ChangeLog:18
> > > +        * MiniBrowser/qt/BrowserView.cpp:
> > 
> > A newline before this would be nice on the eyes.
> > 
> DONE
> > > Tools/MiniBrowser/qt/BrowserWindow.cpp:107
> > > +    enableDeveloperExtras = toolsMenu->addAction("Enable Developer Extras", this, SIGNAL(enteredDeveloperExtrasMode(bool)));
> > 
> > The QObject/member pair passed to addAction() is typically supposed to be a receiver/slot, rather than a sender/signal.
> > The fact that it isn't makes the following code particularly hard to follow.
> > 
> > It would be much more readable if the addAction() call would connect to a SLOT(onDeveloperExtrasModeChanged(bool)) (or some similar name.)
> > Same thing for the web inspector action.
> > 
> DONE. If you look BrowserWindow.cpp and find same issues with:
> 
>   QAction* toggleFullScreen = windowMenu->addAction("Toggle FullScreen", this, SIGNAL(enteredFullScreenMode(bool)));
> 
> JUST REMEMBER it is not Genisim's code.

Code is not perfect otherwise we could call WebKit done. Things can be improved, let's make the new code the best we can.

> 
> > > Tools/MiniBrowser/qt/BrowserWindow.cpp:171
> > > +void BrowserWindow::setEnabledInvert(bool enable)
> > > +{
> > > +    enableDeveloperExtras->setEnabled(!enable);
> > > +}
> > 
> > This function needs a new name, badly.
> > To clarify, it is not obvious what a function called "setEnabledInvert" will do if I call it.
> > In fact, it would be much nicer to fold this logic into the onWebInspectorModeChanged() slot suggested above.
> >
> DONE partially (name changed). Logic did not moved to the onWebInspector... 
> > > Tools/MiniBrowser/qt/BrowserWindow.h:87
> > > +    QAction* enableDeveloperExtras;
> > 
> > Style, missing m_ prefix on member variable.
> 
> DONE

[Alexis Menard (darktears)]

(In reply to comment #71)
> (In reply to comment #70)
> > (In reply to comment #68)
> > > (From update of attachment 105726 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=105726&action=review
> > > 
> > > > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:80
> > > > +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") + QObject::tr(url.utf8().data()));
> > > 
> > > Problem here as we said earlier, you don't translate URLs.
> > 
> > Can you, please, explain how translate URL. 
> 
> QObject::tr will trigger the translation system, you don't need to do that as there is no need to translate an URL, just like Andreas said. It didn't bug/warn/surprised you when you typed on the keyboard "Can you, please, explain how translate URL."? -> this sentence is totally odd.
> 
> No translation of URLs, or should I say : "please remove the QObject::tr() call of QObject::tr(url.utf8().data())) and replace it by QObject::tr(Web Inspector - %1).arg(url.utf8().data()) as Andreas suggested".

And btw my head is not a compiler, check that it compiles + there is no better way than url.utf8().data(). 

> 
> Seriously...
> 
> > 
> > I guess you check my comments.
> > This methods updates Web Inspector title. And if you compare results one can see
> > in Web Inspector safari and Web Inspector qt, results are the same.

So url is the string "Qt" I'm confused.

[Alexis Menard (darktears)]

Comment on attachment 105726 [details]
Patch to add Web Inspector to WebKit2 updated.

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

> Source/WebKit2/UIProcess/WebInspectorProxy.h:33
> +#include <qdesktopwebview.h>

This is wrong it will not build on other ports. Protect it just like below.

-> translated to "#if PLATFORM(QT) ... #endif"

[Genisim]

Created attachment 105798 [details]
Patch to add Web Inspector to WebKit2 updated.

[Noam Rosenthal]

Comment on attachment 105798 [details]
Patch to add Web Inspector to WebKit2 updated.

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

> Source/WebKit2/ChangeLog:13
> +        Implemented Qt platform methods for Web Inspector
> +        One will activate Web Inspector from qdesktopwebview
> +        and qtouchwebview views. With current patch Web Inspector
> +        for both cases using qdesktopwebview
> +        "Enable Developer Extras" and "Toggle WebInspector" features
> +        implemented in QtWebPageProxy common for both views

Grammar for this Changelog entry is totally unreadable.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:57
> +#if PLATFORM(QT)
> +#include <qdesktopwebview.h>
> +#include <qsgview.h>
> +
> +class QDesktopWebView;
> +class QObject;
> +#endif

If you include those files declaring the classes is redundant.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:45
> +    ASSERT(m_inspectorView);

Please remove this assert.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:57
> +    ASSERT(m_view);

Please remove this assert.

> Tools/MiniBrowser/qt/BrowserView.cpp:104
> +void BrowserView::enableDeveloperExtrasMode(bool enable)
> +{
> +    if (desktopWebView())
> +        desktopWebView()->setDeveloperExtrasEnabled(enable);
> +    else
> +        touchWebView()->setDeveloperExtrasEnabled(enable);
> +}
> +
> +void BrowserView::toggleWebInspectorMode(bool enable)
> +{
> +    if (desktopWebView())
> +        desktopWebView()->setWebInspectorEnabled(enable);
> +    else
> +        touchWebView()->setWebInspectorEnabled(enable);
> +}
> +

Naming consistency:
setDeveloperExtrasEnabled
setWebInspectorModeEnabled

> Tools/MiniBrowser/qt/BrowserWindow.cpp:119
> +    toggleWebInspector->connect(this, SIGNAL(enteredDeveloperExtrasMode(bool)), SLOT(setEnabled(bool)));
> +    toggleWebInspector->connect(this, SIGNAL(enteredWebInspectorMode(bool)), SLOT(setChecked(bool)));
> +    m_enableDeveloperExtras->connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(changeDeveloperExtrasMode(bool)));
> +    connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(toggleWebInspectorMode(bool)));

Instead of doing this with signals and slot, please call those functions explicitly in onDeveloperExtrasModeChanged or onWebInspectorModeChanged. will be much more readable, and you won't need changeDeveloperExtrasMode.

[Genisim]

Created attachment 105830 [details]
Patch to add Web Inspector to WebKit2 updated.

Changes done according Noam's comments:
  asserts - removed
  Source/WebKit2/ChangeLog - updated
  Naming consistency - no changes Noam agreed with current names
  hard understandable SIGNAL - SLOT - removed, functionality moves to suggested functions

Please review.

[Genisim]

Created attachment 105836 [details]
Patch to add Web Inspector to WebKit2 updated.

[Noam Rosenthal]

Comment on attachment 105836 [details]
Patch to add Web Inspector to WebKit2 updated.

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

> Source/WebKit2/ChangeLog:9
> +        With current patch Web Inspector page using qdesktopwebview only.

QDesktopWebView and not qdesktopwebview.
Add something like "We create a QDesktopWebView that render the inspector, and a QSGView to display it", since that's what the patch does.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:68
> +    if (m_view) {
> +        m_view->close();
> +        m_view.clear();
> +    }

m_view.clear() should be enough, no need for the null check and close IIRC.

> Tools/ChangeLog:15
> +        1. "Enable Developer Extras" available, not selected
> +           "Toggle Web Inspector" unavailable, not selected
> +        2. "Enable Developer Extras" available, selected
> +            "Toggle Web Inspector" available, not selected
> +        3. "Enable Developer Extras" unavailable, selected
> +           "Toggle Web Inspector" available, selected

No need for this. The next two lines say enough.

> Tools/ChangeLog:16
> +        User can start Web Inspector only after "Enable Developer Extras" was selected

Period at end of sentence.

> Tools/ChangeLog:17
> +        User can't disable "Enable Developer Extras" when Web Inspector is running

Period at end of sentence.

> Tools/MiniBrowser/qt/BrowserWindow.h:59
> +    void enteredWebInspectorMode(bool on);
> +    void enteredDeveloperExtrasMode(bool on);

This is a bit strange, but I guess it's consistent with enteredFullScreenMode.

[Genisim]

Created attachment 105850 [details]
Patch to add Web Inspector to WebKit2 updated.

Patch updated according to latest comments.

Please review

[Noam Rosenthal]

(In reply to comment #80)
> Created an attachment (id=105850) [details]
> Patch to add Web Inspector to WebKit2 updated.
> 
> Patch updated according to latest comments.
> 
> Please review

I'm generally ok with this patch. Since there have been a lot of people involved, I'd appreciate if additional people would approve.

[Kenneth Rohde Christiansen]

Comment on attachment 105850 [details]
Patch to add Web Inspector to WebKit2 updated.

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

How can a patch go through so many iterations and still so so wrong?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:53
> +namespace WebKit {
> +    class WebInspectorProxy;
> +}

We don't indent in namespaces.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> +    void setDeveloperExtrasEnabled(bool);
> +    void setWebInspectorEnabled(bool);

We don't have settings on the QDesktopWebView.  That is just plain wrong.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:743
> +void QtWebPageProxy::setDeveloperExtrasEnabled(bool enable)
> +{
> +    m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);
> +}

This totally doesn't belong here

[Andreas Kling]

Adding some people who care about Qt API's.

[Jocelyn Turcotte]

Comment on attachment 105850 [details]
Patch to add Web Inspector to WebKit2 updated.

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

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:751
> +void QtWebPageProxy::setWebInspectorEnabled(bool enable)
> +{
> +    if (enable)
> +        m_webPageProxy->inspector()->show();
> +    else
> +        m_webPageProxy->inspector()->close();
> +}

Before trying to create APIs blindly by binding to Qt what is already in WebKit2 we should ask ourself how is the inspector going to be used, and how we can make it both easy to use and flexible.

Especially for the touch view, a cross-device remote inspector is necessary, and this is the main concern for WebKit2 and Qt5 right now.
In the end we want QtCreator or any desktop front-end of the developer to have the ability to inspect a page on a development device. I think that the desktop should also be considered but that it's way of doing things should be as near as possible to the way of the touch view since it will always be simpler.

I think that magically popping a top level window is not the right approach for those reasons, and we should focus first on designing remote inspection before exposing API. Local inspection within the MiniBrowser could then just use the remote channel and pop the top level window itself.

[Jocelyn Turcotte]

(In reply to comment #84)
Ok I was wrong seeing too far in the future here. Having something that works now is possible and this is a step forward so ignore my previous comment.

What really annoys me is that we were able to avoid those set***Enabled things to grow all over the place until now but this day has come and that's all. We will have to move them to some QWebSettings equivalent at some point.
The inspector will need to be enabled anyway to open the back door, and we can change this setWebInspectorEnabled method to open this door instead of popping up the window.

[Noam Rosenthal]

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> > +    void setDeveloperExtrasEnabled(bool);
> > +    void setWebInspectorEnabled(bool);
> 
> We don't have settings on the QDesktopWebView.  That is just plain wrong.
> 

I think this is an outcome of not having a settings class. How do you suggest we proceed? Not have inspector in until we have a settings class?

[Genisim]

(In reply to comment #82)
> (From update of attachment 105850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105850&action=review
> 
> How can a patch go through so many iterations and still so so wrong?
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:53
> > +namespace WebKit {
> > +    class WebInspectorProxy;
> > +}
> 
> We don't indent in namespaces.
> 


 1. no namespace brackets generates follow errors

UIProcess/API/qt/qdesktopwebview.h:124: error: ‘WebInspectorProxy’ in namespace ‘WebKit’ does not name a type

/scratchbox/users/genisim/home/genisim/swork/webkit_upstream/webkit_trunk/trunk_qt5/Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:124: error: friend declaration does not name a class or function

 2. I got suggestions don't use includes and use classes forward declarations.

 3. If it is a rule - "don't indent in namespaces", will be good clean a code.
If you check code, you will find many places with "indent in namespaces".
Or maybe it is a special exceptions - here allow to use and here not.

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> > +    void setDeveloperExtrasEnabled(bool);
> > +    void setWebInspectorEnabled(bool);
> 
> We don't have settings on the QDesktopWebView.  That is just plain wrong.
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:743
> > +void QtWebPageProxy::setDeveloperExtrasEnabled(bool enable)
> > +{
> > +    m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);
> > +}
> 
> This totally doesn't belong here

It is a long history for this patch and many different variants:
  1. API calling directly from app MiniBrowser (declined)
  2. Was asking - maybe need create new class (declined)
  3. Was calling set... into qdesktopwebview and qtouchwebview (declined. Make sense to me, because from both views calling same API. Maybe make sense if API will be different)

Please come to consensus and send me comments (summary) which is accepted by
all of you (I understand how difficult do this, but please try)

All platforms are using Web Inspector - MAC, Win, and I hope Qt will join to platforms which using Web Inspector too.

[Jesus Sanchez-Palencia]

(In reply to comment #85 and #86)
I guess there is a general "urge" for discussing Qt API's for WK2. This is becoming more clear every day...


(In reply to comment #87)
> > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:53
> > > +namespace WebKit {
> > > +    class WebInspectorProxy;
> > > +}
> > 
> > We don't indent in namespaces.
> > 
>
>
>  1. no namespace brackets generates follow errors

I think here the reviewer meant that you shouldn't indent code inside the namespace scope and not that you should remove it (the brackets).

[Simon Hausmann]

Jocelyn is on this right now and after much discussion we realize that we can cover the majority of particularly interesting use-cases right now with support for the remote web inspector. A patch is in progress that will allow enabling it via an environment variable, thus making it accessible to any Qt 5 application using the QQuickWebView.

[Rafael Brandao]

(In reply to comment #89)
> Jocelyn is on this right now and after much discussion we realize that we can cover the majority of particularly interesting use-cases right now with support for the remote web inspector. A patch is in progress that will allow enabling it via an environment variable, thus making it accessible to any Qt 5 application using the QQuickWebView.

Simon, is this ready? If so, how can we use it?

[Jocelyn Turcotte]

(In reply to comment #90)
> Simon, is this ready? If so, how can we use it?

Not yet, see bug #73094.

[Yael]

(In reply to comment #90)
> (In reply to comment #89)
> > Jocelyn is on this right now and after much discussion we realize that we can cover the majority of particularly interesting use-cases right now with support for the remote web inspector. A patch is in progress that will allow enabling it via an environment variable, thus making it accessible to any Qt 5 application using the QQuickWebView.
> 
> Simon, is this ready? If so, how can we use it?

At least for me this does not work out of the box. I applied Jocelyn's patches from 73092, 73852, 73093, 73855 and 73094.
So far, I have been fighting with qrc resources not being found, and it is not clear to me if we can really load qrc resources that are in the web process, through the UI process. e.g. we try to load inspector.html from the UI process, but it is packaged with the web process, so we don't find it.

[Sergio Villar Senin]

What't the status of this bug? I wouldn't mind helping BTW

[Jocelyn Turcotte]

(In reply to comment #93)
> What't the status of this bug? I wouldn't mind helping BTW

We have decided to use only the remote web inspector for Qt. Landed in bug #73855 a few months ago.