Chromium Code Reviews
Help | Chromium Project | Sign in
(443)

Issue 2893015: Do not wrap non-HTTP(s) requests. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by stoyan
Modified:
4 years ago
Reviewers:
tommi, tommi
CC:
chromium-reviews, amit
Visibility:
Public.

Description

Do not wrap non-HTTP(s) requests. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52494

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -1 line) Patch
M chrome_frame/protocol_sink_wrap.cc View 1 chunk +7 lines, -1 line 2 comments Download
Trybot results:
Commit: CQ not working?

Messages

Total messages: 7 (0 generated)
stoyan
4 years, 10 months ago (2010-07-14 19:06:44 UTC) #1
stoyan
4 years, 10 months ago (2010-07-14 19:07:25 UTC) #2
tommi
lgtm with one comment. http://codereview.chromium.org/2893015/diff/4001/5001 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2893015/diff/4001/5001#newcode152 chrome_frame/protocol_sink_wrap.cc:152: if ((url != StrStrW(url, L"http://")) ...
4 years, 10 months ago (2010-07-14 19:18:43 UTC) #3
stoyan
http://codereview.chromium.org/2893015/diff/4001/5001 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2893015/diff/4001/5001#newcode152 chrome_frame/protocol_sink_wrap.cc:152: if ((url != StrStrW(url, L"http://")) && (url != StrStrW(url, ...
4 years, 10 months ago (2010-07-14 19:34:10 UTC) #4
tommi
crap. I guess you could use base::strncasecmp like StartsWith does? :-/ also, you don't need ...
4 years, 10 months ago (2010-07-14 19:39:31 UTC) #5
stoyan
On 2010/07/14 19:39:31, tommi wrote: > crap. I guess you could use base::strncasecmp like StartsWith ...
4 years, 10 months ago (2010-07-14 19:46:52 UTC) #6
tommi
4 years, 10 months ago (2010-07-14 19:54:07 UTC) #7
On Wed, Jul 14, 2010 at 3:46 PM, <stoyan@chromium.org> wrote:

> On 2010/07/14 19:39:31, tommi wrote:
>
>> crap.  I guess you could use base::strncasecmp like StartsWith does? :-/
>>
>
> strncasecmp - 1) ignores the case 2) is ASCII only.


ah, right! details details.


>
>  also, you don't need do include the //, right?
>>
>
> Is including "//" wrong? I was aiming for the longest match.

nope, it's not wrong but neither is http:.  I was thinking that there's
probably a constant string for that somewhere already.
It's up to you though and it's just hairsplitting.  I think we've already
spent enough time, so just submit :)

>
>
>  E.g. just compare against "http:" or "https:"?
>>
>
>
>
> http://codereview.chromium.org/2893015/show
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be