Remove CWV class and move setting User Agent to CWVWebViewConfiguration.

Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287
Review-Url: https://codereview.chromium.org/2791403005
Cr-Commit-Position: refs/heads/master@{#464848}
Committed: https://chromium.googlesource.com/chromium/src/+/f21fa55c1594deac368f09c8847b451bea0c1bc2

[michaeldo, 2017-04-04 21:40:33]

michaeldo@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-04-04 21:50:26]

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view_configuration.mm:63: +
(void)setupWebClientIfNeeded {
Does this method need a different name? It does not do anything with
"WebClient".

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view_configuration.mm:63: +
(void)setupWebClientIfNeeded {
Please add comments.

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/public/cwv_web...
File ios/web_view/public/cwv_web_view_configuration.h (right):

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/public/cwv_web...
ios/web_view/public/cwv_web_view_configuration.h:27: // format "product/1.0".
NOTE: This must be called before any
>> NOTE: This must be called before any
This is pretty strict requirement. Is it temporary? Or so, please add a TODO. I
think we should avoid API that "must be called" before anything else.

[michaeldo, 2017-04-07 22:31:35]

PTAL.

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view_configuration.mm:63: +
(void)setupWebClientIfNeeded {
On 2017/04/04 21:50:26, Eugene But wrote:
> Does this method need a different name? It does not do anything with
> "WebClient".

PTAL, in newest patch, I renamed to webClient and it returns the value.

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/public/cwv_web...
File ios/web_view/public/cwv_web_view_configuration.h (right):

https://codereview.chromium.org/2791403005/diff/1/ios/web_view/public/cwv_web...
ios/web_view/public/cwv_web_view_configuration.h:27: // format "product/1.0".
NOTE: This must be called before any
On 2017/04/04 21:50:26, Eugene But wrote:
> >> NOTE: This must be called before any
> This is pretty strict requirement. Is it temporary? Or so, please add a TODO.
I
> think we should avoid API that "must be called" before anything else.

Moved to CWVWebView as static method per discussion. Added better comment there
as well.

[Eugene But (OOO till 7-30), 2017-04-08 00:34:36]

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:52: +
(ios_web_view::WebViewWebClient*)webClient {
This whole method looks awkward, and I think that's because of the ownership
problems. How about this?:
 - Make WebViewConfiguration own BrowserState, so Configuration does not depend
on WebClient
 - Make WebClient variable static and initialize inside -[CWVWebView initialize]
 - Move WebViewWebMainDelegate, and WebMainParams creation to -[CWVWebView
initialize]

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:63: +
(void)setUserAgentProduct:(NSString*)product;
Should this be setUserAgent to allow better flexibility and API clarity?

[Eugene But (OOO till 7-30), 2017-04-08 00:49:57]

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:63: return
static_cast<ios_web_view::WebViewWebClient*>(web::GetWebClient());
After giving some thoughts, I this we can avoid casting like this:
namespace ios_web_view {
WebViewWebClient* GetWebClient() {
  static WebViewWebClient* web_client;
  web::SetWebClient(&web_client);
  return &web_client;
}
}
And then ios/web_view should only call ios_web_view::GetWebClient() instead of
web::GetWebClient().

WDYT?

[michaeldo, 2017-04-11 17:02:33]

Description was changed from

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.

BUG=703359, 706287
==========

to

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287
==========

[michaeldo, 2017-04-11 17:02:59]

Description was changed from

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287
==========

to

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287
==========

[michaeldo, 2017-04-11 17:07:48]

PTAL at latest patch. Sorry the size of this CL is growing, but with last
suggestions about ownership, it required cleanup in additional files of old
browser state getters/storage.

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:52: +
(ios_web_view::WebViewWebClient*)webClient {
On 2017/04/08 00:34:36, Eugene But wrote:
> This whole method looks awkward, and I think that's because of the ownership
> problems. How about this?:
>  - Make WebViewConfiguration own BrowserState, so Configuration does not
depend
> on WebClient
>  - Make WebClient variable static and initialize inside -[CWVWebView
initialize]
>  - Move WebViewWebMainDelegate, and WebMainParams creation to -[CWVWebView
> initialize]
>   

I was able to fix ownership of BrowserState. WebClient lives in the
configuration object because it is allocated first. PTAL.

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:63: return
static_cast<ios_web_view::WebViewWebClient*>(web::GetWebClient());
On 2017/04/08 00:49:57, Eugene But wrote:
> After giving some thoughts, I this we can avoid casting like this:
> namespace ios_web_view {
> WebViewWebClient* GetWebClient() {
>   static WebViewWebClient* web_client;
>   web::SetWebClient(&web_client);
>   return &web_client;
> }
> }
> And then ios/web_view should only call ios_web_view::GetWebClient() instead of
> web::GetWebClient().
> 
> WDYT?

I was able to get rid of the cast by moving webClient static ref to this class
and return correct type directly.

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/40001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:63: +
(void)setUserAgentProduct:(NSString*)product;
On 2017/04/08 00:34:36, Eugene But wrote:
> Should this be setUserAgent to allow better flexibility and API clarity? 

Per offline discussion, setUserAgentProduct should be sufficient. We can add an
additional API in the future if there is demand for it.

[Eugene But (OOO till 7-30), 2017-04-11 17:52:42]

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:49: // The configuration of the web view.
s/The configuration of the web view/Redefine the property as readwrite ?

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:27: // Initialize
configuration with the specified browser state.
Would you mind fixing this comment s/Initialize/Initializes ?

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:28: -
(instancetype)initOffTheRecord:(BOOL)offTheRecord;
I think that initializer should always start with "initWith" prefix and it's
quite hard to name this method. 
How about keeping the original signature?:

+ (instancetype)defaultConfiguration {
  web::BrowserState* browserState =
base::MakeUnique<ios_web_view::WebViewBrowserState>(false /*off_the_record*/);
  return [[self alloc] initWithBrowserState:std::move(browserState)];
}

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: static
std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
Is there a reason for making this global? It is used only in one method and can
be a local variable, unless I'm missing something.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:39: return
[[CWVWebViewConfiguration alloc] initOffTheRecord:false];
Do you want to keep old code ([self class]) to allow subclassing?

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:49: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate>
Do you want to move globals initialization to +[CWVWebViewConfiguration
initialize]? This way we can add more designated initializers in the future and
will not clutter this init code.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:94: +
(ios_web_view::WebViewWebClient*)webClient {
I don't feel that WebClient conceptually belongs to Configuration. Should this
be a separate C-function?

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:95: webClient =
base::MakeUnique<ios_web_view::WebViewWebClient>();
This will recreate |webClient| every time when this method is called, which is
pretty unexpected side effect.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_web_client.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/w...
ios/web_view/internal/web_view_web_client.h:29: void SetProduct(const
std::string& user_agent_product);
Do you want to split ownership refactoring and adding new API into different
CLs?

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:60: // Setting |product| is only guaranteed
to affect web views which have not yet
Do you want to document how calling this API will actually affect final UA
string?

[michaeldo, 2017-04-11 21:57:03]

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:49: // The configuration of the web view.
On 2017/04/11 17:52:41, Eugene But wrote:
> s/The configuration of the web view/Redefine the property as readwrite ?

Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:27: // Initialize
configuration with the specified browser state.
On 2017/04/11 17:52:42, Eugene But wrote:
> Would you mind fixing this comment s/Initialize/Initializes ?

Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:28: -
(instancetype)initOffTheRecord:(BOOL)offTheRecord;
On 2017/04/11 17:52:42, Eugene But wrote:
> I think that initializer should always start with "initWith" prefix and it's
> quite hard to name this method. 
> How about keeping the original signature?:
> 
> + (instancetype)defaultConfiguration {
>   web::BrowserState* browserState =
> base::MakeUnique<ios_web_view::WebViewBrowserState>(false /*off_the_record*/);
>   return [[self alloc] initWithBrowserState:std::move(browserState)];
> }

I agree. Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: static
std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
On 2017/04/11 17:52:41, Eugene But wrote:
> Is there a reason for making this global? It is used only in one method and
can
> be a local variable, unless I'm missing something.

WebClient is assumed to be static in web::WebClient so we would need to relax
that assumption first if I understand it correctly.

https://cs.chromium.org/chromium/src/ios/web/public/web_client.h?rcl=4fad7069...

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:39: return
[[CWVWebViewConfiguration alloc] initOffTheRecord:false];
On 2017/04/11 17:52:41, Eugene But wrote:
> Do you want to keep old code ([self class]) to allow subclassing?

Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:49: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate>
On 2017/04/11 17:52:42, Eugene But wrote:
> Do you want to move globals initialization to +[CWVWebViewConfiguration
> initialize]? This way we can add more designated initializers in the future
and
> will not clutter this init code.

Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:94: +
(ios_web_view::WebViewWebClient*)webClient {
On 2017/04/11 17:52:41, Eugene But wrote:
> I don't feel that WebClient conceptually belongs to Configuration. Should this
> be a separate C-function?

I agree it's odd here. I can actually delete this completely by storing the user
agent product as a static string inside CWVWebView and fetching it when needed
from WebViewWebClient. WDYT? (Please see the latest patch.)

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:95: webClient =
base::MakeUnique<ios_web_view::WebViewWebClient>();
On 2017/04/11 17:52:42, Eugene But wrote:
> This will recreate |webClient| every time when this method is called, which is
> pretty unexpected side effect.

This was my mistake. Moved to static initialize method.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_web_client.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/w...
ios/web_view/internal/web_view_web_client.h:29: void SetProduct(const
std::string& user_agent_product);
On 2017/04/11 17:52:42, Eugene But wrote:
> Do you want to split ownership refactoring and adding new API into different
> CLs?

I tried to do this, but I would have to set user_agent_product_ below to
something and the existing api is removed with the refactoring. I can certainly
remove the addition of the new PAI entry point, but User Agent setting would be
broken once the CL landed because of CWV removal unless I temporarily added API
to pass user agent through into the configuration object initialization.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:60: // Setting |product| is only guaranteed
to affect web views which have not yet
On 2017/04/11 17:52:42, Eugene But wrote:
> Do you want to document how calling this API will actually affect final UA
> string?

I added a note that |product| is inserted into the UA string. I don't know if
it's helpful to show a full example here as it will change and the documentation
will likely be out of date. WDYT?

[Eugene But (OOO till 7-30), 2017-04-12 00:46:18]

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: static
std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
On 2017/04/11 21:57:02, michaeldo wrote:
> On 2017/04/11 17:52:41, Eugene But wrote:
> > Is there a reason for making this global? It is used only in one method and
> can
> > be a local variable, unless I'm missing something.
> 
> WebClient is assumed to be static in web::WebClient so we would need to relax
> that assumption first if I understand it correctly.
> 
>
https://cs.chromium.org/chromium/src/ios/web/public/web_client.h?rcl=4fad7069...

My proposal was this:
+ (void)initialize {
  static std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
  web::SetWebClient(&webClient);
}

|webClient| will be constructed when |initialize| is called and destructed at
application exit. webClient will remain the same so
|web::SetWebClient(&webClient);| call will be idempotent.

But you can just move |static std::unique_ptr<ios_web_view::WebViewWebClient>
webClient;| to initialize, unless I'm missing something.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:60: // Setting |product| is only guaranteed
to affect web views which have not yet
On 2017/04/11 21:57:03, michaeldo wrote:
> On 2017/04/11 17:52:42, Eugene But wrote:
> > Do you want to document how calling this API will actually affect final UA
> > string?
> 
> I added a note that |product| is inserted into the UA string. I don't know if
> it's helpful to show a full example here as it will change and the
documentation
> will likely be out of date. WDYT?
I think it is helpful to explain why clients would want to call this method and
what will happen if they don't call it (I assume that UA will look silly in this
case).

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:71: gUserAgentProduct = product;
gUserAgentProduct = [product copy];

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: + (void)initialize;
nit: There is no need to redeclare NSObject's methods.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:43: [[self class]
initialize];
I don't think you need to call this explicitly. |initialize| should be called by
Objective-C runtime.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:58: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate> webMainDelegate;
if (self != [CWVWebViewConfiguration class])
  return;

initialize is called for subclasses (or super classes).

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:80:
downloadManager->set_request_context(_browserState->GetRequestContext());
Sorry, I missed this during the previous round. Looks like
translate::TranslateDownloadManager will have request context from the latest
created browser state, which looks like a bug to me. Do you know if Chrome has
the same problem with TranslateDownloadManager singleton?

[michaeldo, 2017-04-12 17:16:48]

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:71: gUserAgentProduct = product;
On 2017/04/12 00:46:17, Eugene But wrote:
> gUserAgentProduct = [product copy];

Done.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: + (void)initialize;
On 2017/04/12 00:46:17, Eugene But wrote:
> nit: There is no need to redeclare NSObject's methods.

Done.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:43: [[self class]
initialize];
On 2017/04/12 00:46:17, Eugene But wrote:
> I don't think you need to call this explicitly. |initialize| should be called
by
> Objective-C runtime.

Done.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:58: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate> webMainDelegate;
On 2017/04/12 00:46:17, Eugene But wrote:
> if (self != [CWVWebViewConfiguration class])
>   return;
> 
> initialize is called for subclasses (or super classes).

Done. But wouldn't we want this to still execute for subclasses?

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:80:
downloadManager->set_request_context(_browserState->GetRequestContext());
On 2017/04/12 00:46:17, Eugene But wrote:
> Sorry, I missed this during the previous round. Looks like
> translate::TranslateDownloadManager will have request context from the latest
> created browser state, which looks like a bug to me. Do you know if Chrome has
> the same problem with TranslateDownloadManager singleton?

Sorry I missed this too. Chrome actually has a global translatE_service_ios that
sets up the translate singleton with a global request context.
(GetApplicationContext()->GetSystemURLRequestContext()). We should likely do
something similar here.

[Eugene But (OOO till 7-30), 2017-04-12 17:48:28]

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: static
std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
On 2017/04/12 00:46:17, Eugene But wrote:
> On 2017/04/11 21:57:02, michaeldo wrote:
> > On 2017/04/11 17:52:41, Eugene But wrote:
> > > Is there a reason for making this global? It is used only in one method
and
> > can
> > > be a local variable, unless I'm missing something.
> > 
> > WebClient is assumed to be static in web::WebClient so we would need to
relax
> > that assumption first if I understand it correctly.
> > 
> >
>
https://cs.chromium.org/chromium/src/ios/web/public/web_client.h?rcl=4fad7069...
> 
> My proposal was this:
> + (void)initialize {
>   static std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
>   web::SetWebClient(&webClient);
> }
> 
> |webClient| will be constructed when |initialize| is called and destructed at
> application exit. webClient will remain the same so
> |web::SetWebClient(&webClient);| call will be idempotent.
> 
> But you can just move |static std::unique_ptr<ios_web_view::WebViewWebClient>
> webClient;| to initialize, unless I'm missing something.
> 
Please take a look at this comment

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:60: // Setting |product| is only guaranteed
to affect web views which have not yet
On 2017/04/12 00:46:17, Eugene But wrote:
> On 2017/04/11 21:57:03, michaeldo wrote:
> > On 2017/04/11 17:52:42, Eugene But wrote:
> > > Do you want to document how calling this API will actually affect final UA
> > > string?
> > 
> > I added a note that |product| is inserted into the UA string. I don't know
if
> > it's helpful to show a full example here as it will change and the
> documentation
> > will likely be out of date. WDYT?
> I think it is helpful to explain why clients would want to call this method
and
> what will happen if they don't call it (I assume that UA will look silly in
this
> case).
Please take a look at this comment.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:58: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate> webMainDelegate;
On 2017/04/12 17:16:48, michaeldo wrote:
> On 2017/04/12 00:46:17, Eugene But wrote:
> > if (self != [CWVWebViewConfiguration class])
> >   return;
> > 
> > initialize is called for subclasses (or super classes).
> 
> Done. But wouldn't we want this to still execute for subclasses?
For CWVConfiguration subclass this will be called multiple times.

[michaeldo, 2017-04-12 18:22:55]

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:32: static
std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
On 2017/04/12 17:48:27, Eugene But wrote:
> On 2017/04/12 00:46:17, Eugene But wrote:
> > On 2017/04/11 21:57:02, michaeldo wrote:
> > > On 2017/04/11 17:52:41, Eugene But wrote:
> > > > Is there a reason for making this global? It is used only in one method
> and
> > > can
> > > > be a local variable, unless I'm missing something.
> > > 
> > > WebClient is assumed to be static in web::WebClient so we would need to
> relax
> > > that assumption first if I understand it correctly.
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/ios/web/public/web_client.h?rcl=4fad7069...
> > 
> > My proposal was this:
> > + (void)initialize {
> >   static std::unique_ptr<ios_web_view::WebViewWebClient> webClient;
> >   web::SetWebClient(&webClient);
> > }
> > 
> > |webClient| will be constructed when |initialize| is called and destructed
at
> > application exit. webClient will remain the same so
> > |web::SetWebClient(&webClient);| call will be idempotent.
> > 
> > But you can just move |static
std::unique_ptr<ios_web_view::WebViewWebClient>
> > webClient;| to initialize, unless I'm missing something.
> > 
> Please take a look at this comment

Sorry, yes you're correct. I forgot to move this after we stopped needing
webClient for the getter.

Done.

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2791403005/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_web_view.h:60: // Setting |product| is only guaranteed
to affect web views which have not yet
On 2017/04/12 17:48:27, Eugene But wrote:
> On 2017/04/12 00:46:17, Eugene But wrote:
> > On 2017/04/11 21:57:03, michaeldo wrote:
> > > On 2017/04/11 17:52:42, Eugene But wrote:
> > > > Do you want to document how calling this API will actually affect final
UA
> > > > string?
> > > 
> > > I added a note that |product| is inserted into the UA string. I don't know
> if
> > > it's helpful to show a full example here as it will change and the
> > documentation
> > > will likely be out of date. WDYT?
> > I think it is helpful to explain why clients would want to call this method
> and
> > what will happen if they don't call it (I assume that UA will look silly in
> this
> > case).
> Please take a look at this comment.

I've added example UA.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view_configuration.mm (right):

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:58: static
std::unique_ptr<ios_web_view::WebViewWebMainDelegate> webMainDelegate;
On 2017/04/12 17:48:27, Eugene But wrote:
> On 2017/04/12 17:16:48, michaeldo wrote:
> > On 2017/04/12 00:46:17, Eugene But wrote:
> > > if (self != [CWVWebViewConfiguration class])
> > >   return;
> > > 
> > > initialize is called for subclasses (or super classes).
> > 
> > Done. But wouldn't we want this to still execute for subclasses?
> For CWVConfiguration subclass this will be called multiple times.

Ah, I see. Thanks.

https://codereview.chromium.org/2791403005/diff/80001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view_configuration.mm:80:
downloadManager->set_request_context(_browserState->GetRequestContext());
On 2017/04/12 17:16:48, michaeldo wrote:
> On 2017/04/12 00:46:17, Eugene But wrote:
> > Sorry, I missed this during the previous round. Looks like
> > translate::TranslateDownloadManager will have request context from the
latest
> > created browser state, which looks like a bug to me. Do you know if Chrome
has
> > the same problem with TranslateDownloadManager singleton?
> 
> Sorry I missed this too. Chrome actually has a global translatE_service_ios
that
> sets up the translate singleton with a global request context.
> (GetApplicationContext()->GetSystemURLRequestContext()). We should likely do
> something similar here.

I've created a bug for this.

[Eugene But (OOO till 7-30), 2017-04-12 18:26:17]

lgtm!

[michaeldo, 2017-04-14 23:22:45]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-04-14 23:22:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2791403005/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-14 23:23:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-15 01:45:16]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492212165723640,
"parent_rev": "c77104dfbdbdb4e4952dad12fa2d2352a07e65ff", "commit_rev":
"f21fa55c1594deac368f09c8847b451bea0c1bc2"}

[commit-bot: I haz the power, 2017-04-15 01:46:04]

Description was changed from

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287
==========

to

==========
Remove CWV class and move setting User Agent to CWVWebViewConfiguration.
Move Browser State and Web Client ownership to CWVWebViewConfiguration.
CWVWebViewConfiguration will now create it’s own browser state and will not
share a browser state between copies of the configuration.

BUG=690182, 703359, 706287

Review-Url: https://codereview.chromium.org/2791403005
Cr-Commit-Position: refs/heads/master@{#464848}
Committed:
https://chromium.googlesource.com/chromium/src/+/f21fa55c1594deac368f09c8847b...
==========

[commit-bot: I haz the power, 2017-04-15 01:46:05]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f21fa55c1594deac368f09c8847b...