Closed
Bug 159337
Opened 22 years ago
Closed 18 years ago
Disable "View Source" from menu/key command when source is in view
Categories
(Camino Graveyard :: Toolbars & Menus, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.5
People
(Reporter: stf, Assigned: stuart.morgan+bugzilla)
References
()
Details
(Keywords: fixed1.8.1.1, Whiteboard: [Good First Bug])
Attachments
(2 files, 3 obsolete files)
24.52 KB,
patch
|
froodian
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
25.77 KB,
application/octet-stream
|
froodian
:
review+
|
Details |
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".
Updated•22 years ago
|
Target Milestone: --- → Chimera0.5
Updated•22 years ago
|
Target Milestone: Chimera0.5 → Chimera0.9
Updated•22 years ago
|
Summary: View Source several times → Should have only one View Source per page
Updated•22 years ago
|
QA Contact: winnie → sairuh
Comment 2•22 years ago
|
||
*** Bug 183616 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 3•22 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•22 years ago
|
||
That would be dagley's auto downloading code.
Updated•22 years ago
|
Target Milestone: Camino0.9 → Camino1.1
Comment 5•21 years ago
|
||
*** Bug 217626 has been marked as a duplicate of this bug. ***
Comment 6•21 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.
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•21 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
Comment 9•20 years ago
|
||
Ping. Status update. Has anyone looked at this recently?
Comment 10•19 years ago
|
||
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: 316835
Comment 11•19 years ago
|
||
This should be a pretty easy fix, no?
if (activeURL == view-source://someurl) disable menu item
QA Contact: bugzilla → toolbars
Updated•19 years ago
|
Whiteboard: [Good First Bug]
Comment 12•19 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•19 years ago
|
||
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•19 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
Comment 15•19 years ago
|
||
(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•19 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.
Comment 17•19 years ago
|
||
(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•19 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•19 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
Comment 20•19 years ago
|
||
data: ?
Comment 21•19 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•19 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•18 years ago
|
||
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•18 years ago
|
Attachment #242465 -
Flags: review? → review?(hwaara)
Comment 24•18 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•18 years ago
|
Attachment #242465 -
Flags: superreview?(mikepinkerton)
Comment 25•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
||
(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•18 years ago
|
Attachment #242507 -
Flags: review?(stridey)
Comment 32•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
||
>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•18 years ago
|
||
Attachment #242842 -
Flags: review?(stridey)
Assignee | ||
Updated•18 years ago
|
Attachment #242841 -
Flags: review?(hwaara)
Assignee | ||
Updated•18 years ago
|
Attachment #242842 -
Flags: review?(hwaara)
Comment 42•18 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•18 years ago
|
Attachment #242842 -
Flags: review?(hwaara)
Comment 43•18 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•18 years ago
|
Attachment #242842 -
Flags: review?(stridey) → review+
Comment 44•18 years ago
|
||
This needs a new nib file - I just bitrotted this one (sorry).
Assignee | ||
Comment 45•18 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.
Comment 46•18 years ago
|
||
+- (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•18 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 48•18 years ago
|
||
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•18 years ago
|
||
This needs love on checkin, per comment 45.
Comment 50•18 years ago
|
||
Checked in on 1.8branch and trunk (with the nib love).
Status: ASSIGNED → RESOLVED
Closed: 18 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.
Description
•