Last Comment Bug 112848 - Does your grandmother know what POSTDATA is?
: Does your grandmother know what POSTDATA is?
Status: RESOLVED FIXED
[see comment #45 for text to use]
: ux-jargon
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P2 normal with 6 votes (vote)
: mozilla1.9
Assigned To: Magnus Melin
: Adam Lock
Mentors:
: 301063 341691 (view as bug list)
Depends on: 359857 392407
Blocks: 333520 376040
  Show dependency treegraph
 
Reported: 2001-11-30 07:30 PST by Jonas Jørgensen
Modified: 2010-04-26 15:02 PDT (History)
40 users (show)
chofmann: blocking‑aviary1.0PR-
darin.moz: blocking1.8.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (17.60 KB, patch)
2006-10-16 10:43 PDT, Magnus Melin
darin.moz: review+
bzbarsky: superreview-
Details | Diff | Splinter Review
proposed fix, v2 (17.92 KB, patch)
2006-11-06 11:41 PST, Magnus Melin
bzbarsky: superreview+
Details | Diff | Splinter Review
proposed fix, v2 (diff -w) (11.19 KB, patch)
2006-11-06 11:43 PST, Magnus Melin
no flags Details | Diff | Splinter Review

Description Jonas Jørgensen 2001-11-30 07:30:06 PST
"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 2001-11-30 10:40:33 PST
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 [:bz] (TPAC) 2001-11-30 10:52:55 PST
ccing ui design
Comment 3 Darin Fisher 2001-11-30 11:29:09 PST
-> docshell
Comment 4 Adam Lock 2001-12-03 04:26:15 PST
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 Jens Martin Schlatter 2001-12-06 09:09:22 PST
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) 2001-12-06 11:34:26 PST
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 2002-04-03 22:20:14 PST
adding self to cc list
Comment 8 Adam Lock 2002-05-01 09:42:36 PDT
Reassigning to UI design
Comment 9 Matthew Paul Thomas 2002-08-01 23:00:59 PDT
*** Bug 160144 has been marked as a duplicate of this bug. ***
Comment 10 Jonas Jørgensen 2002-09-17 13:39:16 PDT
--> docshell.

User Interface Design component is being removed; see bug 167289.
Comment 11 John Flynn 2003-02-21 14:04:09 PST
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 2003-10-18 04:28:29 PDT
Mind if I take this?
Comment 13 Adam Lock 2003-10-18 05:19:45 PDT
Please do
Comment 14 Brian 'netdragon' Bober 2003-10-18 13:16:07 PDT
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 2003-10-18 13:16:52 PDT
Taking
Comment 16 Matthew Paul Thomas 2004-01-11 15:55:27 PST
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 2004-01-11 17:01:48 PST
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 2004-01-15 09:20:43 PST
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 2004-07-17 03:43:59 PDT
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 2004-07-18 10:22:19 PDT
"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) 2004-07-22 12:43:01 PDT
Darin thinks this text sounds better, so reassigning to um.. 
Comment 22 Ben Goodger (use ben at mozilla dot org for email) 2004-07-26 14:07:34 PDT
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 2004-08-04 14:28:36 PDT
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 2004-08-15 10:59:50 PDT
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 [:bsmedberg] 2004-08-20 08:33:23 PDT
This affects l10n, PR or bust.
Comment 26 José Jeria 2004-08-20 11:56:46 PDT
(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 2004-08-22 12:14:29 PDT
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 2004-08-22 12:26:21 PDT
(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 2004-08-22 13:14:22 PDT
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 2004-08-22 13:29:36 PDT
> 
> 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 2004-08-22 13:36:57 PDT
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 2004-08-22 13:39:59 PDT
(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 2004-08-22 13:53:28 PDT
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 2004-08-22 14:00:43 PDT
(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 2004-08-24 15:41:12 PDT
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 2004-08-27 16:05:31 PDT
(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 2004-09-16 15:05:19 PDT
(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 2005-07-19 08:36:51 PDT
*** Bug 301063 has been marked as a duplicate of this bug. ***
Comment 39 Mike Connor [:mconnor] 2006-01-10 08:05:22 PST
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 2006-01-17 11:46:24 PST
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] 2006-01-18 15:49:28 PST
taking for Fx2 (assuming you don't mind, darin ...)
Comment 42 Darin Fisher 2006-01-18 17:08:52 PST
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) 2006-06-15 17:21:09 PDT
*** Bug 341691 has been marked as a duplicate of this bug. ***
Comment 44 Darin Fisher 2006-06-26 12:35:35 PDT
No patch; not going to get fixed for FF2 beta1.
Comment 45 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-29 01:57:11 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-06-29 02:19:09 PDT
Taking.
Comment 47 Radek 'sysKin' Czyz 2006-07-12 23:21:55 PDT
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 2006-10-16 10:38:08 PDT
I've got a patch for this.... hope you don't mind;)
Comment 49 Magnus Melin 2006-10-16 10:43:46 PDT
Created attachment 242418 [details] [diff] [review]
proposed fix

Use the string(s) from comment #45.
Comment 50 Unknown W. Brackets 2006-10-16 10:53:25 PDT
(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 2006-10-18 08:48:31 PDT
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 [:bz] (TPAC) 2006-10-18 09:54:56 PDT
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) 2006-10-18 13:32:28 PDT
>  CallGetInterface(this, getter_AddRefs(prompter))

do_GetInterface?
Comment 54 Boris Zbarsky [:bz] (TPAC) 2006-10-18 14:24:59 PDT
> 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) 2006-10-18 14:41:40 PDT
ah right, forgot about the multiple inheritance issue.
Comment 56 Magnus Melin 2006-11-06 10:47:06 PST
(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 [:bz] (TPAC) 2006-11-06 10:54:25 PST
> 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 2006-11-06 11:03:25 PST
(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 [:bz] (TPAC) 2006-11-06 11:07:03 PST
Oh, hmm.  You might need NS_STATIC_CAST(nsIPrompt**, getter_AddRefs(prompter)) as the last arg.
Comment 60 Magnus Melin 2006-11-06 11:41:19 PST
Created attachment 244826 [details] [diff] [review]
proposed fix, v2

Addressing review comments. (Thanks a lot for the help boris!)
Comment 61 Magnus Melin 2006-11-06 11:43:16 PST
Created attachment 244827 [details] [diff] [review]
proposed fix, v2 (diff -w)

diff -w patch for easier review
Comment 62 Boris Zbarsky [:bz] (TPAC) 2006-11-06 19:22:37 PST
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 [:bz] (TPAC) 2006-11-06 19:25:11 PST
> 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 [:bz] (TPAC) 2006-11-06 19:28:57 PST
Checked in.  Thanks for the patch!
Comment 65 Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-11-07 12:28:47 PST
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) 2006-11-13 21:42:43 PST
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 [:bz] (TPAC) 2006-11-13 23:27:09 PST
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) 2006-11-14 00:32:07 PST
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 2007-01-08 16:28:20 PST
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. 

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