Overview Description: Current scroll position not retained, reloading multipart/x-mixed-replace This specific problem was noted when the general case of retaining scroll position when reloading a page became broken. The bugzilla pages are sent out with 'Content-Type: multipart/x-mixed-replace'; when you scroll down a bug list, then hit reload, the scroll position is reset to the top of the page. Steps to Reproduce: 1) Get a bugzilla bug list as the result of some query. (or use this handy-dandy simple test page http://jrgm/cgi-bin/multipart-x-mixed-replace.pl) 2) Scroll down the page some distance 3) reload the page. For those outside the netscape firewall, here is the simple perl script to set up such a test page: ---8<----------------------------------------------------------------- #!/usr/bin/perl -w # print <<"ENDOFSTREAM"; Content-Type: multipart/x-mixed-replace;boundary=thisrandomstring --thisrandomstring Content-type: text/html <p>Please stand by ... </p> --thisrandomstring Content-type: text/html <p> just a long page </p> ... and a bunch of text here to lengthen the page --thisrandomstring-- ENDOFSTREAM ---8<----------------------------------------------------------------- Actual Results: Scroll position is set to top of page upon a reload of the page. Expected Results: Scroll position should be at the same location after the reload. Reproducibility: always Build Date & Platform Bug Found: 2000072809 win95 Additional Information: Currently the general case of saving scroll position is broken (see bug 46877). This bug report is for a special case that I believe was not working all along (but corrections to that are welcome). I don't believe this bug should be addressed at all in the current schedule. This bug should be futured.

pollmann is dealing with this right now

multipart/x-mixed-replace is not extrememly common, and we don't do anything horribly wrong in this case - just not enabling a usability feature. Moving to Future.

nav triage team: not a beta stopper.

Fixing bug 55055, "Go Back broken for form post results", would take care of the most common case of this bug (going back from a bug to a bug list).

This bug makes browsing bugzilla bug lists very difficult - especially when viewing long buglists. It's just one of those little annoying things that bites you on a daily basis - for that reason I am nominating nsCatFood.

Bulk reassigning form bugs to Alex

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

removed myself from CC:

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

Is any work being done on this bug at all? It bites me everyday when I view Bugzilla daily bug report lists. I could help if someone pointed me to the source files. This bug was reported more than 2 years ago and IMHO, is quite a nuisance.

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

The effect of a patch was checked by mozilla-1.2.1.

Hideo Saito, could you explain why that patch fixes this bug? Preferably in comment form in the patch? Also, please don't use a goto. Just move that code up above the current |if (x || y)| check like this: if (!x && !y) { // Code to compute new x and y } if (x || y) { // save state } You'll want to ask jkeiser@netscape.com for the review; I can do the super-review (sorta did already, earlier in this comment ;) )

Sorry, I cannot reply clearly to why this patch fixes this bug. Scroll position of mixed multipart page is relatively made the same with the current scroll position by this patch.

Why is multipart/x-mixed-replace different from other documents with respect to scroll position restoration? Do we just not store the scroll position using our standard mechanism?

In the usual scrolling, each functions are called as follows. nsScrollBoxFrame::SaveState() nsScrollBoxFrame::RestoreState() nsScrollBoxFrame::DoLayout() In the scrolling of the mixed multipart page, nsScrollBoxFrame::SaveState() nsScrollBoxFrame::RestoreState() <- A nsScrollBoxFrame::SaveState() <- B nsScrollBoxFrame::RestoreState() nsScrollBoxFrame::DoLayout() This patch passes the scroll position of A to B.

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

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

I am experiencing the exact same bug on WinXP Pro. Firebird 0.7 While viewing a page such as this bug list, I will click on link to a bug then click the back button and the page scroll reverts back to the top of the page again! This is VERY annoying when dealing with long lists of links and I may discontinue using this browser if not resolved. My friend who uses the same OS and browser does not have the same problem...

