Closed Bug 558995 Opened 14 years ago Closed 14 years ago

Port Bug 463387 [Add an API for getting web progress notifications for all tabs] to SeaMonkey

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attachment #438682 - Flags: feedback?(philip.chee)
Attachment #438682 - Flags: feedback?(neil)
Blocks: 558996
Attachment #438682 - Flags: feedback?(neil) → feedback-
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
Attached patch WIP 2 (obsolete) — Splinter Review
Second pass, removed unnecessary stuff, trying to keep suite style.
Attachment #438682 - Attachment is obsolete: true
Attachment #439209 - Flags: review?(neil)
Attachment #439209 - Flags: feedback?(philip.chee)
Attachment #438682 - Flags: feedback?(philip.chee)
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.]
Attachment #439209 - Flags: review?(neil) → review-
Attached patch patch (obsolete) — Splinter Review
(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.
Attachment #439209 - Attachment is obsolete: true
Attachment #439483 - Flags: review?(neil)
Attachment #439209 - Flags: feedback?(philip.chee)
Attached patch patch plus test v1 (obsolete) — Splinter Review
Well, i realised what You mean :) Also i added test, it passes first two with exception that notification came from wrong browser.
Attachment #439483 - Attachment is obsolete: true
Attachment #439531 - Flags: review?(neil)
Attachment #439483 - Flags: review?(neil)
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).
(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.
Attached patch patch plus test v2 (obsolete) — Splinter Review
> 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.
Attachment #439531 - Attachment is obsolete: true
Attachment #439791 - Flags: review?(neil)
Attachment #439531 - Flags: review?(neil)
Forgot to mention, test is completely stolen from ff, including styling.
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 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 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.)
Attached patch patch plus test v3 (obsolete) — Splinter Review
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 ?
Attachment #439791 - Attachment is obsolete: true
Attachment #439795 - Flags: review?(neil)
Attachment #439791 - Flags: review?(neil)
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).
Attached patch patch plus test v4 (obsolete) — Splinter Review
Fixes all comments above, all tests passing, added REFRESH observer.
Attachment #439795 - Attachment is obsolete: true
Attachment #439808 - Flags: review?(neil)
Attachment #439795 - Flags: review?(neil)
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>).
(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 ;-)
Attached patch patch plus test v5 (obsolete) — Splinter Review
sorry, i meant false, but somehow it turns true :)
Attachment #439808 - Attachment is obsolete: true
Attachment #439848 - Flags: review?(neil)
Attachment #439808 - Flags: review?(neil)
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
Attachment #439848 - Flags: review?(neil) → review+
Attached patch patch plus test v6 (obsolete) — Splinter Review
How i missed that :(
Attachment #439848 - Attachment is obsolete: true
Attachment #439875 - Flags: superreview?(neil)
Attachment #439875 - Flags: review?(neil)
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.
Attachment #439875 - Flags: superreview?(neil)
Attachment #439875 - Flags: superreview+
Attachment #439875 - Flags: review?(neil)
Attachment #439875 - Flags: review+
Attached patch for checkinSplinter Review
Sorry :(
Nit fixed, carrying forward r+ and sr+ from Neil
Attachment #439875 - Attachment is obsolete: true
Attachment #439879 - Flags: superreview+
Attachment #439879 - Flags: review+
Keywords: checkin-needed
Blocks: SMtabAPI
Pushed http://hg.mozilla.org/comm-central/rev/4279d0dd3dbd
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
The blockautorefresh bits overlap Bug 465303 resulting in an incomplete implementation. Setting some dependencies so that things don't get lost.
Blocks: 465303
You need to log in before you can comment on or make changes to this bug.