Closed Bug 76567 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(11 files)

diff to test the crash business
4.38 KB, patch
Details | Diff | Splinter Review
stack trace and so forth
4.05 KB, text/plain
Details
fix
6.51 KB, patch
Details | Diff | Splinter Review
patch to load the CSS in view source, not in layout
10.96 KB, patch
Details | Diff | Splinter Review
cleaned patch (no xml) which exposes a bug
16.56 KB, patch
Details | Diff | Splinter Review
Patch to nsViewSourceHTML.cpp that will work even when there is no stylesheet
13.76 KB, patch
Details | Diff | Splinter Review
Patch incorporating rbs' suggestions
27.05 KB, patch
Details | Diff | Splinter Review
Patch that will actually compile
26.52 KB, patch
Details | Diff | Splinter Review
Patch that tries to create a less bloated DOM
26.70 KB, patch
Details | Diff | Splinter Review
Final proposed patch for checkin - includes the doc title
27.79 KB, patch
Details | Diff | Splinter Review
patch that includes some changes suggested by jst
26.95 KB, 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
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
          
          
        fix
        — 
          — 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: 23 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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: