Closed Bug 199335 Opened 21 years ago Closed 21 years ago

[minimo]make all viewsource stuff configurable

Categories

(Core Graveyard :: View Source, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.4beta

People

(Reporter: cathleennscp, Assigned: alecf)

References

Details

(Keywords: memory-footprint)

Attachments

(1 file)

 
Hrm? A little more info would be nice. Isn't there a pref to turn off viewsource
parsing?
I presume this is for HTML colorization, which warps the source code in HTML.
Unfortunately, it also tends to mangle the source in odd little ways; see most
of the dependencies of bug 57724. Bug 188609 requests a separate tokenizer for
it, but just removing it would not be a bad solution for the time being.
This is for minimo - the idea is to have an #ifdef to remove anything having to
do with viewsource..
Status: NEW → ASSIGNED
Keywords: footprint
Summary: remove viewsource parsing → [minimo]make all viewsource stuff configurable
Sounds good, but I wonder if removing viewsource will help footprint.
It will.  We would be able to compile out the viewsource channel impl, the 
viewsource protocol impl, the viewsource DTD impl, some bits and pieces in 
nsContentDLF, and other random bits in the parser (eg code in CNavDTD that 
checks the parse mode....)
Target Milestone: --- → mozilla1.4beta
Priority: -- → P2
Sounds like view source is overengineered. How hard can it be to pop open a
window containing a bunch of text? I think the colorization can definately go,
but view source is a very important feature for any browser, mini or not.
> How hard can it be to pop open a window containing a bunch of text?

Trivial

> I think the colorization can definately go

Then you don't need all the overengineering -- that exists solely to handle
syntax highlighting.

> view source is a very important feature for any browser, mini or not.

You seem to have a deeper knowledge of the target platform/audience/etc of
Minimo than I do (for one thing, what makes you think it's a browser as opposed
to an HTML renderer for use in a controlled environment with minimal user
options and possibly no user interaction?)
ccing aaron so he can explain to us how he would engineer syntax highlighting
better.
Sorry, I'm not familiar with minimo. If minimo is supposed to be just gecko and
its important dependencies than everything I've said is irrelevant. Doron's
comment lead me to assume that there was some tradeoff associated with removing
viewsource - presumably losing the ability to display the source of a rendered
web page in an environment where that would be a useful feature.

I never saw highlighting of viewed source as a very useful feature. I guess it
marks comments distinctively when someone is too lazy to open their editor of
choice when reading the source for a page. View source doesn't seem to colorize
javascript code though, which is a lanaguage where highlighting would seem handier.
view source is an "advanced" user feature. Anything "mini" doesn't need it :)
Attached patch patch v.1Splinter Review
Attachment #128526 - Flags: superreview?(darin)
Attachment #128526 - Flags: review?(bzbarsky)
Comment on attachment 128526 [details] [diff] [review]
patch v.1

r=me if you also put ifdefs around the viewsource MIME type in nsContentDLF's
list of "html types".
Attachment #128526 - Flags: review?(bzbarsky) → review+
Blocks: 213938
Comment on attachment 128526 [details] [diff] [review]
patch v.1

>Index: htmlparser/src/CNavDTD.cpp

> #include "nsTime.h"
>-#include "nsViewSourceHTML.h"
> #include "nsParserNode.h"
> #include "nsHTMLEntities.h"
> #include "nsLinebreakConverter.h"
>@@ -68,6 +67,10 @@
> #include "nsUnicharUtils.h"
> #include "prmem.h"
> #include "nsIServiceManager.h"
>+
>+#ifdef MOZ_VIEW_SOURCE
>+#include "nsViewSourceHTML.h"
>+#endif

this is kind of wierd... if the only change to this file is to conditionally
include nsViewSourceHTML.h then why are we ever including it?  seems like you
would need an #ifdef somewhere else in the file, no?  same question applies
to COtherDTD.cpp

sr=darin with this issue resolved.
Attachment #128526 - Flags: superreview?(darin) → superreview+
Comment on attachment 128526 [details] [diff] [review]
patch v.1

it may be possible to just remove the header all together.  I will take a look.
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1259; previous revision: 1.1258
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 3.291; previous revision: 3.290
done
Checking in htmlparser/src/CNavDTD.cpp;
/cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v  <--  CNavDTD.cpp
new revision: 3.421; previous revision: 3.420
done
Checking in htmlparser/src/COtherDTD.cpp;
/cvsroot/mozilla/htmlparser/src/COtherDTD.cpp,v  <--  COtherDTD.cpp
new revision: 3.132; previous revision: 3.131
done
Checking in htmlparser/src/Makefile.in;
/cvsroot/mozilla/htmlparser/src/Makefile.in,v  <--  Makefile.in
new revision: 1.87; previous revision: 1.86
done
Checking in htmlparser/src/nsParser.cpp;
/cvsroot/mozilla/htmlparser/src/nsParser.cpp,v  <--  nsParser.cpp
new revision: 3.331; previous revision: 3.330
done
Checking in htmlparser/src/nsParserModule.cpp;
/cvsroot/mozilla/htmlparser/src/nsParserModule.cpp,v  <--  nsParserModule.cpp
new revision: 1.50; previous revision: 1.49
done
Checking in layout/build/nsContentDLF.cpp;
/cvsroot/mozilla/layout/build/nsContentDLF.cpp,v  <--  nsContentDLF.cpp
new revision: 1.48; previous revision: 1.47
done

most of viewsource is now able to be disabled.  
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I think you disabled it by default... all the builds dropped by 17k...
my 2003072922 linux build doesn't want to show me the source anymore.
/tmp/<something> could not be saved, because the source file could not be read.

could be related?
I guess that the bug wasn't actually resoved, as we're not _able_ to exclude it,
we simply do exclude it - and I'm not sure if we're able to include it ;-)

Anyways, it introduced a regression of view source not working anywhere.
Blocks: 214445
ccing dougt so he will actually see the comments or something.  Please fix, or I
will back the checkin out sometime this evening.
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The problem appears to simply be that the configure auto-update is not working;
the configure script was never updated to match the configure.in change.
leaf, can you get the auto regen configure script working again?
> The problem appears to simply be that the configure auto-update is not 
> working

Indeed. Rebuilding with -DMOZ_VIEW_SOURCE=1 restores functionality.

Seems to me that mozilla needs to tell the user that view-source is 
disabled rather than a completely misleading message.
This is meant to be used together with disabling the view-source protocol
handler, at which point mozilla will report that view-source: is an unknown
protocol.
so it sounds like this is fixed and should be closed... right?
yes.  
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
Product: SeaMonkey → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: