159337 - Disable "View Source" from menu/key command when source is in view

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Stephane Moureau]

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

[saari (gone)]

yeah, minor polish IMO

Comment 2

[Steve Smart]

*** Bug 183616 has been marked as a duplicate of this bug. ***

Comment 3

[Stephane Moureau]

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

[Simon Fraser [no longer active]]

That would be dagley's auto downloading code.

Comment 5

[Chris Casciano]

*** Bug 217626 has been marked as a duplicate of this bug. ***

Comment 6

[Chris Casciano]

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

[Jasper]

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

[Chris Casciano]

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

[Samuel Sidler (old account; do not CC)]

Ping. Status update. Has anyone looked at this recently?

Comment 10

[Samuel Sidler (old account; do not CC)]

Updating summary to reflect issue.

Comment 11

[Samuel Sidler (old account; do not CC)]

This should be a pretty easy fix, no?

if (activeURL == view-source://someurl) disable menu item

Comment 12

[Chris Lawson (gone)]

There is no reason whatsoever this should block bug 316835.

Patch coming, btw.

cl

Comment 13

[Chris Lawson (gone)]

Attachment: 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

[Håkan Waara]

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

[Samuel Sidler (old account; do not CC)]

(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

[Håkan Waara]

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

[Samuel Sidler (old account; do not CC)]

(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

[Stuart Morgan]

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

[Chris Lawson (gone)]

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

[Mike Pinkerton (not reading bugmail)]

data: ?

Comment 21

[Chris Lawson (gone)]

(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

[Chris Casciano]

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

[Stuart Morgan]

Attachment: disables for view-source and about

Fix. I also fixed Fill Form since it was right there.

Comment 24

[Håkan Waara]

Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Looks good.

Comment 25

[froodian (Ian Leue)]

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

[Stuart Morgan]

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

[Håkan Waara]

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

[froodian (Ian Leue)]

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

[froodian (Ian Leue)]

Comment on attachment 242465 [details] [diff] [review]
disables for view-source and about

Bugger all, wrong bug.  sorry. /me hides

Comment 30

[froodian (Ian Leue)]

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

[Stuart Morgan]

Attachment: 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

[Håkan Waara]

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

[Stuart Morgan]

(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

[Håkan Waara]

(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

[Stuart Morgan]

(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

[Håkan Waara]

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

[Stuart Morgan]

(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

[Simon Fraser [no longer active]]

(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

[froodian (Ian Leue)]

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

[Stuart Morgan]

Attachment: 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

[Stuart Morgan]

Attachment: new MainMenu.nib

Comment 42

[Håkan Waara]

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

[froodian (Ian Leue)]

Comment on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

r=me

Comment 44

[froodian (Ian Leue)]

This needs a new nib file - I just bitrotted this one (sorry).

Comment 45

[Stuart Morgan]

(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

[Mike Pinkerton (not reading bugmail)]

+- (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

[Stuart Morgan]

(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

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

well ok then ;) sr=pink

Comment 49

[Stuart Morgan]

This needs love on checkin, per comment 45.

Comment 50

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk (with the nib love).