47350 - Current scroll position not retained, reloading or going back to multipart/x-mixed-replace

Status: RESOLVED FIXED

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

Description

[John Morrison]

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.

Comment 1

[Radha on family leave (not reading bugmail)]

pollmann is dealing with this right now

Comment 2

[Eric Pollmann]

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.

Comment 3

[Viswanath Ramachandran]

nav triage team: not a beta stopper. 

Comment 4

[Jesse Ruderman]

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).

Comment 5

[Jarrod Gray]

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.

Comment 6

[Kevin McCluskey (gone)]

Bulk reassigning form bugs to Alex

Comment 7

[R.K.Aa.]

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

Comment 8

[Cyrus Patel]

removed myself from CC:

Comment 9

[Boris 'pi' Piwinger]

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

Comment 10

[Manoj]

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.

Comment 11

[Radha on family leave (not reading bugmail)]

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

Comment 12

[Hideo Saito]

Attachment: patch

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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 [e-mail] for the review; I can do the
super-review (sorta did already, earlier in this comment ;) )

Comment 14

[Hideo Saito]

Attachment: patch

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.

Comment 15

[John Keiser (jkeiser)]

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?

Comment 16

[Hideo Saito]

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.

Comment 17

[WD]

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

Comment 18

[Vanessa Dannenberg]

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

Comment 19

[Rodney]

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...

Comment 20

[Hideo Saito]

Attachment: patch

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 &&

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Hideo Saito]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Andrew Hagen]

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

Comment 25

[Hideo Saito]

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 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Ratko Tomic]

> 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).

Comment 28

[R.K.Aa.]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 30

[Ratko Tomic]

(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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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.)

Comment 32

[Ratko Tomic]

> 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). 

Comment 33

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

(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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145929 [details] [diff] [review]
patch

This patch is incorrect, in my opinion, as previously described in this bug....

Comment 36

[john]

...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,

Comment 37

[Hideo Saito]

Attachment: patch for released mozilla-1.8a2

> (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.

Comment 38

[Hideo Saito]

Attachment: patch for released mozilla-1.8a3

Comment 39

[Boris Zbarsky [:bzbarsky]]

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?

Comment 40

[Hideo Saito]

> 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.

Comment 41

[Hideo Saito]

Attachment: patch for released mozilla-1.8a3

I did a diff with -pu8

Comment 42

[Boris Zbarsky [:bzbarsky]]

(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 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158545 [details] [diff] [review]
patch for released mozilla-1.8a3

Marking review- to get answers to those questions...

Comment 44

[Boris Zbarsky [:bzbarsky]]

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

Comment 45

[Hideo Saito]

Attachment: patch

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.

Comment 46

[Hideo Saito]

> 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'

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[Lars Petersen]

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...

Comment 49

[Dennis]

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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

(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"....

Comment 51

[john]

...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.

Comment 52

[Hideo Saito]

Attachment: patch

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 53

[Boris Zbarsky [:bzbarsky]]

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.

Comment 54

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

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

Comment 55

[Boris Zbarsky [:bzbarsky]]

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 56

[Hideo Saito]

Attachment: patch

Comment 57

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

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 58

[Boris Zbarsky [:bzbarsky]]

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.

Comment 59

[Hideo Saito]

Attachment: patch

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

Comment 60

[Hideo Saito]

> > 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.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> > 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.

Comment 62

[Hideo Saito]

Attachment: patch

Comment 63

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

nsIMultiPartChannel.idl needs a new IID

Comment 64

[Boris Zbarsky [:bzbarsky]]

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.

Comment 65

[Hideo Saito]

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)

Comment 66

[Boris Zbarsky [:bzbarsky]]

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

Comment 67

[Hideo Saito]

Attachment: patch

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

Comment 68

[Boris Zbarsky [:bzbarsky]]

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.

Comment 69

[Hideo Saito]

Attachment: patch

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 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171120 [details] [diff] [review]
patch

r=bzbarsky, if you update the nsIMultiPartChannel iid, like biesi and I asked
you to.

Comment 71

[Johnny Stenback (:jst)]

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...

Comment 72

[Hideo Saito]

Attachment: patch for checkin

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

Comment 73

[Boris Zbarsky [:bzbarsky]]

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... :(

Comment 74

[Hideo Saito]

Attachment: patch for checkin

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

Comment 75

[Boris Zbarsky [:bzbarsky]]

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

Comment 76

[Hideo Saito]

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

Comment 77

[zug_treno]

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.

Comment 78

[Boris Zbarsky [:bzbarsky]]

> 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.