Bug 14348 – Messing up the inspector by dragging an URL into it

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

[Hannes Petri]

If you bring up the Web Inspector, and then drag a URL into it, the page located at the URL will be loaded inside the inspector.

[Timothy Hatcher]

<rdar://problem/5283620>

[Adam Roben (:aroben)]

<rdar://problem/5712808>

[Matt Lilek]

Created attachment 19217 [details]
implement drag destination delegate

Two questions:

- On Windows, do I actually need to implement the ref counting or does the Drosera approach work here as well (that code & comments are just copied and pasted from Drosera) - I don't think I do, but I'm not 100% sure.

- Which radar is the right one to put in the ChangeLogs?

[Darin Adler]

Comment on attachment 19217 [details]
implement drag destination delegate

Looks good.

+        * ChangeLog:

Should not be in there.

+#ifndef WebInspectorBaseDelegate_H

Should be _h, not _H.

+    // COM ref-counting isn't useful to us because we're in charge of the lifetime of the WebView.

Are you sure this is OK?

r=me

[Adam Roben (:aroben)]

(In reply to comment #3)
> - On Windows, do I actually need to implement the ref counting or does the
> Drosera approach work here as well (that code & comments are just copied and
> pasted from Drosera) - I don't think I do, but I'm not 100% sure.

The WebInspectorClient must not be deleted before a) inspectorDestroyed() is called, and b) the last reference to it is released. Currently we delete the WebInspectorClient within inspectorDestroyed(). There's nothing in this patch that guarantees that the last reference has been released by then, so I think we're playing with fire here. I think the simplest solution is to make a separate object that functions as the UI delegate for this new WebView.

[Matt Lilek]

Comment on attachment 19217 [details]
implement drag destination delegate

Clearing review flag based on Adam's comment

[Matt Lilek]

Created attachment 19369 [details]
revised patch

Revised to address Adam's comments

[Matt Lilek]

Forgot to mention in the mac ChangeLog that removing this line:

166      [preferences setLoadsImagesAutomatically:YES];

is removing a dupe setting, not removing that setting :)

[Timothy Hatcher]

We should really do this with a policy delegate too to prevent loading any page that isn't the inspector.html file.

[Adam Roben (:aroben)]

Comment on attachment 19369 [details]
revised patch

+    , m_delegate(new WebInspectorDelegate())

I don't think there's any need for the WebInspectorClient to hold onto the delegate.

+class WebInspectorDelegate : public IWebUIDelegate {
+public:
+    WebInspectorDelegate();

Our COM classes normally have a private constructor/destructor and a static createInstance() method that returns a new instance with a refcount of 1. You should make this class follow that pattern.

Other than those comments, this looks good to me. r- so we can get those things fixed up.

[Matt Lilek]

Created attachment 19407 [details]
revised patch round 2

[Adam Roben (:aroben)]

Comment on attachment 19407 [details]
revised patch round 2

+    if (FAILED(m_webView->setUIDelegate(WebInspectorDelegate::createInstance())))
+        return 0;

I think this is now leaking the delegate. On Windows, WebView retains its delegates (this doesn't match the Mac model, and we may change it someday). You should be able to fix the leak by doing this:

m_webView->setUIDelegate(COMPtr<WebInspectorDelegate>(AdoptCOM, WebInspectorDelegate::createInstance()).get())

+WebInspectorDelegate::WebInspectorDelegate()
+    :m_refCount(1)
+{
+}
+
+WebInspectorDelegate* WebInspectorDelegate::createInstance()
+{
+    WebInspectorDelegate* instance = new WebInspectorDelegate();
+    return instance;
+}

Normally our COM classes start out with a refcount of 0 and then we call AddRef inside the createInstance method.

You can also just say "new WebInspectorDelegate" without the parentheses.

r=me if you fix the issues above.

[Matt Lilek]

Landed in <http://trac.webkit.org/projects/webkit/changeset/30631> with the changes Adam mentioned.

[Matt Lilek]

(In reply to comment #9)
> We should really do this with a policy delegate too to prevent loading any page
> that isn't the inspector.html file.
> 

Filed bug 17577.