STR: 1. Go to any bug. 2. Tools->Inspect. 3. Select the link displaying the name of the assignee of the bug. 4. Click the edit link next to it. This causes the DOM to change dynamically. Inspect still highlights the old span element, which no longer exists in DOM, and doesn't show the new changes in DOM. No amount of expanding and collapsing the rows in the tree fixes this situation.

I wonder if we want to haul in full DOM listening into the inspector? We could make it an option (with a checkbox) as there is some performance impact with these installed on a webpage. Alternatively, we could provide a "refresh" button in the toolbar to update the contents of the tree on demand.

The listener can be added/removed as the Inspector is opened/closed, right? So there would only be a potential performance impact when the Inspector is open. Obviously, the ideal is that everything "just works" without user intervention, so it seems worthwhile to head down that route first and see if we can make it work reasonably.

I began working on this bug with writing a test case, first. I found, to my surprise, that the current tree panel does update itself dynamically. WFM. Please see this test case. The new tree panel that is in the works (see bug 572038) is indeed oblivious to changes. I believe I should write a patch to be applied on top of the patches Robert has in bug 572038. Is this correct?

that is correct, sir, on all counts. Proceed!

Will this also simultaneously fix bug 566092?

I believe that bug will need more work. Definitely this bug fix will be of help for that. The problem at hand is the tree panel automatic updating of structure based on DOM changes. The other panels will be notified that something changes / they'll be asked for a refresh of some sorts, and that will be something to do for the highlighter. Hopefully that will fix the issue. Thanks for pointing out that bug!

Robert: I am now working on the handling of the cases of when the page is still loading, or when a new iframe/frame is added/removed. I have some thoughts (and questions): - nsIWebProgressListener does not notify us when an iframe is removed from the parent document (with removeChild() or such). Thus, we need to use an iframeWindow.onunload event handler and/or a parentDocument.DOMNodeRemoved listener for this. (note that DOMNodeRemoved mutation events are fired *before* the actual remove occurs) - We need to listen for the DOMContentLoaded/load event for each loading iframe. We can use an nsIWebProgressListener but we'll end up with a hybrid of unload event handling and nsIWebProgressListeners... not nice, but I am not sure if we can avoid it. Thoughts? - If we put only iframeWindow.onload event handlers and remove them immediately after they are executed (to avoid memleaks), we don't catch subsequent loads in the iframes. So, we either use an nsIWebProgressListener or we keep onload event handlers for later. Thoughts? - The tree panel structure for the content of iframes become stale once the user navigates away in the iframe. We may want to prompt the user about navigating away? (Like we do for the main window in bug 566084). That would seem like overkill. Or do we simply update the structure and attach to the newly loaded page? My thoughts: I believe we cannot get away without using window.onunload for each window (and iframe/frame), so we use this event. For loading, we should go for the nsIWebProgressListener, because this gives us the ability to track frame navigation. Also, I believe that the inspector tree panel should be live - no user notification. If we *really* want to avoid window.onunload, we can rely only on DOMNodeRemoved and remove the event listeners from every iframe/frame that is removed. I think this will be kind of shoddy/error prone. Please let me know how you want me to go further. Thanks!

This is the patch that fixes the issue. To apply the patch cleanly please do: - clean default branch from mozilla-central. - apply attachment 464784 [details] [diff] [review] from bug 573102, then attachment 463629 [details] [diff] [review] from bug 561782, then attachment 463349 [details] [diff] [review] and attachment 463325 [details] [diff] [review] from bug 572038. Finally, apply this patch. I used a progress listener for tracking navigation inside iframes. The DOMNodeRemoved mutation event is used for removing the events when iframes are removed from the parent document. Any comments and suggestions for improvements are welcome!

Updated patch. Yesterday I forgot to clean mutation listeners from the progress listener when the user navigates away in iframes. I also made some fixes to the test code.

Regarding bug 566092: the highlighter fails to show properly on Linux and I was not able to test this entirely. The test code in the above patch highlights an element (see panelRefreshTest()), then it changes the class to apply a style which changes the size of the element. The code then checks if the highlighter properly changed its size after the DOM mutation event. The test passes, but I have not seen this working in user testing, because the highlighter has display issues. Thus, I cannot confirm if bug 566092 is fixed by this patch. Please test. I expect the bug is fixed, because the automated test code does something quite similar to the bug description.

