Bug 14348 - Messing up the inspector by dragging an URL into it
: Messing up the inspector by dragging an URL into it
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2007-06-23 15:57 PST by
Modified: 2008-02-27 15:19 PST (History)


Attachments
implement drag destination delegate (22.01 KB, patch)
2008-02-19 17:52 PST, Matt Lilek (pewtermoose)
no flags Review Patch | Details | Formatted Diff | Diff
revised patch (20.61 KB, patch)
2008-02-25 19:39 PST, Matt Lilek (pewtermoose)
aroben: review-
Review Patch | Details | Formatted Diff | Diff
revised patch round 2 (20.03 KB, patch)
2008-02-27 14:30 PST, Matt Lilek (pewtermoose)
aroben: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-06-23 15:57:21 PST
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.
------- Comment #1 From 2007-06-25 14:10:55 PST -------
<rdar://problem/5283620>
------- Comment #2 From 2008-01-29 10:57:49 PST -------
<rdar://problem/5712808>
------- Comment #3 From 2008-02-19 17:52:37 PST -------
Created an attachment (id=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?
------- Comment #4 From 2008-02-20 09:22:11 PST -------
(From update of attachment 19217 [details])
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
------- Comment #5 From 2008-02-25 14:35:13 PST -------
(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.
------- Comment #6 From 2008-02-25 14:37:52 PST -------
(From update of attachment 19217 [details])
Clearing review flag based on Adam's comment
------- Comment #7 From 2008-02-25 19:39:37 PST -------
Created an attachment (id=19369) [details]
revised patch

Revised to address Adam's comments
------- Comment #8 From 2008-02-25 19:43:50 PST -------
Forgot to mention in the mac ChangeLog that removing this line:

166      [preferences setLoadsImagesAutomatically:YES];

is removing a dupe setting, not removing that setting :)
------- Comment #9 From 2008-02-26 21:35:53 PST -------
We should really do this with a policy delegate too to prevent loading any page that isn't the inspector.html file.
------- Comment #10 From 2008-02-27 13:55:21 PST -------
(From update of attachment 19369 [details])
+    , 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.
------- Comment #11 From 2008-02-27 14:30:44 PST -------
Created an attachment (id=19407) [details]
revised patch round 2
------- Comment #12 From 2008-02-27 14:36:55 PST -------
(From update of attachment 19407 [details])
+    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.
------- Comment #13 From 2008-02-27 15:10:17 PST -------
Landed in <http://trac.webkit.org/projects/webkit/changeset/30631> with the changes Adam mentioned.
------- Comment #14 From 2008-02-27 15:19:28 PST -------
(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.