[Webkit-unassigned] [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64297





--- Comment #57 from Genisim <genisim at yahoo.com>  2011-08-29 23:06:02 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.