Bug 14348 - Messing up the inspector by dragging an URL into it
Summary: Messing up the inspector by dragging an URL into it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Matt Lilek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-06-23 15:57 PDT by Hannes Petri
Modified: 2008-02-27 15:19 PST (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hannes Petri 2007-06-23 15:57:21 PDT
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 Timothy Hatcher 2007-06-25 14:10:55 PDT
<rdar://problem/5283620>
Comment 2 Adam Roben (:aroben) 2008-01-29 10:57:49 PST
<rdar://problem/5712808>
Comment 3 Matt Lilek 2008-02-19 17:52:37 PST
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?
Comment 4 Darin Adler 2008-02-20 09:22:11 PST
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
Comment 5 Adam Roben (:aroben) 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 Matt Lilek 2008-02-25 14:37:52 PST
Comment on attachment 19217 [details]
implement drag destination delegate

Clearing review flag based on Adam's comment
Comment 7 Matt Lilek 2008-02-25 19:39:37 PST
Created attachment 19369 [details]
revised patch

Revised to address Adam's comments
Comment 8 Matt Lilek 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 Timothy Hatcher 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 Adam Roben (:aroben) 2008-02-27 13:55:21 PST
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.
Comment 11 Matt Lilek 2008-02-27 14:30:44 PST
Created attachment 19407 [details]
revised patch round 2
Comment 12 Adam Roben (:aroben) 2008-02-27 14:36:55 PST
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.
Comment 13 Matt Lilek 2008-02-27 15:10:17 PST
Landed in <http://trac.webkit.org/projects/webkit/changeset/30631> with the changes Adam mentioned.
Comment 14 Matt Lilek 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.