Consolidate scheme checks into an easy GURL method

Introduce GURL::SchemeIsHttpFamily(), which returns true for http and https.

BUG=274679
TEST=N/A
TBR=cbentzel, jamesr, simonjam, tzik, stevet, mpcomplete

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218893

[abarth-chromium, 2013-08-19 18:10:26]

https://codereview.chromium.org/23064011/diff/5001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23064011/diff/5001/url/gurl.h#newcode207
url/gurl.h:207: bool SchemeIsHttp() const;
SchemeIsHttpFamily?

It would surprise me if a function called SchemeIsHttp() returned true for
https.

[abarth-chromium, 2013-08-19 18:11:49]

https://codereview.chromium.org/23064011/diff/5001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_util.cc (right):

https://codereview.chromium.org/23064011/diff/5001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_util.cc:84: }
Shoudl we just inline this function into its callers now that it's trivial?

[Cris Neckar, 2013-08-19 18:12:47]

On 2013/08/19 18:10:26, abarth wrote:
> https://codereview.chromium.org/23064011/diff/5001/url/gurl.h
> File url/gurl.h (right):
> 
> https://codereview.chromium.org/23064011/diff/5001/url/gurl.h#newcode207
> url/gurl.h:207: bool SchemeIsHttp() const;
> SchemeIsHttpFamily?
> 
> It would surprise me if a function called SchemeIsHttp() returned true for
> https.

Perhaps SchemeIsHttpFamily()?

[Cris Neckar, 2013-08-19 18:14:00]

On 2013/08/19 18:12:47, Cris Neckar wrote:
> On 2013/08/19 18:10:26, abarth wrote:
> > https://codereview.chromium.org/23064011/diff/5001/url/gurl.h
> > File url/gurl.h (right):
> > 
> > https://codereview.chromium.org/23064011/diff/5001/url/gurl.h#newcode207
> > url/gurl.h:207: bool SchemeIsHttp() const;
> > SchemeIsHttpFamily?
> > 
> > It would surprise me if a function called SchemeIsHttp() returned true for
> > https.
> 
> Perhaps SchemeIsHttpFamily()?

Or SchemeIsWeb... Either works for me.

[abarth-chromium, 2013-08-19 18:56:10]

On 2013/08/19 18:14:00, Cris Neckar wrote:
> Or SchemeIsWeb... Either works for me.

ftp is part of the web.  I'd prefer HttpFamily.

Adam

[Cris Neckar, 2013-08-19 20:15:48]

https://codereview.chromium.org/23064011/diff/5001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_util.cc (right):

https://codereview.chromium.org/23064011/diff/5001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_util.cc:84: }
On 2013/08/19 18:11:49, abarth wrote:
> Shoudl we just inline this function into its callers now that it's trivial?

Done.

https://codereview.chromium.org/23064011/diff/5001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23064011/diff/5001/url/gurl.h#newcode207
url/gurl.h:207: bool SchemeIsHttp() const;
On 2013/08/19 18:10:26, abarth wrote:
> SchemeIsHttpFamily?
> 
> It would surprise me if a function called SchemeIsHttp() returned true for
> https.

Done.

[abarth-chromium, 2013-08-19 20:18:36]

url/   <--- lgtm

[Cris Neckar, 2013-08-20 17:57:25]

owners reviews please :)

chrome/browser/android/
nyquist@chromium.org

chrome/browser/extensions/api/dial
justinlin@chromium.org

chrome/browser/google/
pkasting@chromium.org

chrome/browser/metrics/variations
stevet@chromium.org

chrome/browser/prerender
cbentzel@chromium.org

chrome/common/extensions
mpcomplete@chromium.org


content/browser/download
net
chrome/browser/net
rdsmith@chromium.org

content/browser/loader
simonjam@chromium.org

content/renderer/media
dalecurtis@chromium.org

content/renderer
jamesr@chromium.org

webkit/browser/fileapi
tzik@chromium.org

[Peter Kasting, 2013-08-20 18:01:23]

On 2013/08/20 17:57:25, Cris Neckar wrote:
> owners reviews please :)

For mechanical changes like this, just use as few top-level OWNERs as possible,
or TBR as necessary.  Waiting on ten more reviewers just bogs you down.

That said, I hate to bikeshed on the name, but how about "SchemeIsHTTPOrHTTPS"? 
This is only 1 character longer and seems noticeably clearer to me, not least
because I don't know what "family" means w.r.t. schemes.

With that, rubber-stamp LGTM on google/.

[Randy Smith (Not in Mondays), 2013-08-20 18:02:38]

content/browser/download/* and net/* LGTM.

[justinlin, 2013-08-20 18:13:41]

chrome/browser/extensions/api/dial/* LGTM

[cdn1, 2013-08-20 18:32:20]

Thanks Peter, I was just going to TBR anyone I didn't hear back from today.

Adam, any preference on naming? "Family" seems less clunky to me but
what do I know.

On Tue, Aug 20, 2013 at 11:13 AM,  <justinlin@chromium.org> wrote:
> chrome/browser/extensions/api/dial/* LGTM
>
> https://codereview.chromium.org/23064011/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2013-08-20 18:37:08]

content/renderer/media lgtm

[jamesr, 2013-08-20 18:55:19]

Just one content/ and one chrome/ owners should be fine, or just TBR.  It's not
a good use of 12 separate reviewer's time to look at a patch like this

[grt (UTC plus 2), 2013-08-20 19:57:49]

chrome_frame/ LGTM.

Please make the CL description a little more descriptive so that readers know
what's going on without seeing the code. Something like:

"Introduce GURL::SchemeIsHttpFamily(), which returns true for http and https."

Would be nice. Thanks.

[nyquist, 2013-08-20 21:05:44]

chrome/browser/android/ LGTM

[commit-bot: I haz the power, 2013-08-20 21:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/23064011/14001

[abarth-chromium, 2013-08-20 21:43:39]

On 2013/08/20 18:32:20, cdn1 wrote:
> Adam, any preference on naming? "Family" seems less clunky to me but
> what do I know.

Either is fine with me.  SchemeIsHTTPOrHTTPS is probably better because it's
more explicit.

[tzik, 2013-08-21 02:30:11]

w/b/f LGTM

[commit-bot: I haz the power, 2013-08-21 03:54:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[grt (UTC plus 2), 2013-08-21 14:50:12]

On 2013/08/20 19:57:49, grt wrote:
> Please make the CL description a little more descriptive so that readers know
> what's going on without seeing the code. Something like:
> 
> "Introduce GURL::SchemeIsHttpFamily(), which returns true for http and https."
> 
> Would be nice. Thanks.

Please update the CL description before re-trying to land. Thanks.

[Cris Neckar, 2013-08-21 17:21:45]

On 2013/08/21 14:50:12, grt wrote:
> On 2013/08/20 19:57:49, grt wrote:
> > Please make the CL description a little more descriptive so that readers
know
> > what's going on without seeing the code. Something like:
> > 
> > "Introduce GURL::SchemeIsHttpFamily(), which returns true for http and
https."
> > 
> > Would be nice. Thanks.
> 
> Please update the CL description before re-trying to land. Thanks.

apologies grt, I thought I had updated it already. Done.

[commit-bot: I haz the power, 2013-08-21 17:40:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/23064011/34001

[commit-bot: I haz the power, 2013-08-22 01:43:38]

Change committed as 218893