112848 - Does your grandmother know what POSTDATA is?

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Jonas Jørgensen]

"The page you are trying to view contains POSTDATA. If you resend the data, any
action the form carried out (such as a search or online purchase) will be
repeated. To resend the data, click Ok. Otherwise, click Cancel."

That message is very confusing for normal people. Mr. End User & his dog,
neither of whom can tell the difference between DNS and DSL, surely have never
heard of POSTDATA. It should be changed to:

"The page you are trying to view was created using information you submitted in
a form. If you resend the data, any action the form carried out (such as a
search or online purchase) will be repeated. To resend the data, click Ok.
Otherwise, click Cancel."

Comment 1

[Ken Harris]

Why have extra instructions for what "OK" means in this context?
How about removing those last 2 sentences and calling the "OK" button
"Resend data"?

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing ui design

Comment 3

[Darin Fisher]

-> docshell

Comment 4

[Adam Lock]

It's just possible that the user DIDN'T submit a form (at least not knowingly),
so saying "using information you submitted in a form" is not correct either.
e.g. the Shields Up! https://grc.com/x/ne.dll?bh0bkyd2 page contains a hidden
form submitted as POSTDATA, but Joe User doesn't know that.

Comment 5

[Martin Sch]

This message appears nearly every minute while surfing and is very annoying. In
netscape, it only showed up in rare cases. Something is buggy in mozilla with
forms. See also bug 102407.

Comment 6

[Radha on family leave (not reading bugmail)]

It shouldn't appear everyminute you are surfing, unless

1) you are using NS6.0 where this dialog would appear for all postdata result
pages , irrespective of whether it is expired from cache or not. In NS6.1 and
above, the dialog would appear *only* if the postdata has expired from cache.

2) You are using NS 6.1 and above and are surfing heavily on pages that have
cache control: no-store or no-cache.

Comment 7

[Ian Pottinger]

adding self to cc list

Comment 8

[Adam Lock]

Reassigning to UI design

Comment 9

[Matthew T (active 1999-2002)]

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

Comment 10

[Jonas Jørgensen]

--> docshell.

User Interface Design component is being removed; see bug 167289.

Comment 11

[John Flynn]

Another thing that should be added to the POSTDATA message is a button that will
simply show the results of the last time the page was POSTed. It's very annoying
to perform a search, click on a link in the search results, then have to wait
for the search again when you try to go back. 

Sure, you could open the links on the search results in new windows or tabs, but
why not just have a button that will show you the results of the last POST?

Just a thought...

-John

Comment 12

[Brian 'netdragon' Bober]

Mind if I take this?

Comment 13

[Adam Lock]

Please do

Comment 14

[Brian 'netdragon' Bober]

Adam:

XUL Change:

"Information has already been posted automatically or by you in a form. This
request has expired, and has to be resent. If you resend the data, any action
the form carried out (such as a search or online purchase) will be repeated. To
resend the data, click Resend. Otherwise, click Cancel."

[Resend] [Cancel]

Sound good? Will the Resend and Cancel get swapped on the Macintosh? Is that
alright if it does?


John Flynn:

>Another thing that should be added to the POSTDATA message is a button that will
>simply show the results of the last time the page was POSTed. It's very annoying
>to perform a search, click on a link in the search results, then have to wait
>for the search again when you try to go back. 

Good idea to just pull the result up from the cache. Sounded like something that
would already have existed, but I looked for a bug filed on this, and I didn't
find one. Please file a new bug with adequate information so your request can be
looked at because this is a seperate issue. When you do, please provide the bug
number here for any parties that might be interested.

Comment 15

[Brian 'netdragon' Bober]

Taking

Comment 16

[Matthew T (active 1999-2002)]

Brian, your buttons are good (and yes, on the Mac "Cancel" should always be 
second from the right). But your text is longer than two short sentences, 
therefore it's too long.

I suggest: "To display this page, &productname; needs to resend the information 
it sent last time. Are you sure you want to continue?" When bug 28586 is fixed, 
this alert should be replaced by a page with no buttons, the same first sentence, 
and this second sentence: "If you're sure you want to do this, click Reload."

