566084 - Highlighter should be disabled when navigating to new pages

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[(no longer active)]

This is very similar to bug 566082, with the same set of STR.  Basically, the panel just stays there, showing info from the old page, and the element highlighting UI doesn't get activated again.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Need to an a location changed listener to the inspector. We would either want to close the inspector on navigation to a new page or reinitialize on the new page. Probably the former, I think.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: browser-inspector-location-changed-test

test-case failing when navigating away from a test page to another with the highlighter open.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Attachment: onreadystatechange

simple patch to close the inspector onreadystatechange.

Comment 5

[Rob Campbell [:rc] (:robcee)]

another nice feature might be to display a notification that you're about to navigate away from a webpage and close the inspector.

Comment 6

[Rob Campbell [:rc] (:robcee)]

throwing this back into the pool...

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 454626 [details] [diff] [review]
onreadystatechange

this patch doesn't work. I thought I might get an easy out with something like this, but I think we'll need a web progress listener to detect navigation away from the page.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch + test code

This is the patch + test code that fixes this bug, as discussed with Robert on IRC. This patch should apply cleanly on mozilla-central default branch.

The previous test code and fix files are both obsolete. The bug fix approach is different now, and the test code has changed a lot to be able to properly test the fix.

Any feedback for improvements is very much welcome, thanks!

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 461008 [details] [diff] [review]
patch + test code

In inspector.js,

