83265 - UAAG: Add a way to disable HTTP-EQUIV=refresh (block automatic meta redirection, lock on current page)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement)

Description

[Chris Bitmead]

This browser feature is so heavily abused by web sites these days
that I have to consider that the inability to disable it is a bug.
The problem is that half the web sites out there are trying to fake
their banner impressions by using the HTTP-EQUIV=refresh feature
with a small timeout. This is terribly annoying for a number of reasons
and needs a config option to turn off. The reasons it is annoying is:
1) It refreshes while you are halfway through reading something.
2) It wastes bandwidth (and therefore $$$ !!), if you leave the browser
open on that page. Something that costs you money when you do nothing
has got to be a bug.

This feature, when used to redirect to a different URL is generally
harmless however, so you need this as a separate option.
CHECKBOX: Disable Refresh to different URL?
CHECKBOX: Disable Refresh to same URL?

Comment 1

[Asa Dotzler [:asa]]

over to necko.

Comment 2

[timeless]

does caps allow this?

Comment 3

[Mitchell Stoltz (not reading bugmail)]

No, caps doesn't control this. I suggest using nsIContentPolicy hooks to
implement a refresh-blocker.

Comment 4

[Chris Bitmead]

I think it would be best if this feature could also be customised on a site
by site basis just like the reject cookies feature. Mostly using the refresh
feature is harmless. It's only those sites that you visit frequently that
abuse it you want to disable it for. I guess the ideal interface would be 
a button labeled "disable refresh for this site". Then it goes onto a list
of sites that you can edit. I guess people want the same kind of site list
for cookies, javascript and other features that are abused.

Comment 5

[Matthias Versen [:Matti]]

*** Bug 111861 has been marked as a duplicate of this bug. ***

Comment 6

[Matthias Versen [:Matti]]

*** Bug 123516 has been marked as a duplicate of this bug. ***

Comment 7

[benc]

qa -> tever.

Comment 8

[Matthias Versen [:Matti]]

*** Bug 152841 has been marked as a duplicate of this bug. ***

Comment 9

[Doug Turner (:dougt)]

moving neeti's futured bugs for triaging.

Comment 10

[Darren Salt]

A mechanism similar to that of Acorn Browse would be of use here: if the page
uses the refresh feature, the Stop button remains available (and the refresh
timer is active) until clicked, at which point it becomes unavailable, the timer
is stopped and the page won't be automatically refreshed.

Comment 11

[Jay Davis]

Darren, I disagree with taking the Acorn approach.  Most pages that people visit
are used just at that time, in my opinion, and they are not used in 2 minute or
other intervals continually.  This is the case even when it's a news page of
some sort.  Yes, that web page wishes you to keep seeing new material, but is
that what the viewer *normally* wants?  NO.  And so, what is required to result
from this bug report is that a preference must be included somewhere that does
not permit the page refresh on any page, so that the viewer can decide when he
wants to refresh.  Now, this issue could be further dealt with by using a
context menu choice.  Or there could be a whitelist type treatment in the
preferences menus to deal with it on a per page basis.  But, at the very least,
there should be a global way to turn off self-refreshing, even if it just a
preference in the prefs.js.

Comment 12

[Darren Salt]

I agree that there should be a configuration option controlling whether
self-refreshing is enabled, and won't argue about whitelisting; but I will say
that where self-refreshing is enabled, there *must* be a way to disable it
locally and without affecting the browser configuration.

(Incidentally, Browse has such a config option, but it affects all automatic
refreshing.)

Comment 13

[Jay Davis]

Darren,

It would seem that Tabbrowser Extensions has implemented the control of page
refreshing in the Nov 19th version, though I'm not sure it's functioning totally
bug-free so far.  It is not doing everything that people discuss in this feature
request, but it's nice.  So now that extension controls both tab reloading
intervals and disabling of refresh on a specific tab basis if not on a website
basis.

Comment 14

[Jay Davis]

It appears that Tabbrowser Extensions now is capable of being set per bookmark
to disable autorefresh.  Again, I don't know if it's completely bug-free but
I've been using it and it's pretty sweet so far.

Comment 15

[Jo Hermans]

*** Bug 182890 has been marked as a duplicate of this bug. ***

Comment 16

[Darin Fisher]

meta refresh is handled by docshell

Comment 17

[R.K.Aa.]

similar: bug 78636

Comment 18

[Rob Stow]

Re: Comment #12 from Darren Salt
   "Incidentally, Browse has such a config option, 
   but it affects all automatic refreshing."

Where is this option ?   I have looked in all of
the prefs file - including the one's in the "defaults"
folder - for words like "refresh", "auto-refresh" and 
"autorefresh" and there is *nothing*.

Re: Comment #14 from Jay Davis
   "It appears that Tabbrowser Extensions now is capable 
    of being set per bookmark"

Unfortunately when you disable refreshing for a bookmark it
doesn't disable bookmarking for locations you click your way to
from there - even if the new URL is in the "directory tree" of
the bookmarked one.  Example, if you disable refresh for
www.somedomain.com\index.html and while on that page click a
link to www.somedomain.com\nextpage.html, then "nextpage" will
be able to autorefresh.



What would suit my needs would be a way to have refreshing default
to "off" *except* where I have told Mozilla otherwise.  Ie.,  we need
a permissions manager for refreshing just like the one currently 
available for images and just like the ones desperately needed for Flash
and sound.

Comment 19

[Stein M. Hugubakken]

Another useful option would be that only the focused tab should be allowed to
refresh.

Comment 20

[Lars P M]

HTTP concerns are not on the client side, and is not for user agents to
influence. This is a matter for servers. To be able to disable this feature
would be to control a server, i.e. disabling a server feature. What should the
browser do next? Disable asp? Disable php?

Comment 21

[Darren Salt]

> HTTP concerns are not on the client side, and is not for user agents to
> influence. This is a matter for servers.

False. HTTP-EQUIV is a matter for the client. Let's just say that I'd be *very*
concerned were a server to try to establish an unauthorised connection to any of
my machines. (Not that it could anyway...)

> To be able to disable this feature would be to control a server, i.e.
> disabling a server feature.

