Closed
Bug 94514
Opened 23 years ago
Closed 17 years ago
POST result page should not appear in global history or history autocomplete results
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta4
People
(Reporter: neil, Assigned: sdwilsh)
References
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
I was trying to look for a form in history (autocomplete) and accidentally
selected the form results page instead of the form itself. It doesn't make sense
to store the URL of the form results page in global history.
Comment 1•23 years ago
|
||
argh.. take two at reassigning history bugs to new owner
Assignee: alecf → blakeross
Updated•23 years ago
|
Target Milestone: --- → mozilla1.2
Updated•23 years ago
|
Target Milestone: mozilla1.2 → Future
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
I know very little about network, so I'm not sure if this patch is correct.
Tested on a few POST forms. I notice that IE does show such form results pages
in the UI. Cc'ing bbaetz.
Comment 4•23 years ago
|
||
This patch is slightly wonrg, becasue its possible for
http://foo.bar.com/xxx.cgi to be a page which posts to ittself. The first view
should be in global histroyy, the second one shouldn't. However, both should be
in local history.
POST result pages use a cache key which is != the document url, so I guess that
page should be hidden instead.
cc darin, who knows how that works.
Comment 5•22 years ago
|
||
*** Bug 152219 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Assignee: bross2 → nobody
QA Contact: claudius → history.global
Comment 7•17 years ago
|
||
In addition to the concerns in comment 4, this patch is also a horrible hack. Rather than adding to history like normal, and then hiding the page, the POST state should be sent to history so the URL can be added with the correct codepath using the correct flags.
For Places, you probably want a new transition type in addition to TYPED, etc. that corresponds to POST requests. This is so even in the example where a page posts to itself, the first visit will appear in history properly, but the post won't.
Comment 8•17 years ago
|
||
morphing bug to become a places bug
Component: History: Global → Places
OS: Windows 95 → All
Product: Core → Firefox
QA Contact: history.global → places
Hardware: PC → All
Target Milestone: Future → ---
Updated•17 years ago
|
Flags: blocking-firefox3?
Comment 9•17 years ago
|
||
one reason why this would be good for fx 3 is because of recent url bar autocomplete changes.
Depends on: 389491
Updated•17 years ago
|
Summary: POST result page should not appear in global history. → POST result page should not appear in global history or history autocomplete results
Comment 10•17 years ago
|
||
Comment 11•17 years ago
|
||
Actually, this bug is more serious than just the highlighted area: of the top six results, five are clearly POST results!
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Comment 12•17 years ago
|
||
as discussed today, this causes a problem with the url bar autocomplete as posting could impact visit count.
Comment 13•17 years ago
|
||
with the upcoming change for bug #399477, this is going to be really noticable for bugzilla users, see https://bugzilla.mozilla.org/attachment.cgi?id=291339
Comment 14•17 years ago
|
||
note, in the case of that scree shot, I've clearly also re-visited https://bugzilla.mozilla.org/post_bug.cgi upon restarting the browser after quit or crash, but we have a bug on not counting those visits as well.
Updated•17 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Updated•17 years ago
|
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → sdwilsh
Whiteboard: [needs patch]
Assignee | ||
Comment 16•17 years ago
|
||
So, I'm working with two scenarios:
1) We post to a page, and it displays something given those results. We DO NOT want to count this.
2) We post to a page, and it redirects us to some other page. We DO want to count this.
Does anyone disagree with this?
Status: NEW → ASSIGNED
Comment 17•17 years ago
|
||
If the redirect is to a "GET" and the POST was initiated by a clear user action (e.g. a click), counting it seems reasonable.
Assignee | ||
Comment 18•17 years ago
|
||
Not requesting review on this until I get tests written, but feel free to comment.
This works with case 1 at least for me.
Attachment #69914 -
Attachment is obsolete: true
Assignee | ||
Comment 19•17 years ago
|
||
Here we go. Now with a test. I couldn't think of a way to test case two, but I'm open to suggestions.
Dietrich, Mano, and Seth:
Might this cause a problem in places with visit chains now? We won't notice any pages with post results, so we'll be missing navigation paths (one could probably argue that in most cases it's a good thing). For example:
1) Start off at https://bugzilla.mozilla.org/show_bug.cgi?id=94514
2) Make a change to the bug, you end up at https://bugzilla.mozilla.org/process_bug.cgi
3) Click a link to another bug on that page
We'll miss the page on step two, and step 1 and 3 will not be connected. Is this bad? I really don't know...
Attachment #300808 -
Attachment is obsolete: true
Attachment #300998 -
Flags: superreview?(bzbarsky)
Attachment #300998 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs patch] → [has patch][needs r bz][needs sr bz]
Comment 20•17 years ago
|
||
Comment on attachment 300998 [details] [diff] [review]
v1.1
The code looks OK. I have some issues with the test....
> bug404548-subframe.html \
>+ test_bug94514.html \
Please add it in the right place so that the tests are in order.
>+if (!window.location.href.match("posted=1")) {
It looks like you're doing SimpleTest.waitForExplicitFinish() in one Window, and calling finish() in another one. I don't believe there's any guarantee that this would work. Please just do the form submit in a different window from the one the test is running in (e.g. window.open a window and do the submit in there, or do it in a frame, or whatever).
>+ var form = document.getElementById("testForm");
$("testForm")
>+ // We need to check that this was not added to global history.
Shouldn't the test start make sure to remove it from global history or something? Otherwise if this ever regresses, the test will keep failing even if the regression is fixed.
r+sr=bzbarsky on the code, but please fix up the test, ok?
Attachment #300998 -
Flags: superreview?(bzbarsky)
Attachment #300998 -
Flags: superreview+
Attachment #300998 -
Flags: review?(bzbarsky)
Attachment #300998 -
Flags: review+
Assignee | ||
Comment 21•17 years ago
|
||
Alright, I can fix those. Admittedly, this was my first mochitest.
What do you mean by "in order"? In order of bug numbers or what?
Whiteboard: [has patch][needs r bz][needs sr bz] → [needs new patch][has r][has sr]
Assignee | ||
Comment 22•17 years ago
|
||
Carrying forward sr
(In reply to comment #20)
> Shouldn't the test start make sure to remove it from global history or
> something? Otherwise if this ever regresses, the test will keep failing even
> if the regression is fixed.
There's no API to remove something from global history, but I check and make sure it's not added first.
Attachment #300998 -
Attachment is obsolete: true
Attachment #301382 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs new patch][has r][has sr] → [has patch][needs r bz][has sr]
Comment 23•17 years ago
|
||
Uh... how can you assert that the initial page is not in global history? That might be true for a fully automated mochitest run, but for someone running them by hand (multiple times in a row on the same profile, in that case), that would simply fail, no?
Assignee | ||
Comment 24•17 years ago
|
||
(In reply to comment #23)
> Uh... how can you assert that the initial page is not in global history? That
> might be true for a fully automated mochitest run, but for someone running them
> by hand (multiple times in a row on the same profile, in that case), that would
> simply fail, no?
Yeah, you can only run it once. It sucks, but how do I remove it from global history exactly?
Comment 25•17 years ago
|
||
Comment on attachment 301382 [details] [diff] [review]
v1.2
OK, but please change the messages to make it clear that the error could just be from running twice without a restart.
And perhaps file a bug on improving the history API.
Attachment #301382 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 26•17 years ago
|
||
Fixes comment. This is for checkin.
Attachment #301382 -
Attachment is obsolete: true
Assignee | ||
Comment 27•17 years ago
|
||
So, I talked with dietrich about comment 19. While this patch breaks session tracking, no user facing feature uses that yet. In addition, we don't expect there to be one in time for 1.9/Fx3. The functionality that this will provide, however, is a big win for users (anyone who uses bugzilla should be able to attest to that). So, we'll do a follow-up (bug 415955) to get nsIGlobalHistory extended again to better track this for the next major release.
Checking in docshell/base/nsDocShell.cpp;
new revision: 1.884; previous revision: 1.883
Checking in docshell/test/Makefile.in;
new revision: 1.11; previous revision: 1.10
Checking in docshell/test/bug94514-postpage.html;
initial revision: 1.1
Checking in docshell/test/test_bug94514.html;
initial revision: 1.1
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs r bz][has sr]
Comment 28•17 years ago
|
||
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
Comment 29•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•