Last Comment Bug 558995 - Port Bug 463387 [Add an API for getting web progress notifications for all tabs] to SeaMonkey
: Port Bug 463387 [Add an API for getting web progress notifications for all ta...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 463387
Blocks: SMtabAPI 465303 558996
  Show dependency treegraph
 
Reported: 2010-04-12 23:34 PDT by Misak Khachatryan
Modified: 2010-05-08 03:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP 1 (22.31 KB, patch)
2010-04-12 23:34 PDT, Misak Khachatryan
neil: feedback-
Details | Diff | Splinter Review
WIP 2 (12.50 KB, patch)
2010-04-15 04:45 PDT, Misak Khachatryan
neil: review-
Details | Diff | Splinter Review
patch (11.07 KB, patch)
2010-04-16 00:43 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
patch plus test v1 (22.43 KB, patch)
2010-04-16 07:50 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
patch plus test v2 (23.04 KB, patch)
2010-04-18 10:07 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
patch plus test v3 (23.47 KB, patch)
2010-04-18 11:17 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
patch plus test v4 (24.15 KB, patch)
2010-04-18 11:56 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
patch plus test v5 (24.13 KB, patch)
2010-04-18 22:45 PDT, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
patch plus test v6 (24.13 KB, patch)
2010-04-19 02:33 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
for checkin (24.13 KB, patch)
2010-04-19 02:51 PDT, Misak Khachatryan
misak.bugzilla: review+
misak.bugzilla: superreview+
Details | Diff | Splinter Review

Description Misak Khachatryan 2010-04-12 23:34:08 PDT
Created attachment 438682 [details] [diff] [review]
WIP 1

From Parent bug:

tabbrowser has addProgressListener, this allows you to register a progress
listener for whichever is the visible tab.

Frequently in extension development you want to hear about events from all
tabs. There are also likely to be useful uses for this within the product (bug
386835 will need this f.e.).

I propose adding a pair of new methods to tabbrowser, addAllProgressListener
(please someone think of a better name) and removeAllProgressListener. It will
register a progress listener that gets notified for all tabs, background or
foreground. It will be passed a variant on nsIWebProgressListener. Essentially
the object will have all the same methods with an additional argument, the
browser element that fired the event.



I have WIP patch, which starts SeaMonkey browser without errors, but breaks many tests, so just requesting feedback from Neil and Philip Chee to correct obvious things. This patch needs gIdentityHandler to be implemented, but i'll do it in separate bug.
Comment 1 neil@parkwaycc.co.uk 2010-04-14 13:30:18 PDT
Comment on attachment 438682 [details] [diff] [review]
WIP 1

>+//      gCrashReporter.annotateCrashReport("URL", aRequest.URI.spec);
[Not sure what this is trying to do]

>+      FullZoom.onLocationChange(aLocationURI, false, aBrowser);
I don't think our zoom wants this.

>+  onRefreshAttempted: function (aBrowser, aWebProgress, aURI, aDelay, aSameURI) {
This belongs in notification.xml

>+                                             "chrome://browser/skin/Info.png",
This is definitely wrong!

>diff --git a/suite/browser/nsBrowserStatusHandler.js b/suite/browser/nsBrowserStatusHandler.js
Not sure what these changes are for.

>-              if (aMaxTotalProgress > 0)
>-                this.mTab.setAttribute("progress", Math.floor(aCurTotalProgress * 9.9 / aMaxTotalProgress));
More unrelated changes?

>-              this.mTabBrowser.mProgressListeners.forEach(
>-                function notifyProgressChange(element) {
Please stick to this style of array enumeration.

>+      <method name="addTabsProgressListener">
>+        <parameter name="aListener"/>
>+        <body>
>+          this.mTabsProgressListeners.push(aListener);
>+        </body>
>+      </method>
>+
>+      <method name="removeTabsProgressListener">
Please copy the style of add/removeProgressListener
Comment 2 Misak Khachatryan 2010-04-15 04:45:38 PDT
Created attachment 439209 [details] [diff] [review]
WIP 2

Second pass, removed unnecessary stuff, trying to keep suite style.
Comment 3 neil@parkwaycc.co.uk 2010-04-15 14:41:38 PDT
Comment on attachment 439209 [details] [diff] [review]
WIP 2

>diff --git a/suite/browser/browser.js b/suite/browser/browser.js
>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
These changes have no useful effect. Just remove them.

>               this.mTabBrowser.mProgressListeners.forEach(
>                 function notifyProgressChange(element) {
>+                    try {
Hmm, I think you might have removed too much here, since the ordinary progress listeners should only fire for the current tab, but you then go on to do the tabs progress listeners. (The onRefreshAttempted version gets that bit right at least.)

>               this.mTabBrowser.mProgressListeners.forEach(
>-                function notifyLocationChange(element) {
>+                function notifyStateChange(element) {
>                   try {
>                     element.onLocationChange(aWebProgress, aRequest, aLocation);
This change looks wrong somehow. (Possibly it just needs more context?)

>+            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
This won't work unless you advertise nsIWebProgressListener2 in QueryInterface.

>+                for (var i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
This doesn't use the forEach style.

>+            _startStalledTimer: function () {
This bit doesn't belong, I think.

>diff --git a/suite/common/bindings/notification.xml b/suite/common/bindings/notification.xml
These changes belong in their own bug, but it would be nice to have this feature, so I'll review it anyway.

>+      <method name="onRefreshAttempted">
This won't work unless you advertise the interface in the implements list.

>+            if (Services.prefs.getBoolPref("accessibility.blockautorefresh")) {
>+              let brandBundle = document.getElementById("bundle_brand");
>+              let brandShortName = brandBundle.getString("brandShortName");
Please use the prefs and bundles provided by the binding.

>+              let refreshButtonText =
>+                gNavigatorBundle.getString("refreshBlocked.goButton");
>+              let refreshButtonAccesskey =
>+                gNavigatorBundle.getString("refreshBlocked.goButton.accesskey");
>+              let message =
>+                gNavigatorBundle.getFormattedString(aSameURI ? "refreshBlocked.refreshLabel"
>+                                                             : "refreshBlocked.redirectLabel",
>+                                                    [brandShortName]);
These strings need to be added to the relevant .properties file.

>+              let docShell = aWebProgress.DOMWindow
>+                                         .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                                         .getInterface(Components.interfaces.nsIWebNavigation)
>+                                         .QueryInterface(Components.interfaces.nsIDocShell);
Just store the webProgress object directly. (Someone didn't know that it's the same object...)

>+                notification.refreshURI = aURI;
>+                notification.delay = aDelay;
>+                notification.docShell = docShell;
Put these after the if so you don't have to duplicate them. Also I'd just call the aUri a uri rather than refreshURI since that creates confusion with the refreshURI variable.

>+                                                     "chrome://communicator/skin/icons/application.png",
[I didn't realise we had that one. I'll have to take a look at it.]
Comment 4 Misak Khachatryan 2010-04-16 00:43:55 PDT
Created attachment 439483 [details] [diff] [review]
patch

(In reply to comment #3)
> >               this.mTabBrowser.mProgressListeners.forEach(
> >                 function notifyProgressChange(element) {
> >+                    try {
> Hmm, I think you might have removed too much here, since the ordinary progress
> listeners should only fire for the current tab, but you then go on to do the
> tabs progress listeners. (The onRefreshAttempted version gets that bit right at
> least.)

I just restored old notifier code and added mTabsProgressListeners part.

> 
> >diff --git a/suite/common/bindings/notification.xml b/suite/common/bindings/notification.xml
> These changes belong in their own bug, but it would be nice to have this
> feature, so I'll review it anyway.

Do you know bug number, I'll add dependency.

> >+                                                     "chrome://communicator/skin/icons/application.png",
> [I didn't realise we had that one. I'll have to take a look at it.]

I used first suitable image. Please suggest the right one.
Comment 5 Misak Khachatryan 2010-04-16 07:50:49 PDT
Created attachment 439531 [details] [diff] [review]
patch plus test v1

Well, i realised what You mean :) Also i added test, it passes first two with exception that notification came from wrong browser.
Comment 6 neil@parkwaycc.co.uk 2010-04-18 06:33:59 PDT
Comment on attachment 439531 [details] [diff] [review]
patch plus test v1

>               if (this.mBlank || this.mTabBrowser.mCurrentTab != this.mTab)
>                 return;
> 
>-              this.mTabBrowser.mProgressListeners.forEach(
>+              if (this.mTabBrowser.mCurrentTab == this.mTab) {
Except you probably need to remove the mCurrentTab part of the check above...
(happens in at least five places)

>                   } catch (e) {
>+                       // don't inhibit other listeners
Not sure why you only added this in one place, and the indent is wrong anyway

>               if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
>                   aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
>+                  aIID.equals(Components.interfaces.nsIWebProgressListener2) ||
>                   aIID.equals(Components.interfaces.nsISupports))
Nit: please list the two nsIWebProgressListener* interfaces together and keep the two nsISupports* interfaces together

>+                 browser_alltabslistener.js \
>+		 alltabslistener.html \
Nit: wrong indentation, probably spaces/tabs mismatch?

>+  info("\nTest 1");
I don't recognise this style, did you copy it from another test?

>-    <implementation implements="nsIObserver, nsIWebProgressListener, nsIDOMEventListener">
>+    <implementation implements="nsIObserver, nsIWebProgressListener, nsIDOMEventListener, nsIWebProgressListener2">
Nit: would like the two nsIWebProgressListener* together here too

>+              let refreshButtonText =
>+                gNavigatorBundle.getString("refreshBlocked.goButton");
Wrong bundle (you added the strings to the right file though).

>+              let notificationBox = getBrowser().getNotificationBox(aBrowser);
In notification.xml we can just use "this" ;-)

>+              notification.docShell = docShell;
Please call the property webProgress, and add it first, and use aWebProgress directly (eliminating docShell).
Comment 7 neil@parkwaycc.co.uk 2010-04-18 06:35:57 PDT
(In reply to comment #4)
>(In reply to comment #3)
>>>diff --git a/suite/common/bindings/notification.xml b/suite/common/bindings/notification.xml
>>These changes belong in their own bug, but it would be nice to have this
>>feature, so I'll review it anyway.
>Do you know bug number, I'll add dependency.
Sorry, but I don't even know if there is a bug.

(In reply to comment #5)
>Also i added test, it passes first two with
>exception that notification came from wrong browser.
The test just seemed to stop running when I tried it.
Comment 8 Misak Khachatryan 2010-04-18 10:07:19 PDT
Created attachment 439791 [details] [diff] [review]
patch plus test v2

> The test just seemed to stop running when I tried it.
I fixed all but Your last comment, not sure how to add property. What happening with test ? On my PC it running with this patch and passes all tests now, except passing correct browser.
Comment 9 Misak Khachatryan 2010-04-18 10:08:12 PDT
Forgot to mention, test is completely stolen from ff, including styling.
Comment 10 neil@parkwaycc.co.uk 2010-04-18 10:09:03 PDT
Comment on attachment 439531 [details] [diff] [review]
patch plus test v1

>+      <method name="onRefreshAttempted">
>+        <parameter name="aBrowser" />
I've just realised that this is for the tabs progress listener version; you
don't need this parameter here. But you do need to fix addProgressListener to
notify for the REFRESH notification.

>+        <parameter name="aWebProgress" />
>+        <parameter name="aURI" />
>+        <parameter name="aDelay" />
...
>+              notification.uri = aURI;
>+              notification.delay = aDelay;
>+              notification.docShell = docShell;
Regarding your request for clarification, these sets of lines should match.
Comment 11 neil@parkwaycc.co.uk 2010-04-18 10:26:49 PDT
Comment on attachment 439791 [details] [diff] [review]
patch plus test v2

>+    ok(aBrowser == gTestBrowser, state + " notification came from the correct browser");
>+    ok(gAllNotificationsPos < gAllNotifications.length, "Got an expected notification for the all notifications listener");
>+    is(state, gAllNotifications[gAllNotificationsPos], "Got a notification for the all notifications listener");
>+    gAllNotificationsPos++;
>+
>+    if ((aStateFlags & gCompleteState) == gCompleteState) {
>+      ok(gAllNotificationsPos == gAllNotifications.length, "Saw the expected number of notifications");
>+      ok(gFrontNotificationsPos == gFrontNotifications.length, "Saw the expected number of frontnotifications");
If you'd used is(x, y, z) instead of ok(x == y, z) then wouldn't the test have told you that x is undefined? (As yet I don't know why that should be.)
Comment 12 neil@parkwaycc.co.uk 2010-04-18 10:30:20 PDT
Comment on attachment 439791 [details] [diff] [review]
patch plus test v2

>+              this.mTabBrowser.mProgressListeners.forEach(
>+                function notifySecurityChange(element) {
>+                  if (element && "onRefreshAttempted" in element) {
>+                    try {
>+                      if (!element.onRefreshAttempted(this.mBrowser, aWebProgress, aURI, aDelay, aSameURI))
>+                        allowRefresh = false;
>+                    } catch (e) {
>+                      Components.utils.reportError(e);
>+                    }
>+                  }
>+                }
>+              );
OK, so the problem is that you're trying to use this.mBrowser inside the notifySecurityChange callback. But in order to do that, you have to pass this as the second parameter to forEach. (Or you could pass this.mBrowser as the second parameter to forEach, and then pass this to onRefreshAttempted, but that would probably be way too sneaky and confusing.)
Comment 13 Misak Khachatryan 2010-04-18 11:17:03 PDT
Created attachment 439795 [details] [diff] [review]
patch plus test v3

Thanks, all tests are passing now, even test for bug 524745, which uses notifications !

(In reply to comment #10)
> I've just realised that this is for the tabs progress listener version; you
> don't need this parameter here. But you do need to fix addProgressListener to
> notify for the REFRESH notification.
> 
How to do this ?
Comment 14 neil@parkwaycc.co.uk 2010-04-18 11:35:16 PDT
Comment on attachment 439795 [details] [diff] [review]
patch plus test v3

>+                  function notifyProgressChange(element) {
>+                      try {
>+                      element.onProgressChange(aWebProgress, aRequest,
>+                                               aCurSelfProgress, aMaxSelfProgress,
>+                                               aCurTotalProgress, aMaxTotalProgress);
>+                      } catch (e) {
>+                        Components.utils.reportError(e);
>+                      }
>+                  }
Nit: indentation of some of the lines is wrong.

>+                         // don't inhibit other listeners
>+                       // don't inhibit other listeners
Remove these please.

>+            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
>+            {
>+              var allowRefresh = true;
>+              if (this.mTabBrowser.mCurrentTab == this.mTab) {
>+                this.mTabBrowser.mProgressListeners.forEach(
>+                  function notifySecurityChange(element) {
This (and the one below) should be called notifyRefreshAttempted.

>-            }
>+            },
I don't think this change is right.

>+    ok(aBrowser == gTestBrowser, state + " notification came from the correct browser");
>+    ok(gAllNotificationsPos < gAllNotifications.length, "Got an expected notification for the all notifications listener");
>+    is(state, gAllNotifications[gAllNotificationsPos], "Got a notification for the all notifications listener");
>+    gAllNotificationsPos++;
>+
>+    if ((aStateFlags & gCompleteState) == gCompleteState) {
>+      ok(gAllNotificationsPos == gAllNotifications.length, "Saw the expected number of notifications");
>+      ok(gFrontNotificationsPos == gFrontNotifications.length, "Saw the expected number of frontnotifications");
Please change these to use is(x, y, z) instead of ok(x == y, z) [6 changes]

>+            if (this._prefs.getBoolPref("accessibility.blockautorefresh")) {
Do we have to add this to browser-prefs.js?

>+                this._stringBundle.getFormattedString(aSameURI ? "refreshBlocked.refreshLabel"
>+                                                             : "refreshBlocked.redirectLabel",
Nit: ? and : should line up.

>+              let notificationBox = this.getNotificationBox(aBrowser);
notification.xml's "this" is the notificationBox, so just use "this" instead.

>+              let notification = notificationBox.getNotificationWithValue("refresh-blocked");
e.g. let notification = this.getNotificationWithValue("refresh-blocked");

>+              notification.uri = aURI;
>+              notification.delay = aDelay;
>+              notification.webProgress = aWebProgress;
Still not in the order I wanted (see comment #10).
Comment 15 Misak Khachatryan 2010-04-18 11:56:09 PDT
Created attachment 439808 [details] [diff] [review]
patch plus test v4

Fixes all comments above, all tests passing, added REFRESH observer.
Comment 16 neil@parkwaycc.co.uk 2010-04-18 13:00:09 PDT
Comment on attachment 439808 [details] [diff] [review]
patch plus test v4

>+// Blocks auto refresh if true
Hmm, Firefox defaults to false.

>+pref("accessibility.blockautorefresh",    true);
Nit: why all the spaces? One will do ;-)

>+                    try {
>+                    element.onProgressChange(aWebProgress, aRequest,
>+                                             aCurSelfProgress, aMaxSelfProgress,
>+                                             aCurTotalProgress, aMaxTotalProgress);
Nit: indentation still not quite right ;-)

>+              this.activeBrowser.webProgress.addProgressListener(this, Components.interfaces.nsIWebProgress.NOTIFY_LOCATION,
>+                                                                 Components.interfaces.nsIWebProgress.NOTIFY_REFRESH);
Not quite right; the two values need to be joined with the | operator, not a comma (and also aligned with each other, not the bracket, although the line is far too long anyway and it might be better to rewrap it some other way).

>+              let brandShortName = this._brandStringBundle.GetStringFromName("brandShortName");
>+              let refreshButtonText =
>+                this._stringBundle.getString("refreshBlocked.goButton");
>+              let refreshButtonAccesskey =
>+                this._stringBundle.getString("refreshBlocked.goButton.accesskey");
>+              let message =
>+                this._stringBundle.getFormattedString(aSameURI ? "refreshBlocked.refreshLabel"
>+                                                               : "refreshBlocked.redirectLabel",
>+                                                    [brandShortName]);
Having checked that you did the right thing for the brand, I totally overlooked that you were using the wrong methods for the other text - since we're using a nsIStringBundle object rather than a <stringbundle> object here, you have to call GetStringFromName instead of getString, and you have to call formatStringFromName instead of getFormattedString, and pass it a third parameter which is the length of the array (i.e. 1). (The indentation of [brandShortName] was also wrong but you need to change it anyway.)

>+                notification =
>+                             this.appendNotification(message, "refresh-blocked",
>+                                                     "chrome://communicator/skin/icons/application.png",
>+                                                     this.PRIORITY_INFO_MEDIUM,
>+                                                     buttons);
Nit: wrong indentation again. Not sure which is nicest; one possibility is
notification = this.appendNotification(message,
                                       "refresh-blocked",
etc. or you could leave it wrapped but not indented so far i.e.
notification =
  this.appendNotification(message, "refresh-blocked",
etc.

I did get one other error in the JS console but I couldn't reproduce it.

Final Nit: git-apply complained about four lines with trailing whitespace (in fact three of those lines were blank, and the other one was </body>).
Comment 17 neil@parkwaycc.co.uk 2010-04-18 15:43:24 PDT
(In reply to comment #16)
>(From update of attachment 439808 [details] [diff] [review])
>>+// Blocks auto refresh if true
>Hmm, Firefox defaults to false.
Let's default it to false for now, so as to avoid surprising nightly users ;-)
Comment 18 Misak Khachatryan 2010-04-18 22:45:34 PDT
Created attachment 439848 [details] [diff] [review]
patch plus test v5

sorry, i meant false, but somehow it turns true :)
Comment 19 neil@parkwaycc.co.uk 2010-04-19 01:51:04 PDT
Comment on attachment 439848 [details] [diff] [review]
patch plus test v5

Yes! Well, near enough; usual story - indentation!

>+                this._stringBundle.formatStringFromName(aSameURI ? "refreshBlocked.refreshLabel"
>+                                                               : "refreshBlocked.redirectLabel",
>+                                                               [brandShortName], 1);
: lines up with ?
[brandShortName] lines up with aSameURI
Comment 20 Misak Khachatryan 2010-04-19 02:33:14 PDT
Created attachment 439875 [details] [diff] [review]
patch plus test v6

How i missed that :(
Comment 21 neil@parkwaycc.co.uk 2010-04-19 02:48:53 PDT
Comment on attachment 439875 [details] [diff] [review]
patch plus test v6

>+                this._stringBundle.formatStringFromName(aSameURI ? "refreshBlocked.refreshLabel"
>+                                                                 : "refreshBlocked.redirectLabel",
>+                                                                 [brandShortName], 1);
Nit: [brandShortName] lines up with aSameURI

But don't bother asking me for review again.
Comment 22 Misak Khachatryan 2010-04-19 02:51:32 PDT
Created attachment 439879 [details] [diff] [review]
for checkin

Sorry :(
Nit fixed, carrying forward r+ and sr+ from Neil
Comment 23 Misak Khachatryan 2010-04-20 00:17:04 PDT
Pushed http://hg.mozilla.org/comm-central/rev/4279d0dd3dbd
Comment 24 Philip Chee 2010-05-08 03:40:01 PDT
The blockautorefresh bits overlap Bug 465303 resulting in an incomplete implementation. Setting some dependencies so that things don't get lost.

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