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

VERIFIED FIXED in mozilla0.9.1

Status

()

--
minor
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

({memory-footprint, perf})

Trunk
mozilla0.9.1
memory-footprint, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

(Reporter)

Description

18 years ago
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

18 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: harishd → bzbarsky
Keywords: footprint, perf
(Assignee)

Comment 2

18 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

18 years ago
Created attachment 31794 [details] [diff] [review]
diff to test the crash business
(Assignee)

Comment 4

18 years ago
Created attachment 31795 [details]
stack trace and so forth

Comment 5

18 years ago
Created attachment 32003 [details] [diff] [review]
fix

Comment 6

18 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

18 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

18 years ago
Created attachment 32010 [details] [diff] [review]
patch to load the CSS in view source, not in layout

Comment 9

18 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

18 years ago
Do you notice the difference in speed?
(Assignee)

Comment 11

18 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

18 years ago
Created attachment 32103 [details] [diff] [review]
cleaned patch (no xml) which exposes a bug
(Assignee)

Comment 13

18 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

18 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)

Updated

18 years ago
Depends on: 77499
(Assignee)

Comment 15

18 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

18 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

18 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

18 years ago
Created attachment 32279 [details] [diff] [review]
Patch to nsViewSourceHTML.cpp that will work even when there is no stylesheet

Comment 19

18 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

18 years ago
Created attachment 32410 [details] [diff] [review]
Patch incorporating rbs' suggestions
(Assignee)

Comment 21

18 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...
Keywords: patch, review

Comment 22

18 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

18 years ago
Created attachment 32426 [details] [diff] [review]
Patch that will actually compile
(Assignee)

Comment 24

18 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

18 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

18 years ago
Created attachment 32448 [details] [diff] [review]
Patch that tries to create a less bloated DOM

Comment 27

18 years ago
Created attachment 32516 [details] [diff] [review]
Final proposed patch for checkin - includes the doc title

Comment 28

18 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.

Updated

18 years ago
Depends on: 60892
i would recommend you hunt down the people who rewrote the viewsource for r=/sr=

great work!

Comment 30

18 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

18 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

18 years ago
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?  

Comment 34

18 years ago
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?

Comment 36

18 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

18 years ago
rbs, could you attach your latest patch with the title stuff?  I'd like to test
a bit....

Comment 38

18 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

18 years ago
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
(Assignee)

Comment 41

18 years ago
Created attachment 32736 [details] [diff] [review]
patch that includes some changes suggested by jst
(Assignee)

Comment 42

18 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

18 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

18 years ago
bzbarsky, 
to recover your changes, I just have to s/dumpfiledebug/DUMP_TO_FILE/g, right?
(Assignee)

Comment 45

18 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

18 years ago
Got sr=jst, seeking r=..., harishd?

Comment 47

18 years ago
rbs: I'm in the process of reviewing....
+       contenttitlesettting="true"

Too many tees!

/be

Comment 49

18 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

18 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

18 years ago
Resolving fixed for real.  Just pulled a tree and it has the changes.  Built it
and it works.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 52

18 years ago
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.