Emaphasize "https" instead of "view-source" when user accesses secure/insecur...

Emaphasize "https" instead of "view-source" when user accesses secure/insecure URLs with view-source prefix.

For example, this fix changes the text emphasized as follows:

*view-source*:https://example.com/ => view-source:*https*://example.com/

BUG=2349

[Peter Kasting, 2009-04-09 17:43:51]

http://codereview.chromium.org/62094/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/62094/diff/1/2#newcode1876
Line 1876: AutocompleteInput::Parse(url_text, model_->GetDesiredTLD(), &parts,
&scheme);
Nit: Might make sense to save the desired TLD into a temp since you're going to
use it below too

http://codereview.chromium.org/62094/diff/1/2#newcode1900
Line 1900: // For view-source scheme, we must emphasize the scheme of the URL
Nit: "For view-source" -> "For the view-source", "must" -> "should"

http://codereview.chromium.org/62094/diff/1/2#newcode1902
Line 1902: if (LowerCaseEqualsASCII(scheme, chrome::kViewSourceScheme)) {
You probably want "&& (url_text.length() > parts.scheme.end())" too

http://codereview.chromium.org/62094/diff/1/2#newcode1903
Line 1903: // Obtain the URL prefiexed by view-source and parse it.
Nit: "prefiexed" -> "prefixed"

http://codereview.chromium.org/62094/diff/1/2#newcode1908
Line 1908: scheme_begin = parts.scheme.end() + 1 + real_parts.scheme.begin;
Should probably bracket all this with "if (real_parts.scheme.is_nonempty()) {"

[hamaji, 2009-04-09 20:13:20]

Thanks for the review!

http://codereview.chromium.org/62094/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/62094/diff/1/2#newcode1876
Line 1876: AutocompleteInput::Parse(url_text, model_->GetDesiredTLD(), &parts,
&scheme);
On 2009/04/09 17:43:51, pkasting wrote:
> Nit: Might make sense to save the desired TLD into a temp since you're going
to
> use it below too

Done.

http://codereview.chromium.org/62094/diff/1/2#newcode1900
Line 1900: // For view-source scheme, we must emphasize the scheme of the URL
On 2009/04/09 17:43:51, pkasting wrote:
> Nit: "For view-source" -> "For the view-source", "must" -> "should"

Done.

http://codereview.chromium.org/62094/diff/1/2#newcode1902
Line 1902: if (LowerCaseEqualsASCII(scheme, chrome::kViewSourceScheme)) {
On 2009/04/09 17:43:51, pkasting wrote:
> You probably want "&& (url_text.length() > parts.scheme.end())" too

Added, but I think parts.scheme.end()+1 would be better. parts.scheme.end() is
strlen("view-source") and we need an additional byte for colon.

http://codereview.chromium.org/62094/diff/1/2#newcode1903
Line 1903: // Obtain the URL prefiexed by view-source and parse it.
On 2009/04/09 17:43:51, pkasting wrote:
> Nit: "prefiexed" -> "prefixed"

Done.

http://codereview.chromium.org/62094/diff/1/2#newcode1908
Line 1908: scheme_begin = parts.scheme.end() + 1 + real_parts.scheme.begin;
On 2009/04/09 17:43:51, pkasting wrote:
> Should probably bracket all this with "if (real_parts.scheme.is_nonempty()) {"

Done.

[Peter Kasting, 2009-04-09 23:00:34]

LGTM with nit

http://codereview.chromium.org/62094/diff/3001/3002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/62094/diff/3001/3002#newcode1915
Line 1915: (static_cast<int>(url_text.length()) > parts.scheme.end() + 1)) {
Nit: Since you refer to "parts.scheme.end() + 1" a lot I suggest factoring it
into a temp (e.g. "int after_scheme_and_colon = parts.scheme.end() + 1;").

[hamaji, 2009-04-10 05:37:09]

Thanks for your review.

http://codereview.chromium.org/62094/diff/3001/3002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/62094/diff/3001/3002#newcode1915
Line 1915: (static_cast<int>(url_text.length()) > parts.scheme.end() + 1)) {
On 2009/04/09 23:00:34, pkasting wrote:
> Nit: Since you refer to "parts.scheme.end() + 1" a lot I suggest factoring it
> into a temp (e.g. "int after_scheme_and_colon = parts.scheme.end() + 1;").

Done.

[hamaji, 2009-04-10 05:44:18]

I have no permission to check this patch in for now. Could you submit this,
please?

Thanks!

[Peter Kasting, 2009-04-10 19:54:59]

On 2009/04/10 05:44:18, hamaji wrote:
> I have no permission to check this patch in for now. Could you submit this,
> please?
> 
> Thanks!

I patched my local copy and tested this.  It indeed fixes the bug, but there's
another very similar issue that I'm wondering if we should just fix before
checking this in: the "emphasis" (black hostname, versus grey other text) is
broken for view-source:, because we don't parse a hostname out of it.

It seems like the solution to this is mostly to hoist your new code up higher in
the function.  Can you take a look at this?

[hamaji, 2009-04-10 20:06:31]

Ah, I didn't notice the black-hostname & gray-other issue. Yes, I'll look into
this next week.

Thanks,

[hamaji, 2009-04-13 09:06:31]

On 2009/04/10 19:54:59, pkasting wrote:
> On 2009/04/10 05:44:18, hamaji wrote:
> > I have no permission to check this patch in for now. Could you submit this,
> > please?
> > 
> > Thanks!
> 
> I patched my local copy and tested this.  It indeed fixes the bug, but there's
> another very similar issue that I'm wondering if we should just fix before
> checking this in: the "emphasis" (black hostname, versus grey other text) is
> broken for view-source:, because we don't parse a hostname out of it.
> 
> It seems like the solution to this is mostly to hoist your new code up higher
in
> the function.  Can you take a look at this?

Done. Could you take another look? As the source code is getting messy, I added
another function AutocompleteInput::ParseForEmphasizeComponents() and added a
unit test for it. I guess this logic could be used by other OSes as well.

[Peter Kasting, 2009-04-13 13:31:46]

This looks pretty good.  I like how simple the autocomplete edit changes have
become after your refactor.

http://codereview.chromium.org/62094/diff/6001/3006
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/62094/diff/6001/3006#newcode261
Line 261: if (real_parts.scheme.is_nonempty()) {
I'm not sure, but I think you want to set both |scheme| and |host| if either one
is nonempty in |real_parts|.  Otherwise, if |real_parts| only has one of them,
then you get one outparam set from |parts| and the other set from |real_parts|,
which seems bad.

http://codereview.chromium.org/62094/diff/6001/3007
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/62094/diff/6001/3007#newcode176
Line 176: // The results are alomst as same as the result of Parse(), but if the
scheme
Nit: alomst -> almost

http://codereview.chromium.org/62094/diff/6001/3008
File chrome/browser/autocomplete/autocomplete_unittest.cc (right):

http://codereview.chromium.org/62094/diff/6001/3008#newcode289
Line 289: { L"", kInvalidComponent, kInvalidComponent },
Rather than copying the input set for the Parse test and appending one
view-source URL, I suggest having a few cursory non-view-source URLs, and then a
couple different view-source URLs from different schemes, including some invalid
ones.  For example, test "view-source:", "view-source:garbage",
"view-source:http://http://foo", etc.

[hamaji, 2009-04-14 06:27:00]

http://codereview.chromium.org/62094/diff/6001/3006
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/62094/diff/6001/3006#newcode261
Line 261: if (real_parts.scheme.is_nonempty()) {
On 2009/04/13 13:31:46, pkasting wrote:
> I'm not sure, but I think you want to set both |scheme| and |host| if either
one
> is nonempty in |real_parts|.  Otherwise, if |real_parts| only has one of them,
> then you get one outparam set from |parts| and the other set from
|real_parts|,
> which seems bad.

I see. Fixed.

http://codereview.chromium.org/62094/diff/6001/3007
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/62094/diff/6001/3007#newcode176
Line 176: // The results are alomst as same as the result of Parse(), but if the
scheme
On 2009/04/13 13:31:46, pkasting wrote:
> Nit: alomst -> almost

Done.

http://codereview.chromium.org/62094/diff/6001/3008
File chrome/browser/autocomplete/autocomplete_unittest.cc (right):

http://codereview.chromium.org/62094/diff/6001/3008#newcode289
Line 289: { L"", kInvalidComponent, kInvalidComponent },
On 2009/04/13 13:31:46, pkasting wrote:
> Rather than copying the input set for the Parse test and appending one
> view-source URL, I suggest having a few cursory non-view-source URLs, and then
a
> couple different view-source URLs from different schemes, including some
invalid
> ones.  For example, test "view-source:", "view-source:garbage",
> "view-source:http://http://foo", etc.

Removed some test cases which have no view-source and added some cases with
view-source. Please note that this function may return valid components for
invalid URLs. For example, for "view-source:http://ftp://foo", this function
returns http as the scheme and ftp as the host name. This is the limitation of
URLFixerUpper::SegmentURL() and "http://ftp://foo" returns the same result.
Anyway, I think we don't need to fix this since this is harmless.

[Peter Kasting, 2009-04-14 16:47:48]

LGTM

[Peter Kasting, 2009-04-14 17:42:25]

Fixed in r13671.