Enabling "Save as..." in view:source page....

Enabling "Save as..." in view:source page.

BUG=12748
(http://crbug.com/12748)
TEST=Goes to view-source:http://www.google.com/ and see if the menu is enabled.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24079

[tfarina (gmail-do not use), 2009-08-06 18:25:05]

Hi brettw,

Can you review this to me?

Thanks!

[pierre.lafayette, 2009-08-06 19:56:42]

Please list your code review url on the issues page for this item.

[brettw, 2009-08-07 16:35:28]

I think this would be better done by changing the callers to use the "real" URL
rather than the "display" URL. The "real" URL will be the same whether view
source is enabled or not, and the "display" one will have "view-source:"
prepended for display to the user.

If you look at TabContents::GetURL(), you want to do that except use the
entry->url() rather than display_url(). I would put this code in the callsites
rather than adding something new to TabContents.

[tfarina (gmail-do not use), 2009-08-14 01:16:09]

On 2009/08/07 16:35:28, brettw wrote:
> I think this would be better done by changing the callers to use the "real"
URL
> rather than the "display" URL. The "real" URL will be the same whether view
> source is enabled or not, and the "display" one will have "view-source:"
> prepended for display to the user.
> 
> If you look at TabContents::GetURL(), you want to do that except use the
> entry->url() rather than display_url(). I would put this code in the callsites
> rather than adding something new to TabContents.
Brett what did you mean with the "changing the callers"? Is not clear what is to
do.

[brettw, 2009-08-17 15:47:13]

I believe the caller of this function uses the display/virtual URL to compare
against. I think the "real" URL of the page in the NavigationEntry won't include
the view-source. If you change the caller of the function you changed to use
url(), I think the problem might be solved.

[tfarina (gmail-do not use), 2009-08-19 16:03:46]

On 2009/08/17 15:47:13, brettw wrote:
> I believe the caller of this function uses the display/virtual URL to compare
> against. I think the "real" URL of the page in the NavigationEntry won't
include
> the view-source. If you change the caller of the function you changed to use
> url(), I think the problem might be solved.
Hi brett.
Instead of changing the TabContents::GetURL from virtual_url() to just url() I
made the changes outside. I didn't changed GetURL because I'm not sure if
changing to url() in GetURL, will work as intended in others functions that use
GetURL.

[brettw, 2009-08-19 17:07:30]

I think you did this correctly. Can you add a comment in both places about why
we do this instead of GetURL()?

// Use the "real" URL of the page which actually reflects its origin rather than
the display one which may be different (like having "view-source:" on the
front).

[tfarina (gmail-do not use), 2009-08-19 18:10:45]

Hi,
On 2009/08/19 17:07:30, brettw wrote:
> I think you did this correctly. Can you add a comment in both places about why
> we do this instead of GetURL()?
> 
> // Use the "real" URL of the page which actually reflects its origin rather
than
> the display one which may be different (like having "view-source:" on the
> front).
I added the comments why we use the "real" url instead of using the
virtual/display url in this case.

[brettw, 2009-08-19 18:12:08]

http://codereview.chromium.org/165068/diff/4005/4007
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/165068/diff/4005/4007#newcode2266
Line 2266: // than the display one (returned by GetURL) which may be
different(like
Grammar nit: Can you add an extra space before the ( here? Same for the other
case.

[tfarina (gmail-do not use), 2009-08-19 18:15:16]

http://codereview.chromium.org/165068/diff/4005/4007
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/165068/diff/4005/4007#newcode2266
Line 2266: // than the display one (returned by GetURL) which may be
different(like
On 2009/08/19 18:12:08, brettw wrote:
> Grammar nit: Can you add an extra space before the ( here? Same for the other
> case.

Done.

[brettw, 2009-08-19 18:17:26]

LGTM

[tfarina (gmail-do not use), 2009-08-20 17:12:37]

Hi brett,
On 2009/08/19 18:17:26, brettw wrote:
> LGTM
Thanks. Can you land this to me?

[Mohamed Mansour, 2009-08-22 05:30:28]

I will land this once you rebase it? gcl update <your_cl>
A lot of stuff changed since 2 days,

[tfarina (gmail-do not use), 2009-08-22 13:38:42]

On 2009/08/22 05:30:28, Mohamed Mansour wrote:
> I will land this once you rebase it? gcl update <your_cl>
> A lot of stuff changed since 2 days,
Please, take another look Mohamed, I updated the patch.