Fix to respect --explicitly-allowed-ports command line option-Chromium Side

Fix to respect --explicitly-allowed-ports command line option-Chromium Side

[PATCH 1] : This Patch
[PATCH 2] : https://codereview.chromium.org/908483002/

BUG=223927
Committed: https://crrev.com/7e30c90e940c127f8352e5b514cf22814929882a
Cr-Commit-Position: refs/heads/master@{#325263}

[Paritosh Kumar, 2015-03-26 09:13:50]

paritosh.in@samsung.com changed reviewers:
+ eroman@chromium.org, tyoshino@chromium.org

[Paritosh Kumar, 2015-03-26 09:25:34]

@tyoshino:

As per https://codereview.chromium.org/908483002/#msg13
trying to initialize g_explicitly_allowed_ports from renderer process using
RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer().


This cl generates some presubmit check violations, as below:

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:37: #include
"chrome/common/chrome_switches.h"
This violates checdeps rule. Illegal Include.

https://codereview.chromium.org/1036823003/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:36: #include
"chrome/common/chrome_switches.h"
This Violates checkdeps rules.


PTAL.

[tyoshino (SeeGerritForStatus), 2015-03-30 06:41:01]

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:37: #include
"chrome/common/chrome_switches.h"
On 2015/03/26 09:25:34, Paritosh Kumar wrote:
> This violates checdeps rule. Illegal Include.

Yea. It violates the dependency rule. Maybe we need to create net_switches. But
let's wait for eroman's review as you said.

[eroman, 2015-03-30 19:40:02]

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:37: #include
"chrome/common/chrome_switches.h"
On 2015/03/30 06:41:01, tyoshino wrote:
> On 2015/03/26 09:25:34, Paritosh Kumar wrote:
> > This violates checdeps rule. Illegal Include.
> 
> Yea. It violates the dependency rule. Maybe we need to create net_switches.
But
> let's wait for eroman's review as you said.

I believe what you want is content_switches.h

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:507: bool
BlinkPlatformImpl::portAllowed(const unsigned short port, bool isFtp) const
is_ftp. also not typical to mark plain old data like short as "const".

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:511: return
net::IsPortAllowedByDefault(port) || net::IsPortAllowedByOverride(port);
> 80 characters

[tyoshino (SeeGerritForStatus), 2015-03-31 04:45:41]

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:37: #include
"chrome/common/chrome_switches.h"
On 2015/03/30 19:40:02, eroman wrote:
> On 2015/03/30 06:41:01, tyoshino wrote:
> > On 2015/03/26 09:25:34, Paritosh Kumar wrote:
> > > This violates checdeps rule. Illegal Include.
> > 
> > Yea. It violates the dependency rule. Maybe we need to create net_switches.
> But
> > let's wait for eroman's review as you said.
> 
> I believe what you want is content_switches.h

Do you mean adding a new flag definition to content_switches.h but keep the one
in chrome/common/chrome_switches.h? It also sounds good to me.

[Paritosh Kumar, 2015-04-01 13:38:06]

Thanks tyoshino and eroman

Added kExplicitlyAllowedPorts in content_switch.h.

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:37: #include
"chrome/common/chrome_switches.h"
Adding kExplicitlyAllowedPorts flag definition to content_switches.h

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:507: bool
BlinkPlatformImpl::portAllowed(const unsigned short port, bool isFtp) const
On 2015/03/30 19:40:02, eroman wrote:
> is_ftp. also not typical to mark plain old data like short as "const".

Done.

https://codereview.chromium.org/1036823003/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:511: return
net::IsPortAllowedByDefault(port) || net::IsPortAllowedByOverride(port);
On 2015/03/30 19:40:02, eroman wrote:
> > 80 characters

Done.

[eroman, 2015-04-10 01:32:17]

The overall approach looks reasonable. I have some comments on the API being
exposed in Platform but will comment on the other CL for that.

https://codereview.chromium.org/1036823003/diff/20001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/20001/content/child/blink_pla...
content/child/blink_platform_impl.cc:509: if (is_ftp)
{curly braces around this }

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
content/public/common/content_switches.h:148: CONTENT_EXPORT extern const char
kExplicitlyAllowedPorts[];
The same flag shouldn't be defined in both content/ and chrome/.
Right now it looks like this is relying on the linker to find the definition in
chrome which is not good.

Remove the switch from chrome in favor of this one in content (and it needs to
be defined in content_switches.cc probably)

[Paritosh Kumar, 2015-04-10 12:34:33]

Thanks eroman

Updated the cl.

Please have a look.

https://codereview.chromium.org/1036823003/diff/20001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/20001/content/child/blink_pla...
content/child/blink_platform_impl.cc:509: if (is_ftp)
On 2015/04/10 01:32:17, eroman wrote:
> {curly braces around this }
Thanks.
BTW, removing this if condition.

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
content/public/common/content_switches.h:148: CONTENT_EXPORT extern const char
kExplicitlyAllowedPorts[];
On 2015/04/10 01:32:17, eroman wrote:
> The same flag shouldn't be defined in both content/ and chrome/.
> Right now it looks like this is relying on the linker to find the definition
in
> chrome which is not good.
> 
> Remove the switch from chrome in favor of this one in content (and it needs to
> be defined in content_switches.cc probably)

Thanks. Removing it. 
But, https://codereview.chromium.org/1036823003/#msg5 ???

[tyoshino (SeeGerritForStatus), 2015-04-13 14:28:16]

lgtm

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/1036823003/diff/20001/content/public/common/c...
content/public/common/content_switches.h:148: CONTENT_EXPORT extern const char
kExplicitlyAllowedPorts[];
On 2015/04/10 12:34:32, Paritosh Kumar wrote:
> On 2015/04/10 01:32:17, eroman wrote:
> > The same flag shouldn't be defined in both content/ and chrome/.
> > Right now it looks like this is relying on the linker to find the definition
> in
> > chrome which is not good.
> > 
> > Remove the switch from chrome in favor of this one in content (and it needs
to
> > be defined in content_switches.cc probably)
> 
> Thanks. Removing it. 
> But, https://codereview.chromium.org/1036823003/#msg5 ???
> 
> 

I just wanted to confirm what eroman@ had suggested. I'm fine with defining it
only in content_switches.h.

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:516: || net::IsPortAllowedByOverride(port);
|| and && are placed at the end of the previous line in majority of this file.
Let's do that here too.

But rather I suggest we move this to between L513 and L514.

if (net::IsPortAllowedByOverride(port))
  return true;
if (gurl...

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
File content/child/blink_platform_impl_unittest.cc (right):

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:125: TestBlinkPlatformImpl
platform_impl;
two space indent

[eroman, 2015-04-13 15:55:35]

looks good after addressing these comments

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:511: if (gurl.SchemeIsFile())
I think in the case of a file:// URL gurl.has_port() will be false. Please
confirm this. If it is the case, then the test for gurl.has_port() below is
sufficient and no special case for file:// is needed.

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:513: if (gurl.has_port()) {
Let's move this earlier (before calling IntPort()).

For instance:

if (!gurl.has_port())
  return true;

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
File content/child/blink_platform_impl_unittest.cc (right):

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:12: 
Remove this blank line

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:127:
EXPECT_TRUE(platform_impl.portAllowed(GURL("file://example.com")));
please also add a test for file:// containing "ports"

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:131:
EXPECT_TRUE(platform_impl.portAllowed(GURL("http://example.com:80")));
please include tests which contain no explicitly specified port.

[Paritosh Kumar, 2015-04-14 14:50:35]

Thanks tyoshino and eroman

Updated. Please have a look.

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:511: if (gurl.SchemeIsFile())
On 2015/04/13 15:55:34, eroman wrote:
> I think in the case of a file:// URL gurl.has_port() will be false. Please
> confirm this. If it is the case, then the test for gurl.has_port() below is
> sufficient and no special case for file:// is needed.

Thanks. Done.

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:513: if (gurl.has_port()) {
On 2015/04/13 15:55:35, eroman wrote:
> Let's move this earlier (before calling IntPort()).
> 
> For instance:
> 
> if (!gurl.has_port())
>   return true;

Acknowledged.

https://codereview.chromium.org/1036823003/diff/60001/content/child/blink_pla...
content/child/blink_platform_impl.cc:516: || net::IsPortAllowedByOverride(port);
On 2015/04/13 14:28:16, tyoshino wrote:
> || and && are placed at the end of the previous line in majority of this file.
> Let's do that here too.
> 
> But rather I suggest we move this to between L513 and L514.
> 
> if (net::IsPortAllowedByOverride(port))
>   return true;
> if (gurl...
> 

Acknowledged.

[eroman, 2015-04-14 16:38:15]

LGTM after addressing comment below.

https://codereview.chromium.org/1036823003/diff/80001/content/child/blink_pla...
File content/child/blink_platform_impl_unittest.cc (right):

https://codereview.chromium.org/1036823003/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:125:
EXPECT_TRUE(platform_impl.portAllowed(GURL("example.com")));
What does this parse as? Please specify a protocol so it is clear what this is
testing.

As in:  GURL("http://example.com")

Unclear to me if the above is going to parse to an invalid GURL or a file:// or
an http:// URL.

[Paritosh Kumar, 2015-04-15 05:43:18]

paritosh.in@samsung.com changed reviewers:
+ avi@chromium.org, jochen@chromium.org, thestig@chromium.org

[Paritosh Kumar, 2015-04-15 05:43:19]

Thanks eroman
Addressed Your comment.

Adding Lei Zhang and avi for further review.
Lei: Please have a look on chrome/ cahnges.
avi: Please have a look on content/ changes.

https://codereview.chromium.org/1036823003/diff/80001/content/child/blink_pla...
File content/child/blink_platform_impl_unittest.cc (right):

https://codereview.chromium.org/1036823003/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl_unittest.cc:125:
EXPECT_TRUE(platform_impl.portAllowed(GURL("example.com")));
On 2015/04/14 16:38:15, eroman wrote:
> What does this parse as? Please specify a protocol so it is clear what this is
> testing.
> 
> As in:  GURL("http://example.com")
> 
> Unclear to me if the above is going to parse to an invalid GURL or a file://
or
> an http:// URL.

Thanks Done.

[Lei Zhang, 2015-04-15 05:56:19]

chrome/ lgtm

[jochen (gone - plz use gerrit), 2015-04-15 15:29:33]

lgtm

[Avi (use Gerrit), 2015-04-15 15:32:50]

lgtm

[Paritosh Kumar, 2015-04-15 15:48:56]

The CQ bit was checked by paritosh.in@samsung.com

[Paritosh Kumar, 2015-04-15 15:48:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1036823003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-15 15:49:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036823003/100001

[eroman, 2015-04-15 15:58:59]

patchset 6 still LGTM

[commit-bot: I haz the power, 2015-04-15 17:04:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-15 17:05:22]

Patchset 6 (id:??) landed as
https://crrev.com/7e30c90e940c127f8352e5b514cf22814929882a
Cr-Commit-Position: refs/heads/master@{#325263}