Rubbish. HTTP-EQUIV provides information for use by the client. It's part of the
HTML specification and, as such, appears in an HTTP response body; it is content
to be delivered as-is and definitely *not* acted upon by the server or any
proxies (if they do, they're buggy).

> What should the browser do next? Disable asp? Disable php?

It can't. It can't even make any assumptions about the content type whether from
cache or from the "file"name extension (if there is one).

Comment 22

[Rob Stow]

Re: comment #20

HTTP-EQUIV=refresh is NOT a "server feature".  It is merely a code in a web page
asking the browser to periodically download a possibly updated version of the
page.  

Blocking unrequested refreshes is no different than blocking unrequested popups.

Characterizing refresh blocking as "disabling a server feature" implies that we
want to do something to the server, when in fact nothing of the kind is
involved.  It is merely proposing that the user have the option of stopping
*his* browser from constantly refreshing the page when he is trying to read the
damned thing.  

Comment 23

[Lars P M]

O.K. Checked the HTML spec and found out that I was wrong before.

Now hear! No white-list. No additional pref. Navigation bar! Modify the Stop
button. Currently it's red while the page loads. Let it then turn to yellow,
*if* a Refresh action is to come. The user who wants to avoid the refresh,
presses the button and the page will be locked, i.e. Refresh is disabled.

Comment 24

[Craig Ringer]

PeEmm: Its all well and good to be able to disable timed refresh using the stop
button (this would IMHO be a very good thing), however it doesn't fully solve
the problem. Some sites (News.com.au, australianit.com.au, zdnet.* etc) abuse
http-equiv refresh in a way that makes tabbed browsing (a feature amazingly
handy for news sites) very annoying to use. Under the scheme you propose, the
user would have to switch to each tab and then hit the stop button after each
loads. Not very practical.

Ideally, we'd be able to click "stop" to prevent reloads, or alternately set
some sites on a blacklist where HTTP refresh headers and META refresh tags with
times above x seconds are disabled. "x" would be a system pref (or maybe just
hardcode to something like 10 to handle even slow "this site has moved" page
refreshs).

I don't think a stop button enhancement is a full fix for this, since its mostly
a problem when the user loads many pages then proceeds to read them in turn. A
real PITA since on the 3rd or so page, it'll reload mid-read...

Comment 25

[Mason]

http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#3776
A good place to start?

Comment 26

[Bill Mason]

*** Bug 219921 has been marked as a duplicate of this bug. ***

Comment 27

[Daniel B.]

> ------- Additional Comment #4 From Chris Bitmead  2001-06-16 05:40 -------
> 
> I think it would be best if this feature could also be customised on a site
> by site basis just like the reject cookies feature. 

By the way, these various site-sensitive features (e.g., blocking cookies, 
blocking images, remembering passwords, etc.) should use a common framework.
Then when we need the capability of not treating every page in one whole site 
the same way, when that capability gets added, all features inherit the same
new capabability.

Is there any work in that direction yet?

Comment 28

[Jon Henry]

*** Bug 207053 has been marked as a duplicate of this bug. ***

Comment 29

[Damian Yerrick]

Adding some summary words to make duplicate searching easier.  Heck, I
almost confirmed bug 207053 because this bug didn't have a obvious name.

There doesn't seem to be another RFE for adding 'Advanced > Scripts & Plugins >
Allow scripts to > Change location in response to an automatic event', so I'm
going to write a test case and file another bug.

Comment 30

[Damian Yerrick]

...and that would be bug 220118.

Comment 31

[Stewart Gordon]

I'm puzzled.  This previously came up as bug 69457, which was 'fixed' two months
before, and verified only 22 days before, this one was opened.  And there's no
sign of the feature now either.  So the fix was put in and then it vanished,
never to be seen again for three whole years?

Comment 32

[Daniel Veditz [:dveditz]]

bug 69457 added an embedding API that lets this feature be turned off by the
encompassing application. This bug is about making our own browser actually set
that property.

Comment 33

[Stewart Gordon]

So that explains the mysterious component on the other bug ... the bug otherwise
looked like an ordinary browser bug.  Now, finally, each bug knows of the
other's existence....

Comment 34

[Aaron Slunt]

Not sure if this was already mentioned, but redirection headers (Location: url)
should be able to be blocked as well.

Comment 35

[OstGote!]

*** Bug 296569 has been marked as a duplicate of this bug. ***

Comment 36

[Elmar Ludwig]

*** Bug 297115 has been marked as a duplicate of this bug. ***

Comment 37

[Prognathous]

RefreshBlocker is a Firefox Extension that prevents the "browser from following
the forwarding specified by the refresh parameter of the META tag element. If
such a refresh parameter is given on a web page, an infobox will be displayed at
the top of the page."

https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=992

Prog.

Comment 38

[OstGote!]

*** Bug 256982 has been marked as a duplicate of this bug. ***

Comment 39

[Felix Miata]

*** Bug 321627 has been marked as a duplicate of this bug. ***

Comment 40

[Daniel Kabs, reporting bugs since 2002]

Workaround:

If you want do disable "Meta Redirects", install "Web Developer" extension. It's is an essential extension for any web developer/designer as it provides a raft of incredibly useful features all under one roof:

https://addons.mozilla.org/extensions/moreinfo.php?id=60&application=firefox

or visit the Web Developer site:

http://chrispederick.com/work/webdeveloper/

Comment 41

[Mark Pilgrim (inactive)]

Beltzner and I discussed this briefly at CSUN, and since then I've spoken with
Aaron Leventhal about it.  Here's my current thinking:

- one global checkbox in Tools/Options/Content, "Disable automatic
refresh" or similar
- no site-specific exceptions
- autorefresh is enabled by default (like FF1.5)
- if autorefresh is disabled and you hit a page that contains a META
refresh tag, show an info bar (gBrowser.showMessage I think) -- like
the popup blocking info bar
- info bar says "Firefox prevented this site from automatically
refreshing" with a single button labeled "Refresh" that goes to the
refresh URL.

Reasoning:

- Automatic refresh is annoying for screenreader users, since the page
starts over reading at the top
- Some sites (like Blogger) rely on META refresh to do redirects of
untrusted links in comments (through http://www.google.com/sa?url=...
I think, which is a blank page with nothing but a META refresh tag).
This means we can't just silently disable the refresh
(docShell.allowMetaRedirects=false) because then the user can get
stuck.  We need a visible UI to activate the refresh.
- The info bar is accessible (exposed as an alert:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#260
) but relatively visually unobtrusive and modeless.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #41)
> - one global checkbox in Tools/Options/Content, "Disable automatic
> refresh" or similar

"Warn me when a page tries to refresh or redirect me" work better, I think, if the disable action actually just pops an info bar. 

I like this approach to the problem, as mentioned at CSUN. If it's a major nuisance for accessibility reasons, it's worth a pref in my book. I'm not sure how valid the original reporter's concern about abuse is, though - does anyone have a sense of that? If it's not a generally requested option, we might want to put it in Prefs > Advanced > Accessibility or just make it part of a master "Optimize Firefox for Screen Readers" pref.

cheers,
mike

Comment 43

[Darren Salt]

Ref. comment 42: I've seen pages refreshed while I've been reading them due to HTTP-EQUIV=refresh, though these days I normally make use of the permissions provided by Tabbrowser Extensions.

The original comment would appear to cover things like the reload causing redialling or an extra charge for exceeding a download quota. Obviously, caching helps here :-)

BTW, something in comment 18 which I'd forgotten that I'd not answered: Choices:WWW.Browse.Choices contains an option "ClientPull" which can be used to disable automatic (re)fetching of pages. A nearby comment includes the text "e.g. you didn't have time to read the page you were on before something else loaded".

Comment 44

