199335 - [minimo]make all viewsource stuff configurable

Status: RESOLVED FIXED

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

Description

[Cathleen]

Comment 1

[jag (Peter Annema)]

Hrm? A little more info would be nice. Isn't there a pref to turn off viewsource
parsing?

Comment 2

[Christopher Hoess (gone)]

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.

Comment 3

[Alec Flett]

This is for minimo - the idea is to have an #ifdef to remove anything having to
do with viewsource..

Comment 4

[Doron Rosenberg (IBM)]

Sounds good, but I wonder if removing viewsource will help footprint.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Aaron Lehmann]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

ccing aaron so he can explain to us how he would engineer syntax highlighting
better.

Comment 9

[Aaron Lehmann]

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

[Doron Rosenberg (IBM)]

view source is an "advanced" user feature. Anything "mini" doesn't need it :)

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 12

[Boris Zbarsky [: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".

Comment 13

[Darin Fisher]

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.

Comment 14

[Doug Turner (:dougt)]

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

[Doug Turner (:dougt)]

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.  

Comment 16

[Alec Flett]

I think you disabled it by default... all the builds dropped by 17k...

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

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

[Robert Kaiser]

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

[Boris Zbarsky [:bzbarsky]]

ccing dougt so he will actually see the comments or something.  Please fix, or I
will back the checkin out sometime this evening.

Comment 20

[timeless]

backed out

Comment 21

[Brian Ryner (not reading)]

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

[Doug Turner (:dougt)]

leaf, can you get the auto regen configure script working again?

Comment 23

[tenthumbs]

> 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

[Boris Zbarsky [:bzbarsky]]

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

[chris hofmann]

so it sounds like this is fixed and should be closed... right?

Comment 26

[Doug Turner (:dougt)]

yes.