(Lies and the Lying Browsers That Tell Them: MSIE says "or click Cancel to return 
to the page you were trying to view", the opposite of what Cancel really does.)

Comment 17

[Brian 'netdragon' Bober]

mpt: Could we provide a link within the dialog box (or web page) that says:
"More info..." that will bring up a page explaining this in more detail?

Comment 18

[timeless]

Personally I don't like the options in the dialog.

0. Imagine you're using bugzilla (hrm, why would anyone do that?).
1. Load a bug list (10+ bugs)
2. select the first bug on the list
3. click next
4. repeat step 3 five times
5. click commit
6. repeat step 5 five times
7. somehow encourage gecko not to have your postdata pages
8. click back
8' you get this dialog.
9. click cancel
10. click back
10' you're back to the same dialog for the same page as 8'

Compare with links/lynx

instead of giving you the option of reposting and going nowhere fast, they give
you the option of reposting or skipping the page. you can get this repeatedly if
you happen to have navigated a series of posted pages, but at least you're
navigating (which is your intent, you did click 'back' after all, not 'back +
reload + cancel + forward').

Here's the question that concerns me:
> how would your grandmother go back two pages?
expected answer:
< click back twice
(i.e. grandmother when forced to cancel the scary dialog can not go back to any
pages)

i believe the links/lynx options are possible w/ the available interfaces

Comment 19

[Nickolay_Ponomarev]

Firefox 1.0 is the browser for grandmothers! Let this be fixed.
(blocking-aviary1.0RC1?)

IMHO, the confirmation message should be shorter than netdragon's variant, but
more explanatory than mpt's, more specifically "If you resend the data, any action
the form carried out (such as a search or online purchase) will be repeated."
should be added. (I won't compose my own message, because my English is awful,
but the original reporter's one looks good to me, just remove the last sentence).

Comment 20

[Jan Geurtsen]

"The website you are visiting is trying to send information. This send has
failed, and has to be resend. If you resend the data, any action the website was
trying to complete (such as a search or online purchase) will be executed. To
resend the data, click Resend." 

I am not native English, but this seems better to me.

p.s. maybe change information to data or data (2x) to information. That might be
confusing???

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Darin thinks this text sounds better, so reassigning to um.. 

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

This isn't actually a l10n blocker, it's a change to an existing string. ->
1.0+, not 1.0PR+. Feel free to land it sooner though. 

Comment 23

[Darin Fisher]

looking at appstrings.properties, there are actually two versions of this text.

http://lxr.mozilla.org/mozilla/source/docshell/resources/locale/en-US/appstrings.properties#45

the first version (repost) corresponds to the case where a form submission is
automatically retried, but fails to result in a cache hit.  the second version
(repostConfirm), which is what this bug has so far been about, corresponds to
the case where the user presses reload or shift-reload on a form submission
results page.  we should improve the text for both cases.

Comment 24

[Doug Wright]

What do IE and Opera say? There must a good explanation from at least of them
that we could crib from.

Comment 25

[Benjamin Smedberg]

This affects l10n, PR or bust.

Comment 26

[José Jeria]

(In reply to comment #24)
> What do IE and Opera say? There must a good explanation from at least of them
> that we could crib from.

IE:
"The page cannot be refreshed without resending the information.
Click Retry to send the information again,
or click Cancel to return to the page that you were trying to view."

See comment 16 about the last line.

Comment 27

[Doug Wright]

My suggestion

'The page you have asked for is customized by the server for each view, and
requires information from the browser that has already been sent (e.g. a search
request, or online purchase). This information will be resent if you continue.'

Do we really need different text for the 2 situations?

Is this a l10n blocker or not? Ben=no, BSmedberg=yes

Comment 28

[Jan Geurtsen]

(In reply to comment #27)
> My suggestion
> 'The page you have asked for is customized by the server for each view, and
> requires information from the browser that has already been sent (e.g. a 
search
> request, or online purchase). This information will be resent if you 
continue.'
> Do we really need different text for the 2 situations?
> Is this a l10n blocker or not? Ben=no, BSmedberg=yes

nono, you have to remember the average person has to understand the text. Your 
text contains information that is not relevant ('is customized by the server 
for each view'; the average user doesn't care). This is what a text message 
should contain.

Message popup:
1. What is going on?
2. How to fix the problem?
3. Choices

-No information that can confuse the reader.
-No information that is not relevant (short compact sentences).
-Simple wording (no technical wording).

Comment 29

[Brian 'netdragon' Bober]

Darin (re comment #23): Can you please provide more information on the two
cases? For example, what do you mean by automatically retried? Which version
would happen if you pressed back? Which version would happen for a meta http
request?

http://lxr.mozilla.org/mozilla/source/docshell/resources/locale/en-US/appstrings.properties#45

I think we should rename variables repost and repostConfirm. They are not very
helpful names considering what they represent. What about repost

Comment 30

[Doug Wright]

> 
> nono, you have to remember the average person has to understand the text. Your 
> text contains information that is not relevant ('is customized by the server 
> for each view'; the average user doesn't care). This is what a text message 
> should contain.
> 
> Message popup:
> 1. What is going on?
> 2. How to fix the problem?
> 3. Choices
> 
> -No information that can confuse the reader.
> -No information that is not relevant (short compact sentences).
> -Simple wording (no technical wording).

'is customized by the server for each view' - isn't that the whole point of
POSTDATA? That server output directly depends on user input? So it's highly
relevant.

What's your suggestion? (I'm not saying that mine is perfect btw)

Comment 31

[Brian 'netdragon' Bober]

I realized in cases like this, it'd be nice for dialogs like this one to have a
"More Info.." or "Help" button or link that popped up more information about the
message.

I filed bug 256524.

Comment 32

[Jan Geurtsen]

(In reply to comment #30)
> > 
> > nono, you have to remember the average person has to understand the text. 
Your 
> > text contains information that is not relevant ('is customized by the 
server 
> > for each view'; the average user doesn't care). This is what a text 
message 
> > should contain.
> > 
> > Message popup:
> > 1. What is going on?
> > 2. How to fix the problem?
> > 3. Choices
> > 
> > -No information that can confuse the reader.
> > -No information that is not relevant (short compact sentences).
> > -Simple wording (no technical wording).
> 'is customized by the server for each view' - isn't that the whole point of
> POSTDATA? That server output directly depends on user input? So it's highly
> relevant.
> What's your suggestion? (I'm not saying that mine is perfect btw)

I don't think that section contributes to one of the three point I made above. 
I already made my sugestion (comment 20). It is probably not perfect, but it 
follows the three points and is clean.

Comment 33

[Doug Wright]

I apologise - I didn't see #20 (I did look before asking for your suggestion!) I
disagree with it anyway however.

Comment 34

[Jan Geurtsen]

(In reply to comment #33)
> I apologise - I didn't see #20 (I did look before asking for your 
suggestion!) I
> disagree with it anyway however.

You have to see it like this:

question: What is going on?
Answer: The page you have asked for is customized by the server for each view, 
and requires information from the browser that has already been sent (e.g. a 
search request, or online purchase).

Average user: Huh?

Now i'll stop spamming bugmail ;)

Comment 35

[chris hofmann]

no patch avaiable.  need to shutdown for the PR release and UI freeze.  this one
would be good to catch in a future release.

Comment 36

[Darin Fisher]

(In reply to comment #29)
> Darin (re comment #23): Can you please provide more information on the two
> cases? For example, what do you mean by automatically retried?

By "automatically retried" I was referring to the case where the user navigates
via the Back button in session history.


> Which version
> would happen if you pressed back? Which version would happen for a meta http
> request?

I suspect the "repost" version would happen in both of these cases.

"repostConfirm" appears to correspond to the case where Mozilla can tell in
advance that the request from the user is to bypass any cached data (i.e., the
resulting page from a form submission).  that can only be reload or
shift-reload, since in other cases such as Back or Meta refresh Mozilla would
have tried to load the page from the cache.

Comment 37

[Michael Jouravlev]

(In reply to comment #0)
> "The page you are trying to view contains POSTDATA. If you resend the data, any
> action the form carried out (such as a search or online purchase) will be
> repeated. To resend the data, click Ok. Otherwise, click Cancel."
> 
> That message is very confusing for normal people. Mr. End User & his dog,
> neither of whom can tell the difference between DNS and DSL, surely have never
> heard of POSTDATA.

Absolutely agree with that.

> It should be changed to:
> 
> "The page you are trying to view was created using information you submitted in
> a form. If you resend the data, any action the form carried out (such as a
> search or online purchase) will be repeated. To resend the data, click Ok.
> Otherwise, click Cancel."

Do not agree with that. An HTML FORM can be resubmitted in three cases: 

* reloading result page using Refresh/Reload browser button 
  (explicit page reload, implicit resubmit of request);
* clicking Back and then Forward browser buttons 
  (implicit page reload and implicit resubmit of request);
* returning back to HTML FORM after submission, 
  and clicking Submit button on the form again 
  (explicit resubmit of request)

Only in the third case a user is aware about the fact that the form is
resubmitted. The first two cases seem absolutely innocent for a non-geek. To my
shame, they seemed innocent to me as well, before I understood how POST request
works.

I would say something like "The browser is about to resubmit already submitted
data. Which may cause ...". And something else about why its happening and what
consequences it may cause. But no one is going to read message longer than two
sentences ;)

(In reply to comment #11)
> Another thing that should be added to the POSTDATA message is a button that will
> simply show the results of the last time the page was POSTed. It's very annoying
> to perform a search, click on a link in the search results, then have to wait
> for the search again when you try to go back. 
> 
> Sure, you could open the links on the search results in new windows or tabs, but
> why not just have a button that will show you the results of the last POST?

This depends on the server logic and cannot be decided by browser or even a
user. Say, you submitting a shopping cart in online store. Or you are giving a
raise to an employee in an HR system. You do not want to browse back and to
resubmit it again. And you don't want to see some cached result which does not
relate to the server state anymore. At least I do not want this :)

> (Lies and the Lying Browsers That Tell Them: MSIE says "or click Cancel to return 
> to the page you were trying to view", the opposite of what Cancel really does.)

Yes, this is a lie. But this is not a whole story. MSIE actually tries to show
the previous page again, but if it is marked as Expired, MSIE does not have a
better solution as to show the "Page is expired" message. I am not sure is it
better than what Mozilla does, which displays the cached page. MSIE redisplays
the cached page only if it is not expired.

(In reply to comment #20)
> "The website you are visiting is trying to send information. This send has
> failed, and has to be resend. If you resend the data, any action the website was
> trying to complete (such as a search or online purchase) will be executed. To
> resend the data, click Resend." 

Nah, it is the browser which tries to resubmit POST data to the website. And how
do you know that the first submission failed? 

(In reply to comment #23)
> looking at appstrings.properties, there are actually two versions of this text.
> 
>
http://lxr.mozilla.org/mozilla/source/docshell/resources/locale/en-US/appstrings.properties#45
> 
> the first version (repost) corresponds to the case where a form submission is
> automatically retried, but fails to result in a cache hit.  the second version
> (repostConfirm), which is what this bug has so far been about, corresponds to
> the case where the user presses reload or shift-reload on a form submission
> results page.  we should improve the text for both cases.

Are you talking about is the result page, which is loaded from the server (or
from the cache) as the result of POST request? What about the source page? What
about notifying a user that data is resubmitted from a stale page? Sometimes it
does not matter, sometimes it really does. I filed a separate change request on
that: http://bugzilla.mozilla.org/show_bug.cgi?id=259680 .

(In reply to comment #24)
> What do IE and Opera say? There must a good explanation from at least of them
> that we could crib from.

The last time I tried Opera, it silently resubmitted POST request when I
navigated back to the FORM and then forward to the result. This seems wrong to me.

(In reply to comment #36)
> By "automatically retried" I was referring to the case where the user navigates
> via the Back button in session history.

You mean Back and then Forward? Are you talking about silent resubmit? Do not do
this please, this is just wrong.

There are several facets of the problem, like: 

* Why re-POST happens?
* Can it be avoided?
* Can it be resubitted automatically?
* Can a result page be loaded from cache instead of resubmitting a POST?
* Is it really important to distinguish between first submit and resubmits?
* Should a server be notified about resubmit?
* Should a user be notified about resubmit?
* Should a browser allow resubmit from a stale page without its explicit reloading? 

You guys are trying to solve the problem from the browser point of view. But
there is server point of view as well. Which usually boils to this: "Do not
allow POST resubmits". Better web applications also preach "Do not show stale
page" mantra. 

Anyway, there are applications which do not care much about what do they respond
with and what type of requests they receive, but still they prefer not to get
resubmits. For these sucky apps (and there are so many of them!) the best
approach would be "Cache everything, show cached pages on back and forward, do
not do resubmits". This does not apply to search engines because they use GET
instead of POST, which does not require user confirmation for resubmission. Even
simpler, a GET request with bunch of query parameters is not considered as data
submission, because GET (supposedly) should not change server state, and these
parameters are actually what they are called: _query_ parameters.

Other applications try to provide desktop-like feel, which includes:

* View is always in sync with the Model;
* There are no "pages", there are application "screens".

The first one means, that when a user clicks Back, a browser should not show a
stale page from a cache, but should go to the server and grab a fresh version,
which may contain different information because the Model could have been
updated since the last time the page was shown. I like the shopping cart
example: say a form with a cart is submitted, the cart is processed and result
page is shown. Now a user clicks Back button. The user should see an error
message telling that the cart was processed and discarded, instead of stale form
with non-existent cart. This way he would not be able to resubmit it, so no one
would see "POSTDATA will be resent" message! This behavior can be achieved with
MSIE using "no-cache" cache-control header, for Mozilla one has to use
"no-store" as I was pointed to. 

The second one is that application has logical screens. Say, it has welcome
page, input form and result page. A welcome page contains a link to an input
form. I enter a value, it is wrong, the form is redisplayed. I enter another
one, it is wrong again, the form is redisplayed again. And again, and again.
With "screen" approach when I click Back on a form, I would immediately return
to the welcome page, not to the previous version of this form with older invalid
value. So, no "POSTDATA will be resent" message again!

This behavior can be achieved on any browser using redirection and a non-spec
behavior which redirects response from POST to GET if 302 response is received,
and does not ask for user confirmation (do not "fix" that, guys ;) ). Mozilla
works well, as well as MSIE, but Opera shows some weird pages on the way back. I
don't care, I do not use Opera.

Back to the topic. You cannot distinguish bad apps from the good apps. You
cannot know do you really need to reload a page or not. Do you want to think for
the web app developers? Do you want to "correct" their imperfect apps? You may
make bad apps look better, but the good apps may work worse. 

So, instead of trying to cache POST request along with result page, or to
silently resubmit the POST, you should just refine the message and tell a user
what is really happening. The best message is the one which allows a user to see
what is happening and to understand that a web app he is using is a piece of
****. Maybe this would encourage web app developers to write better code. For
crying out loud, go to eBay, go to "Forgotten password" page, and request you
password info. You would have to fill out a form and after that you would see a
result page telling you that an email with password restore link was sent to
your mailbox. Now refresh this result page couple of times. Then count emails in
your mailbox. This is sad, what can be said about smaller websites.

So, this whole issue is not really a browser issue. Refrain from trying to
"improve" bad apps, just show clean, concise and short message. Which one...
Can't tell you, me speak English bad ;) But the one you have now is way too long.

Comment 38

[Elmar Ludwig]

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

Comment 39

[Mike Connor [:mconnor]]

Beltzner, can you hit this up with some wordsmithery?  This is quite possibly our least-friendly warning dialog (aside from the crypto stuff).

Comment 40

[Bastian Grupe]

If I may, here is some inspiration:

"The page you are trying to view contains data sent with the HTTP POST method.
This method is used in order to do sensitive tasks on the server!
Warning: If you resend the data, any action carried out 
(such as a search or online purchase) will be repeated!"
[Resend] [Cancel]

Comment 41

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

taking for Fx2 (assuming you don't mind, darin ...)

Comment 42

[Darin Fisher]

No, not at all.  Thank you!  By the way, it would be even better if we could implement this error dialog as an error page.  That is what IE does, and I think it results in much better UI.  However, that probably requires a bit more work to engineer.

Comment 43

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

Comment 44

[Darin Fisher]

No patch; not going to get fixed for FF2 beta1.

Comment 45

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

This is (thanks to reed for the late night help) what we need:

repost=To display this page, the information previously sent by Firefox must be resent. This will repeat any action (such as a search or order submission) that had been performed earlier.
repostConfirm=To display this page, the information previously sent by Firefox must be resent. This will repeat any action (such as a search or order submission) that had been performed earlier.

We also need the buttons to change to be "Resend" and "Cancel", but I'm really less sure how to do that. Anyone care to help work up a patch?

Comment 46

[Asaf Romano (gone)]

Taking.

Comment 47

[Radek 'sysKin' Czyz]

Has anyone considered not putting non-cached POST responses to history at all, and - as a result - just skipping them over when going back?

The result would be quite similar to most AJAX-based actions, I don't think people often complain that they can't go back with AJAX.

If not, I suggest the verb "action" rather than "(form) data send" because many POST sends are not really "data" in "forms" in the popular meaning of these words (a single textbox or a collection of checkboxes are not what users describe as "forms" or "data").

Comment 48

[Magnus Melin [:mkmelin]]

I've got a patch for this.... hope you don't mind;)

Comment 49

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Use the string(s) from comment #45.

Comment 50

[Unknown W. Brackets]

(In reply to comment #45)
> repost=To display this page, the information previously sent by Firefox must be
> resent. This will repeat any action (such as a search or order submission) that
> had been performed earlier.

Shouldn't that be "may" instead of "will"?  I mean, what if the user wants to repost but the server software has preventions for this (which is often the case in ecommerce, forum software, etc.)?

Or is that just being pedantic?

-[Unknown]

Comment 51

[Darin Fisher]

Comment on attachment 242418 [details] [diff] [review]
proposed fix

either "may" or "will" seems fine to me.

over to boris for final review..

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 242418 [details] [diff] [review]
proposed fix

>Index: docshell/base/nsDocShell.cpp

>+const char* kBrandBundleURL      = "chrome://branding/locale/brand.properties";
>+const char* kAppstringsBundleURL = "chrome://global/locale/appstrings.properties";

Make these |const char[]|, please.

>+NS_IMETHODIMP
>+nsDocShell::ConfirmRepost(PRBool * aRepost)

Just |nsresult|.  This isn't an interface method.  Change the header declaration to |nsresult| instead of NS_IMETHOD as well.

>+  nsCOMPtr<nsIPrompt> prompter;
>+  NS_ENSURE_SUCCESS(GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompter)),
>+                      NS_ERROR_FAILURE);

  CallGetInterface(this, getter_AddRefs(prompter))

please.  NS_GET_IID should be avoided if at all possible.  Yes, I know you just copied the code...

>+      stringBundleService(do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv));
>+  if (NS_FAILED(rv)) return rv;

I think that should be an NS_ENSURE_SUCCESS(rv, rv).

>+  rv = stringBundleService->CreateBundle(kAppstringsBundleURL,
>+                                           getter_AddRefs(appBundle));
>+  if (NS_FAILED(rv)) return rv;

Fix the indentation.  And again, I think this should use NS_ENSURE_SUCCESS.

>+  rv = stringBundleService->CreateBundle(kBrandBundleURL, getter_AddRefs(brandBundle));
>+  if (NS_FAILED(rv)) return rv;

And here.

>+  if (prompter && brandBundle && appBundle) {

It's probably safe to just assert that, but either way. If you do keep this check, make it an early return if one of them is null and outdent the rest of the code.

>+    rv = brandBundle->GetStringFromName(NS_LITERAL_STRING("brandShortName").get(),
>+                        getter_Copies(brandName));

Please line up the two args.

>+    rv = appBundle->FormatStringFromName(NS_LITERAL_STRING("confirmRepost").get(),
>+                          formatStrings, 1, getter_Copies(msgString));

Again, line up the args.  And replace "1" with NS_ARRAY_LENGTH(formatStrings), please.

>+    rv = appBundle->GetStringFromName(NS_LITERAL_STRING("resendButton.label").get(),
>+                          getter_Copies(button0Title));

Again, line up the args.

>+    rv = prompter->ConfirmEx(nsnull, msgString.get(),
>+            (nsIPrompt::BUTTON_POS_0 * nsIPrompt::BUTTON_TITLE_IS_STRING)  +

And here.  Might want to linebreak between the "->" and the 'C'.

The rest looks good at first glance, but it'd be much easier to tell with a diff -w.  Could you please post one in addition to the normal diff when you address the comments?

Comment 53

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

>  CallGetInterface(this, getter_AddRefs(prompter))

do_GetInterface?

Comment 54

[Boris Zbarsky [:bzbarsky]]

> do_GetInterface?

No.  do_GetInterface will not compile, since |this| has ambiguous inheritance from nsISupports.  But CallGetInterface is templatized on its first argument and merely requires it to have a GetInterface method, so will work.  See comments in nsIInterfaceRequestorUtils.h.

Comment 55

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

ah right, forgot about the multiple inheritance issue.

Comment 56

[Magnus Melin [:mkmelin]]

(In reply to comment #52)
> (From update of attachment 242418 [details] [diff] [review] [edit])
> >+const char* kAppstringsBundleURL = "chrome://global/locale/appstrings.properties";
> 
> Make these |const char[]|, please.

For some reason, making them const char[] doesn't seem to compile (error: expected unqualified-id before '[' token).

> >+  nsCOMPtr<nsIPrompt> prompter;
> >+  NS_ENSURE_SUCCESS(GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompter)),
> >+                      NS_ERROR_FAILURE);
> 
>   CallGetInterface(this, getter_AddRefs(prompter))

/opt/CVSROOT/mozilla/docshell/base/nsDocShell.cpp:8345: error: no matching function for call to 'CallGetInterface(nsDocShell* const, nsGetterAddRefs<nsIPrompt>)'

Does someone know what do I need to change to get this working?

Comment 57

[Boris Zbarsky [:bzbarsky]]

> For some reason, making them const char[] doesn't seem to compile (error:
> expected unqualified-id before '[' token).

You used:

const char kAppstringsBundleURL[] = "chrome://global/locale/appstrings.properties";

right?  Or did you do something else?

> error: no matching function for call to 'CallGetInterface

  #include "nsIInterfaceRequestorUtils.h"

should help.

Comment 58

[Magnus Melin [:mkmelin]]

(In reply to comment #57)
> > For some reason, making them const char[] doesn't seem to compile (error:

Too much Java coding;) Had the brackets in the wrong place.

> > error: no matching function for call to 'CallGetInterface
> 
>   #include "nsIInterfaceRequestorUtils.h"
> 
> should help.

Nope, I had that already.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  You might need NS_STATIC_CAST(nsIPrompt**, getter_AddRefs(prompter)) as the last arg.

Comment 60

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Addressing review comments. (Thanks a lot for the help boris!)

Comment 61

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2 (diff -w)

diff -w patch for easier review

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244826 [details] [diff] [review]
proposed fix, v2

>+confirmRepost=To display this page, the information previously sent by %S must be resent. This will repeat any action (such as a search or order submission) that had been performed earlier.

I think this was more suitably scary when it explicitly said "purchase", but I then that's me... I'll poke beltzner about it.

>Index: docshell/base/nsDocShell.cpp
>+         ConfirmEx(nsnull, msgString.get(),
>+                  (nsIPrompt::BUTTON_POS_0 * nsIPrompt::BUTTON_TITLE_IS_STRING) +
>+                  (nsIPrompt::BUTTON_POS_1 * nsIPrompt::BUTTON_TITLE_CANCEL),
>+                  button0Title.get(), nsnull, nsnull, nsnull, nsnull, &buttonPressed);

The indentation here is a little odd (the second and third lines should be indented one more space, and the fourth line two more spaces).

Looks great other that that nit!  I'll fix the indent before checking this in.

sr=bzbarsky

Comment 63

[Boris Zbarsky [:bzbarsky]]

> and the fourth line two more spaces

Er, also just one space.  Just the font that bugzilla uses made it look weird.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the patch!

Comment 65

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This very likely caused bug 359857 in Camino (and the string used in this bug in not embedding-friendly, since it requires branding [%S instead of "this application"], which embedding apps do not ship [bug 302309]).

Comment 66

[Peter Flynn (Oracle)]

The wording "This will repeat any action ... that had been performed earlier" might be overly alarmist.  What about "... that had been performed by loading this page earlier"?

Comment 67

[Boris Zbarsky [:bzbarsky]]

I think we _want_ the wording here to be alarmist.  See comment 62.  We _really_ don't want people resubmitting purchases.

Comment 68

[Peter Flynn (Oracle)]

No, I understand.  I was just trying to help get more specific about which prior actions will be resubmitted -- as-is the wording sounds like anything the user did in the entire session would be resubmitted.  But your call...

Comment 69

[Joseph Kaye]

IS there possibility that one could disable these postdata notifications?  I see these a lot, and I'm not even on a page that I'm buying something.