Last Comment Bug 159337 - Disable "View Source" from menu/key command when source is in view
: Disable "View Source" from menu/key command when source is in view
Status: RESOLVED FIXED
[Good First Bug]
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: Stuart Morgan
:
:
Mentors:
http://bugzilla.mozilla.org/enter_bug...
: 183616 217626 (view as bug list)
Depends on:
Blocks: 328173 341853 342237
  Show dependency treegraph
 
Reported: 2002-07-25 03:54 PDT by Stephane Moureau
Modified: 2006-10-27 14:40 PDT (History)
14 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix, plus a couple stylistic clean-ups and a very minor optimisation (4.73 KB, patch)
2006-05-14 22:13 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
disables for view-source and about (7.36 KB, patch)
2006-10-16 20:30 PDT, Stuart Morgan
hwaara: review+
froodian: review-
Details | Diff | Splinter Review
consolidates validation (15.02 KB, patch)
2006-10-17 09:04 PDT, Stuart Morgan
hwaara: review+
froodian: review-
Details | Diff | Splinter Review
hopefully correct consolidation (24.52 KB, patch)
2006-10-20 00:04 PDT, Stuart Morgan
froodian: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
new MainMenu.nib (25.77 KB, application/octet-stream)
2006-10-20 00:05 PDT, Stuart Morgan
froodian: review+
Details

Description User image Stephane Moureau 2002-07-25 03:54:17 PDT
It's strange that you can ask several times View Source for the same page.
Wouldn't it be better to show/switch to the same tab if it's already opened.

Because now, if you ask View Source when you are IN the View Source tab, a
dialog appears proposing to save the file.
Either View Source should be disabled when already in Source tab, or its menu
should be changed to "View Page of that Source".
Comment 1 User image saari (gone) 2002-07-25 11:26:22 PDT
yeah, minor polish IMO
Comment 2 User image Steve Smart 2002-12-04 17:28:18 PST
*** Bug 183616 has been marked as a duplicate of this bug. ***
Comment 3 User image Stephane Moureau 2002-12-04 18:28:45 PST
Asking Show Source of a Source Tab now (since ?1204) puts a copy of the page
directly on the Desktop. No dialog is displayed and the second view source, the
third tab is remaining in Loading.
Comment 4 User image Simon Fraser 2002-12-04 18:37:20 PST
That would be dagley's auto downloading code.
Comment 5 User image Chris Casciano 2003-08-28 10:45:04 PDT
*** Bug 217626 has been marked as a duplicate of this bug. ***
Comment 6 User image Chris Casciano 2003-08-28 10:47:03 PDT
First off, the 2nd (or 3rd, etc.) view source no longer attempts a download ...
which is very good.

As to cleaning this up futher I think its a tough call with view source being in
a normal tab and view-source: being just another protocol like anything else.
There's nothing inherently wrong (as far as i can see) with
view-source:view-source:view-source:about:blank for instance - try it in Mozilla. 
Comment 7 User image Jasper 2003-08-28 11:00:25 PDT
Ofcourse it's not really wrong perhaps even good that it is possible/alowed,
it's just that it's plain useless and confusing.
Comment 8 User image Chris Casciano 2003-08-28 11:46:08 PDT
of course.

my comment was more related to the severity of this bug now that the accidental
cmd-e doesn't trigger a download and was not meant to imply this issue shouldn't
be looked into any futher
Comment 9 User image Samuel Sidler (old account; do not CC) 2005-05-14 00:16:32 PDT
Ping. Status update. Has anyone looked at this recently?
Comment 10 User image Samuel Sidler (old account; do not CC) 2005-08-02 19:44:01 PDT
Updating summary to reflect issue.
Comment 11 User image Samuel Sidler (old account; do not CC) 2006-03-23 19:05:20 PST
This should be a pretty easy fix, no?

if (activeURL == view-source://someurl) disable menu item
Comment 12 User image Chris Lawson (gone) 2006-05-14 22:07:32 PDT
There is no reason whatsoever this should block bug 316835.

Patch coming, btw.

cl
Comment 13 User image Chris Lawson (gone) 2006-05-14 22:13:11 PDT
Created attachment 222005 [details] [diff] [review]
fix, plus a couple stylistic clean-ups and a very minor optimisation

We had a weak consensus on IRC to disable view-source for about:blank as well. If there's a good reason *not* to do this, it's easy to remove, but the current patch disables both the menu and toolbar items if:

1) the Bookmarks Manager is up (includes History)
2) a view-source window (or tab) is up
3) about:blank is loaded in the frontmost window or tab

One note: I'd like a way to make kViewSourceProtocolString a global. Help?

cl
Comment 14 User image Håkan Waara 2006-05-15 02:57:20 PDT
Comment on attachment 222005 [details] [diff] [review]
fix, plus a couple stylistic clean-ups and a very minor optimisation