[Aaron Leventhal]

(In reply to comment #42)
> "Optimize Firefox for Screen Readers" pref.
It's not just about screen reader users -- also people with mobility impairments, learning disabilities and cognitivie disabilities.

If it was just for screen reader users we could just watch to see if a screen reader was running. At least we can detect that on windows via nsILookAndFeel.

Comment 45

[Mark Pilgrim (inactive)]

(In reply to comment #41)
> - one global checkbox in Tools/Options/Content, "Disable automatic
> refresh" or similar
> - no site-specific exceptions
> - autorefresh is enabled by default (like FF1.5)
> - if autorefresh is disabled and you hit a page that contains a META
> refresh tag, show an info bar (gBrowser.showMessage I think) -- like
> the popup blocking info bar
> - info bar says "Firefox prevented this site from automatically
> refreshing" with a single button labeled "Refresh" that goes to the
> refresh URL.

I shopped this idea around with some screenreader users and vendors, and they are all enthusiastic about this specific approach.  Also, one clarification to a question that Gavin asked in IRC: when should we show the info bar?  My informal poll of screenreader users showed overwhelming support for showing the info bar as early as possible as the page is loading (as opposed to, for example, waiting until the refresh timer expires to show the info bar just as the page would have been refreshed).

Comment 46

[Mark Pilgrim (inactive)]

Attachment: implement refresh block for 1.8 branch

trunk patch is coming in a few days

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220172 [details] [diff] [review]
implement refresh block for 1.8 branch

>Index: uriloader/base/nsIWebProgressListener2.idl
>+   * Notification that the page has requested an automatic refresh or redirect
>+   * via a <meta http-equiv="refresh"> or an HTTP Refresh: header.

How about:

  Notification that a refresh or redirect has been requested in aWebProgress ...

As in, no "the page".  This interface doesn't really know about "pages", nor should it, imo.

Furthermore, things other than "the page" can request refreshes or redirects; this code just doesn't know who did the requesting.

>+   *        <code>true</code> if the page is requesting a refresh of the
>+   *        current URI.
>+   *        <code>false</code> if the page is requesting a redirection to
>+   *        a different URI.

Similar modifications here.  

I'd take out the <code>/</code> stuff in all these comments.

>+   * @see nsIWebProgressListener_MOZILLA_1_8_BRANCH::onRefreshAttempted

Isn't that what I'm looking at right now?  If so, what does that comment mean?  ;)

>Index: uriloader/base/nsDocLoader.cpp
>+nsDocLoader::RefreshAttempted(nsIWebProgress* aWebProgress,

>+   * Operate the elements from back to front so that if items

s/operate/iterate/?

>+    nsCOMPtr<nsIWebProgressListener_MOZILLA_1_8_BRANCH> listener;
>+    listener = do_QueryReferent(info->mWeakListener);
>+    if (!listener) {
>+      // the listener went away. gracefully pull it out of the list.
>+      mListenerInfoList.RemoveElementAt(count);
>+      delete info;
>+      continue;

No, this is no good.  While listeners have to implement nsIWebProgressListener (and hence failure to QI to indicates the object is gone), failure to QI to 
nsIWebProgressListener_MOZILLA_1_8_BRANCH means nothing.  If you want to do this list-trimming here, it needs to be based on QI to nsIWebProgressListener.

>+    nsresult listenerRV;
>+    listenerRV = listener->OnRefreshAttempted(aWebProgress,

Why declare on a separate line?  Just do:

  nsresult listenerRV = listener->OnRefreshAttempted(aWebProgress,

If that makes the lines too long, then:

  nsresult listenerRV = 
    listener->OnRefreshAttempted(aWebProgress,

>+nsBrowserStatusFilter::OnProgressChange64(nsIWebProgress *aWebProgress,

>+    nsCOMPtr<nsIWebProgressListener2> progressListener2;
>+    progressListener2 = do_QueryInterface(mListener);
>+    if (!progressListener2)
>+        return NS_OK;

That's no good, sorry.  That will, generally speaking, drop on the floor progress for things that only implement nsIWebProgressListener.

Here's what I think you want to do here:

1)  Have OnProgressChange just call OnProgressChange64
2)  Have OnProgressChange64 call either OnProgressChange or OnProgressChange64 on
    mListener, depending on whether mListener QIs to nsIWebProgressListener2.

Alternately, just have OnProgressChange64 call OnProgressChange and not really
handle large data sets well.  That's probably easier to implement; in that case file a separate bug on making this class 4GB-happy.

>Index: toolkit/content/widgets/tabbrowser.xml

>+            onProgressChange64 : function (aWebProgress, aRequest,

>+                    p.onProgressChange64(aWebProgress, aRequest,

That's no good either -- that suddenly requires consumers to implement nsIWebProgressListener2.  Again, the simple way to deal here is to call onProgressChange and file a followup bug to make the code better wrt to large data.

>+            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)

Is there a reason we don't notify our listeners to give them a chance to do something interesting here?  I guess that's up to mconnor...  I didn't really review this method; I don't know much about what sort of code is ok here.

That said, it doesn't look like this does the same thing as our current refresh code with referrers; do you care?

>Index: toolkit/locales/en-US/chrome/global/tabbrowser.properties
>\ No newline at end of file

Fix this while here?

Didn't really review the XUL and DTD changes either.

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

some short comments, not the real review yet:
uriloader/base/nsIWebProgressListener2.idl

when porting this to trunk, remember to change the interface description of nsIWebProgressListener2, which sucks (which is my fault)

please consistently use uppercase URI

+   * @param aRefreshUri
+   *        The refresh URI.

I don't think this is a very helpful description, how about mentioning that this is the URI that will be the target of the refresh?

Comment 49

[Mark Pilgrim (inactive)]

Attachment: implement refresh block for 1.8 branch (bz's comments integrated)

Integrated bz's feedback from comment 47 and biesi's feedback from comment 48.

Note: currently, automatic (unblocked) meta refreshes do not send a referrer, so my patch also does not send a referrer when meta-refeshing manually.  See bug 266554.

Comment 50

[Mark Pilgrim (inactive)]

Also note: I took bz's advice and made onProgressChange64 call onProgressChange.  Once this patch is checked in, I will open a new bug to find a better solution to that.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> Note: currently, automatic (unblocked) meta refreshes do not send a referrer,

That's not the point.  They don't _send_ it, but they _set_ it.   As you said, see bug 266554, where it's clearly explained why we need to do this.  Your patch as written will regress bug 264560, no?

Comment 52

[Mark Pilgrim (inactive)]

Attachment: implement refresh block for trunk

Changes from previous patch:
- Implemented on trunk.  Giving up on 1.8 branch for now.
- Integrated new method directly onto nsIWebProgressListener2
- Added method on nsIRefreshURI, forceRefreshURI, which contains the logic previously in nsRefreshTimer::Notify.  tabbrowser.xml now calls forceRefreshURI instead of loadURI so that internal referrers are set correctly during a manual refresh.
- Added a boolean parameter, aForceRefresh, to nsDocShell::RefreshURI.  If true, it immediately calls ForceRefreshURI instead of creating a timer.

I think I patched all the files that implement nsIWebProgressListener2, but I don't know how to test (or even compile) the embedded stuff like photon and powerplant.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Is that patch missing files?  I don't see nsDocShell.h or nsIRefreshURI.idl; not sure what else should be there.

Also, why do the aDelay <= REFRESH_REDIRECT_TIMER at both callers of ForceRefreshURI()?  Why not just pass aDelay to that method and have one copy of the logic?

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 220727 [details] [diff] [review]
implement refresh block for trunk

uriloader/base/nsIWebProgress.idl
+   * NOTIFY_REFRESH
+   *   Receive onRefreshAttempted events.

I wonder if it would be good to mention that this is on nsIWPL2, contrary to all the others?

uriloader/base/nsIWebProgressListener2.idl

I know that this is my fault, but could you do something to the description of this interface? It is now inaccurate.. maybe just something like:

/**
 * An extended version of the nsIWebProgressListener interface.
 */

uriloader/base/nsDocLoader.cpp
+    nsCOMPtr<nsIWebProgressListener> listener;
+    listener = do_QueryReferent(info->mWeakListener);

please merge these two lines, and also the two listener2 lines.

docshell/base/nsDocShell.cpp
+    if (!allowRedirects)
+      return NS_OK;
+
+    if (aForceRefresh) {
+        PRBool loadReplace = PR_FALSE;

inconsistent indentation here...

so forcing a refresh doesn't actually force it if refreshes are disabled? I guess that makes sense. should be documented on the interface.

+    CreateLoadInfo(getter_AddRefs(loadInfo));
+    NS_ENSURE_TRUE(loadInfo, NS_OK);

NS_OK? I think that you should change this; it was ok for Notify() but not this method.

stopping here, I'll continue once you attach the new version...

Comment 55

[Mark Pilgrim (inactive)]

Attachment: implement refresh block for trunk

Included missing nsIRefreshURI.idl.  I don't believe that nsDocShell.h has changed (at least cvs diff tells me it hasn't).  Were you expecting it to?  The new method is defined in nsIRefreshURI.idl, and nsDocShell implements no new interfaces.

Also incorporated all of biesi's feedback to date.

Comment 56

[Mark Pilgrim (inactive)]

I'm updating the status whiteboard field so I can run an internal report.  Don't interpret this as impatience; I know things are crazy right now on trunk.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221335 [details] [diff] [review]
implement refresh block for trunk

>Index: webshell/public/nsIRefreshURI.idl

You need to update the IID here.

So what's the point of having an aForceRefresh arg to refreshURI if all it does is ignore the aRepeat arg and call forceRefreshURI which is also on nsIRefreshURI?  It seems that either you add the forceRefreshURI method or you add the aForceRefresh arg to refreshURI, but not both.... In either case you'll want an nsDocShell::ForceRefreshURI, but you'd just declare it in nsDocShell if you change the refreshURI signature.

>Index: uriloader/base/nsIWebProgressListener2.idl

And here.

>Index: uriloader/base/nsDocLoader.h

>+#include "nsIWebProgressListener2.h"

You don't need this include in the header, do you?  Just the .cpp should be fine.

>Index: docshell/base/nsDocShell.cpp


> nsRefreshTimer::Notify(nsITimer * aTimer)
>+            refreshURI->ForceRefreshURI(mURI, delay, (PRBool)mMetaRefresh);

Why do you need the cast?  It should Just Work without the cast.

>Index: xpfe/browser/src/nsBrowserStatusFilter.cpp
>+nsBrowserStatusFilter::OnRefreshAttempted(nsIWebProgress *aWebProgress,
>+                                          nsIURI *aUri,
>+					  PRInt32 aDelay,
>+					  PRBool aSameUri,
>+					  PRBool *allowRefresh)

Weird indent here... tabs or something?

>+    if (!mListener)
>+        return NS_OK;

No need for that null-check.

>+    nsCOMPtr<nsIWebProgressListener2> listener;
>+    listener = do_QueryInterface(mListener);

Why not:

  nsCOMPtr<nsIWebProgressListener2> listener =
    do_QueryInterface(mListener);

?

>+    return listener->OnRefreshAttempted(aWebProgress, aUri, aDelay, aSameUri,
>+					allowRefresh);

Weird indent.

>Index: toolkit/content/widgets/tabbrowser.xml

This (andthe other toolkit and browser stuff) is going to need review from a toolkit peer (mconnor or someone).

>+              if (!this.mTabBrowser.mPrefs.getBoolPref("accessibility.blockautorefresh", false))

That looks wrong (too many args, doesn't deal with the pref not being set, etc).

>Index: toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
>+        onRefreshAttempted: function( aWebProgress, aURI, aDelay, aSameURI ) {
>+	    return true;
>+	}

Weird indent.

sr- because I'd like to get the API stuff sorted out, but this is real close!

Comment 58

[Mark Pilgrim (inactive)]

Attachment: Implement refresh block for trunk

Incorporated bz's feedback from comment 57.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 222191 [details] [diff] [review]
Implement refresh block for trunk

> +              if (this.mTabBrowser.mPrefs.getBoolPref("accessibility.blockautorefresh")) {

This still doesn't handle the pref not being set; it'll throw an exception if that happens...  Fix that, please.

sr=bzbarsky with that, and make sure to get a toolkit peer review on the toolkit changes (use the "addl. review" field).

Also, note that the change to nsHelperAppDlg.js.in is probably unneccessary, since that object doesn't QI to nsIWebProgressListener2 (or to anything, for that matter).

Comment 60

[Mark Pilgrim (inactive)]

Attachment: Implement refresh block for trunk

Added pref check as recommended in comment 59.

Comment 61

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 222219 [details] [diff] [review]
Implement refresh block for trunk

webshell/public/nsIRefreshURI.idl
+      * Loads a URI immediately.

Hm, this kind of sounds like it's equivalent to nsIWebNavigation::loadURI. Maybe it should make it clear that it loads the URI as if it was a refresh.

uriloader/base/nsIWebProgressListener2.idl

Might be good to document what happens if onRefreshAttempted throws an exception

uriloader/base/nsDocLoader.cpp
+    nsCOMPtr<nsIWebProgressListener> listener;
+    listener = do_QueryReferent(info->mWeakListener);

like I said above, could you make this:

nsCOMPtr<nsIWebProgressListener> listener =
    do_QueryReferent(info->mWeakListener);

similar for this one:
+    nsCOMPtr<nsIWebProgressListener2> listener2;
+    listener2 = do_QueryReferent(info->mWeakListener);

docshell/base/nsDocShell.cpp
+        nsCOMPtr<nsIRefreshURI> refreshURI = do_QueryInterface(mDocShell);
+        if (refreshURI)
+            refreshURI->ForceRefreshURI(mURI, delay, mMetaRefresh);

you could make mDocShell an nsIRefreshURI now, I think, to avoid this QI.

toolkit/content/widgets/tabbrowser.xml
+            onProgressChange64 : function (aWebProgress, aRequest,
+                                         aCurSelfProgress, aMaxSelfProgress,
+                                         aCurTotalProgress, aMaxTotalProgress)

I'd have made onProgressChange call the 64-bit one, although I actually think that here in JS, the result is exactly the same.


don't you need to patch camino as well?

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, and I didn't really review the UI changes, so get a toolkit peer to review them :)

Comment 63

[Mark Pilgrim (inactive)]

Attachment: Implement refresh block for trunk

Incorporated biesi's review from comment 61, except having tabbrowser's onProgressChange64 calling the 64-bit one, which bz specifically rejected in comment 47.

I don't know whether I need to patch Camino, nor do I know how to do that.

Adding beltzner for UI review.  Mike, if you're busy, could you please redirect review to someone else?

Comment 64

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #63)
> Incorporated biesi's review from comment 61, except having tabbrowser's
> onProgressChange64 calling the 64-bit one, which bz specifically rejected in
> comment 47.

That's not what I meant. onProgressChange64 could call listener's onProgressChange method. but this works too.

> I don't know whether I need to patch Camino, nor do I know how to do that.

Just like all the other implementors of nsIWebProgressListener2. See http://lxr.mozilla.org/mozilla/source/camino/src/download/nsDownloadListener.mm#207 and maybe http://lxr.mozilla.org/mozilla/source/camino/src/embedding/CHBrowserListener.mm#557

(i.e. add the onRefreshAttempted method)

Comment 65

[Mark Pilgrim (inactive)]

Attachment: Additional patch for Camino

Implemented new onRefreshChanged method in the two Camino classes that implement nsIWebProgressListener2.  mento asked that this be posted as a separate patch.

Comment 66

[Mark Mentovai]

Comment on attachment 222234 [details] [diff] [review]
Additional patch for Camino

Looks good.  Filed bug 338181 to flesh out the Camino implementation.

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 222226 [details] [diff] [review]
Implement refresh block for trunk

ui-r=me, passing to mconnor for toolkit code review. Thanks, Mark!

Comment 68

[Mark Pilgrim (inactive)]

Attachment: unbitrotted patch (otherwise identical to attachment 222226)

Comment 69

[Asaf Romano (gone)]

Comment on attachment 243790 [details] [diff] [review]
unbitrotted patch (otherwise identical to attachment 222226 [details] [diff] [review])

>Index: browser/components/preferences/advanced.xul
>===================================================================

>       <preference id="browser.cache.disk.capacity"     name="browser.cache.disk.capacity"     type="int"/>
>@@ -138,16 +139,19 @@
>             <checkbox id="useCursorNavigation"
>                       label="&useCursorNavigation.label;"
>                       accesskey="&useCursorNavigation.accesskey;"
>                       preference="accessibility.browsewithcaret"/>
>             <checkbox id="searchStartTyping"
>                       label="&searchStartTyping.label;"
>                       accesskey="&searchStartTyping.accesskey;"
>                       preference="accessibility.typeaheadfind"/>
>+            <checkbox id="blockAutoRefresh" label="&blockAutoRefresh.label;"

nit: one attribute per line.

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

>+            manualRefresh : function(aNotification, aButton)
>+            {
>+              var refreshURI = aButton.docShell.QueryInterface(Components.interfaces.nsIRefreshURI);
>+              refreshURI.forceRefreshURI(aButton.refreshURI, aButton.delay, true);
>+            },
>+
>+            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)

A better plan might be to forward this calls to the tabbrowser's progress listeners (like we do for onLocationChange for instance where we handle other notification, then you could rely on browser/ stuff.

>+            {
>+              var blockRefresh;
>+              try {
>+                blockRefresh = this.mTabBrowser.mPrefs.getBoolPref("accessibility.blockautorefresh");
>+              } catch (e) {
>+                blockRefresh = false;
>+              }

There's no need to catch exceptions here, the pref is set in all.js


>+              if (blockRefresh) {
>+                var brandBundle = document.getElementById("bundle_brand");

You cannot rely on browser.xul elements in a toolkit binding.

>+                var brandShortName = brandBundle.getString("brandShortName");
>+                var refreshButtonText = this.mTabBrowser.mStringBundle.getString("refreshBlocked.goButton");
>+                var refreshButtonAccesskey = this.mTabBrowser.mStringBundle.getString("refreshBlocked.goButton.accesskey");

General comment: try to wrap your lines at 80 characters or less please.

>+                var notificationBox = this.mTabBrowser.getNotificationBox();
...
>+                else {
>+                  var buttons = [{
...
>+                    docShell: this.mTabBrowser.docShell
>+                  }];

This code assumes auto-refresh can only be triggered in the current browser, you need the notificationbox and the docshell of the browser in which auto-refresh has been triggered (Another issue to think of: frames).

>+                  const priority = notificationBox.PRIORITY_INFO_MEDIUM;
>+                  notificationBox.appendNotification(message, "refresh-blocked",
>+                                                     "chrome://browser/skin/Info.png",
>+                                                     priority, buttons);

another browser/ dependency.

Comment 70

[Mark Pilgrim (inactive)]

Attachment: patch with mano's feedback

Moved all browser-specific code to browser.js, made tabbrowser.xml just generically call all progress listeners in onRefreshAttempted.  Also added interface to reporterOverlay.js, because it listens and onRefreshAttempted wasn't defined so it was erroring out in tabbrowser.xml.  (Not sure if that's new or we just missed it before.)  Fixed nits.

Comment 71

[Asaf Romano (gone)]

Comment on attachment 244535 [details] [diff] [review]
patch with mano's feedback

You're still using the selected browser's notificationbox.

Comment 72

[Asaf Romano (gone)]

So this always allows auto-refresh in background tabs?

if (this.mTabBrowser.mCurrentTab == this.mTab) {

This really isn't desired.

Comment 73

[Mark Pilgrim (inactive)]

Attachment: patch to pass correct browser to getNotificationBox

As discussed in IRC, I added a global function in utilityOverlay.js: getBrowserFromContentWindow.  This patch now uses this function to get the proper browser to pass to getNotificationBox, and also to get the proper docshell for the callback if the end user presses the "Allow" button later.

Comment 74

[Asaf Romano (gone)]

Comment on attachment 244806 [details] [diff] [review]
patch to pass correct browser to getNotificationBox

>Index: browser/base/content/browser.js
>===================================================================

>+  manualRefresh : function(aNotification, aButton)
>+  {
>+    var refreshURI = aButton.docShell.QueryInterface(
>+      Components.interfaces.nsIRefreshURI);

nit: make this
>+    var refreshURI = aButton.docShell.QueryInterface(Ci.nsIRefreshURI);

>+    refreshURI.forceRefreshURI(aButton.refreshURI, aButton.delay, true);
>+  },
>+
>+  onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
>+  {
>+    var blockRefresh = gPrefService.getBoolPref(
>+      "accessibility.blockautorefresh");
>+    if (blockRefresh) {

or just |if (gPrefService.getBoolPref("accessibility.blockautorefresh"))
\
>+      var refreshButtonText = 
>+        gNavigatorBundle.getString(
>+          "refreshBlocked.goButton");
>+      var refreshButtonAccesskey = 
>+        gNavigatorBundle.getString(
>+          "refreshBlocked.goButton.accesskey");

nit: make this
>+      var refreshButtonText =
>+         gNavigatorBundle.getString("refreshBlocked.goButton");
>+      var refreshButtonAccesskey = 
>+        gNavigatorBundle.getString("refreshBlocked.goButton.accesskey");

>+      var message;
>+      if (aSameURI)
>+        message = gNavigatorBundle.getFormattedString(
>+          "refreshBlocked.refreshLabel", [brandShortName]);
>+      else
>+        message = gNavigatorBundle.getFormattedString(
>+          "refreshBlocked.redirectLabel", [brandShortName]);
>+      var browser = getBrowserFromContentWindow(aWebProgress.DOMWindow.top);
>+      var notificationBox = gBrowser.getNotificationBox(browser);
>+      var notification = notificationBox.getNotificationWithValue(
>+        "refresh-blocked");
>+      if (notification) {
>+        notification.label = message;
>+      }
>+      else {
>+        var buttons = [{
>+          label: refreshButtonText,
>+          accessKey: refreshButtonAccesskey,
>+          callback: this.manualRefresh,

Hrm, maybe inline the callback function?

>+          refreshURI: aURI,
>+          delay: aDelay,
>+          docShell: browser.docShell

This is wrong for the frame/iframe case; you need the docshell of aWebProgress.DOMWindow itself (not aWebProgress.DOMWindow.top).


>Index: browser/base/content/utilityOverlay.js
>===================================================================

>+
>+function getBrowserFromContentWindow(aContentWindow)
>+{
>+  var browsers = gBrowser.browsers;
>+  var browser;
>+  for (var i = 0; i < browsers.length; i++)
>+    if (browsers[i].contentWindow == aContentWindow) {
>+      browser = browsers[i];
>+      break;
>+    }
>+  return browser;

return inside the if block instead.

>Index: browser/locales/en-US/chrome/browser/browser.properties
>===================================================================

> # tab context menu additions
> tabContext.undoCloseTab=Undo Close Tab
> tabContext.undoCloseTabAccessKey=U
> 
> # History menu
> menuOpenInTabs.label=Open in Tabs
> menuOpenInTabs.accesskey=o
>+
>+# Block autorefresh
>+refreshBlocked.goButton=Allow
>+refreshBlocked.goButton.accesskey=o

Isn't "A" better?

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

>+            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
>+            {
>+              var allowRefresh = true;
>+              for (var i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
>+                var p = this.mTabBrowser.mProgressListeners[i];
>+                if (p)
>+                  if (!p.onRefreshAttempted(aWebProgress, aURI, aDelay, aSameURI))

Make sure there's "onRefreshAttempted" in p here before you're calling it.

>+                    allowRefresh = false;
>+              }
>+              return allowRefresh;

I don't think "last-wins" is the right model here.

Comment 75

[Mark Pilgrim (inactive)]

Attachment: nits

Nits addressed.  However, I disagree with your characterization that this is "last-wins".  It is not.  The algorithm is "everyone who is listening gets notified, but one vote against refresh is enough to block it."  Which is, unless I am very much mistaken, what the code does already.

Comment 76

[Mark Pilgrim (inactive)]

Attachment: nits 2

Sorry, missed one nit (testing p.onRefreshAttempted before calling).

Comment 77

[Asaf Romano (gone)]

Comment on attachment 248419 [details] [diff] [review]
nits 2

>Index: browser/base/content/browser.js
>===================================================================

>   statusText: "",
>   lastURI: null,
> 
>   statusTimeoutInEffect : false,
> 
>   QueryInterface : function(aIID)
>   {
>     if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
>+        aIID.equals(Components.interfaces.nsIWebProgressListener2) ||
>         aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
>         aIID.equals(Components.interfaces.nsIXULBrowserWindow) ||
>         aIID.equals(Components.interfaces.nsISupports))
>       return this;
>     throw Components.results.NS_NOINTERFACE;

mind to switch this method to use the Ci/Cr constants while you're here?

>   onStateChange : function(aWebProgress, aRequest, aStateFlags, aStatus)
>   {
>     const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>     const nsIChannel = Components.interfaces.nsIChannel;
>     if (aStateFlags & nsIWebProgressListener.STATE_START) {
>         // This (thanks to the filter) is a network start or the first
>         // stray request (the first request outside of the document load),
>         // initialize the throbber and his friends.
>@@ -3924,16 +3934,65 @@ nsBrowserStatusHandler.prototype =
>   },
> 
>   onStatusChange : function(aWebProgress, aRequest, aStatus, aMessage)
>   {
>     this.status = aMessage;
>     this.updateStatusField();
>   },
> 
>+  onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
>+  {
...
>+      var topBrowser = getBrowserFromContentWindow(aWebProgress.DOMWindow.top);
>+      var notificationBox = gBrowser.getNotificationBox(topBrowser);
>+      var notification = notificationBox.getNotificationWithValue(
>+        "refresh-blocked");
>+      if (notification) {
>+        notification.label = message;
>+      }
>+      else {
>+        var browser = getBrowserFromContentWindow(aWebProgress.DOMWindow);
>+        var buttons = [{
>+          label: refreshButtonText,
>+          accessKey: refreshButtonAccesskey,
>+          callback: function(aNotification, aButton) {
>+            var refreshURI = aButton.docShell.QueryInterface(Ci.nsIRefreshURI);
>+            refreshURI.forceRefreshURI(aButton.refreshURI, aButton.delay, true);
>+          },
>+          refreshURI: aURI,
>+          delay: aDelay,
>+          docShell: browser.docShell
>+        }];

what's the topBrowser vs. browser thing? browser would be null in the frame/iframe case... here you need the window's docshell, not its browser.

Something like this should work

var docShell = aWebProgress.DOMWindow
                           .QueryInterface(Ci.nsIInterfaceRequestor)
                           .getInterface(Ci.nsIWebNavigation)
                           .QueryInterface(Ci.nsIDocShell);


There's also the edge case of auto-refresh requests in different content windows, you should at least update the button's docshell for this case if there's already a "refresh-blocked" notification open.

>Index: browser/base/content/utilityOverlay.js
>===================================================================

>@@ -519,8 +519,19 @@ function isElementVisible(aElement)
>   // * When an element is hidden, the width and height of its boxObject
>   //   are set to 0
>   // * css-visibility (unlike css-display) is inherited.
>   var bo = aElement.boxObject;
>   return (bo.height != 0 && bo.width != 0 &&
>           document.defaultView
>                   .getComputedStyle(aElement, null).visibility == "visible");
> }
>+
>+function getBrowserFromContentWindow(aContentWindow)
>+{
>+  var browsers = gBrowser.browsers;
>+  var browser;
>+  for (var i = 0; i < browsers.length; i++)
>+    if (browsers[i].contentWindow == aContentWindow) {
>+      browser = browsers[i];
>+      return browser;
>+    }
>+}

make this
>+function getBrowserFromContentWindow(aContentWindow)
>+{
>+  var browsers = gBrowser.browsers;
>+  for (var i = 0; i < browsers.length; i++) {
>+    if (browsers[i].contentWindow == aContentWindow)
>+      return browsers[i];
>+  }
>+}

Comment 78

[Mark Pilgrim (inactive)]

Attachment: nits-3

Comment 79

[Asaf Romano (gone)]

Comment on attachment 251671 [details] [diff] [review]
nits-3

Quote: "There's also the edge case of auto-refresh requests in different content
windows, you should at least update the button's docshell for this case if
there's already a "refresh-blocked" notification open."

Comment 80

[Mark Pilgrim (inactive)]

(In reply to comment #79)
> Quote: "There's also the edge case of auto-refresh requests in different
> content
> windows, you should at least update the button's docshell for this case if
> there's already a "refresh-blocked" notification open."

(Couldn't catch you on IRC.)  I'm sorry, I don't know what this means.  Can you explain this further?

Comment 81

[Mark Pilgrim (inactive)]

Note to self: Mano says the problem is with framed documents where two frames define different meta refreshes.  In this case, the onRefreshAttempted will get called twice, but the second time, this code path will run:

if (notification) {
  notification.label = message;
}

This will leave the notification in an inconsistent state.  The label will mention the second frame's refresh URL, but clicking the "Allow" button will redirect to the first frame's refresh URL.  When we update notification.label with the second frame's message, we also need to find the "Allow" button and update its refreshURI and delay to the values defined by the second frame.

Comment 82

[Mark Pilgrim (inactive)]

Attachment: Update docshell on subsequent refresh notifications

I had to add a "buttons" property on the notification binding, because there was no existing clean way to get a reference to the buttons of an existing notification (without DOM scraping, which seemed ugly).  AFAICT, notificationbox.appendNotification is the only place that creates new notifications, so that's the only place I set the notification.buttons property.

Comment 83

[Mark Pilgrim (inactive)]

Attachment: test case for update-docshell patch

This is a test case for the update docshell patch that I just posted.  index.html contains a frameset with frame1.html and frame2.html.  Before the latest patch, clicking "Allow" would update frame 1 with frame 2's refresh URI.  With the latest patch, clicking "Allow" updates frame 2 with frame 2's refresh URI.

Comment 84

[Asaf Romano (gone)]

Comment on attachment 253430 [details] [diff] [review]
Update docshell on subsequent refresh notifications

>Index: browser/base/content/browser.js
>===================================================================

> 
>+  onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
>+  {
>+    if (gPrefService.getBoolPref("accessibility.blockautorefresh")) {
>+      var brandBundle = document.getElementById("bundle_brand");
>+      var brandShortName = brandBundle.getString("brandShortName");
>+      var refreshButtonText = 
>+        gNavigatorBundle.getString("refreshBlocked.goButton");
>+      var refreshButtonAccesskey = 
>+        gNavigatorBundle.getString("refreshBlocked.goButton.accesskey");
>+      var message;
>+      if (aSameURI)
>+        message = gNavigatorBundle.getFormattedString(
>+          "refreshBlocked.refreshLabel", [brandShortName]);
>+      else
>+        message = gNavigatorBundle.getFormattedString(
>+          "refreshBlocked.redirectLabel", [brandShortName]);
>+      var topBrowser = getBrowserFromContentWindow(aWebProgress.DOMWindow.top);
>+      var notificationBox = gBrowser.getNotificationBox(topBrowser);
>+      var notification = notificationBox.getNotificationWithValue(
>+        "refresh-blocked");
>+      if (notification) {
>+        notification.label = message;
>+        var button = notification.buttons[0];
>+        button.refreshURI = aURI;
>+        button.delay = aDelay;
>+        button.docShell = docShell;

docShell is only set a couple of lines later though. ;)

>+      }
>+      else {
>+        var docShell = aWebProgress.DOMWindow
>+                                   .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                   .getInterface(Ci.nsIWebNavigation)
>+                                   .QueryInterface(Ci.nsIDocShell);




>Index: modules/libpref/src/init/all.js
>===================================================================

>+pref("accessibility.blockautorefresh", false);

firefox-specific preferences should be set in firefox.js.

>Index: toolkit/content/widgets/notification.xml
>===================================================================

>+            newitem.buttons = aButtons;

>+      <property name="buttons">null</property>


1) This returns the buttons-info array, not the button elements. The later is much more useful since you could also change the each button's DOM (e.g. the button label). You can still access the button info with that change (buttons[0].buttonInfo).

2) You've written this property as a xbl field.

The following should work

  <property name="buttons"
            readonly="true">
    <getter>
      <![CDATA[
        return this.getElementsByTagName("button");
      ]]>
    </getter>
  </property>


I'm wondering whether we need this at all though (you could use getElementsByTagName in the browser/ code as well), maybe ask Neil?

Comment 85

[Mark Pilgrim (inactive)]

(In reply to comment #84)
> (From update of attachment 253430 [details] [diff] [review])
> docShell is only set a couple of lines later though. ;)

Bad code merge on my part, good catch on your part.

> >+pref("accessibility.blockautorefresh", false);
> 
> firefox-specific preferences should be set in firefox.js.

This is not a Firefox-specific preference.

> 1) This returns the buttons-info array, not the button elements. The later is
> much more useful since you could also change the each button's DOM (e.g. the
> button label). You can still access the button info with that change
> (buttons[0].buttonInfo).

Well, notificationbox.appendNotification takes an "aButtons" parameter, and browser.js calls it by filling out a "buttons" variable and passing it, so I don't see the inconsistency.  And we don't need to do any DOM manipulation on the <button> element itself, so I don't see why we need a reference to it.

> I'm wondering whether we need this at all though (you could use
> getElementsByTagName in the browser/ code as well), maybe ask Neil?

I considered that but rejected it as unnecessarily ugly (see my previous comment).  Why should the caller be forced to know specific internals of XUL elements that the notification XBL creates internally?

Comment 86

[Asaf Romano (gone)]

(In reply to comment #85)
> > >+pref("accessibility.blockautorefresh", false);
> > 
> > firefox-specific preferences should be set in firefox.js.
> 
> This is not a Firefox-specific preference.
> 

How is it not? It's only checked by browser.js.

> > 1) This returns the buttons-info array, not the button elements. The later is
> > much more useful since you could also change the each button's DOM (e.g. the
> > button label). You can still access the button info with that change
> > (buttons[0].buttonInfo).
> 
> Well, notificationbox.appendNotification takes an "aButtons" parameter, and
> browser.js calls it by filling out a "buttons" variable and passing it, so I
> don't see the inconsistency.  And we don't need to do any DOM manipulation on
> the <button> element itself, so I don't see why we need a reference to it.
> 

*You* don't need to, but if anyone ever wants to change the label of a button inside a notification for example, that would be the way to go. As noted, you could still access the button info object for your use-case via buttons[i].buttonInfo.

> > I'm wondering whether we need this at all though (you could use
> > getElementsByTagName in the browser/ code as well), maybe ask Neil?
> 
> I considered that but rejected it as unnecessarily ugly (see my previous
> comment).  Why should the caller be forced to know specific internals of XUL
> elements that the notification XBL creates internally?

Again, ask Neil. I myself don't like this addition (esp. if it doesn't give access the button's DOM).

Comment 87

[[e-mail]]

I'm not sure which Neil Mano meant, but I don't see why you need to access "buttons" at all - I'd add your extra info to the notification object.

Comment 88

[Asaf Romano (gone)]

Either ;)

That works too.

Comment 89

[Mark Pilgrim (inactive)]

Attachment: put properties on notification widget instead of button

also moved pref init to firefox.js

Comment 90

[Asaf Romano (gone)]

Comment on attachment 254302 [details] [diff] [review]
put properties on notification widget instead of button

very last nits:

>Index: browser/base/content/browser.js
>===================================================================

>+        var buttons = [{
>+          label: refreshButtonText,
>+          accessKey: refreshButtonAccesskey,
>+          callback: function(aNotification, aButton) {
>+            var refreshURI = aNotification.docShell.QueryInterface(
>+              Ci.nsIRefreshURI);
>+            refreshURI.forceRefreshURI(aNotification.refreshURI,
>+              aNotification.delay, true);

please make this
            var refreshURI = aNotification.docShell
                                          .QueryInterface(Ci.nsIRefreshURI);
            refreshURI.forceRefreshURI(aNotification.refreshURI,
                                       aNotification.delay, true);


>Index: browser/base/content/utilityOverlay.js
>===================================================================

>+function getBrowserFromContentWindow(aContentWindow)
>+{
>+  var browsers = gBrowser.browsers;
>+  for (var i = 0; i < browsers.length; i++) {
>+    if (browsers[i].contentWindow == aContentWindow)
>+      return browsers[i];
>+  }
>+}

return null at the end of this function to avoid adding yet another js warning.

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================
>+               var allowRefresh = true;
>+               for (var i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
>+                 var p = this.mTabBrowser.mProgressListeners[i];
>+                 if (p && p.onRefreshAttempted)

Make that |if (p && "onRefreshAttempted" in p)|. Please also add braces around this block for better readability.

r=mano otherwise.

Comment 91

[Asaf Romano (gone)]

And please do file a bug on improving the behavior for the frames case.

Comment 92

[Mark Pilgrim (inactive)]

Attachment: final patch

If someone with commit privs could check this in, I would be eternally grateful.

Comment 93

[Mark Pilgrim (inactive)]

Bug 369652 for handling the multiple frames edge case.

Comment 94

[Asaf Romano (gone)]

mozilla/browser/app/profile/firefox.js 1.170
mozilla/browser/base/content/browser.js 1.760
mozilla/browser/base/content/utilityOverlay.js 1.47
mozilla/browser/components/preferences/advanced.xul 1.35
mozilla/browser/locales/en-US/chrome/browser/browser.properties 1.34
mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd 1.22
mozilla/camino/src/download/nsDownloadListener.mm 1.29
mozilla/camino/src/embedding/CHBrowserListener.mm 1.38
mozilla/docshell/base/nsDocShell.cpp 1.823
mozilla/embedding/browser/cocoa/src/nsDownloadListener.mm 1.10
mozilla/embedding/browser/photon/src/EmbedDownload.cpp 1.4
mozilla/embedding/browser/powerplant/source/UDownload.cpp 1.15
mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js 1.71
mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.js 1.9
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 1.70
mozilla/toolkit/components/downloads/src/nsDownloadProxy.h 1.17
mozilla/toolkit/content/widgets/tabbrowser.xml 1.221
mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in 1.45
mozilla/uriloader/base/nsDocLoader.cpp 3.294
mozilla/uriloader/base/nsDocLoader.h 1.41
mozilla/uriloader/base/nsIWebProgress.idl 1.14
mozilla/uriloader/base/nsIWebProgressListener2.idl 1.2
mozilla/webshell/public/nsIRefreshURI.idl 1.9
mozilla/xpfe/browser/src/nsBrowserStatusFilter.cpp 1.14
mozilla/xpfe/browser/src/nsBrowserStatusFilter.h 1.5
mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp 1.121

Comment 95

[Bill Gianopoulos [:WG9s]]

This appears to be causing tinderbox redness on the new fxdbug-linux-tbox as well as Linux Nye suiteruner (seamonkey ports), Linux Argo (XULRunner) and all of the trunk Camino builds.

Comment 96

[Mats Palmgren (inactive)]

I've checked in a (typo) fix for Camino:
camino/src/embedding/CHBrowserListener.mm 1.39

Comment 97

[Olli Pettay [:smaug][[e-mail]]]

I checked in a fix for gtk embedding/browser/gtk/src/EmbedDownloadMgr.cpp

Comment 98

[Asaf Romano (gone)]

Smaug, Mats, thanks for taking care of this.

Comment 99

[Alex Romosan]

i think the fix for this bug broke the automatic page redirect on my 64-bit seamonkey cvs build (it works fine on my 32-bit laptop). the last version working for me was on 02/07 the day before the above patch was committed. i am trying to understand what exactly broke, but maybe somebody who understands the code better can come up with a fix. i am willing to try patches.

Comment 100

[Boris Zbarsky [:bzbarsky]]

Yeah, this caused bug 372426.

Comment 101

[Chris Bitmead]

Since someone has seen fit to close this bug I submitted, it would be nice if someone summarised how Firefox is now deemed to have solved this issue, because I haven't seen any options appear in Firefox preferences that are applicable.

BTW, as far as I see it is auto-refresh where timeout > 0 which is the problem. Presumably sites like blogger will redirect with 0 timeout.

Comment 102

[Samuel Sidler (old account; do not CC)]

In Firefox 3.0.x and above: Tools -> Options -> Advanced -> General -> "Warn me when web sites try to redirect or reload the page"

Comment 103

[Algot Runebjörk]

Well, what if I DON'T want to be warned about it, just have FireFox ignore the Meta Refresh tag quietly like IE does when META REFRESH is disabled?

Comment 104

[Aaron Leventhal]

Algot, you can fire a feature request for that. You'll need to justify it well (in that bug please) in order to get any traction on it.