357089 - "Syntax Highlighting" menu item doesn't work in "View Selection Source"

Status: RESOLVED WORKSFORME

(Toolkit :: View Source, defect)

Description

[vicky.sun]

User-Agent:       Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1b2) Gecko/20061010 Firefox/2.0b2
Build Identifier: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1b2) Gecko/20061010 Firefox/2.0b2

This bug can be reproduced on sparc and x86 machines (SunOS)

Reproducible: Always

Steps to Reproduce:
1.Launch firefox2.0 beta2.
2.Load any webpage,e.g. www.mozilla.org.
3.Select any content and right-click to select "View selection source".
4.Check/Uncheck "View"->"Syntax Highlighting".

Actual Results:  
No change for source code's syntax.

Expected Results:  
Syntax shall be changed with the command.

Comment 1

[Phil Ringnalda (:philor)]

Can be reproduced on any machine, actually. Error console says:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIWebPageDescriptor.currentDescriptor]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://global/content/viewSource.js :: highlightSyntax :: line 539"  data: no]

Comment 2

[Tae Kim]

Attachment: patch for syntax highlight

The highlightSyntax function essential reloads the page after toggling the syntax preference. This fails for selection source because it never loaded the whole page to begin with. This patch creates a new highlightPartialSyntax function to toggle the highlight syntax pref. The only difference from the original highlightSyntax function is that it calls onLoadViewPartialSource to reload the selection source instead of actually reloading the page.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 243630 [details] [diff] [review]
patch for syntax highlight

Tae Kim: you need to request review by setting the dropdown to "?" and entering the desired reviewer's email address.

Comment 4

[Tae Kim]

Oh ok, I get it now. I'll make sure to do that from now on. Thanks for fixing that.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 243630 [details] [diff] [review]
patch for syntax highlight

Sorry for the long time I took to review this, Tae. I hope you're still interested in fixing this bug.

>Index: viewPartialSource.js

>+// fix for bug #357089: reload partial source instead of the whole page
>+//function to toggle syntax highlighting and set the view_source.syntax_highlight
>+//pref to persist the last state
>+function highlightPartialSyntax()
>+{
>+  var highlightSyntaxMenu = document.getElementById("menu_highlightSyntax");
>+  var highlightSyntax = (highlightSyntaxMenu.getAttribute("checked") == "true");
>+  gPrefs.setBoolPref("view_source.syntax_highlight", highlightSyntax);
>+
>+  onLoadViewPartialSource();
>+}

Since this function is essentially the same as highlightSyntax() but with one line changed, it would be better to factor out the common code. Perhaps you could add a gPartial global variable that's set to true in viewPartialSource.js , and then check it from highlightSyntax() to determine which behavior we want. Or you could keep both highlightSyntax() and highlightPartialSyntax() and move the common code into another function in viewSource.js, something like updateSyntaxMenu().

Also, do we really want to be calling onLoadViewPartialSource() again? It does some initialization that shouldn't be necesssary the second time around, right? You can probably just call viewPartialSourceForSelection or viewPartialSourceForFragment directly.

Comment 6

[vicky.sun]

This bug can also be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1.1) Gecko/20070227 Firefox/2.0.0.1

Comment 7

[vicky.sun]

It can also be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a3pre) Gecko/20070302 Minefield/3.0a3pre.

Comment 8

[Rob Campbell [:rc] (:robcee)]

confirming this on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ID:20100401064631.

Comment 9

[Shriram (irc: Mavericks)]

Attachment: Patch for 'Syntax Highlighting' menu item in 'View Selection Source'

Builds on previous patch

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 695982 [details] [diff] [review]
Patch for 'Syntax Highlighting' menu item in 'View Selection Source'

"updateSyntaxMenu" is a confusing name for a method that just returns the checked state of a menu item. You could instead name it highlightSyntaxCommon and also put the setting of the pref there, then call it from both highlightPartialSyntax and highlightSyntax.

I haven't checked that the rest of the highlightPartialSyntax is correct. When posting an updated patch, you should ask for review from neil@httl.

Comment 11

[Shriram (irc: Mavericks)]

Attachment: Patch for 'Syntax Highlighting' menu item in 'View Selection Source'

Comment 12

[[e-mail]]

Gavin, do you remember whether there is a reason why history is disabled in selection source?

[The reasoning here is that highlightSyntax would work automatically with history enabled, and we can load alternative documents into the view partial source window in a number of ways, and that has the effect of defeating this syntax highlighting code.]

Comment 13

[:Gavin Sharp [email: [e-mail]]]

bug 229995 comment 0? :) Bug 340669 is the toolkit/firefox version which we ported apparently because TB didn't ship session history. Bug 17612 undid that for viewSource.xul, but not viewPartialSource.xul.

Comment 14

[[e-mail]]

Comment on attachment 696203 [details] [diff] [review]
Patch for 'Syntax Highlighting' menu item in 'View Selection Source'

Well in that case I'm going to have to minus this patch...

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] from comment #12)
> [The reasoning here is that highlightSyntax would work automatically with
> history enabled

Why?

Comment 16

[[e-mail]]

(In reply to Gavin Sharp from comment #15)
> (In reply to [e-mail] from comment #12)
> > [The reasoning here is that highlightSyntax would work automatically with
> > history enabled
> 
> Why?

Because it works by reloading the current history entry.

Comment 17

[petr.laskevic]

Hello everybody, I encountered this bug when debugging an addon, but with Javascript. The syntax highlighting works when viewing source on mozilla.org

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:Honza, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to petr.laskevic from comment #17)

Hello everybody, I encountered this bug when debugging an addon, but with Javascript. The syntax highlighting works when viewing source on mozilla.org

Yes, it works for me to.
Closing.