* Would it make sense to disable sendURL: in the same circumstances as view source? (i.e., bookmarks/history is up, or we're viewing source)

* I'm thinking that it would be nice to move this logic into another method on the BWC, considering how many things we need to disable or special-case when we're viewing an "internal URI" (bookmarks, about:blank, etc). Perhaps something like viewingInternalURI:?

* See the "extern" constants for how to make stuff global. Also http://www.cocoadev.com/index.pl?GlobalVariablesInCocoa
Comment 15 User image Samuel Sidler (old account; do not CC) 2006-05-15 10:24:46 PDT
(In reply to comment #13)
> 3) about:blank is loaded in the frontmost window or tab

I honestly see no reason to do this. If the user wants to view-source on an about:blank tab/window, why not let them? Some might use it to grab a valid doctype, for example. But really, why should there be a condition for about:blank but not any of the other about:s?

Are you going to disable it for about:logo and about:bloat? Those two make a lot more sense to me since the former shows the source of an image and the latter shows... nothing.
Comment 16 User image Håkan Waara 2006-05-15 10:30:36 PDT
Instead of scattering code around to disable individual cases, like Samuel points out, I think a better general approach is to disable all about:* URIs and then explicitly enable those that are actually useful.
Comment 17 User image Samuel Sidler (old account; do not CC) 2006-05-15 10:32:06 PDT
(In reply to comment #16)
> Instead of scattering code around to disable individual cases, like Samuel
> points out, I think a better general approach is to disable all about:* URIs
> and then explicitly enable those that are actually useful.
> 

I think that's the wrong way too. Who decides what about: URIs are worth enabling? All of them should be enabled except about:bookmarks and about:history, imo. They all contain a "web page", more or less, and should be able to be sourced.
Comment 18 User image Stuart Morgan 2006-05-23 20:28:05 PDT
Comment on attachment 222005 [details] [diff] [review]
fix, plus a couple stylistic clean-ups and a very minor optimisation

(In reply to comment #14)
> I'm thinking that it would be nice to move this logic into another method on
> the BWC, considering how many things we need to disable or special-case when
> we're viewing an "internal URI" (bookmarks, about:blank, etc). Perhaps
> something like viewingInternalURI:?

Agreed.


(In reply to comment #17)
> Who decides what about: URIs are worth enabling?

Camino developers, obviously.

> All of them should be enabled except about:bookmarks and about:history, imo.
> They all contain a "web page", more or less, and should be able to be sourced.

No, they all contain content that happens to be implemented in terms of HTML.  Implementation details should be masked from users.

(In reply to comment #16)
> Instead of scattering code around to disable individual cases, like Samuel
> points out, I think a better general approach is to disable all about:* URIs
> and then explicitly enable those that are actually useful.

Agreed.
Comment 19 User image Chris Lawson (gone) 2006-06-20 20:59:52 PDT
One more question for the rest of the team while I'm at it...

Are there any other protocols besides about: and view-source: that we should be worrying about here?

resource: comes to mind (acts just like file:, except restricted to the app package itself), but I can't find any others that actually do anything in Camino.

cl
Comment 20 User image Mike Pinkerton (not reading bugmail) 2006-06-21 08:53:57 PDT
data: ?
Comment 21 User image Chris Lawson (gone) 2006-06-21 09:09:22 PDT
(In reply to comment #20)
> data: ?
> 

I think data: should be treated just like an HTML page, since that's exactly what it is.

Why anyone would *want* to view source on the HTML code they type into the location bar is another matter entirely ;)

cl
Comment 22 User image Chris Casciano 2006-06-21 09:22:23 PDT
data can surely be more then just html one types themselves... inline images as an example

I would think it should jsut follow whatever rules are already established for the mime / determined file type in question (so viewing source on an image is silly, but css not as much, etc.)

and my take on this whole thing in general 9after the irc discussions last night) is that one should only block view source from things that will break the viewer (images, bookmarks) or that are dumb /broken (like recusrive view source'ing as the case with the original file)

anything else (about:mozilla), if a person wants to explore the underpinnings so be it.. why go the extra mile to stop them?
Comment 23 User image Stuart Morgan 2006-10-16 20:30:39 PDT
Created attachment 242465 [details] [diff] [review]
disables for view-source and about

Fix. I also fixed Fill Form since it was right there.
Comment 24 User image Håkan Waara 2006-10-17 05:40:29 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Looks good.
Comment 25 User image froodian (Ian Leue) 2006-10-17 06:51:59 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

As we discussed last night, we should be talking about this in tomorrow's status meeting, since there's a lot of controversy over what the correct behavior is here.  If dad says no, please don't ask mom.
Comment 26 User image Stuart Morgan 2006-10-17 07:01:23 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

(In reply to comment #25)
> (From update of attachment 242465 [details] [diff] [review] [edit])
> As we discussed last night, we should be talking about this in tomorrow's
> status meeting, since there's a lot of controversy over what the correct
> behavior is here.  If dad says no, please don't ask mom.

Um, you don't r- a patch because a decision hasn't been made yet. I asked hwaara to review it because there's no reason not to have the code reviewed in the interim, and I sr?d it so it doesn't get lost.
Comment 27 User image Håkan Waara 2006-10-17 07:04:10 PDT
The patches changes from ad-hoc checks all over the place, into a more reasonable approach where we will specifically enable what is interesting, I think this is the right way to go.

For specific bugs/discussions about what internal URIs should be enabled where, this will be simple to fix in followups, as I see it.
Comment 28 User image froodian (Ian Leue) 2006-10-17 07:10:21 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Well, let me r- this for a real reason then.  Since we do this check three times, I think it merits a helper method to make sure they stay in sync.
Comment 29 User image froodian (Ian Leue) 2006-10-17 07:21:30 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Bugger all, wrong bug.  sorry. /me hides
Comment 30 User image froodian (Ian Leue) 2006-10-17 07:24:09 PDT
Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Grrr, sorry.  Way too early.

This is the right bug. OK:

Conceptually, I think that we should decide together.

Codewise, I think this needs a helper method.

Sorry for the confusion.
Comment 31 User image Stuart Morgan 2006-10-17 09:04:44 PDT
Created attachment 242507 [details] [diff] [review]
consolidates validation

(In reply to comment #30)
> (From update of attachment 242465 [details] [diff] [review] [edit])
> Codewise, I think this needs a helper method.

I was going to do this in a follow-up bug, but now works too.  This gives a central method where all BWC actions can be validated, used by the menu and toolbar.  In later bugs we can start using it for more of the context menus as well.
Comment 32 User image Håkan Waara 2006-10-18 18:02:50 PDT
Comment on attachment 242507 [details] [diff] [review]
consolidates validation

You can use a switch-statement in both places where there are lots comparisons to |action|, I think it may look nicer.
Comment 33 User image Stuart Morgan 2006-10-18 19:32:27 PDT
(In reply to comment #32)
> (From update of attachment 242507 [details] [diff] [review] [edit])
> You can use a switch-statement in both places where there are lots comparisons
> to |action|, I think it may look nicer.

Selectors aren't integer values.
Comment 34 User image Håkan Waara 2006-10-19 01:08:07 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 242507 [details] [diff] [review] [edit] [edit])
> > You can use a switch-statement in both places where there are lots comparisons
> > to |action|, I think it may look nicer.
> 
> Selectors aren't integer values.

Actually, under the hood they are just pointers, and since you're doing direct comparison, this should be no problem.
Comment 35 User image Stuart Morgan 2006-10-19 06:26:40 PDT
(In reply to comment #34)
> Actually, under the hood they are just pointers, and since you're doing direct
> comparison, this should be no problem.

I dislike using an opaque type in a way that relies on specific implementation details of that type; I find that substantially uglier than a series of if statements.
Comment 36 User image Håkan Waara 2006-10-19 06:56:51 PDT
Comment on attachment 242507 [details] [diff] [review]
consolidates validation

OK, apart from my preference for switch-statements, the approachs looks OK.

I defer to froodian for reviewing the actual changes to what things are now enabled/disabled (the changes aren't to me obvious, except for that in the bug summary).
Comment 37 User image Stuart Morgan 2006-10-19 07:06:08 PDT
(In reply to comment #36)
> I defer to froodian for reviewing the actual changes to what things are now
> enabled/disabled (the changes aren't to me obvious, except for that in the bug
> summary).

It's of course possible that I made mistakes in moving stuff around, but the intentional changes are, for reference:
- disable view source for internal URIs (currently defined as view-source: and about:), and non-textual content (not a change)
- disable fill form in the same cases
- disable send url for internal URIs
- disable the print toolbar icon when the print menu is disabled
The last three were just obviously wrong as I moved/tested, and trivial to fix during the consolidation.
Comment 38 User image Simon Fraser 2006-10-19 09:10:51 PDT
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (From update of attachment 242507 [details] [diff] [review] [edit] [edit] [edit])
> > > You can use a switch-statement in both places where there are lots comparisons
> > > to |action|, I think it may look nicer.
> > 
> > Selectors aren't integer values.
> 
> Actually, under the hood they are just pointers, and since you're doing direct
> comparison, this should be no problem.

You can't switch on values that aren't known at compile time.
Comment 39 User image froodian (Ian Leue) 2006-10-19 12:59:07 PDT
Comment on attachment 242507 [details] [diff] [review]
consolidates validation

Should this be |validateActionBySelector|?  You'd know Cocoa conventions better than I, but I feel like I've seen a lot of fooBySelector methods.

I was under the impression that using else if increased perf in a case like this, since we know it's only ever one selector, and once we find it we don't have to check against all the others.  Is that untrue?

This loses validation for doReload: (main Reload menu item), since |validateAction| is only set up to respond to reload:. Same with doStop:, goBack:, goForward:

For closeTab: you replace a getKeyWindowBrowserController call with getMainWindowBrowserController + isKeyWindow.  This seems like it'd be the same to me, but all I really know is that that code has been fragile in the past.  If you're confident that it doesn't change behavior, I'll go with it.

Is there a reason you moved addBookmark from BWC's |validateToolbarItem| to |validateAction|, since it's only validated from the toolbar item?
Comment 40 User image Stuart Morgan 2006-10-20 00:04:38 PDT
Created attachment 242841 [details] [diff] [review]
hopefully correct consolidation

>Should this be |validateActionBySelector|?

Sounds good; done.

> I was under the impression that using else if increased perf in a case like
> this, since we know it's only ever one selector, and once we find it we don't
> have to check against all the others.  Is that untrue?

Every case is a return, so we never check the others anyway.

> This loses validation for doReload: (main Reload menu item), since
> |validateAction| is only set up to respond to reload:. Same with doStop:,
> goBack:, goForward:

Yep, that was dumb. I think everything is right this time.  In fixing it, I realized that embedding MainController selectors in BWC was ugly, so what I've done here is:
- rename the MainController selector to match the BWC selector it falls through to in almost every case, since that seems like a good idea anyway.
- translates the two items (goBack: and goForward:) that I didn't translated (because the MC name is better) before passing into the BWC validator.

I'm not going to do the BWC renames of back: and forward: here because it's much harder to rename a BWC selector and be confident that I haven't missed a caller than it is for MC, and it would cause this patch to spread out into more and more files. The cost for the translation is negligible.

> For closeTab: you replace a getKeyWindowBrowserController call with
> getMainWindowBrowserController + isKeyWindow.

That was already the code for the toolbar item, so it's not new code.

> Is there a reason you moved addBookmark from BWC's |validateToolbarItem| to
> |validateAction|, since it's only validated from the toolbar item?

I think everything that doesn't have a specific need to be outside the common validation routine should be in it, to keep the validation code as central as possible.
Comment 41 User image Stuart Morgan 2006-10-20 00:05:21 PDT
Created attachment 242842 [details]
new MainMenu.nib
Comment 42 User image Håkan Waara 2006-10-24 05:05:59 PDT
Comment on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

froodian's review should be enough here, I think.  Looks good to me though, I like consolidation. :)
Comment 43 User image froodian (Ian Leue) 2006-10-24 20:33:54 PDT
Comment on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

r=me
Comment 44 User image froodian (Ian Leue) 2006-10-25 11:47:49 PDT
This needs a new nib file - I just bitrotted this one (sorry).
Comment 45 User image Stuart Morgan 2006-10-26 23:03:42 PDT
(In reply to comment #44)
> This needs a new nib file - I just bitrotted this one (sorry).

Since your changes were minor, it would be great if you could take my nib, make your changes to it, and post that.
Comment 46 User image Mike Pinkerton (not reading bugmail) 2006-10-27 07:05:45 PDT
+- (BOOL)isInternalURI
+{
+  NSString* currentURI = [self getCurrentURI];
+  return ([currentURI hasPrefix:@"about:"] || [currentURI hasPrefix:@"view-source:"]);

don't we already have this code added for another bug? Or did I miss something. SR+ with that answered.
Comment 47 User image Stuart Morgan 2006-10-27 07:50:38 PDT
(In reply to comment #46)
> +- (BOOL)isInternalURI
> +{
> +  NSString* currentURI = [self getCurrentURI];
> +  return ([currentURI hasPrefix:@"about:"] || [currentURI
> hasPrefix:@"view-source:"]);
> 
> don't we already have this code added for another bug? Or did I miss something.
> SR+ with that answered.

IsInternalURI is called from bug 159337, which you already sr'd, so that's probably what you are thinking of; it depended on this landing.
Comment 48 User image Mike Pinkerton (not reading bugmail) 2006-10-27 08:39:48 PDT
Comment on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

well ok then ;) sr=pink
Comment 49 User image Stuart Morgan 2006-10-27 13:14:38 PDT
This needs love on checkin, per comment 45.
Comment 50 User image froodian (Ian Leue) 2006-10-27 14:40:36 PDT
Checked in on 1.8branch and trunk (with the nib love).

Note You need to log in before you can comment on or make changes to this bug.