I think overall this looks really solid. Some questions: https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/domplate.jsm_sec2 why the removal of SourceText check above? domplate.DIV({"class": "docType"}, why the removal of "$object" from that template? insertChildBoxBefore: removeChildBox: I see you've re-added some of the API we removed during a previous sweep. I guess we know why it was there now. ;) in https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/inspector.js_sec8 we had an iterateWindows method before and it was removed because it was deemed somewhat risky. It doesn't appear to be used anywhere? findNextSibling and findNodeAttrBox don't have named functions. The progress listener looks fine. And the test code looks pretty fine, on a quick glance. Thanks for the patch!

Comment on attachment 465217 [details] [diff] [review] updated patch + test code with decent answers to the above and the couple of minor cleanups (missing function names, mostly)

(In reply to comment #11) > I think overall this looks really solid. > > Some questions: > > https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/domplate.jsm_sec2 > > why the removal of SourceText check above? DOM.SourceText is undefined. It did throw when I worked on that. > domplate.DIV({"class": "docType"}, > > why the removal of "$object" from that template? I reported this bug in the main tree panel bug 572038 and I think I included this fix in one of my patches (the test code patch). If we put "class": "docType $object" we get something like this from domplate: <div class="docType [XPCNativeWrapper [object ...]]">&lt;!DOCTYPE html&gt;</div> By removing "$object" we only output class="docType ". (I do have a nicely indented domplate source output laying around. I used it for the patch of this bug. It's really interesting to study the output of the domplate-based tree panel.) > insertChildBoxBefore: > removeChildBox: > > I see you've re-added some of the API we removed during a previous sweep. I > guess we know why it was there now. ;) Hehe. "back to the roots!" :) > in > https://bugzilla.mozilla.org/attachment.cgi?id=465217&action=diff#a/browser/base/content/inspector.js_sec8 > > we had an iterateWindows method before and it was removed because it was deemed > somewhat risky. It doesn't appear to be used anywhere? We had ... in Firebug or in Inspector? I use the iterateWindows method in attachMutationListeners and detachMutationListeners. This allows me to easily/quickly add/remove the mutation listeners from nested iframes (eg. when a DOMNodeRemoved event is fired). If there's a problem with this, please let me know. > findNextSibling and findNodeAttrBox don't have named functions. Oh, I forgot to fix that. Will fix now. > The progress listener looks fine. > > And the test code looks pretty fine, on a quick glance. Thanks for the patch! You're welcome! Thanks for the feedback+!

Updated patch based on feedback. I added function names to the findNextSibling and findNodeAttrBox methods.

Blocking+. This is a must-have feature if we're shipping the inspector.

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Removing items from kd4b6.

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Rebased patch on top of the latest mozilla-central default branch code. Nothing else changed, except for updates related to the changes in m-c (nothing big).

Comment on attachment 475897 [details] [diff] [review] rebased patch This was bitrotten by bug 653528. I kind of wish we could avoid going the mutation event listener route, since they have such bad side effects.

(In reply to comment #20) > Comment on attachment 475897 [details] [diff] [review] [review] > rebased patch > > This was bitrotten by bug 653528. I kind of wish we could avoid going the > mutation event listener route, since they have such bad side effects. Are there different ways to keep up with DOM changes I could use? Something with better performance.

I don't think so.

(In reply to comment #22) > I don't think so. Then I'll just have to rebase the patch. :)

We need to investigate if the new API from bug 641821 would be sufficient for our needs here. If yes, then that API would give us a nice perf boost - DOM mutation events are much slower.

Rebased the patch on top of the latest fxteam repo. We cannot yet use the API from bug 641821 - there are still some planned changes over there. Once that lands we can make a follow up patch - should be simple to migrate. That will give us a really nice perf boost. Looking forward to your review. Thank you!

I don't think we should fix this until we can use bug 641821 - we don't need to rush a fix for this, IMO.

will this bug fix bug 665421 ?

(In reply to comment #27) > will this bug fix bug 665421 ? That one is handled by bug 566084.

(In reply to comment #26) > I don't think we should fix this until we can use bug 641821 - we don't need > to rush a fix for this, IMO. I expect that bug 641821 is going to take quite some more time, depending on the specs, the talks there, the API decisions and lack of decisions. It started in March. I am fine with waiting, but the Inspector tool will lack an important bit of polish.

Things seem to be moving at a decent pace, I don't expect it to take too much time. This bug has been around for a while too, so while it would obviously be nice to fix, I don't think we need to spend time on a solution we know we're going to want to replace.

Agree with Gavin, we can reconsider if there's still no solution available when this becomes more pressing.

Comment on attachment 545221 [details] [diff] [review] rebase to fxteam I thought we didn't want to do this until the new API was ready?

Comment on attachment 545221 [details] [diff] [review] rebase to fxteam Yeah, we can cancel review request for now.

Another patch rebase.

aaq

aaq

rohit: please don't use actual bugs for testing. Use landfill.bugzilla.org.

Rebased patch. As agreed with dcamp we are aiming to land this patch rebased, maybe in time for Firefox 10 aurora merge (if you have time to review it!). We'll switch to the new mutations API when it will be available. We are putting this feature behind a pref - default to false. Added an option to the register tools API to allow a mutation event handler. Added mutation event handling mechanisms to the Style Inspector and to the HTML Tree Panel. Please let me know if further changes are needed. Thank you! I pushed the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=60916cd5d720 Builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-60916cd5d720

(In reply to Rob Campbell [:rc] (robcee) from comment #1) > I wonder if we want to haul in full DOM listening into the inspector? We > could make it an option (with a checkbox) as there is some performance > impact with these installed on a webpage. (In reply to Kevin Dangoor from comment #2) > The listener can be added/removed as the Inspector is opened/closed, right? > So there would only be a potential performance impact when the Inspector is > open. You misunderstood. Just adding a mutation event listeners once will slow down all DOM mutations in that browser window for the rest of its life. This is unacceptable and there's no point in landing it pref'd off, as we'll never want to flip the pref.

err, s/browser window/content window/ (since this is where you're adding the event listener)

Comment on attachment 570832 [details] [diff] [review] rebased patch we're not likely to use this approach, so cancelling this review request. Feel free to re-ask if this becomes important again.

Based on the original STR, this bug is now fixed. When I inspect the assignee's name, and click Edit, the highlight is no longer displayed and the "infobar" goes to the top of the window because the original span is no longer visible. As I understand it, the solution to the problem reported here fixes the highlighter but does not allow other tools to update based on DOM mutations. Still, it seems to me that the problem tracked in this bug is fixed. I'm sure someone will reopen it if I'm wrong. See bug 566092.