+    gBrowser.addProgressListener(InspectorProgressListener,
+        Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
...
+    gBrowser.removeProgressListener(InspectorProgressListener);

I don't think we want to be notified of changes on all tabs, do we? Can we do gBrowser.selectedTab.addProgressListener() instead? (Might need to use the this.browser element, not sure which one of these will work)

+  onStateChange: function IPL_onStateChange(aProgress, aRequest, aFlag, aStatus) {
+    // remove myself if the Inspector is no longer open.
+    if (!InspectorUI.isPanelOpen) {
+      gBrowser.removeProgressListener(InspectorProgressListener);
+      return;

and here.

+    // Unknown values are considered 0.
+    if (!pref || (pref != 1 && pref != 2)) {

this is confusing logic. If you want undefined or 0,

  if (!pref) { ...

+      let prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"].
+          getService(Ci.nsIPromptService);

Services.prompt. No need to create a variable since you can look it up directly.

+    }
+
+    if (pref == 1) {

  } else if (pref == 1) {

+    } else if (pref == 2) {
+      // The pref is set to never confirm page navigation.
+      BrowserStop();
+    }

maybe include an extra else clause here to do defaults and sanity checking on the stored pref. Chances are, if the pref was set to something > 2, they probably wanted never confirm. 

+  onLocationChange: function() {},    <- trailing whitespace
+  onProgressChange: function() {},

in the inspector.properties file.

+confirmNavigationAway.title=Inspector: page navigation
+confirmNavigationAway.message=You are about to leave the current page, losing the current DOM. Are you sure you want to continue?

I might use, "You are about to leave the current location which will close the Inspector, losing any changes you may have made. Are you sure you want to continue?"

In any case, we'll need a ui-review on this.

+confirmNavigationAway.rememberChoice=Remember my choice.

Not sure about this either. Remember my preference? Need ui-review.

Test code looks good, except for the gBrowser.addProgressListener again.

Comment 10

[Rob Campbell [:rc] (:robcee)]

oh, when you accept assignment on a bug, you should change the status to Assigned.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Thanks for your feedback, Robert!

I have updated the patch as requested. The gBrowser.addProgressListener() stays in place, as we agreed on IRC. For reference, according to:

https://developer.mozilla.org/en/Code_snippets/Progress_Listeners

... the gBrowser progress events are only triggered for the active tab, which is exactly what we want. This is confirmed by the testing of the fix I did, as well.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 461286 [details] [diff] [review]
updated patch + test code

ok, looks good.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 461286 [details] [diff] [review]
updated patch + test code

Asking for review from Mossop, as Gavin is leaving really soon on vacation.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

You should use XPCOMUtils.generateQI rather than rolling your own QI.

The (win != InspectorUI.win) check makes the earlier subframe checks redundant, I think.

I'm not sure I like the idea of calling BrowserStop() from within a WPL callback. It's not immediately obvious that it will always lead to the correct result.

Do we really need a pref for this? I would kind of prefer to just always hide the inspector on navigation...

Comment 15

[Mihai Sucan [:msucan]]

Gavin: Thanks for your comment! I can switch the code to use generateQI - I only learned of it later - after I wrote this patch.

I agree, the earlier subframe check can be removed. It was used during different iterations of the code, hehe.

Wrt. BrowserStop() ... is there a better way to cancel the page navigation request?

Adding the preference was suggested by Robert. I would like to keep it, because the behavior to always hide the inspector on navigation is really ugly for me as a web developer - I wouldn't like this. (just my suggestion!)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> Wrt. BrowserStop() ... is there a better way to cancel the page navigation
> request?

I asked on #developers: call aRequest.cancel(Cr.NS_BINDING_ABORTED);

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Prompting+pref just seems like so much extra unneeded overhead. I could live with just the pref, I suppose, but prompting really doesn't feel right.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Updated the code as suggested by Gavin. The prompt is still there, I'd like feedback on this from Limi.

Thanks Gavin!

Comment 19

[Mihai Sucan [:msucan]]

Attachment: screenshot of the prompt

Attaching a screenshot, as requested by Limi.

Comment 20

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 463293 [details] [diff] [review]
updated patch + test code

UI-wise, I think this is fine for a development tool. You probably don't want the inspector to follow you around if you leave the current page, and having the ability to let it not notify you about losing your changes in the future is OK too. 

I assume that if you manually close the inspector, this warning doesn't happen when you navigate away from the page.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 463293 [details] [diff] [review]
updated patch + test code

I'm not a browser peer, so I cannot review this, but dolske can!

Comment 22

[Mihai Sucan [:msucan]]

Limi: that's correct. Once the Inspector is closed, the prompt also stops showing up. Thanks for your ui-review+!

Comment 23

[Rob Campbell [:rc] (:robcee)]

That's not quite the prompt I was expecting. :) I was trying to suggest using one of the sliding prompts like the password change notifications.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml

see, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js#775

for an example.

But, this is definitely a good start.

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 463293 [details] [diff] [review]
updated patch + test code

A few code nits, but I think there's a bigger problem here... These kinds of prompts can be _really_ annoying.  I don't see any kind of dirty-bit setting/checking, so it would seem if someone quicky opens the inspector to examine something, they'll get an annoying prompt when navigating away even though nothing's been changed.

The UI also seems rather unfortunate if you select Cancel + Remember -- forever more, navigating just stops working when the Inspector is open.

I wonder if the prompt should be disabled by default, and have some kind of "lock me on this page" UI menu/checkbox be exposed by the Inspector, so that users doing page modifications can opt-in to stay stuck on the page, but that choice would be temporary to the page/session.

Worth thinking about a way to have the Inspector stay open when navigating, but update it to watch the new page?

>+++ b/browser/app/profile/firefox.js
...
>+// Page navigation behaviour when the Inspector tool is enabled:
>+// 0  ask the user to confirm page navigation.
>+// 1  always allow page navigation.
>+// 2  never allow page navigation.
>+pref("browser.inspector.pageNavigation", 0);

s/behaviour/behavior/ :)


>+++ b/browser/base/content/inspector.js

>+    // Read user pref.
>+    try {
>+      pref = gPrefService.getIntPref("browser.inspector.pageNavigation");
>+    } catch (ex) { }

This patch adds a default to firefox.js, so you can assume it always has a value (no need for try/catch).

>+    if (pref != 0 && pref != 1 && pref != 2) {
>+      pref = 0;
>+    }

< 0 || > 2

>+      let result = Services.prompt.confirmCheck(null, strTitle, strMessage,
>+        strRemember, rememberCheckbox);

Instead of null, the prompt ought to be tied to a specific window when possible.

Comment 25

[Mihai Sucan [:msucan]]

Dolske: Thanks for your review! You have a good point. One of the ideas I've discussed with Robert would be to switch away from the prompt to a notificationbox. The UI would certainly be nicer *if* I can make it modal (I haven't tried that yet).

However, that would not solve the issue you raised in your comment. It's still annoying, irrespective of the actual way we ask the user to allow page navigation or not.

I really like your idea. I could add a checkbox to the Inspector toolbar "Lock me on this page". Wehn that is enabled, page navigation would be canceled - I think we don't need to use the prompt, because the user explicitly elects to be locked on the page once he enables the given option. This setting would be stored per tab.

Also, regarding the Inspector staying open after page navigation: I am all for that! I suggested this to Robert, but he prefers we close the Inspector.

Robert: what do you think? Should I go for the approach suggested by Dolske with the checkbox? I'd really like that - as a user. :) When page navigation is allowed, should I close the Inspector or update it with the new page? I'd really like, again as a user, the latter behavior.

Comment 26

[Rob Campbell [:rc] (:robcee)]

I think the addition of an isDirty flag to the inspector code makes a lot of sense. Only prompt the user if they've made changes to the page through the inspector and navigation would cause them to lose those changes.

Comment 27

[Mihai Sucan [:msucan]]

Robert: I'll add a flag for that then. Thanks! Given that there is no editor at the moment I only add the flag, and in the future it will be set to true. So, basically, the Inspector will just close when the user navigates away - until we have the editors.

Comment 28

[Rob Campbell [:rc] (:robcee)]

Sounds reasonable. We'll use "isDirty" for the property name and you can just check for its truthiness in your code until the editors make use of it.

Comment 29

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch, as it was decided: we now use a dirty flag for edits in the Inspector panels. I also made the changes you requested. Thanks!

Comment 30

[Kevin Dangoor]

This is actually a blocker for string freeze, not betaN... re-requesting blocking status.

Comment 31

[Dietrich Ayala (:dietrich)]

Blocking+, yeah that behavior is pretty broken and b6+ due to strings.

Comment 32

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Patch rebased to trunk.

Comment 33

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 34

[Kevin Dangoor]

Removing items from kd4b6.

Comment 35

[Kevin Dangoor]

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

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 471203 [details] [diff] [review]
Proposed patch.

>+    // Read user pref.
>+    let pref = gPrefService.getIntPref("browser.inspector.pageNavigation");
>+
>+    if (pref < 0 || pref > 2) {
>+      pref = 0;
>+    }
>+
>+    if (!pref) {
>+      // Ask the user what to do with the page navigation request.

Clearer as: if (pref == 0)

>+      pref = result ? 1 : 2;

You're sorta reusing this variable name for both the pref and the user's selected action. If the checkbox (to remember the action) isn't saved, that's a bit confusing.

>+    if (pref == 1) {
>+      // The pref is set to always confirm page navigation.
>+      InspectorUI.closeInspectorUI(true);
>+    } else if (pref == 2) {
>+      // The pref is set to never confirm page navigation.
>+      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
>+    }

Still a concern here -- if the pref gets set to this value, navigation is silently suppressed and the user might be left wondering why their browser is broken. A notification bar (in the browser? in the inspector?) is probably the easiest solution. And if you do that, it could just replace the prompt entirely. Maybe the automatic action isn't even needed, since I assume people won't be randomly editing random pages. Just always show a "Page navigation to etld.site.com canceled, you have unsaved Inspector changes for this page. [Discard and Go Anyway]".

Also, since I didn't actually test this patch, does it correctly handle switching tabs, and navigations triggered in a background tab being Inspected?

Comment 37

[Mihai Sucan [:msucan]]

(In reply to comment #36)
> Comment on attachment 471203 [details] [diff] [review]
> Proposed patch.
> 
> >+    // Read user pref.
> >+    let pref = gPrefService.getIntPref("browser.inspector.pageNavigation");
> >+
> >+    if (pref < 0 || pref > 2) {
> >+      pref = 0;
> >+    }
> >+
> >+    if (!pref) {
> >+      // Ask the user what to do with the page navigation request.
> 
> Clearer as: if (pref == 0)
> 
> >+      pref = result ? 1 : 2;
> 
> You're sorta reusing this variable name for both the pref and the user's
> selected action. If the checkbox (to remember the action) isn't saved, that's a
> bit confusing.

Thanks for your review, Justin. Will change these parts.


> >+    if (pref == 1) {
> >+      // The pref is set to always confirm page navigation.
> >+      InspectorUI.closeInspectorUI(true);
> >+    } else if (pref == 2) {
> >+      // The pref is set to never confirm page navigation.
> >+      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
> >+    }
> 
> Still a concern here -- if the pref gets set to this value, navigation is
> silently suppressed and the user might be left wondering why their browser is
> broken. A notification bar (in the browser? in the inspector?) is probably the
> easiest solution. And if you do that, it could just replace the prompt
> entirely. Maybe the automatic action isn't even needed, since I assume people
> won't be randomly editing random pages. Just always show a "Page navigation to
> etld.site.com canceled, you have unsaved Inspector changes for this page.
> [Discard and Go Anyway]".

You're correct, but using a notification like that complicates things:

- if I just always cancel the navigation, and the user selects "go anyway", then? The progress listener and the request are both lost already - I can't "resume" them. The request might be a POST, a GET or anything else.

- if I suspend the request and show the notification box, then sure, we can ask the user, like with the prompt "you have unsaved changes, do you want to continue? [yes] or [no]". This sounds doable, if the request can be nicely suspended indefinitely. I haven't tested.

Thoughts?


> Also, since I didn't actually test this patch, does it correctly handle
> switching tabs, and navigations triggered in a background tab being Inspected?

I have tested it. The current behavior is: the Inspector is open only for the current tab. When the user switches to a different tab, it closes and only saves some state info, and it detaches all event listeners. This means that when the user returns, the Inspector reopens. If the user reloads that tab, or if the page changes location without user interaction, that goes unnoticed, because the Inspector is closed.

Should we change the behavior?

Comment 38

[Mihai Sucan [:msucan]]

Attachment: notification patch

This patch includes the changes you have requested, and temporary code that switches to a nicer notification. Thanks again for your review!

I bumped into a blocker. The nsIRequest.suspend() call has no effect, which means I cannot block/suspend the page load, to wait for further user action from the new async notification. Somehow, the prompt did that for me - it blocked the request, waiting for user action.

Currently, any page load is not blocker, the notification only shows and disappears.

According to the API, nsIRequest.suspend() should work:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRequest.idl#100

still... there's a "catch": "NOTE: some implementations are unable to immediately suspend, and may continue to deliver events already posted to an event queue. In general, callers should be capable of handling events even after suspending a request."

Did I hit that case? If yes, then how can I make sure I "force" a suspend? In the way the prompt service does.

Thanks!

Comment 39

[Rob Campbell [:rc] (:robcee)]

dolske: any suggestions on how to proceed with the above? the nsIRequest.suspend() function not working is turning into a bit of a show-stopper for this. With no way to suspend the request, it's not possible to prevent a user's navigation from proceeding.

anyone?

Comment 40

[Justin Dolske [:Dolske]]

Sorry, missed the comments here in all the bugmail. :/

I'm not sure why .suspend() isn't working, I'm not really familiar with that code. Sounds like a question for biesi or bz?

Comment 41

[Rob Campbell [:rc] (:robcee)]

Thanks, we can ping them with questions.

Also mentioned in IRC, investigate how safebrowsing accomplishes this.

Comment 42

[Mihai Sucan [:msucan]]

Attachment: Updated patch

Updated patch. This patch uses a notification bar which allows the user to cancel or confirm page navigation.

As discussed with Robert, I have removed the "Remember my choice" option because it doesn't make sense. I also tested the behavior with multiple tabs and it does what we expect it - the Inspector closes when the user moves to a different tab, which means page navigation is not prevented. Additionally, the progress listener only and always affects the active tab - other tabs/windows where the Inspector is not open are not affected by this change.

The problem I had with nsIRequest.suspend() is no longer "valid". I presume it was a bug in the mozilla-central build I used. Today I pulled from mozilla-central and made new optimized and debug builds. Both work fine.

Comment 43

[Justin Dolske [:Dolske]]

Comment on attachment 477502 [details] [diff] [review]
Updated patch

>+    gBrowser.removeProgressListener(InspectorProgressListener);
>+

Hmm, is it possible to get an inspector open for 2 documents? (open tab, inspect, switch tab, inspect)

>+  showNotification: function IPL_showNotification(aRequest)
>+  {
...
>+    if (notification) {
>+      notification.nsIRequest.cancel(Cr.NS_BINDING_ABORTED);
>+      notification.nsIRequest.resume();
>+      notification.nsIRequest = aRequest;
>+      return;
>+    }

Can you just remove the old notification, and continue with adding the new notification? The old notification should clean up after itself.

>+    notification.nsIRequest = aRequest;

It's a bit weird to be using an interface name as a property... Just .request or .linkedRequest? Or hang a .cancelRequest function off it which holds the request via a closure (since the same thing is done in a couple of places)?

>+    notification.addEventListener("DOMNodeRemoved", function(aEvent) {


Sigh. File a followup to add support for a removed() callback? I think the new doorhanger notifications support such a thing. DOM mutation listeners are best to avoid.


>+++ b/browser/locales/en-US/chrome/browser/inspector.properties
...
>+confirmNavigationAway.message=You are about to leave the current location which will close the Inspector, losing any changes you may have made. Are you sure you want to continue?
>+confirmNavigationAway.buttonYes=Yes
>+confirmNavigationAway.buttonYesAccesskey=Y
>+confirmNavigationAway.buttonNo=No
>+confirmNavigationAway.buttonNoAccesskey=N

Yes/No dialogs are generally bad UI. It's better to have the buttons indicate the action that will be performed. The string is also a bit long.

The strings should probably be similar to what was just changed in bug 588292, since they're very similar in purpose.

I'd suggest something like:
.message = Leaving this page will close the Inspector and the changes you have made will be lost.
button1=Leave Page
button2=Stay on Page

Comment 44

[Mihai Sucan [:msucan]]

(In reply to comment #43)
> Comment on attachment 477502 [details] [diff] [review]
> Updated patch
> 
> >+    gBrowser.removeProgressListener(InspectorProgressListener);
> >+
> 
> Hmm, is it possible to get an inspector open for 2 documents? (open tab,
> inspect, switch tab, inspect)

Yes and no.

Yes, you can open a tab, Inspect, then open a new tab, and Inspect. Then switch back to the first tab to get the Inspect for the first tab. However, the Inspector closes whenever you switch a tab, and reopens if it has remembered a state for the new tab. So, you never get two Inspectors open at the same time, in the same window.

This means that when you switch to a new tab, the progress listener and all events, everything ... from the old tab are removed. Changing this behavior would be beyond the purpose of this patch.


> >+  showNotification: function IPL_showNotification(aRequest)
> >+  {
> ...
> >+    if (notification) {
> >+      notification.nsIRequest.cancel(Cr.NS_BINDING_ABORTED);
> >+      notification.nsIRequest.resume();
> >+      notification.nsIRequest = aRequest;
> >+      return;
> >+    }
> 
> Can you just remove the old notification, and continue with adding the new
> notification? The old notification should clean up after itself.

Yeah, I think that's possible, but I did not do this for a few reasons:

- there's no point in animating a slide out and then back a slide-in ... with identical question and options.

- having read other code through Firefox ... I saw the same approach: reuse the same notification. This is why getNotificationWithValue() is there (iianm).

- the lack of an elegant callback for when the notification is removed. (see the DOMNodeRemoved event handler).

Nonetheless, if you want, I can change the code. Please let me know.


> >+    notification.nsIRequest = aRequest;
> 
> It's a bit weird to be using an interface name as a property... Just .request
> or .linkedRequest? Or hang a .cancelRequest function off it which holds the
> request via a closure (since the same thing is done in a couple of places)?

Agreed. Will look into fixing this.


> >+    notification.addEventListener("DOMNodeRemoved", function(aEvent) {
> 
> Sigh. File a followup to add support for a removed() callback? I think the new
> doorhanger notifications support such a thing. DOM mutation listeners are best
> to avoid.

I was also surprised to see the lack of a removed() callback. Where shall I file the bug? Which component?


> >+++ b/browser/locales/en-US/chrome/browser/inspector.properties
> ...
> >+confirmNavigationAway.message=You are about to leave the current location which will close the Inspector, losing any changes you may have made. Are you sure you want to continue?
> >+confirmNavigationAway.buttonYes=Yes
> >+confirmNavigationAway.buttonYesAccesskey=Y
> >+confirmNavigationAway.buttonNo=No
> >+confirmNavigationAway.buttonNoAccesskey=N
> 
> Yes/No dialogs are generally bad UI. It's better to have the buttons indicate
> the action that will be performed. The string is also a bit long.
> 
> The strings should probably be similar to what was just changed in bug 588292,
> since they're very similar in purpose.
> 
> I'd suggest something like:
> .message = Leaving this page will close the Inspector and the changes you have
> made will be lost.
> button1=Leave Page
> button2=Stay on Page

Will change the strings.

Thanks for your review!

Comment 45

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch based on review.

Changes:

- no longer using notification.nsIRequest, nor any other property to link back to the nsIRequest object. Callbacks use the aRequest variable from the scope.

- filed bug 600501 about notification removed() callback. (found where to do it)

- updated the strings as suggested. It looks much clearer now.

Thanks!

Comment 46

[Justin Dolske [:Dolske]]

Comment on attachment 479347 [details] [diff] [review]
updated patch

AIUI this code isn't enabled in FF4 any more, so clearing review. Reflag when we start working on post-FF4 things.

Comment 47

[Mihai Sucan [:msucan]]

Attachment: new patch

Updated/rebased patch on top of the latest Inspector code.

- made some minor code quality improvements (hey, learned some stuff since last autumn!).
- some minor test improvements.

Please let me know if this patch needs further changes. Thank you!


(Note that the UI and behavior was decided quite some time ago. Please read the comments above. If you want changes, I can do that, of course.)

Comment 49

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 543909 [details] [diff] [review]
new patch

nit:

+  onStateChange:
+  function IPL_onStateChange(aProgress, aRequest, aFlag, aStatus) {
+    // Remove myself if the Inspector is no longer open.

Brace on next line for functions! (file-consistency)

+    notification = notificationBox.appendNotification(message,
+      "inspector-page-navigation", "chrome://browser/skin/Info.png",
+      notificationBox.PRIORITY_WARNING_HIGH, buttons);
+
+    notification.persistence = 10;

bit of a magic number. Why 10? This should probably be a const.

+++ b/browser/base/content/test/inspector/browser_inspector_bug_566084_location_changed.js

+
+  gBrowser.addProgressListener(TestProgressListener1);

not sure we want to use a progress listener for this test.

+  onLocationChange: function TPL1_onLocationChange(aProgress, aRequest, aURI) {
+    gBrowser.removeProgressListener(TestProgressListener1);
+    ok(false, "navigation was not cancelled");
+    testEnd();
+  },

definitely don't like this.

Why not check for the existence of the notificationBox? You can listen for AlertActive and do away with this progress listener.

Otherwise, I like the addition of the notificationbox. This should look good when it's ready.

r- for the magic number and test implementation, but it's close.

Comment 50

[Mihai Sucan [:msucan]]

Attachment: new patch 2

Updated the patch to address the review comments.

Thank you!

Comment 51

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 546829 [details] [diff] [review]
new patch 2

I don't like having different success/fail conditions inside if statements.

+function onPageLoad() {
+  if (content.location.href.indexOf("test1") > -1) {
+    ok(false, "page navigation was not canceled");
+  } else if (content.location.href.indexOf("test2") > -1) {
+    gBrowser.selectedBrowser.removeEventListener("load", onPageLoad, true);
+    ok(true, "page navigation was allowed");
+

Couldn't we do away with the if() test and do:

ok(content.location.href.indexOf("test2") > -1, "page navigation allowed");

Then the rest of the tests get run and pass or fail. The if seems unnecessary.

(do we get a load event when the progressListener is canceled? That seems surprising)

r+ with the above tweak.

Comment 52

[Mihai Sucan [:msucan]]

(In reply to comment #51)
> Comment on attachment 546829 [details] [diff] [review] [review]
> new patch 2
> 
> I don't like having different success/fail conditions inside if statements.
> 
> +function onPageLoad() {
> +  if (content.location.href.indexOf("test1") > -1) {
> +    ok(false, "page navigation was not canceled");
> +  } else if (content.location.href.indexOf("test2") > -1) {
> +    gBrowser.selectedBrowser.removeEventListener("load", onPageLoad, true);
> +    ok(true, "page navigation was allowed");
> +
> 
> Couldn't we do away with the if() test and do:
> 
> ok(content.location.href.indexOf("test2") > -1, "page navigation allowed");
> 
> Then the rest of the tests get run and pass or fail. The if seems
> unnecessary.

Good point.

Comment 53

[Mihai Sucan [:msucan]]

Attachment: new patch 3

Test tweaked.

Pushed to the try server:

http://tbpl.mozilla.org/?tree=Try&rev=cb21ca1e2e7e

Comment 54

[Mihai Sucan [:msucan]]

Results are green. This patch can land in fx-team.

Comment 55

[Mihai Sucan [:msucan]]

Comment on attachment 546853 [details] [diff] [review]
new patch 3

Asking for review from a browser peer.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

It looks like is isDirty is never set to true in that patch (apart from in the test). Is that intentional?

Comment 57

[Mihai Sucan [:msucan]]

(In reply to comment #56)
> It looks like is isDirty is never set to true in that patch (apart from in
> the test). Is that intentional?

Yes. Once editing capabilities are implemented, isDirty will be set to true, and users will be asked to confirm they want to lose changes they made. Otherwise page navigation is not canceled. (as was decided)

Comment 59

[Justin Dolske [:Dolske]]

Comment on attachment 546853 [details] [diff] [review]
new patch 3

I started thinking "hmm, maybe this should be a doorhanger now"... And then thought "self, how about we land this damn thing first, and iterate on it later."

Comment 60

[Rob Campbell [:rc] (:robcee)]

(In reply to Justin Dolske [:Dolske] from comment #59)
> Comment on attachment 546853 [details] [diff] [review]
> new patch 3
> 
> I started thinking "hmm, maybe this should be a doorhanger now"... And then
> thought "self, how about we land this damn thing first, and iterate on it
> later."

I think you should listen to that fellow. He has good ideas.

Personally, I like the notification box, but would not mind a doorhanger. Where would we attach it? The location bar? The originating link?

Let's land this puppy!

Comment 61

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] rebased patch

Rebased the patch.

Comment 62

[Mihai Sucan [:msucan]]

Comment on attachment 553210 [details] [diff] [review]
[in-fx-team] rebased patch

Landed:
http://hg.mozilla.org/integration/fx-team/rev/7715bba5cc3a

Dolske: thanks for the review+!

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 553210 [details] [diff] [review]
[in-fx-team] rebased patch

>+    gBrowser.addProgressListener(InspectorProgressListener);

>+/**
>+ * The InspectorProgressListener object is an nsIWebProgressListener which
>+ * handles onStateChange events for the inspected browser. If the user makes
>+ * changes to the web page and he tries to navigate away, he is prompted to
>+ * confirm page navigation, such that he's given the chance to prevent the loss
>+ * of edits.
>+ */
>+var InspectorProgressListener = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener]),

There is no XPCOM involved for progress listeners added via gBrowser.addProgressListener. Nothing expects this object to implement nsIWebProgressListener.

>+  onLocationChange: function() {},
>+  onProgressChange: function() {},
>+  onStatusChange: function() {},
>+  onSecurityChange: function() {},

And these can be dropped.

Comment 64

[Dão Gottwald [:dao]]

Comment on attachment 553210 [details] [diff] [review]
[in-fx-team] rebased patch

>+++ b/browser/base/content/inspector.js

>+    // We need a removed() callback for notifications. See bug 600501.
>+    notification.addEventListener("DOMNodeRemoved",

Erm, I think this would be a blocker for this to land on mozilla-central. Adding a single mutation event listener still slows down all DOM mutations in that window even after you've removed it again. You're in the browser window here, so this isn't acceptable.

Comment 65

[Dão Gottwald [:dao]]

-> backed out

Comment 66

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #65)
> -> backed out

Thanks for your review comments! Good catches. We'll update the patch as needed. We'll go for fixing bug 600501.

I was about to backout this patch and I saw you already did. Thanks!

Comment 67

[Rob Campbell [:rc] (:robcee)]

yes. Thanks for the quick catch and backout, Dao!

Comment 68

[Mihai Sucan [:msucan]]

Attachment: [checked-in] address dão's comments

Updated the patch to address Dão's comments, and rebased on top of the patch submitted for bug 600501. Changes:

- updated the InspectorProgressListener as suggested.
- no longer using the DOM mutation event listener. using the new removed event callback from bug 600501.

Please let me know if this patch needs further changes. Thank you!

Comment 69

[Mihai Sucan [:msucan]]

Pushed this patch and the new patch from bug 600501:

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=117225c63f43

Green results!

Comment 70

[Dão Gottwald [:dao]]

Comment on attachment 555182 [details] [diff] [review]
[checked-in] address dão's comments

r=me on the interdiff

Comment 71

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #70)
> Comment on attachment 555182 [details] [diff] [review]
> address dão's comments
> 
> r=me on the interdiff

Thank you Dão!

Comment 72

[Mihai Sucan [:msucan]]

Comment on attachment 555182 [details] [diff] [review]
[checked-in] address dão's comments

Landed:
http://hg.mozilla.org/integration/fx-team/rev/6f44a95fa1f5

Comment 73

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 555182 [details] [diff] [review]
[checked-in] address dão's comments

http://hg.mozilla.org/mozilla-central/rev/6f44a95fa1f5

Comment 74

[Teodosia Pop]

Verified fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1

Comment 75

[u279076]

Given that this has been set for milestone=Firefox9 and verified fixed on Firefox 10, I'm asking for tracking/status.

I'm guessing this status-firefox10:fixed and status-firefox9:affected?

Comment 76

[Dão Gottwald [:dao]]

Target milestone = Firefox 9 means that this was fixed when Firefox 9 was on mozilla-central. Since mozilla-central later became the starting point for Firefox 10 (and Firefox 11 now), it contains this patch as well as any future release. This is true for all patches landing on mozilla-central. status-firefoxXX flags need to be set for branch landings. Last but not least, tracking request are something entirely different and not needed in this case...

Comment 77

[u279076]

Thanks Dao.

Teodosia, please verify if this is fixed on Firefox 9. Thanks.