Closed
Bug 300800
Opened 19 years ago
Closed 19 years ago
Unintended (?) differences between LOAD_NORMAL and LOAD_NORMAL_EXTERNAL
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla1.8beta4
People
(Reporter: bzbarsky, Assigned: Biesinger)
References
Details
(Keywords: fixed1.8)
Attachments
(1 file, 1 obsolete file)
1.22 KB,
patch
|
bryner
:
review+
bzbarsky
:
superreview+
mtschrep
:
approval1.8b4+
|
Details | Diff | Splinter Review |
The patch checked in for bug 298255 makes LOAD_NORMAL and LOAD_NORMAL_EXTERNAL differ in some ways that seem pretty major to me. At least, we have: 1) LOAD_NORMAL_EXTERNAL isn't allowed to just scroll for anchor loads (because the old page is gone by that point, it can't really happen; best case is that we just reload the page, worst case is that we just show about:blank when this is tried). 2) fastback isn't allowed for a page left via a LOAD_NORMAL_EXTERNAL load (again, because the page is gone). More precisely, because we nuke the old viewer, I think fastback will end up finding the about:blank viewer, so going back to a page that we left via LOAD_NORMAL_EXTERNAL wouldn't work at all.
Reporter | ||
Comment 1•19 years ago
|
||
We should really get this fixed before we ship these changes...
Flags: blocking1.8b4?
Reporter | ||
Updated•19 years ago
|
Blocks: blazinglyfastback
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Reporter | ||
Comment 2•19 years ago
|
||
Issue (3): Loading the same URI via LOAD_NORMAL_EXTERNAL will convert the load to LOAD_NORMAL_REPLACE and reuse the old session history entry. Has someone tested that this actually works correctly given that we nuke the old viewer?
Comment 3•19 years ago
|
||
(In reply to comment #1) > We should really get this fixed before we ship these changes... Too late -- 1.0.5 escaped the lab, and the patch for bug 298255 is in the 1.0.6 candidate builds; it caused bug 301073 as well. /be
Comment 4•19 years ago
|
||
I'm not retargetting this since it covers multiple issues, but the bfcache portion of this is not a release blocker IMO since it just means we miss a cache opportunity. I was not able to reproduce any case where we return to the about:blank viewer.
Comment 5•19 years ago
|
||
probably causes bug 300114
Comment 6•19 years ago
|
||
bz, biesi, can one of you look into this? Dan's on vacation and we'd like to get this.
Reporter | ||
Comment 7•19 years ago
|
||
I'm not going to have a chance to look at this for at least a week and a half. I suspect the right fix is to: 1) Nuke the content viewer much later (after we try anchor scrolling, to be precise). 2) Ensure the resulting code plays nice enough with bfcache.
Reporter | ||
Comment 8•19 years ago
|
||
I probably won't be able to look for at least a week. Note also bug 295058 and bug 300114 which seem to be related and for which I have no idea why they happen.
Assignee | ||
Updated•19 years ago
|
Assignee: dveditz → cbiesinger
Target Milestone: --- → mozilla1.8beta4
Updated•19 years ago
|
Flags: blocking1.8b4rc?
Updated•19 years ago
|
Flags: blocking1.8b4rc? → blocking1.8b4rc+
Comment 9•19 years ago
|
||
Bz and/or Biesi - either of you going to take a look at this? If not who else can help?
Assignee | ||
Comment 10•19 years ago
|
||
I'll try to look at this tomorrow. I want to note, though, that there is code in docshell that changes LOAD_NORMAL_EXTERNAL to LOAD_NORMAL. this doesn't affect the anchor scroll part of comment 0; haven't looked at the other parts yet.
Assignee | ||
Comment 11•19 years ago
|
||
> going back to a page that we left via LOAD_NORMAL_EXTERNAL wouldn't work at
> all.
that should still work, as CreateAboutBlankContentViewer nulls out mOSHE.
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #2) > Issue (3): Loading the same URI via LOAD_NORMAL_EXTERNAL will convert the load > to LOAD_NORMAL_REPLACE and reuse the old session history entry. Has someone > tested that this actually works correctly given that we nuke the old viewer? bz, I'm not sure how you arrived at this conclusion... I don't see a way for this to happen; CreateAboutBlankContentViewer sets mCurrentURI to about:blank, thus the OnNewURI code that does that resetting should not fire. Am I missing something?
Assignee | ||
Comment 13•19 years ago
|
||
Sorry for splitting this up in two parts, but I distributed the patch over two computers...
Attachment #194496 -
Flags: superreview?(bzbarsky)
Attachment #194496 -
Flags: review?(bryner)
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #194497 -
Flags: superreview?(bzbarsky)
Attachment #194497 -
Flags: review?(bryner)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Updated•19 years ago
|
Attachment #194496 -
Flags: review?(bryner) → review+
Updated•19 years ago
|
Attachment #194497 -
Flags: review?(bryner) → review+
Reporter | ||
Comment 15•19 years ago
|
||
> CreateAboutBlankContentViewer sets mCurrentURI to about:blank
Ah, ok. Clearly I should have read CreateAboutBlankContentViewer() a lot more
carefully... :(
Reporter | ||
Updated•19 years ago
|
Attachment #194496 -
Flags: superreview?(bzbarsky) → superreview+
Reporter | ||
Updated•19 years ago
|
Attachment #194497 -
Flags: superreview?(bzbarsky) → superreview+
Comment 16•19 years ago
|
||
This has nothing to do with bug 295058 and bug 300114
Assignee | ||
Comment 17•19 years ago
|
||
Comment on attachment 194497 [details] [diff] [review] part II: Fix anchor scrolling hm... this patch shouldn't actually be needed, now that I thought about it some more: clearly, the URIs don't compare equal (see above about CreateAboutBlankContentViewer setting mCurrentURI) it's a bit of a perf enhancement I guess, and might make the code clearer... should I check it in? I'm leaning towards no.
Assignee | ||
Comment 18•19 years ago
|
||
Comment on attachment 194496 [details] [diff] [review] part I: Fix fastback patch checked in to trunk This should be a pretty low risk patch to make fastback work when a page is loaded externally (e.g. XRemote, or the windows remoting code) Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.738; previous revision: 1.737 done
Attachment #194496 -
Flags: approval1.8b4?
Comment 19•19 years ago
|
||
once that first fix is verified on the trunk, please re-request approval for the branch. thanks.
Assignee | ||
Updated•19 years ago
|
Attachment #194496 -
Flags: approval1.8b4?
Assignee | ||
Comment 20•19 years ago
|
||
marking fixed, if someone thinks I should check in the shistory patch please speak up.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•19 years ago
|
||
I meant the anchor scrolling one.
Comment 22•19 years ago
|
||
I think both patches should go on the branch, if they test well on the trunk. /be
Assignee | ||
Comment 23•19 years ago
|
||
(In reply to comment #22) > I think both patches should go on the branch, if they test well on the trunk. the anchor scrolling one isn't currently checked in on trunk; see comment 17 as for why. or, did you mean that I should check it in on trunk?
Updated•19 years ago
|
Attachment #194496 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #194497 -
Flags: approval1.8b4?
Comment 24•19 years ago
|
||
after this is verified on the trunk, we'll consider for branch approval.
Comment 25•19 years ago
|
||
need a tescase. How do we test/verify this?
Assignee | ||
Comment 26•19 years ago
|
||
testcase: - run seamonkey (or firefox) - load a url in it - click on a uri in another program (say, thunderbird) or: start/run, type in a uri - wait for uri to load in firefox - click back: Fastback should be used for going back here testcase2: - load a url that includes named anchors (e.g. lxr) e.g. http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp - start/run, use the URI from before, but make sure that it includes a named anchor e.g. http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5160 The end result should be that the URI is loaded + scrolled to that anchor
Comment 27•19 years ago
|
||
Sorry if this is a stupid question - but: testcase1: how do you tell if fastback is being used testcase2: This passes for me on the 9-4 branch build. Must be doing something wrong . . .
Assignee | ||
Comment 28•19 years ago
|
||
(In reply to comment #27) > testcase1: how do you tell if fastback is being used That's a good question :) "it should be fast" > testcase2: This passes for me on the 9-4 branch build. Must be doing something > wrong . . . Nah, that's ok, just wanted to make sure (like comment 17 said, I didn't think that that part needed any work)
Updated•19 years ago
|
Attachment #194497 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 30•19 years ago
|
||
Comment on attachment 194497 [details] [diff] [review] part II: Fix anchor scrolling actually, per the results of comment 27 this patch isn't needed... but approval on the other one would still be good.
Attachment #194497 -
Attachment is obsolete: true
Updated•19 years ago
|
Attachment #194496 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 31•19 years ago
|
||
attachment 194496 [details] [diff] [review] checked in to MOZILLA_1_8_BRANCH: Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.719.2.14; previous revision: 1.719.2.13 done
Keywords: fixed1.8
You need to log in
before you can comment on or make changes to this bug.
Description
•