following condition is added if true, has a multipart/mixed content + if (aPresContext->HasMultiMixedScrolling()) { if true, the scroll position of the page after restoration was reset + if (x == 0 && y == 0 && + mLastPos.x == 0 && mLastPos.y == 0 &&

So the problem arising here is because in a multipart document, each part can have an independent scroll position (and general layout state, in fact) and we have no way to identify them with our "one layout state per history entry, one history entry per load" setup.

My approach means that each part of multipart document has the same scroll position relatively, however the independent scroll position of each part will be realized in the future. this patch has an effect for dealing with long lists of bugzilla.

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

Is this patch ready for check-in? Does it need more comments? Is it ready for submission to review and superreview?

Comment on attachment 145929 [details] [diff] [review] patch I request for review. This patch is only useful for bug list of bugzilla, because I do not have any other testcase for this bug. If this patch is not correct, please show me another testcase.

Comment on attachment 145929 [details] [diff] [review] patch This patch is just wallpaper over the issue I mention in comment 21. I'd have to do some extensive testing with multipart pages to do anything like mark review on it. I'm pretty sure it breaks cases when the parts of a page have significant delays between them, for example... In any case, I will not have that sort of time until at least after July 11.

> I request for review. This patch is only useful for bug list of bugzilla, > because I do not have any other testcase for this bug. If this patch is not > correct, please show me another testcase. > This bug is still here with 1.7 release (testing on win2k, 1280x1024x32 x 3 monitors). I am now seeing sites which worked ok with Browser-Back and don't remember position (randomly) any more. The Mozilla bug list is one that never worked and still doesn't work -- just select a bug from a longer list and click back, it almost never goes to the original scroll position. Make sure the window is _maximized_ when you do it. The odd thing is that after you press Back and the bug list gets shown at the top of the list (instead at the middle where you clicked a link to view a bug), if you click restore-window (i.e. to go from max to smaller size), the browser repositions to the correct place in the list, i.e. the position is not forgotten. It seems, from the incosistent reproducibility, this is a timing problem with the repaint, i.e. when you click Back the browser always goes into repaint from the top (it shouldn't do this anyway, it should stay where it is until ready to paint at the right place), then on some message (probably from the drawing/parsing engine when it is done) it moves to the correct scroll position. If the "done" message from the engine comes before the original paint-from-the-top is completed, the browser never goes to the newly constructed position. If you "restore" window, the browser finds "done" and paints the page at the correct position. To make things easily reproducable, one should debug by inserting and moving around, some Sleep(1000); statements, or similar, into the repaint code (especially the one which always paints at the top, following Back) to ferret out the race conditions. I think this whole strategy of refreshing and reconstructing a new page on "Back" is a bad idea in the first place. The "Back" ought to go instantly to the exaclty same document (if in cache) and same position as when the link was followed, no matter what the site's hhtp server tells the browser (like refresh to the top to see the ads again or some such). If user wishes to see the updated page, he can always click refresh. That would be, of course, user optimized behavior. The advertiser optimized behavior is to go re-fetch the latest ads and force user to scroll through the long list again. In any case, even if one thinks the "correct" behavior on the Back is to refresh, it should be a browser option so everyone can be happy (like the rolling and flashing images and other marketoid invented annoyances).

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

> It seems, from the incosistent reproducibility, this is a timing > problem with the repaint, No. It's a problem caused by the fact that a buglist is actually two separate HTML documents sharing the same session history entry. The rest of that paragraph is based on a false premise. > I think this whole strategy of refreshing and reconstructing a new page on > "Back" is a bad idea in the first place. That's actually fairly irrelevant to this bug (and has a separate bug filed on it). Note that we do in fact get the page from the cache; it's just that we cache the HTML, not a parsed version.

(In reply to comment #29) > > It seems, from the incosistent reproducibility, this is a timing > > problem with the repaint, > > No. It's a problem caused by the fact that a buglist is actually > two separate HTML documents sharing the same session history entry. The problem occurs even when the page is in the browser cache, i.e. no second fetch of the page from the network occurs (to check whether the page changed; I verified this with packet grabber). Take for example this page: http://cul.arxiv.org/list/hep-th/new Scroll down the list several pages, then click on some "abs" link to see the paper abstract. Then go back. No change occurs on the starting page and the browser gets it out of its cache. If you go to an "abs" and back several times, sometimes it comes back to the correct position, sometimes it goes back to the top. Interestingly, when this return-to-top occurs, if you toggle the browser window size (e.g. to "restored" size from the "max" size), the position repainted in the new window size is correct again (most of the time). That obviously has no relation with the page change at the server, it is purely an internal interaction between the painting code, html parser and the page cache. The problem has gotten worse in the latest Mozilla (1.7). This particular site used to work fine with an earlier Mozilla (1.5). And it works fine and has always worked fine with IE and Opera (the bug list works with them fine as well). Since you don't know what the problem cause is, you can't claim the race condition hypothesis is invalid unless you can show that it directly contradicts some factual behavior of the browser. As an experienced real-time network programmer looking at this problem from outside, all the symptoms point to a timing/race condition problem. The alleged claimed fixes so far seem to be merely shifting the timings randomly between different components/events and thus purely accidentally making the bug show more or less often. > > > I think this whole strategy of refreshing and reconstructing a new page on > > "Back" is a bad idea in the first place. > > That's actually fairly irrelevant to this bug (and has a separate bug > filed on it). It is relevant in the sense that if the browser had an approach to restore the exact image to the exact place, regardless of what the http told it about page change (user can always press refresh if he wishes to see whether there is a newer version; most of the time, when clicking "back" user wants to be exactly where he was when he clicked the link, so he can continue to the next link), the problem wouldn't exist. > Note that we do in fact get the page from the cache; > it's just that we cache the HTML, not a parsed version. That's where I think the problem arises. The browser, for some archaic reason repaints first the original page at the top, then (presumably after it finishes reconstructing the rest of the page) it moves down to the original place. That is a bad strategy -- it is disruptive to the user's eyeballs even when it works correctly. There is no real need to flash the old page at the top for a fraction of a second, then jump down to the right place. The browser should leave whatever is on the screen alone until it has the right stuff (at the right position) to show, then show it in one step. However intersting it may be to the developer, the end user doesn't really care to watch the phases of the drawing engine unfold in front of his eyes for the millionth time. Considering the important symptom of the correct position being recovered on resize (after it got stuck at the top), in combination with your description of the repeated reconstruction from the cached HTML, it seems that the parser module signals to the repaint module when it is done with the first visible page, the repaint module starts painting that part at the top, then parser signals when it has finished the whole page (or maybe the notifications go in many steps, one for each chunk/line completed). If at this point (of receiving the final parser completion message) the paint engine is caught wrong-footed (e.g. still painting the top of the old page) the parser done-notification is ignored or forgotten. When the initial repaint-at-the-top completes no "go to the old scroll position" is done. Now if you toggle the window size, the paint engine must be rechecking the parser status, and upon finding out the parser is done with the whole document it can go to the right place again (which it does). It could be that the check for the parser-done status is made only once at some point in the repaint-at-the-top phase and if it parser-done doesn't occur by the time of the check, no recheck and the go-to-the-correct-place is invoked.

> http://cul.arxiv.org/list/hep-th/new That's not a multipart page, so it's not this bug. If there is a problem with that page, please file a _new_ bug on it. > the bug list works with them fine as well How do you know? Bugzilla doesn't send a multipart response to either IE or Opera (IE doesn't support them at all, and Opera may not either; in any case, the bugzilla browser-sniffer sends opera the single-part version of buglists). So you're comparing apples and oranges. > Since you don't know what the problem cause is Huh? I know exactly what the problem cause is, based on tracing the program execution during a buglist load (verified in a debugger and all). I'm just not sure what a decent way to fix it is. > The alleged claimed fixes so far seem to be merely shifting the timings > randomly I'm afraid you've misunderstood the proposed patch. What it does is to attempt to scroll again if the first scroll didn't go anywhere. In the case of a multipart document in which the parts before the "last" one are all shorter than the viewport (eg bugzilla buglists), that happens to help. > if the browser had an approach to restore the exact image to the exact place If the page were an image, this would be trivial. The problem is that there is a lot more to a page than just the bitmap. Restoring the various datastructures associated with a page is a fairly non-trivial undertaking. I agree that it would eliminate the buglist issue, since it would mean we would only render the "last" part of the multipart document (whatever that means, in general). The rest of the comments, again, have nothing to do with this bug. This bug is about the fact that we're rendering _two_ separate HTML documents and only have _one_ stored scroll position. So when we render the first document we scroll to the scroll position we have stored, and then when we render the second HTML document we don't scroll. The key problem here is that we're just not storing enough data to properly restore state. (If you're interested, feel free to mail me for a reasonable description of how the parser and layout engine actually interact here.)

> The key problem here is that we're just not storing > enough data to properly restore state. I'm afraid that this is not all there is to the scroll position restore problem. You have found one among multiple causes of failed scroll restoration. Since this one is deterministic (the variable will always get overwritten on this type of document), it almost surely won't fix the wider scroll problem which does have random activation e.g. the url and the description I gave earlier produce the scroll problem erratically even when exactly the same sequence of steps is repeated (the page changes once a day and the server response is identical if you repeat the same link clicks).

(In reply to comment #32) > I'm afraid that this is not all there is to the scroll > position restore problem. indeed. but this bug is about a _specific_ problem, that only happens with x-mixed-replace. other problems should go into other bugs. feel free to file them if they are not alraedy known.

Forget it. I'm tired of the unrelated spam this bug is getting.

Comment on attachment 145929 [details] [diff] [review] patch This patch is incorrect, in my opinion, as previously described in this bug....

...this is the NUMBER ONE bug keeping me from using mozilla regularly as many of my sites are long lists, news scrolls, archives, not to mention my fun sites, ahem, which are also long lists. I have been watching this bug for almost 2 years. Most of the pages i look at are not multi-part htmls like the bugzilla list, however, all the related bugs i was watching got marked as duplicates of this one after someone posted one example of a multi-part page. i don't know if the single-page scroll was fixed or not, if they are unrelated or not, but to now say "file a new bug if its not multi-part" -- especially to those of us who don't know how to fix/program anything... seems to be a large loop. caching bug http://bugzilla.mozilla.org/show_bug.cgi?id=56346 should have delt with this back, right? as the years march on, i'm not sure how, even though these 'bugs' are separate bugs, they are still related as all deal with the specific problem of caching, back, history, tables, form data, sessions, and multi-part pages. Going back today and reading all the related bugs, marked as dupes, etc., there were several problems that grow and fix as a result of dealing with another one individually without thinking about associated bugs. Might be a good time to review them and the general problem with "back" button people have had, either with caching, form data, multi-part, session times, etc. -- while they all may be separate problems, they are all 'back button' problems. no sense creating another one when fixing one. some bugs of the past: http://bugzilla.mozilla.org/show_bug.cgi?id=74639 http://bugzilla.mozilla.org/show_bug.cgi?id=56346 http://bugzilla.mozilla.org/show_bug.cgi?id=40867 i wish i could give advice or new info; i simply don't know enough to add anything meaningful to the discussion. thanks for listening. have a good day,

> (From update of attachment 145929 [details] [diff] [review]) > This patch is incorrect, in my opinion, as previously described in this bug.... bzbarsky, thanks for your indication. Although a patch has a little change that is |mRestoreRect| is saved to |aPresContext|, the basic idea has not changed.

Hideo Saito, could you please do a diff with more context and function names? Something like -up8 should do the job... Also, does this patch work right if there are more than two parts? It seems that it resets the multimixed scrolling boolean after the first part... Finally, if I have a 20-minute delay between two parts, what will be the behavior of this patch?

> Hideo Saito, could you please do a diff with more context and function names? > Something like -up8 should do the job... I will do so for next patch. > Also, does this patch work right if there are more than two parts? It seems > that it resets the multimixed scrolling boolean after the first part... I think that this patch is not right for more than two parts. However I think this problem is able to be corrected by setting multimixed scrolling boolean again when restoredRect is set. > Finally, if I have a 20-minute delay between two parts, what will be the > behavior of this patch? For example, first part page 'B' replaces current page 'A' that is second part page, and new page 'C' replaces page 'B'. If page 'A' differs from page 'C' or even if both pages are equal, the scroll position of the page 'A' is passed to page 'B' as relative scroll position by this patch.

I did a diff with -pu8

(In reply to comment #40) > I think that this patch is not right for more than two parts. Right. So why are we special-casing the two-part case? > For example, first part page 'B' replaces current page 'A' that is second part > page, and new page 'C' replaces page 'B'. > If page 'A' differs from page 'C' or even if both pages are equal, the scroll > position of the page 'A' is passed to page 'B' as relative scroll position by > this patch. I'm not following, to be truthful... Say I load a page with two parts 'A' and 'B'. I scroll halfway down part A, then part B loads. I scroll all the way to the bottom of part B. I then reload the page. Part A loads again (and part B will load 20 minutes afterwards). What should our scrolling behavior be when A loads during the reload? When B loads? What's the behavior with this patch?

Comment on attachment 158545 [details] [diff] [review] patch for released mozilla-1.8a3 Marking review- to get answers to those questions...

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

A few was changed. However, a basic idea does not change. That is, a scroll position is passed as a relative position. However, an original scroll position is saved if the page does not have a scroll bar. It acts as follows with loading page 'A'. ----+-+ ----+ ----+-+ |-| AAA | |-| AAA | | | AAA | | <-- keep the original position |-| | |-| ----+-+ ----+ ----+-+ reload 'A' reload 'A' reload 'A' width="400" width="100" width="1000" It acts as follows with loading page 'A' and multipart page 'B' and 'C'. ----+-+ ----+-+ ----+-+ ----+-+ |-| |-| |-| |-| AAA | | BBB | | CCC | | AAA | | <-- keep |-| |-| |-| |-| the near position ----+-+ ----+-+ ----+-+ ----+-+ reload 'A' load 'B' load 'C' load 'A' width="400" width="500" width="1000" width="500" If the page has multipart pages and the pages have scroll position, since a relative position is changed into a pixel value, don't return to the original position completely.

> Say I load a page with two parts 'A' and 'B'. I scroll halfway down part A, > then part B loads. I scroll all the way to the bottom of part B. > I then reload the page. Part A loads again (and part B will load 20 minutes > afterwards). What should our scrolling behavior be when A loads during the reload? > When B loads? What's the behavior with this patch? I explain the behavior as follows. 1. 2. 3. 4. ----+-+ ----+-+ ----+-+ ----+-+ |-| BBB | | | | | | AAA | | |-| |-| |-| |-| | | BBB | | AAA | | ----+-+ ----+-+ ----+-+ ----+-+ part 'A' load part 'B' scroll down reload the page 5. 6. ----+-+ ----+-+ |-| |-| AAA | | BBB | | |-| |-| ----+-+ ----+-+ load part 'B' as 2. -- the initial position of the loaded part 'B' is top as 4. -- the position of the reloaded part 'A' is passed from part 'B' as 6. -- the position of the part 'B' is passed from part 'A'

Right. So why are we using the scroll position of A for part B? They're different webpages....

In 30 secs I found two dupes (sort of): https://bugzilla.mozilla.org/show_bug.cgi?id=258133 https://bugzilla.mozilla.org/show_bug.cgi?id=216376 Pressing "Back" that goes to a page that set nocache/expires headers doesn't remember the page position either...

I'm not clear if this bug has been fixed. If there is a patch that is ready for the end user, could someone point me toward the process for installing a patch on my PC.

(In reply to comment #48) > In 30 secs I found two dupes (sort of): Neither of those is a dup of this bug. > Pressing "Back" that goes to a page that set nocache/expires headers doesn't > remember the page position either... That's bug 215405 (In reply to comment #49) > I'm not clear if this bug has been fixed. If it had been, it would be marked "fixed"....

...by the way, i've been watching this forever. Just wanted to say that it has shown up for me in firefox a couple times, although not nearly as much as in mozilla (which i had to give up on). Dont know tech of why its happening or how to help, but, anyone else notice it there too? oh well......i.e. hangs on for a few sites. i'm so disappointed.

This approach differs from previous patches. I try to number the multipart pages at nsMultiMixedConv.cpp and this number passes to a new member mMultiMixedContentID added to a class of the nsPresContext by using nsDocShell::CreateContentViewer. If the argument aID of the function of the nsContentUtils::GenerateStateKey is not eNoID, since aID is added aSubID that is the number of multipart, each saved and restored status of the multipart pages are distinguished, though the multipart pages have same history state(nsILayoutHistoryState).

Comment on attachment 170151 [details] [diff] [review] patch >diff -p8 -Naur ./netwerk/base/public/nsIChannel.idl.org nsIChannel is a frozen interface. I wouldn't make changes to it.... It also seems to me that this would belong better on nsIMultiPartChannel >diff -p8 -Naur ./content/base/src/nsContentUtils.cpp.org > nsContentUtils::GenerateStateKey(nsIContent* aContent, > nsIStatefulFrame::SpecialStateID aID, >- nsACString& aKey) >+ nsACString& aKey, >+ PRInt32 aSubID) Please document the new argument? And I'm not sure that's the best name for the argument.... >- KeyAppendInt(aID, aKey); >+ KeyAppendInt(aID + aSubID, aKey); This looks wrong, since it can cause ID duplication when both aID and aSubID actually differ. >diff -p8 -Naur ./docshell/base/nsDocShell.cpp.org This code could be simplified to just get the part id off the multipart channel. >--- ./layout/base/src/nsPresContext.cpp.org 2004-09-17 16:27:01.000000000 +1000 >+++ ./layout/base/src/nsPresContext.cpp 2005-01-03 23:36:02.716836800 +0900 >@@ -772,16 +772,17 @@ nsPresContext::UpdateCharSet(const nsAFl >+ mMultiMixedContentID = 0; Why? >diff -p8 -Naur ./layout/html/base/src/nsFrameManager.cpp.org >+ PRInt32 subID = presContext ? presContext->GetMultiMixedContentID() : 0; Why do you need a null-check here? >+ PRInt32 subID = presContext ? presContext->GetMultiMixedContentID() : And here? Overall, this is a much better approach. Once the details are resolved, I would be happy with reviewing it, I think.

I do think that flags can be added to nsIChannel, despite it being frozen; as long as people deal with the flag being never set by certain impls. the comment is somewhat misleading since it is not actually the uriloader which sets it

Oh, sure. But in this case there is absolutely zero need for an nsIChannel flag. Just exposing the part number on the part channel is cleaner and leads to simpler code.

Comment on attachment 170459 [details] [diff] [review] patch ./netwerk/base/public/nsIMultiPartChannel.idl + * Access to the index of the multipart/mixed content. + */ + attribute PRInt32 multiMixedContentID; why not make this readonly, and to set it, pass it to the constructor?

Comment on attachment 170459 [details] [diff] [review] patch >diff -p8 -Naur ./netwerk/base/public/nsIMultiPartChannel.idl.org >+ /** >+ * Access to the index of the multipart/mixed content. >+ */ >+ attribute PRInt32 multiMixedContentID; There's nothing in this interface specific to multipart/mixed. It could just as well be used for multipart/alternative, say. I'd call that attribute partID, and clearly say that it's just guaranteed to be different for different parts of the same multipart document. I'd also make this readonly, as biesi suggests. >diff -p8 -Naur ./netwerk/streamconv/converters/nsMultiMixedConv.h.org >+ PRInt32 mMultiPartID; // actual number of the index of the multipart/mixed content Call this mCurrentPartID, please. And nix the comment -- it's more confusing than helpful. > ./netwerk/streamconv/converters/nsMultiMixedConv.cpp.org >+ PRInt32 mMultiMixedContentID; // a copy of the index of the multipart/mixed content Perhaps mPartID? And maybe document this as "unique ID that can be used to identify this part of the multipart document"? > nsPartChannel::nsPartChannel(nsIChannel *aMultipartChannel) : >+ mMultiMixedContentID = 0; Please just pass the ID to the constructor as biesi suggested. And use a C++ initializer (and use one for mMultipartChannel too?). >diff -p8 -Naur ./content/base/src/nsContentUtils.cpp.org >+static inline void KeyAppendIntWithPartID(PRInt32 aInt, nsACString& aKey, PRInt32 aMultiPartID) >+{ >+ KeyAppendInt(aInt + aMultiPartID, aKey); >+} This is wrong, as I said in comment 53. > nsContentUtils::GenerateStateKey(nsIContent* aContent, Don't you always want the state key to account for the aMultiPartID? Otherwise form controls in one part will affect those in another part, no? Which means that the part id should not be optional, btw. >diff -p8 -Naur ./docshell/base/nsDocShell.cpp.org >+ nsCOMPtr<nsIPresShell> shell; >+ rv = GetPresShell(getter_AddRefs(shell)); This seems backwards. Don't you want to QI to nsIMultiPartChannel and then only do this stuff if the QI succeeds? >+ presContext->SetMultiMixedContentID(multiMixedContentID); SetPartID please. Nothing specific to multipart/mixed about it.. >diff -p8 -Naur ./layout/base/public/nsPresContext.h.org The "a copy" comments don't really make sense. Just say that if this prescontext is for a document that's part of a multipart document, the ID can be used to distinguish it from the other parts. On a more serious note, why is this being stored in the prescontext? I'd store it in the document. There's nothing presentation-specific about the part id. >diff -p8 -Naur ./layout/base/src/nsPresContext.cpp.org Again, please use C++ initializers in constructors.

Sorry, I can not fully understand your suggestion. Please let me know more concretely if this patch is wrong yet.

> > nsPartChannel::nsPartChannel(nsIChannel *aMultipartChannel) : > >+ mMultiMixedContentID = 0; > Please just pass the ID to the constructor as biesi suggested. > And use a C++ initializer (and use one for mMultipartChannel too?). Sorry, I can not fully understand your suggestion. Please let me know more concretely if this patch is wrong yet.

> > Please just pass the ID to the constructor as biesi suggested. >> And use a C++ initializer (and use one for mMultipartChannel too?). > Sorry, I can not fully understand your suggestion. 1) Instead of having a SetPartID method, make the part id one of the arguments you pass to the nsPartChannel constructor. So change: nsPartChannel(nsIChannel *aMultipartChannel); to nsPartChannel(nsIChannel *aMultipartChannel, PRUint32 aPartID); (and make it an unsigned int throughout -- we don't plan to have negative part ids, do we?) 2) Instead of doing + mCurrentPartID = 0; in the nsMultiMixedConv constructor, add mCurrentPartID(0) in the right place in the initializers before the body of the constructor. Similar for other places where you're modifying constructors. Apart from that, you are now losing aID in GenerateStateKey and you didn't fix the other parts of GenerateStateKey like I said you should.

nsIMultiPartChannel.idl needs a new IID

Comment on attachment 170665 [details] [diff] [review] patch >diff -p8 -Naur ./netwerk/base/public/nsIMultiPartChannel.idl.org Like biesi said, please change the IID. >+ * readonly attribute to be used for multipart/alternative. This line is really not needed. How about just: Attribute guaranteed to be different for different parts of the same multipart document. >diff -p8 -Naur ./content/base/public/nsContentUtils.h.org >+ nsIDocument *doc = aContent->GetDocument(); >+ PRUint32 partID = doc ? doc->GetPartID() : 0; >+ > // SpecialStateID case - e.g. scrollbars around the content window > // The key in this case is the special state id (always < min(contentID)) > if (nsIStatefulFrame::eNoID != aID) { >+ KeyAppendInt(partID, aKey); // first append a partID > KeyAppendInt(aID, aKey); > return NS_OK; > } > > // We must have content if we're not using a special state id > NS_ENSURE_TRUE(aContent, NS_ERROR_FAILURE); So if we're using a special state id, we can have a null aContent? But then your code crashes, doesn't it? Please check on this and either fix the new code or the old code? The rest of the patch looks fine; maybe file a followup bug on the fact that getting the document in docshell is such a chore? CC me on that bug, please.

The function of nsContentUtils::GenerateStateKey is only called from ./content/html/content/src/nsGenericHTMLElement.cpp and ./layout/html/base/src/nsFrameManager.cpp. It seems that aContent is not null now. Is a following modification right? nsIDocument *doc = aContent ? aContent->GetDocument() : nsnull; PRUint32 partID = doc ? doc->GetPartID() : 0; or should partID be passed as an argument? nsContentUtils::GenerateStateKey(nsIContent* aContent, nsIStatefulFrame::SpecialStateID aID, nsACString& aKey, PRUint32 aPartID)

I'd prefer that the part id (or just the document, which is probably cleaner) got passed to the function...

If the content is null, the changes for nsFrameManager::CaptureFrameStateFor cause to exit from the function for the same reason as nsFrameManager::RestoreFrameStateFor.

So... could we make up our minds here? If the aContent for GenerateStateKey must always be non-null, please document that, remove null-checks in GenerateStateKey(), change callers accordingly as this patch does? If it's allowed to be null, why are we changing CaptureFrameStateFor()? Also, please use GetCurrentDoc() instead of GetDocument() to get the document from the content.

I think that it is not necessary to exit from CaptureFrameStateFor() in the case of the context is null, because null-check of the document is done in GenerateStateKey().

Comment on attachment 171120 [details] [diff] [review] patch r=bzbarsky, if you update the nsIMultiPartChannel iid, like biesi and I asked you to.

Comment on attachment 171120 [details] [diff] [review] patch - In nsIDocument.h: + void SetPartID(PRUint32 aID) { + mPartID = aID; + } + + /** + * Return the ID used to identify this part of the multipart document + */ + PRUint32 GetPartID() const { return mPartID; } Since you split the first one-line inline function onto more than one line, split this one too, and put the opening brace on its own line to follow the existing style in this file. sr=jst with that, and what bz said...

I modified IID of nsIMultiPartChannel.idl and nsIDocument.h. Masayuki Nakano(Mozilla Japan) created new IID from vc++.

Hmm.. that patch doesn't seem to apply against a current trunk build. In particular, hunks in netwerk/streamconv/converters/nsMultiMixedConv.h, netwerk/streamconv/converters/nsMultiMixedConv.cpp, layout/html/base/src/nsFrameManager.cpp (which no longer exists, since that file was moved in the layout reorg to layout/base/nsFrameManager.cpp). Could you make the patch against a current tree? And also, change the iid of nsIDocument, please? Sorry I didn't catch that the first time through... :(

I modified IID of nsIDocument.h using source on latest-trunk. I created IID from uuidgen on Linux.

Patch checked in for 1.8b. Hideo Saito, thank you for being persistent!

bzbarsky, I appreciate for your many advices. I regret that this solution was delayed by having persisted in the first idea.

Thanks for fixing this annoying bug, but it seems that when I set View -> Character Encoding -> Auto-Detect to Universal and I go back to/reload a long b.m.o. buglist, its scroll position is reset to the top of the page like in earlier releases. With View -> Character Encoding -> Auto-Detect-> (Off) (or a different character encoding) the buglist does retain its previous position. I see this with Mozilla 1.8b.

> when I set View -> Character Encoding -> Auto-Detect to Universal That's a general problem (with bugs filed on it), iirc... Not specific to buglists.