Closed
Bug 199335
Opened 21 years ago
Closed 21 years ago
[minimo]make all viewsource stuff configurable
Categories
(Core Graveyard :: View Source, defect, P2)
Core Graveyard
View Source
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.4beta
People
(Reporter: cathleennscp, Assigned: alecf)
References
Details
(Keywords: memory-footprint)
Attachments
(1 file)
7.36 KB,
patch
|
bzbarsky
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Comment 1•21 years ago
|
||
Hrm? A little more info would be nice. Isn't there a pref to turn off viewsource parsing?
Comment 2•21 years ago
|
||
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.
Assignee | ||
Comment 3•21 years ago
|
||
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
Comment 4•21 years ago
|
||
Sounds good, but I wonder if removing viewsource will help footprint.
Comment 5•21 years ago
|
||
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....)
Assignee | ||
Updated•21 years ago
|
Target Milestone: --- → mozilla1.4beta
Assignee | ||
Updated•21 years ago
|
Priority: -- → P2
Comment 6•21 years ago
|
||
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.
Comment 7•21 years ago
|
||
> 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?)
Comment 8•21 years ago
|
||
ccing aaron so he can explain to us how he would engineer syntax highlighting better.
Comment 9•21 years ago
|
||
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.
Comment 10•21 years ago
|
||
view source is an "advanced" user feature. Anything "mini" doesn't need it :)
Comment 11•21 years ago
|
||
Updated•21 years ago
|
Attachment #128526 -
Flags: superreview?(darin)
Attachment #128526 -
Flags: review?(bzbarsky)
Comment 12•21 years ago
|
||
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+
Comment 13•21 years ago
|
||
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 14•21 years ago
|
||
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.
Comment 15•21 years ago
|
||
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
Assignee | ||
Comment 16•21 years ago
|
||
I think you disabled it by default... all the builds dropped by 17k...
Comment 17•21 years ago
|
||
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?
Comment 18•21 years ago
|
||
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.
Comment 19•21 years ago
|
||
ccing dougt so he will actually see the comments or something. Please fix, or I will back the checkin out sometime this evening.
Comment 21•21 years ago
|
||
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.
Comment 22•21 years ago
|
||
leaf, can you get the auto regen configure script working again?
Comment 23•21 years ago
|
||
> 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.
Comment 24•21 years ago
|
||
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.
Comment 25•21 years ago
|
||
so it sounds like this is fixed and should be closed... right?
Comment 26•21 years ago
|
||
yes.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•