Fix bug: AppShell: CHECK failure in PeerConnection init.

Fix bug: AppShell: CHECK failure in PeerConnection init.

The bug was caused due to a missing step to reset the url_request_getter in resource_context_ after initializing a url_request_context_getter in extensions::ShellBrowserContext, refer to CL (https://codereview.chromium.org/615583003/). To fix this bug, a refactor is done in this CL, including:

- ExtensionURLRequestContextGetter was originally implemented in the wrong directory. It's only used by app_shell, hence it belongs in extensions/shell/browser. Rename and move ExtensionURLRequestContextGetter to extensions::ShellURLRequestContextGetter and put it in extensions/shell/browser. Also share code as much as possible with content::ShellURLRequestContextGetter;

- Rename and move ExtensionNetworkDelegate to extensions::ShellNetworkDelegate and live in extensions/shell/browser;

- Reuse content/shell/common/shell_switches.h for kDumpRenderTree.

BUG=420698
Committed: https://crrev.com/3328d99286e14e8c31b922c995fb7de7804bba69
Cr-Commit-Position: refs/heads/master@{#298862}

[Xi Han, 2014-10-07 14:57:36]

Patchset #1 (id:1) has been deleted

[Xi Han, 2014-10-07 14:57:43]

Patchset #1 (id:20001) has been deleted

[Xi Han, 2014-10-07 15:17:13]

hanxi@chromium.org changed reviewers:
+ jamescook@chromium.org

[Xi Han, 2014-10-07 15:17:13]

James: please take a look at all the changes. Thanks. It will be great if you
can try this patch and let me know whether it fixes the crash on Envoy.

[James Cook, 2014-10-07 16:24:55]

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:32: class
ShellBrowserContext::ShellResourceContext : public ResourceContext {
Another way to solve this problem would be to move the declaration of
ShellResourceContext into the header file (public or protected) and make
resource_context_ protected. This would let extensions::ShellBrowserContext call
set_url_request_context_getter() on it.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:151: url_request_getter_ = new
ShellURLRequestContextGetter(
It you decide to go the SetURLRequestContextGetter() route, this code should
call it.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:37: void
set_url_request_context_getter(ShellURLRequestContextGetter* getter);
I would make these methods protected to help emphasize that only subclasses
should call them.

Also, since they are not inlined:
SetURLRequestContextGetter();
GetURLRequestContextGetter();

And the first one in particular needs a comment about why it exists.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:20: namespace
extensions {
Delete this. It's not needed, and you can't refer to extensions code from
//content anyhow.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:47: virtual
net::URLRequestContext* GetURLRequestContext() OVERRIDE;
We're transitioning to C++11, this should be lower-case override.  You might
need to bypass a presubmit check -- I'm not sure if those are working correctly.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:51: virtual
net::NetworkDelegate* CreateNetworkDelegate();
brief comment please

https://codereview.chromium.org/631203003/diff/40001/extensions/common/switch...
File extensions/common/switches.h (left):

https://codereview.chromium.org/631203003/diff/40001/extensions/common/switch...
extensions/common/switches.h:17: extern const char kDumpRenderTree[];
Hooray!

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:67: content:: BrowserThread::
PostTask(
formatting

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:78:
get_url_request_context_getter()->GetURLRequestContext();
This needs a comment explaining that GetURLRequestContext() has the side-effect
of creating the context.

These initialization functions seem really weird to me.  Init() isn't really
needed.  Also "Init()" implies the whole class is being initialized.  I would:

Eliminate Init().
Rename InitializationOnIOThread() to InitURLRequestContextOnIOThread() or
something similar to make it clear that's the only piece being initialized.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.h:11: #include
"extensions/shell/browser/shell_url_request_context_getter.h"
Do you need this include?

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.cc:42: 
Hooray for code sharing!  This looks much much better.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:27: explicit
ShellURLRequestContextGetter(
no explicit

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:28: void*
browser_context,
Does this have to be void* or can it be content::BrowserContext?

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:39: virtual
net::NetworkDelegate* CreateNetworkDelegate() OVERRIDE;
I think we prefer lower-case override now.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:45: void*
browser_context_;
BrowserContext* please

[Xi Han, 2014-10-07 19:29:14]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2014-10-07 19:29:54]

James: please take another look, thanks!

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:32: class
ShellBrowserContext::ShellResourceContext : public ResourceContext {
On 2014/10/07 16:24:54, James Cook wrote:
> Another way to solve this problem would be to move the declaration of
> ShellResourceContext into the header file (public or protected) and make
> resource_context_ protected. This would let extensions::ShellBrowserContext
call
> set_url_request_context_getter() on it.

Done.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:132: void
ShellBrowserContext::set_url_request_context_getter(
Change it to a regular inline setter function and move it to the header file.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:139:
ShellBrowserContext::get_url_request_context_getter() {
Change it to a regular inline getter function and move it to the header file.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:151: url_request_getter_ = new
ShellURLRequestContextGetter(
I adopt your suggestion to declare resource_context_ as protected, so the
extensions::ShellBrowserContext can use it directly. In this way, maybe we don't
need this SetURLRequestContextGetter() function, and still sets
resource_context_ and url_request_getter_ separately. I keep the
set_url_request_context_getter() and get_url_request_context_getter() as regular
setter and getter and make them as inline functions. 


On 2014/10/07 16:24:54, James Cook wrote:
> It you decide to go the SetURLRequestContextGetter() route, this code should
> call it.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:37: void
set_url_request_context_getter(ShellURLRequestContextGetter* getter);
Good idea. Make these methods protected, but leave them as inline functions
here. Update the set_url_request_context_getter and make it just work as a
setter.


On 2014/10/07 16:24:54, James Cook wrote:
> I would make these methods protected to help emphasize that only subclasses
> should call them.
> 
> Also, since they are not inlined:
> SetURLRequestContextGetter();
> GetURLRequestContextGetter();
> 
> And the first one in particular needs a comment about why it exists.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
File content/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:20: namespace
extensions {
Oops, thanks for catching it! Removed.

On 2014/10/07 16:24:54, James Cook wrote:
> Delete this. It's not needed, and you can't refer to extensions code from
> //content anyhow.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:47: virtual
net::URLRequestContext* GetURLRequestContext() OVERRIDE;
Updated.

On 2014/10/07 16:24:54, James Cook wrote:
> We're transitioning to C++11, this should be lower-case override.  You might
> need to bypass a presubmit check -- I'm not sure if those are working
correctly.

https://codereview.chromium.org/631203003/diff/40001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.h:51: virtual
net::NetworkDelegate* CreateNetworkDelegate();
On 2014/10/07 16:24:54, James Cook wrote:
> brief comment please

Done.

https://codereview.chromium.org/631203003/diff/40001/extensions/common/switch...
File extensions/common/switches.h (left):

https://codereview.chromium.org/631203003/diff/40001/extensions/common/switch...
extensions/common/switches.h:17: extern const char kDumpRenderTree[];
:)

On 2014/10/07 16:24:54, James Cook wrote:
> Hooray!

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:67: content:: BrowserThread::
PostTask(
On 2014/10/07 16:24:54, James Cook wrote:
> formatting

Done.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:78:
get_url_request_context_getter()->GetURLRequestContext();
On 2014/10/07 16:24:54, James Cook wrote:
> This needs a comment explaining that GetURLRequestContext() has the
side-effect
> of creating the context.
> 
> These initialization functions seem really weird to me.  Init() isn't really
> needed.  Also "Init()" implies the whole class is being initialized.  I would:
> 
> Eliminate Init().
> Rename InitializationOnIOThread() to InitURLRequestContextOnIOThread() or
> something similar to make it clear that's the only piece being initialized.

Remove Init() and rename.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.h:11: #include
"extensions/shell/browser/shell_url_request_context_getter.h"
Removed.

On 2014/10/07 16:24:54, James Cook wrote:
> Do you need this include?

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.cc:42: 
Totally agree:)

On 2014/10/07 16:24:54, James Cook wrote:
> Hooray for code sharing!  This looks much much better.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:27: explicit
ShellURLRequestContextGetter(
On 2014/10/07 16:24:54, James Cook wrote:
> no explicit

Done.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:28: void*
browser_context,
On 2014/10/07 16:24:54, James Cook wrote:
> Does this have to be void* or can it be content::BrowserContext?

Done.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:39: virtual
net::NetworkDelegate* CreateNetworkDelegate() OVERRIDE;
On 2014/10/07 16:24:54, James Cook wrote:
> I think we prefer lower-case override now.

Done.

https://codereview.chromium.org/631203003/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:45: void*
browser_context_;
On 2014/10/07 16:24:54, James Cook wrote:
> BrowserContext* please

Done.

[James Cook, 2014-10-07 20:14:17]

Looking better.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:15: #include
"content/shell/browser/shell_url_request_context_getter.h"
You should be able to forward declare ShellURLRequestContextGetter

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:70: ShellResourceContext() :
getter_(NULL) {}
Implement these in the .cc file

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:93: ShellURLRequestContextGetter*
get_url_request_context_getter() {
Just use url_request_context_getter().  And traditionally these go above the
setters.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:94: return
url_request_getter_.get(); }
formatting

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:49: new
extensions::ShellURLRequestContextGetter(
Don't need "extensions::" here

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_network_delegate.h (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_network_delegate.h:17: explicit
ShellNetworkDelegate(
no explicit (only use explicit when there is a single parameter)

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:50: bool
ignore_certificate_errors_;
I don't think you need this member.

[Xi Han, 2014-10-07 21:08:32]

Patchset #3 (id:100001) has been deleted

[Xi Han, 2014-10-07 21:10:18]

James: I am not sure about the part of removing member variable
ignore_certificate_errors_, whether my solution is the one that you are looking
for. PTAL. Thanks.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:15: #include
"content/shell/browser/shell_url_request_context_getter.h"
On 2014/10/07 20:14:17, James Cook wrote:
> You should be able to forward declare ShellURLRequestContextGetter

It is needed here because the method  void
set_url_request_context_getter(ShellURLRequestContextGetter* getter) sets the
scoped_refptr<ShellURLRequestContextGetter> url_request_getter_.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:70: ShellResourceContext() :
getter_(NULL) {}
On 2014/10/07 20:14:16, James Cook wrote:
> Implement these in the .cc file

Done.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:93: ShellURLRequestContextGetter*
get_url_request_context_getter() {
On 2014/10/07 20:14:17, James Cook wrote:
> Just use url_request_context_getter().  And traditionally these go above the
> setters.

Done.

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:94: return
url_request_getter_.get(); }
On 2014/10/07 20:14:16, James Cook wrote:
> formatting

Done.

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_browser_context.cc:49: new
extensions::ShellURLRequestContextGetter(
On 2014/10/07 20:14:17, James Cook wrote:
> Don't need "extensions::" here

Removed.

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_network_delegate.h (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_network_delegate.h:17: explicit
ShellNetworkDelegate(
Oh, I see, thanks a lot!

On 2014/10/07 20:14:17, James Cook wrote:
> no explicit (only use explicit when there is a single parameter)

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_url_request_context_getter.h (right):

https://codereview.chromium.org/631203003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_url_request_context_getter.h:50: bool
ignore_certificate_errors_;
On 2014/10/07 20:14:17, James Cook wrote:
> I don't think you need this member.

I remove this member and check its value when creating a new
ShellURLRequestContextGetter().

[Xi Han, 2014-10-07 21:48:03]

hanxi@chromium.org changed reviewers:
+ yoz@chromium.org

[Xi Han, 2014-10-07 21:48:04]

yoz@chromium.org: Please review changes in：

-extensions/browser
-extensions/common
-extensions/extensions.gyp

Thanks!

[James Cook, 2014-10-07 22:38:45]

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:15: #include
"content/shell/browser/shell_url_request_context_getter.h"
On 2014/10/07 21:10:18, hanxi wrote:
> On 2014/10/07 20:14:17, James Cook wrote:
> > You should be able to forward declare ShellURLRequestContextGetter
> 
> It is needed here because the method  void
> set_url_request_context_getter(ShellURLRequestContextGetter* getter) sets the
> scoped_refptr<ShellURLRequestContextGetter> url_request_getter_.

Huh? set_url_request_context_getter() just sets getter_, which is not a
scoped_ptr<>

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:71: virtual
~ShellResourceContext() {}
implement in .cc file also

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:93: ShellURLRequestContextGetter*
url_request_context_getter() {
nit: move this above set_url_request_context_getter

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:22: if
(cmd_line->HasSwitch(::switches::kIgnoreCertificateErrors) ||
no need for an if(), just return the result of the expression

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:23:
cmd_line->HasSwitch(switches::kDumpRenderTree))
You don't need kDumpRenderTree. We're not going to use that in app_shell.  Just
get rid of it and eliminate the #include

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:59: ignore_certificate_errors,
just call IgnoreCertificateErrors() here

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_url_request_context_getter.cc:31:
ignore_certificate_errors_(ignore_certificate_errors),
You never use this member variable. You can get rid of it.

[Yoyo Zhou, 2014-10-07 23:31:56]

LGTM after James's comments are addressed

[Xi Han, 2014-10-08 13:48:48]

James: PTAL. Thanks!

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:15: #include
"content/shell/browser/shell_url_request_context_getter.h"
There are two set_url_request_context_getter() in this file defined with the
same signatures, and I guess we were looking at different ones. I mean the
protected function 
ShellBrowserContext::set_url_request_context_getter(ShellURLRequestContextGetter*
getter) {
  url_request_getter_ = getter;
}

This method needs to call the constructor of ShellURLRequestContextGetter. Sorry
I didn't realize this.


On 2014/10/07 22:38:45, James Cook wrote:
> On 2014/10/07 21:10:18, hanxi wrote:
> > On 2014/10/07 20:14:17, James Cook wrote:
> > > You should be able to forward declare ShellURLRequestContextGetter
> > 
> > It is needed here because the method  void
> > set_url_request_context_getter(ShellURLRequestContextGetter* getter) sets
the
> > scoped_refptr<ShellURLRequestContextGetter> url_request_getter_.
> 
> Huh? set_url_request_context_getter() just sets getter_, which is not a
> scoped_ptr<>

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:71: virtual
~ShellResourceContext() {}
On 2014/10/07 22:38:45, James Cook wrote:
> implement in .cc file also

Moved.

https://codereview.chromium.org/631203003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:93: ShellURLRequestContextGetter*
url_request_context_getter() {
On 2014/10/07 22:38:45, James Cook wrote:
> nit: move this above set_url_request_context_getter

Done.

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:22: if
(cmd_line->HasSwitch(::switches::kIgnoreCertificateErrors) ||
On 2014/10/07 22:38:45, James Cook wrote:
> no need for an if(), just return the result of the expression

Done.

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:23:
cmd_line->HasSwitch(switches::kDumpRenderTree))
On 2014/10/07 22:38:45, James Cook wrote:
> You don't need kDumpRenderTree. We're not going to use that in app_shell. 
Just
> get rid of it and eliminate the #include

Done.

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:59: ignore_certificate_errors,
On 2014/10/07 22:38:45, James Cook wrote:
> just call IgnoreCertificateErrors() here

Removed.

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/631203003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_url_request_context_getter.cc:31:
ignore_certificate_errors_(ignore_certificate_errors),
On 2014/10/07 22:38:45, James Cook wrote:
> You never use this member variable. You can get rid of it.

Thank you for catching it. I forgot to update here. Removed.

[James Cook, 2014-10-08 14:21:22]

LGTM with one tiny nit

Thanks for getting this working!

https://codereview.chromium.org/631203003/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:21: return
cmd_line->HasSwitch(::switches::kIgnoreCertificateErrors) ?
nit: Just "return cmd_line->HasSwitch()", no need for ? :

[Xi Han, 2014-10-08 14:29:15]

hanxi@chromium.org changed reviewers:
+ creis@chromium.org

[Xi Han, 2014-10-08 14:29:15]

creis@chromium.org: Please review changes in:

-content/shell/browser.

Thanks!

https://codereview.chromium.org/631203003/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/631203003/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_context.cc:21: return
cmd_line->HasSwitch(::switches::kIgnoreCertificateErrors) ?
On 2014/10/08 14:21:22, James Cook wrote:
> nit: Just "return cmd_line->HasSwitch()", no need for ? :

Totally agree.

[Xi Han, 2014-10-08 15:51:12]

hanxi@chromium.org changed reviewers:
+ avi@chromium.org

[Xi Han, 2014-10-08 15:51:12]

+avi@chromium.org:

Please review changes in:
-content/shell/browser

Thanks!

[Avi (use Gerrit), 2014-10-08 15:58:17]

content/shell/browser lgtm

[Xi Han, 2014-10-08 16:00:35]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-10-08 16:01:23]

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

[commit-bot: I haz the power, 2014-10-08 17:07:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 17:07:21]

Failed to apply patch for extensions/common/switches.cc:
While running git apply --index -3 -p1;
  error: patch failed: extensions/common/switches.cc:19
  Falling back to three-way merge...
  Applied patch to 'extensions/common/switches.cc' with conflicts.
  U extensions/common/switches.cc

Patch:       extensions/common/switches.cc
Index: extensions/common/switches.cc
diff --git a/extensions/common/switches.cc b/extensions/common/switches.cc
index
d9eba9ebc163e7361c5abd57e02df58323785fe6..0f5675792e83e3fc8dc8f7a362820e88c53e981e
100644
--- a/extensions/common/switches.cc
+++ b/extensions/common/switches.cc
@@ -19,9 +19,6 @@ const char kAllowLegacyExtensionManifests[] =
 // Enables the <appview> tag in platform apps.
 const char kAppView[] = "app-view";
 
-// Request pages to be dumped as text once they finished loading.
-const char kDumpRenderTree[] = "dump-render-tree";
-
 // Enables extension options to be embedded in chrome://extensions rather than
 // a new tab.
 const char kEmbeddedExtensionOptions[] = "embedded-extension-options";

[commit-bot: I haz the power, 2014-10-08 17:07:29]

The CQ bit was unchecked by commit-bot@chromium.org

[Xi Han, 2014-10-08 17:31:23]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-10-08 17:31:52]

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

[commit-bot: I haz the power, 2014-10-08 19:55:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 19:55:44]

Failed to apply the patch.

[Xi Han, 2014-10-08 20:07:16]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:10:37]

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

[commit-bot: I haz the power, 2014-10-08 21:15:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 21:15:21]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[James Cook, 2014-10-09 00:17:04]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-10-09 00:21:52]

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

[commit-bot: I haz the power, 2014-10-09 01:46:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 01:46:30]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[Xi Han, 2014-10-09 02:30:44]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:37:49]

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

[commit-bot: I haz the power, 2014-10-09 03:47:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 03:47:15]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Xi Han, 2014-10-09 11:36:20]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-10-09 11:37:32]

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

[commit-bot: I haz the power, 2014-10-09 13:27:21]

Committed patchset #7 (id:200001) as 4368fee5985cd6dfca1b04a7c62c7913297a1199

[commit-bot: I haz the power, 2014-10-09 13:28:17]

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