The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Stephane Moureau, Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1
Dependency tree / graph

Details

(Whiteboard: [Good First Bug], URL)

Attachments

(2 attachments, 3 obsolete attachments)

24.52 KB, patch
froodian (Ian Leue)
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
25.77 KB, application/octet-stream
froodian (Ian Leue)
: review+
Details
(Reporter)

Description

15 years ago
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

15 years ago
yeah, minor polish IMO
Assignee: saari → pinkerton
Target Milestone: --- → Chimera0.5
Target Milestone: Chimera0.5 → Chimera0.9

Updated

15 years ago
Summary: View Source several times → Should have only one View Source per page
QA Contact: winnie → sairuh

Comment 2

15 years ago
*** Bug 183616 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 3

15 years ago
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

15 years ago
That would be dagley's auto downloading code.
Target Milestone: Camino0.9 → Camino1.1

Comment 5

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

Comment 6

14 years ago
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

14 years ago
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

14 years ago
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
Ping. Status update. Has anyone looked at this recently?
Updating summary to reflect issue.
Summary: Should have only one View Source per page → Disable "View Source" from menu/key command when source is in view
Blocks: 328173
Blocks: 316835
This should be a pretty easy fix, no?

if (activeURL == view-source://someurl) disable menu item
QA Contact: bugzilla → toolbars

Updated

11 years ago
Whiteboard: [Good First Bug]

Comment 12

11 years ago
There is no reason whatsoever this should block bug 316835.

Patch coming, btw.

cl
Assignee: mikepinkerton → bugzilla
No longer blocks: 316835

Comment 13

11 years ago
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
Attachment #222005 - Flags: review?(stuart.morgan)

Comment 14

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

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

Comment 18

11 years ago
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.
Attachment #222005 - Flags: review?(stuart.morgan) → review-
Blocks: 341853

Comment 19

11 years ago
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
data: ?

Comment 21

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

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

Comment 23

11 years ago
Created attachment 242465 [details] [diff] [review]
disables for view-source and about

Fix. I also fixed Fill Form since it was right there.
Assignee: bugzilla → stuart.morgan
Attachment #222005 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #242465 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #242465 - Flags: review? → review?(hwaara)

Updated

11 years ago
Blocks: 342237

Comment 24

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

Looks good.
Attachment #242465 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #242465 - Flags: superreview?(mikepinkerton)

Comment 25

11 years ago
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.
Attachment #242465 - Flags: superreview?(mikepinkerton) → review-
(Assignee)

Comment 26

11 years ago
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.
Attachment #242465 - Flags: superreview?(mikepinkerton)

Comment 27

11 years ago
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

11 years ago
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.
Attachment #242465 - Flags: superreview?(mikepinkerton)

Comment 29

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

Bugger all, wrong bug.  sorry. /me hides
Attachment #242465 - Flags: review- → superreview?(mikepinkerton)

Comment 30

11 years ago
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.
Attachment #242465 - Flags: superreview?(mikepinkerton) → review-
(Assignee)

Comment 31

11 years ago
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.
Attachment #242465 - Attachment is obsolete: true
Attachment #242507 - Flags: review?(hwaara)
(Assignee)

Updated

11 years ago
Attachment #242507 - Flags: review?(stridey)

Comment 32

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

Comment 33

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

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

Comment 35

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

11 years ago
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).
Attachment #242507 - Flags: review?(hwaara) → review+
(Assignee)

Comment 37

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

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

11 years ago
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?
Attachment #242507 - Flags: review?(stridey) → review-
(Assignee)

Comment 40

11 years ago
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.
Attachment #242507 - Attachment is obsolete: true
Attachment #242841 - Flags: review?(stridey)
(Assignee)

Comment 41

11 years ago
Created attachment 242842 [details]
new MainMenu.nib
Attachment #242842 - Flags: review?(stridey)
(Assignee)

Updated

11 years ago
Attachment #242841 - Flags: review?(hwaara)
(Assignee)

Updated

11 years ago
Attachment #242842 - Flags: review?(hwaara)

Comment 42

11 years ago
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. :)
Attachment #242841 - Flags: review?(hwaara)

Updated

11 years ago
Attachment #242842 - Flags: review?(hwaara)

Comment 43

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

r=me
Attachment #242841 - Flags: superreview?(mikepinkerton)
Attachment #242841 - Flags: review?(stridey)
Attachment #242841 - Flags: review+

Updated

11 years ago
Attachment #242842 - Flags: review?(stridey) → review+

Comment 44

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

Comment 45

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

Comment 47

11 years ago
(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 on attachment 242841 [details] [diff] [review]
hopefully correct consolidation

well ok then ;) sr=pink
Attachment #242841 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [Good First Bug] → [Good First Bug][needs checkin]
(Assignee)

Comment 49

11 years ago
This needs love on checkin, per comment 45.

Comment 50

11 years ago
Checked in on 1.8branch and trunk (with the nib love).
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [Good First Bug][needs checkin] → [Good First Bug]
You need to log in before you can comment on or make changes to this bug.