Closed
Bug 277224
Opened 20 years ago
Closed 20 years ago
[FIXr]location.replace("#foo") stops document loads
Categories
(Core :: DOM: Navigation, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(3 files, 1 obsolete file)
159 bytes,
text/html
|
Details | |
5.40 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
12.42 KB,
patch
|
Details | Diff | Splinter Review |
This is akin to bug 233963, but for location.replace(). The same solution as there will not work, since we want different load flags. So I'm left wondering -- why is the Stop() call in nsLocation anyway? Wouldn't it be better to put it in docshell, which already does Stop() inside InternalLoad(), and just have a way for the caller to indicate that Stop() should get the STOP_CONTENT flag? That way the hack from bug 233963 could be removed, too.
Assignee | ||
Updated•20 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Comment 1•20 years ago
|
||
Comment 2•20 years ago
|
||
Stop has interesting side-effects. Do we need to worry about the ordering of events at all? If Stop it delayed until InternalLoad could we be changing the order of the side-effects in any way? (I sort of suspect not; your plan sounds like a good one.)
Assignee | ||
Comment 3•20 years ago
|
||
The basic idea is to not Stop() when we're just doing an anchor scroll. Since InternalLoad() would be getting called sync from the code in question, I suspect it's fine. The Stop() is just there to avoid us parsing any more data (which may have come in from the network already) after the location has been changed. For that purpose, it's ok as long as it happens within the location setter somewhere.
Comment 5•20 years ago
|
||
This fixes this bug by using nsIURL::GetRetRelativeSpec() to check the difference from the current URI and the URI we're loading, and if we're loading a ref, we won't STOP_CONTENT at all.
Attachment #170489 -
Flags: superreview?(darin)
Attachment #170489 -
Flags: review?(bzbarsky)
Comment 6•20 years ago
|
||
Comment on attachment 170489 [details] [diff] [review] Fix (diff -w). Note that this is a diff -w
Attachment #170489 -
Attachment description: Fix. → Fix (diff -w).
Comment 7•20 years ago
|
||
bz, sorry, didn't read your initial comment before I started looking into this. Yes, ideally the docshell would deal with all of this...
Assignee | ||
Comment 8•20 years ago
|
||
Johnny, do you still want review on that diff?
Comment 9•20 years ago
|
||
Hmm, I guess I don't care, though the changes to nsStandardURL still apply (even if not needed for this change per se). So yeah, but only the nsStandardURL part.
Assignee | ||
Comment 10•20 years ago
|
||
Attachment #171371 -
Flags: superreview?(jst)
Attachment #171371 -
Flags: review?(darin)
Assignee | ||
Updated•20 years ago
|
Summary: location.replace("#foo") stops document loads → [FIX]location.replace("#foo") stops document loads
Assignee | ||
Comment 11•20 years ago
|
||
Comment on attachment 170489 [details] [diff] [review] Fix (diff -w). >Index: netwerk/base/src/nsStandardURL.cpp >+ if (((*thatIndex == '#') && !*thisIndex) || foundHash || So that first condition is "this has no anchor and the other does", "foundHash" is "both have anchors".... >+ ((thatIndex != startCharPos) && (*(thatIndex - 1) == '#'))) { What case does this condition correspond to? Document it, please (as well as the other two)?
Comment 12•20 years ago
|
||
Comment on attachment 171371 [details] [diff] [review] Solution with load flags >Index: docshell/base/nsIWebNavigation.idl >+ /* >+ * XXXbz note that these flags are generally mutually exclusive; the two >+ * exceptions are that one can specify both LOAD_FLAGS_BYPASS_CACHE and >+ * LOAD_FLAGS_BYPASS_PROXY together and one can specify both >+ * LOAD_FLAGS_REPLACE_HISTORY and LOAD_FLAGS_STOP_CONTENT together. >+ */ >+ /* >+ * XXXbz some of these flags only apply to reload(), but that's not >+ * clearly documented >+ */ Ack.. see my patch for bug 99625. I really need to polish off that patch! I'd rather you leave out these comments so that there is less to conflict with when I try to update the other (which I plan to do once that POPUP load flag is sorted out). > /** >+ * Flag to force stopping of content if the load ends up happening >+ * (default is to only stop network activity). >+ */ >+ const unsigned long LOAD_FLAGS_STOP_CONTENT = 0x0800; Might be good to mention what stopping content means here or at least to refer to the STOP_CONTENT flag. Also, why is the default to only stop network activity? Is that because we want to keep animating images during page transition? (BTW, I assume the content is stopped once the load that "ends up happening" is handled by this webnav, and not before, right? >Index: docshell/base/nsDocShell.cpp >+ } else if (LOAD_TYPE_HAS_FLAGS(aLoadType, LOAD_FLAGS_STOP_CONTENT)) { >+ rv = Stop(nsIWebNavigation::STOP_CONTENT); > } else { > rv = Stop(nsIWebNavigation::STOP_NETWORK); Does STOP_CONTENT also imply STOP_NETWORK? Don't we need to stop both content and network? r=darin
Attachment #171371 -
Flags: review?(darin) → review+
Assignee | ||
Comment 13•20 years ago
|
||
> Ack.. see my patch for bug 99625. OK. I'll take these changes out, but could you roll something equivalent into your patch? > Might be good to mention what stopping content means here or at least > to refer to the STOP_CONTENT flag. Good point. Will do that. > Also, why is the default to only stop network activity? Is that because we > want to keep animating images during page transition? I guess so... Not actually sure. ;) The checkin comments aren't very helpful here. > (BTW, I assume the content is stopped once the load that "ends up happening" > is handled by this webnav, and not before, right? Nope. The Stop() call is made right before we try to NS_NewChannel. The main reason for this is that we want to stop before we add the new channel to our loadgroup; stopping after would stop the new channel as well. A vicious cycle, but I'm not sure how to get around it with the existing loadgroup setup and api.... > Does STOP_CONTENT also imply STOP_NETWORK? Good catch. It does not. I'll switch to STOP_ALL and combine this condition with the |zombieViewer| condition.
Comment 14•20 years ago
|
||
Comment on attachment 171371 [details] [diff] [review] Solution with load flags sr=jst
Attachment #171371 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Updated•20 years ago
|
Summary: [FIX]location.replace("#foo") stops document loads → [FIXr]location.replace("#foo") stops document loads
Assignee | ||
Comment 15•20 years ago
|
||
Attachment #171371 -
Attachment is obsolete: true
Assignee | ||
Comment 16•20 years ago
|
||
Comment on attachment 170489 [details] [diff] [review] Fix (diff -w). Marking r- per comment 11. I don't see why the code I ask about there is correct....
Attachment #170489 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 17•20 years ago
|
||
Fixed. Marking so, but I agree that we should get the second part of jst's patch in...
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 18•19 years ago
|
||
Comment on attachment 170489 [details] [diff] [review] Fix (diff -w). assuming you don't want me to review this since it has bz's r-
Attachment #170489 -
Flags: superreview?(darin)
Assignee | ||
Comment 20•19 years ago
|
||
*** Bug 236793 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•