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

NEW
Assigned to

Status

()

Toolkit
View Source
11 years ago
5 years ago

People

(Reporter: vicky.sun, Assigned: Tae Kim)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
OS: SunOS → Opensolaris
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]
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Opensolaris → All
Hardware: PC → All
Summary: "Syntax Highlighting" doesn't work in "View Selection Source" → "Syntax Highlighting" menu item doesn't work in "View Selection Source"
Version: unspecified → Trunk
(Assignee)

Comment 2

11 years ago
Created attachment 243630 [details] [diff] [review]
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.
Attachment #243630 - Flags: review+
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.
Attachment #243630 - Flags: review+ → review?(gavin.sharp)
(Assignee)

Comment 4

11 years ago
Oh ok, I get it now. I'll make sure to do that from now on. Thanks for fixing that.
Assignee: nobody → kimchi31415
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.
Attachment #243630 - Flags: review?(gavin.sharp) → review-
(Reporter)

Comment 6

11 years ago
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
(Reporter)

Comment 7

11 years ago
It can also be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a3pre) Gecko/20070302 Minefield/3.0a3pre.
Product: Firefox → Toolkit
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.
Created attachment 695982 [details] [diff] [review]
Patch for 'Syntax Highlighting' menu item in 'View Selection Source'

Builds on previous patch
Attachment #243630 - Attachment is obsolete: true
Attachment #695982 - Flags: feedback?(gavin.sharp)
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.
Attachment #695982 - Flags: feedback?(gavin.sharp) → feedback+
Created attachment 696203 [details] [diff] [review]
Patch for 'Syntax Highlighting' menu item in 'View Selection Source'
Attachment #695982 - Attachment is obsolete: true
Attachment #696203 - Flags: review?(neil)
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.]
Flags: needinfo?(gavin.sharp)
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.
Flags: needinfo?(gavin.sharp)
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...
Attachment #696203 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #12)
> [The reasoning here is that highlightSyntax would work automatically with
> history enabled

Why?
(In reply to Gavin Sharp from comment #15)
> (In reply to neil@parkwaycc.co.uk 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.
You need to log in before you can comment on or make changes to this bug.