Closed Bug 582795 Opened 15 years ago Closed 14 years ago

Changing encoding in view source doesn't update page

Categories

(Toolkit :: View Source, defect)

Product:
Toolkit
Component:
View Source
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: stream, Assigned: justin.lebar+bug)

References

Details

(Keywords: qawanted, regression)

Attachments

(1 file, 5 obsolete files)

Patch v4.1
1.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
from bug 582788 WIth Firefox 4 b2 when changing encoding in View source, nothing happens until i refresh manually.
Can someone please contribute a confirmation and a regression range?
Keywords: qawanted, regressionwindow-wanted
Regression window: Works: http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201094116 Fails: http://hg.mozilla.org/mozilla-central/rev/5c6c40887584 Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201174142 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c1aa8a2485c&tochange=5c6c40887584 I revert to afff5c13d296 in local build, then the problem is gone. So, Changeset a2f468118868 of Bug 500328 causes the problem.
Blocks: 500328
I'll have a look at what's going on.
Alice or Emil, could you give please link me to a page to test this on?
(In reply to comment #4) > Alice or Emil, could you give please link me to a page to test this on? http://mozilla.jp/firefox/ -- this is UTF-8 page. and I changed to Western (ISO-8859-1) in viewsource window and confirmed the issue.
Yes, actually you can do it from any page including in this bug, just change it to something different than the auto detected.
BTW the list with recent used encodings is not updated after using another encoding, maybe the problem is in this bug. In Firefox 3.6.8 i have all kinds of UTF when testing, but in
BTW the list with recent used encodings is not updated after using another encoding, maybe the problem is in this bug. In Firefox 3.6.8 i have all kinds of UTF when testing, but in latest nightly there are only the original two Western and UTF-8. ps sorry for my previous post i clicked accidentally on 'save changes' button when trying to close the view source...
I can confirm that this regressed with changeset a2f468118868. I have no idea why, but I'll look into it.
Status: NEW → ASSIGNED
Aha. The problem is that pushState introduces the idea of navigating between two SHEntries with the same document identifier. If the document identifier is the same, we short-circuit loads between them (just as we short-circuit loads between foo.html#bar and foo.html#baz) [1]. I wonder what the right fix is here. We could increment the document identifier when we change the encoding, although we'd probably only want to do that for view-source URIs, as doing otherwise would break pushstate. [1]: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7948
So what caused the reload before pushState? And what kind of reload?
Blocks final, please make sure it is tested though
blocking2.0: ? → final+
Flags: in-testsuite?
(In reply to comment #12) > what kind of reload? In nsDocShell::InternalLoad, aLoadType is LOAD_HISTORY. > So what caused the reload before pushState? Before the pushstate patch, InternalLoad did a regular, non-short-circuiting load instead of falling into the anchor load case.
The load type comes from nsDocShell::LoadPage(), which is called from JS. I'm not sure where in JS; I thought I just had to |call DumpJSStack()|, but that doesn't output anything.
Oh. gdb tui was messing me up. The JS stack is 1 BrowserSetForcedCharacterSet(aCharset = "UTF-8") ["chrome://global/content/viewSource.js":675] this = [object ChromeWindow @ 0x230ab70 (native @ 0x2360398)] 2 SetForcedCharset(charset = "UTF-8") ["chrome://global/content/charsetOverlay.js":115] this = [object ChromeWindow @ 0x230ab70 (native @ 0x2360398)]
Attached patch Partial fix (obsolete) — — Splinter Review
The view-source charset updates when you change it with this patch applied. No idea if this breaks anything else... The selected charset still doesn't show up in the list of recently-used charsets. The obvious thing to try would be to make the same change BrowserSetForcedDetector(), but unfortunately that doesn't seem to work. :)
Comment on attachment 463423 [details] [diff] [review] Partial fix Actually, that patch fixes nothing. I was testing on the wrong head. :( In other news, changing the view-source encoding doesn't add that encoding to the recently-used list in changeset afff5c (the one right before the regression). So maybe we don't have to worry about that here.
Attachment #463423 - Attachment is obsolete: true
Isnt it possible to reuse the code from the main menu bar? Its working without any problems there.
Attached patch Patch v1 (obsolete) — — Splinter Review
This seems to fix the problem, although who knows what it breaks. There are some comments nearby that seem anachronistic -- the first one apparently refers to the wrong bug. How would one go about writing a test for this? And who can we cc that understands this code so they can tell me why my change is a bad idea? :)
I don't think this should block, but if it does, it needs a testcase. Olli or bz, do you have any idea how to test this?
You can just loadPage in an arbitrary docshell, etc, no?
(In reply to comment #22) > You can just loadPage in an arbitrary docshell, etc, no? Could you elaborate on this a bit?
Load something in a frame. Create another frame. Get the latter's docshell. loadPage the desscriptor from the first frame in the second frame's docshell. Then change the encoding on the second docshell and see whether the DOM is correct?
Sorry, Boris; I still don't quite grock. From a mochitest (?), create two iframes. Load a page into one iframe. Into the second iframe, load the descriptor from the first page's docshell. Then change the second frame's encoding and check the DOM. Is the descriptor you speak of the viewsource:// URI corresponding to the URI I loaded into the first frame?
> Is the descriptor you speak of the viewsource:// URI No. It's "pageCookie" gotten in BrowserViewSourceOfDocument here: http://hg.mozilla.org/mozilla-central/file/11e4faad5545/browser/base/content/browser.js#l2259 And it's used like so: http://hg.mozilla.org/mozilla-central/file/11e4faad5545/toolkit/components/viewsource/content/viewSource.js#l188
Okay, I got that to work. But now how do I call viewSource.js's version of BrowserSetForcedCharacterSet()?
Summary: Changing encoding is not reloading source automatically → Changing encoding in view source doesn't refresh page
You don't. You copy and paste it into your code, I would think.
Hm. Is it even worth writing a testcase, then, considering that the only changes I made were to BrowserSetForcedCharacterSet and BrowserSetForcedDetector?
Oh, I see. I didn't realize you were doing all this on the chrome side only... You could try to write a browser test that opens the viewsource window and calls the viewsource functions, I guess.... But maybe it's not worth it.
I'm happy not testing it if it's going to be a pain and/or result in a fragile test.
Comment on attachment 463661 [details] [diff] [review] Patch v1 Requesting bz's review here because I have no idea who owns this code.
Attachment #463661 - Flags: review?(bzbarsky)
Summary: Changing encoding in view source doesn't refresh page → Changing encoding in view source doesn't update page
So I assume you tested that this doesn't regress bug 229503 or bug 136322? In particular, you're just backing out the fix for bug 136322, right? Did you test on source of no-cache POST results?
You know, when I originally wrote this, I looked up those bugs, but I must have mistyped both bug numbers, because neither pointed to anything related to view source. I'll go do the testing I should have done.
Attachment #463661 - Flags: review?(bzbarsky)
bug 136322 isn't regressed (that's obvious from testing for this bug). bug 229503 isn't strictly regressed, but I'd like to test on a site where changing the auto-detect setting has an effect. Will test no-cache POST results in a moment.
With a no-cache POST result page, it pops up and asks if I want to re-post. That's probably not what we want, is it?
No, we don't. Does it not do that without this patch?
(In reply to comment #36)> With a no-cache POST result page, it pops up and asks if I want to re-post. > That's probably not what we want, is it?Turns out that you get the same prompt if you change the encoding of a no-cache POST result outside of view-source. bz and I conferred on IRC and think that we can add this behavior to view-source and then fix it at once both inside and outside view-source.The only thing that should be left to do for this patch is test that it doesn't render auto-detect useless.
(Sorry about the lack of linebreaks in comment 38. Bug 596752.)> The only thing that should be left to do for this patch is> test that it doesn't render auto-detect useless.Simon, do you know of any pages where auto-detect charset should have an effect, or how to make one? My attempts thus far have not been fruitful.
You could try any of the test files for autodetection at extensions/universalchardet/tests/bug*.html (Using a local copy rather than mxr, because mxr sends a Content-Type header with charset, so autodetection won't kick in)
Interesting. Changing the auto-detect setting in view-source has no effect on trunk.
Should this really block? It's seen no activity in well over a month and (at least IMHO) isn't that important.
No. This is a developer-oriented feature with a very simple workaround. It should not block the release. Would take a low-risk, reviewed patch with tests.
blocking2.0: final+ → -
> No. This is a developer-oriented feature with a very simple workaround. What workaround? There is no workaround that actually works. Reloading is not a workaround, since it totally fails on POST requests. Resetting the blocking flag, since comment 43 was based on incorrect premises... Note that the underlying issue here is the same as in bug 585298, btw. So it's really two blockers for the price of one.
blocking2.0: - → final+
Attached patch Patch v2 (obsolete) — — Splinter Review
bz, what do you think of this patch? It also fixes bug 585298. I'm not sure it's possible to properly test this with an automated test. We could certainly write a manual test for this and for bug 585298, though.
Attachment #463661 - Attachment is obsolete: true
Attachment #492452 - Flags: review?(bzbarsky)
Why's the unique identifier in an else clause, not unconditional? That comment below the if/else block doesn't make sense at that location...
Attached patch Patch v3 (obsolete) — — Splinter Review
Attachment #492452 - Attachment is obsolete: true
Attachment #492751 - Flags: review?(bzbarsky)
Attachment #492452 - Flags: review?(bzbarsky)
Comment on attachment 492751 [details] [diff] [review] Patch v3 Let's not condition this on the scheme either, ok?
Attachment #492751 - Flags: review?(bzbarsky) → review-
Attached patch Patch v4 (obsolete) — — Splinter Review
sgtm.
Attachment #492751 - Attachment is obsolete: true
Attachment #492805 - Flags: review?(bzbarsky)
Attached patch Patch v4.1 — — Splinter Review
Forgot to qref.
Attachment #492805 - Attachment is obsolete: true
Attachment #492816 - Flags: review?(bzbarsky)
Attachment #492805 - Flags: review?(bzbarsky)
Comment on attachment 492816 [details] [diff] [review] Patch v4.1 "independent of", and sold!
Attachment #492816 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/35f5ea086390
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee: nobody → justin.lebar+bug
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: