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)
Core
DOM: HTML Parser
Tracking
()
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: memory-footprint, perf)
Attachments
(11 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•24 years ago
|
||
Adding perf, footprint keywords. Also reassigning to bzbarsky and ccing rbs
(oops, I meant to do that as I was filing the bug.)
Assignee | ||
Comment 2•24 years ago
|
||
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
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
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.
Assignee | ||
Comment 7•24 years ago
|
||
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.
Assignee | ||
Comment 8•24 years ago
|
||
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.
Comment 10•24 years ago
|
||
Do you notice the difference in speed?
Assignee | ||
Comment 11•24 years ago
|
||
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? :)
Comment 12•24 years ago
|
||
Assignee | ||
Comment 13•24 years ago
|
||
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.
Comment 14•24 years ago
|
||
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;
Assignee | ||
Comment 15•24 years ago
|
||
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.
Comment 16•24 years ago
|
||
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.
Assignee | ||
Comment 17•24 years ago
|
||
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.
Assignee | ||
Comment 18•24 years ago
|
||
Comment 19•24 years ago
|
||
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.
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
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...
Comment 22•24 years ago
|
||
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'
Assignee | ||
Comment 23•24 years ago
|
||
Assignee | ||
Comment 24•24 years ago
|
||
Attachment 32426 [details] [diff] is that same as attachment 32410 [details] [diff] [review] but has the extraneous lines
that kept it from building removed.
Assignee | ||
Comment 25•24 years ago
|
||
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).
Assignee | ||
Comment 26•24 years ago
|
||
Comment 27•24 years ago
|
||
Comment 28•24 years ago
|
||
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.
Comment 29•24 years ago
|
||
i would recommend you hunt down the people who rewrote the viewsource for r=/sr=
great work!
Comment 30•24 years ago
|
||
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?
Comment 31•24 years ago
|
||
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?
Comment 32•24 years ago
|
||
Err... I have just noted that the SetTitle() trick only work in Viewer (where I
was testing) and not in SeaMonkey.
Comment 33•24 years ago
|
||
how does navigator do it's title setting?
Comment 34•24 years ago
|
||
It just show: "Source of: url" (like the trick does -- or was supposed to do :)
Comment 35•24 years ago
|
||
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?
Comment 36•24 years ago
|
||
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);
+
Assignee | ||
Comment 37•24 years ago
|
||
rbs, could you attach your latest patch with the title stuff? I'd like to test
a bit....
Comment 38•24 years ago
|
||
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.
Assignee | ||
Comment 39•24 years ago
|
||
One nitpick. Make sure dumpfiledebug is not defined by default when you check
in... The rest works well for me.
Comment 40•24 years ago
|
||
Looks good to me, great job Boris! sr=jst
Assignee | ||
Comment 41•24 years ago
|
||
Assignee | ||
Comment 42•24 years ago
|
||
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
Comment 43•24 years ago
|
||
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: ">
Comment 44•24 years ago
|
||
bzbarsky,
to recover your changes, I just have to s/dumpfiledebug/DUMP_TO_FILE/g, right?
Assignee | ||
Comment 45•24 years ago
|
||
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.
Comment 46•24 years ago
|
||
Got sr=jst, seeking r=..., harishd?
Comment 47•24 years ago
|
||
rbs: I'm in the process of reviewing....
Comment 48•24 years ago
|
||
+ contenttitlesettting="true"
Too many tees!
/be
Comment 49•24 years ago
|
||
http://lxr.mozilla.org/seamonkey/search?string=contenttitlesettting
another 'referer' vs. 'referrer' ... Ok, I could do the search'n replace too :-)
Comment 50•24 years ago
|
||
Just landed on the trunk, with additional suggestions sent by harishd.
Resolving as FIXED.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 51•24 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•