Closed Bug 76567 Opened 24 years ago Closed 24 years ago

load viewsource.css from view source window instead of on startup

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(11 files)

The viewsource stylesheet should be loaded from viewsource itself instead of on startup. (Bzbarsky set it to load on startup as part of the view-source speedup in bug 74486 because he was having trouble loading the stylesheet dynamically using DOM2.) This may depend on one of bug 7515 or bug 34849 being fixed: bug 7515 dynamically added <link rel="stylesheet"> doesn't work bug 34849 dynamically added <style> doesn't work
Adding perf, footprint keywords. Also reassigning to bzbarsky and ccing rbs (oops, I meant to do that as I was filing the bug.)
Assignee: harishd → bzbarsky
Keywords: footprint, perf
OK. I have some changes that look to me like they should work and load a stylesheet using a <link> element. They load the stylesheet.. and then they crash the parser for reasons I cannot figure out. I will attach a diff that should allow people to test it after they apply it, and some debug output from the crash.
OS: Windows 98 → All
Hardware: PC → All
Attached patch diff to test the crash business (deleted) — Splinter Review
Attached file stack trace and so forth (deleted) —
Attached patch fix (deleted) — Splinter Review
I iterated on your patch. The main trick was to honor the return value of the parser after making sure to close the <head>. The content model is interrupted until completion of the CSS machinery. With this fix all the previous code isn't necessary anymore, and can be removed. Sounds also like a good time to get rid of the #idef XML stuff. It hasn't been maintained and I guess nobody is willing to go back to that.
rbs, thanks! I have a patch that includes the changes you made and removes the machinery added in layout to load the viewsource stylesheet. Works great. Attaching that. Should we kill off the #ifndef VIEW_SOURCE_HTML stuff in this bug or check this change in and open a new bug for that? I agree that we should do it -- it's not likely to be resurrected and killing it off will make the code a lot more readable.
Note: it can be slightly perceived that this approach is slower than the existing code. Here, the view-source stylesheet is created on the fly. And during this time, nothing else happens. Then, the stylesheet is destroyed. On the other approach, the stylesheet is kept and re-used with no creation cost.
Do you notice the difference in speed?
That's true. I've tested this a bit, and it seems fairly fast. No perceptible slowdown and still a lot faster than what we used to have. But I have a fast computer. I was talking to jst about this a week or so ago and he said that he would much prefer this approach -- that the cost of reparsing the stylesheet is fairly low.... We should try some performance testing. Doron? :)
I looks like nsViewSourceHTML is being used to display text/css files in the main browser (instead of handling them just like text/plain). It also looks like somewhere as we do this we fail to properly check error values. So we end up inserting the data into the content model twice with this patch -- once right after </head> and before <body> and once inside <pre>. The result is very broken, needless to say. Working on figuring out how to just make text/css display like text/plain.
To proceed by elimination, you could try this change to see what happens: - if(NS_SUCCEEDED(result)) mHasOpen[Root,Body]=PR_TRUE; + mHasOpen[Root,Body]=PR_TRUE;
Depends on: 77499
OK. The real problem is that the display of text/css content was messed up. Bug 77499 covers that and has a patch. If that patch is applied, that makes this one much happier.
Did you try turning off the #ifdef that I indicated on the patch. I am seeing some weirdness when I do that. Turning off the #ifdef will avoid adding the attributes to the <link> tag, i.e., it disables loading the viewsource.css file. In principle there shouldn't be a problem.
OK. When I turn off the #if we don't open the <body> and <pre> tags. That makes sense. Since we don't have an href, we don't load a stylesheet, so we don't block, 'result' is never set to NS_ERROR_HTMLPARSER_BLOCK, and after skipping over the "else if (!mOpenBody)" part we proceed to build the content model instead of returning and being called again. It looks screwy because we never open the <pre> tag. Attaching patch that fixes that problem -- even were the stylesheet not to be found we would do the right thing with this one.
OK, I see. Indeed, it makes sense, and your iteration fixes the problem. Another possible iteration could be to clean up the "#ifdef rickgdebug" stuff (or perhaps to adapt it to the HTML version). These #ifdef were seemingly useful to dump the XML-viewsource on a file but they don't work anymore in the HTML context. I have seen some people filing bugs for "viewsource-of-viewsource" (so that they can copy-paste the nicely ready-made stylable fragments...). So it might be nice to re-instate the debug dump in case other folks later want to further iterate in other directions.
Summary of attachment 32410 [details] [diff] [review]: 1) Load the stylesheet from view source 2) Only load the stylesheet if mSyntaxHighlight is set. It's not really needed otherwise... 3) Clean out the XML stuff 4) Update the debug file dump to work completely correctly with the new HTML world I'm actually fairly happy with this one...
Keywords: patch, review
I am getting a compile error when trying the patch (remnants of some of your other works): C:\Mozilla\src\mozilla\htmlparser\src\nsViewSourceHTML.cpp(408) : error C2065: kTextJSContentType' : undeclared identifier C:\Mozilla\src\mozilla\htmlparser\src\nsViewSourceHTML.cpp(409) : error C2065: kApplicationJSContentType' : undeclared identifier NMAKE : fatal error U1077: 'cl' : return code '0x2'
Attachment 32426 [details] [diff] is that same as attachment 32410 [details] [diff] [review] but has the extraneous lines that kept it from building removed.
Actually, one more thing. Attaching yet another patch. This one also 5) Only outputs <span>s for things that are not plain text and moves the place where </span> is emitted to eliminate nested <span> elements. Doron tested this one for perf and says that he actually sees a slight perf improvement over what we have currently (on the order of 15% or so).
6) While here I made another iteration to set the title for free (BTW, this will cater for bug 10066 and shows the title like 4.x does). Looks good enough. Time to hunt for r/sr... Seeking for r=? and sr=? to check-in.
Depends on: 60892
i would recommend you hunt down the people who rewrote the viewsource for r=/sr= great work!
Boris and I aren't suitable reviewers, as we have been reading this code "too much" and could easily keep overlooking something... Others are most welcome to r/sr. What about you Doron? Or others on the cc:list?
Cc:ing jce2@po.cwru.edu <Jason Eager> who initially rewrote the HTML viewsource for possible review of these subsequent iterations on the attached patch?
Err... I have just noted that the SetTitle() trick only work in Viewer (where I was testing) and not in SeaMonkey.
how does navigator do it's title setting?
It just show: "Source of: url" (like the trick does -- or was supposed to do :)
i got the "source of: " part to work, by moving the titlemodifier in the xul into the title attribute, which is an ugly way. Perhaps we should just set it via js?
Interesting. Let's do that in xul then. (And perhaps find more about this title vs. titlemodifier issue on another bug. There could be a c++ bug in the xul code) Reviewers: the following fragment is not relevant anymore. I will take that out prior checkin in. That would even fix the localization problem I had. (I am nevertheless leaving the other bit that initializes mFilename (i.e., the url in fact), for the <title>...</title> section in the dump file.) + + nsAutoString titleText; + titleText.Assign(NS_LITERAL_STRING("Source of: ")); // XXX L10N + titleText.Append(mFilename); + mSink->SetTitle(titleText); +
rbs, could you attach your latest patch with the title stuff? I'd like to test a bit....
Unfortunately, I am behind a restricted firewall and don't have cvs access at the moment. Attachment 32516 [details] [diff] is the latest -- provided there has been other changes in tree touching the files since the attachment was made.
One nitpick. Make sure dumpfiledebug is not defined by default when you check in... The rest works well for me.
Looks good to me, great job Boris! sr=jst
Changes from attachment 32516 [details] [diff] [review] to attachment 32736 [details] [diff] [review]: 1) make the dump file #define all-caps 2) remove the mSink->SetTitle(titleText); call and associated lines
Found the correct fix for the title... The bug was in ViewSource.xul. An attribute that XUL expects was missing. Below is what I will checkin: SetTitle() is needed, otherwise XUL doesn't know what the title is. Once it knows the title, it automatically adds "Source of: " + url + "- Mozilla". (It was interesting to discover that all the pieces are controllable in XUL, even the seemingly meaningless dash '-' :-) [With this however, viewer will only show the url being supplied by the mSink->SetTitle().] In nsViewSourceHTML.cpp: + // Note that XUL with automatically add the preface "Source of: " + mSink->SetTitle(mFilename); And elsewhere: Index: browser/resources/content/viewSource.xul =================================================================== RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/viewSource.xul,v retrieving revision 1.28 diff -u -r1.28 viewSource.xul --- browser/resources/content/viewSource.xul 2001/04/18 04:40:18 1.28 +++ browser/resources/content/viewSource.xul 2001/05/01 08:17:01 @@ -14,6 +14,7 @@ <window id="main-window" xmlns:html="http://www.w3.org/1999/xhtml" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="onLoadViewSource();" + contenttitlesettting="true" title="&mainWindow.title;" titlemodifier="&mainWindow.titlemodifier;" titlepreface="&mainWindow.preface;" Index: browser/resources/locale/en-US/viewSource.dtd =================================================================== RCS file: /cvsroot/mozilla/xpfe/browser/resources/locale/en-US/viewSource.dtd,v retrieving revision 1.12 diff -u -r1.12 viewSource.dtd --- browser/resources/locale/en-US/viewSource.dtd 2001/03/23 03:26:54 1.12 +++ browser/resources/locale/en-US/viewSource.dtd 2001/05/01 08:17:02 @@ -6,4 +6,4 @@ <!ENTITY mainWindow.titlemodifier "&brandShortName;"> <!-- LOCALIZATION NOTE (mainWindow.titlemodifierseperator) : DONT_TRANSLATE --> <!ENTITY mainWindow.titlemodifierseperator " - "> -<!ENTITY mainWindow.preface "Source for: "> +<!ENTITY mainWindow.preface "Source of: ">
bzbarsky, to recover your changes, I just have to s/dumpfiledebug/DUMP_TO_FILE/g, right?
rbs, that and change "bzbarsky@mit.edu <Boris Zbarsky>" to "Boris Zbarsky <bzbarsky@mit.edu>" Good to know we have this working with SetTitle() and all.
Got sr=jst, seeking r=..., harishd?
rbs: I'm in the process of reviewing....
+ contenttitlesettting="true" Too many tees! /be
http://lxr.mozilla.org/seamonkey/search?string=contenttitlesettting another 'referer' vs. 'referrer' ... Ok, I could do the search'n replace too :-)
Just landed on the trunk, with additional suggestions sent by harishd. Resolving as FIXED.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
Resolving fixed for real. Just pulled a tree and it has the changes. Built it and it works.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Marking verified as per developer comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: