Upstream ios/web_view source code.

Upstream ios/web_view source code.

ios/web_view will provide a view similar to WKWebView while additionally exposing Chromium features.

BUG=622967
Review-Url: https://codereview.chromium.org/2643773005
Cr-Commit-Position: refs/heads/master@{#446056}
Committed: https://chromium.googlesource.com/chromium/src/+/05089d2310a1aa7158a569fc6994672842fe6e44

[michaeldo, 2017-01-19 20:21:04]

michaeldo@chromium.org changed reviewers:
+ juliatuttle@chromium.org, rohitrao@chromium.org, sdefresne@chromium.org,
thakis@chromium.org

[michaeldo, 2017-01-19 20:21:05]

rohitrao@chromium.org for review

For DEPS approvals:
juliatuttle@chromium.org
thakis@chromium.org
sdefresne@chromium.org

[Julia Tuttle, 2017-01-19 22:28:24]

ios/web_view/internal/DEPS lgtm

[sdefresne, 2017-01-20 10:28:13]

Some of those comments may be addressed in a followup CL if you want to avoid
adding too much changes to what is just code upstreaming, but could you at least
please fix the CreateWebMainParts and also consider removing the
NSURLProtocol/RequestTracker initialisation code?

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv.mm
File ios/web_view/internal/criwv.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv.mm:86: _httpProtocolDelegate.reset(new
web::WebHTTPProtocolHandlerDelegate(
This is only required for UIWebView IIUC. Given that ios/web_view presents
itself as a better WKWebView, I assume it is based on it and thus NSURLProtocol
and RequestTracker are unnecessary. Can we remove them before committing to an
API? AFAICT, removing everything below in that method should not change the app
behaviour unless it uses UIWebView (which it should not be doing).

If you remove those lines, then don't forget to remove the corresponding
cleanup, ivar and DEPS exceptions.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_browser_state.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_browser_state.mm:26: const char
kPreferencesFilename[] = "Preferences";
nit: const char kPreferencesFilename[] = FILE_PATH_LITERAL("Preferences");

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_client.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_client.mm:21: web_main_parts_.reset(new
CRIWVWebMainParts(delegate_));
This is seriously incorrect as this will cause two std::unique_ptr<> to owns the
same object.

Look, this is how web calls CreateWebMainParts:

  void WebMainLoop::Init() {
    parts_.reset(web::GetWebClient()->CreateWebMainParts());
  }

So the value returned by this factory method will be owned by WebMainLoop. It
should not be owned by CRIWVWebClient otherwise it will lead to undefined
behaviour when the last of those two objects is detroyed (I guess this never
happen currently because there is no clean shutdown of the app).

BTW, this is why factory method should be defined to return std::unique_ptr<> to
make it clear what code owns the object. Please fix CRIWVWebClient to not own
the object and create a bug to fix CreateWebMainParts() API.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_delegate.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_delegate.mm:12:
CRIWVWebMainDelegate::CRIWVWebMainDelegate(id<CRIWVDelegate> delegate) {
nit:

CRIWVWebMainDelegate::CRIWVWebMainDelegate(id<CRIWVDelegate> delegate)
    : delegate_(delegate) {}

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_delegate.mm:19: web_client_.reset(new
CRIWVWebClient(delegate_));
nit: web_client_ = base::MakeUnique<CRIWVWebClient>(delegate_);

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_parts.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:27: "", NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
s/""/std::string()/

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:30: pak_file =
pak_file.Append("web_view_resources.pak");
nit: pak_file = pak_file.Append(FILE_PATH_LITERAL("web_view_resources.pak"));
or:  pak_file = pak_file.AppendASCII("web_view_resources.pak");

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:34: browser_state_.reset(new
CRIWVBrowserState);
nit: browser_state_ = base::MakeUnique<CRIWVBrowserState>();

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_view_impl.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.h:4: #ifndef
IOS_WEB_VIEW_INTERNAL_CRIWV_WEB_VIEW_IMPL_H_
Insert blank line.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.h:15: // Designated initializer.
Remove comment and instead add NS_DESIGNATED_INITIALIZER macro after the
initializer.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_view_impl.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:52: _webStateImpl.reset(new
web::WebStateImpl(_browserState));
nit: _webStateImpl = base::MakeUnique<web::WebStateImpl>(_browserState);

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:55: _webStateDelegate.reset(new
web::WebStateDelegateBridge(self));
nit: _webStateDelegate = base::MakeUnique<web::WebStateDelegateBridge>(self);

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:69: // TODO(crbug.com/679895): Stop
using the private CRWWebController API.
I think you can fix this TODO easily by changing this method to the following
implementation:

- (web::WebState*)webState {
  return )webStateImpl.get();
}

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:86: return [_webController
webStateImpl]->IsLoading();
ditto, here you should be able to just use _webStateImpl directly:

- (BOOL)isLoading {
  return _webStateImpl->IsLoading();
}

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:90: return net::NSURLWithGURL([self
webState]->GetVisibleURL());
nit: use "self.webState" instead of "[self webState]"

BTW, I think you should be consistent and use either "self.webState" everywhere
or use "_webStateImpl".

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:93: - (NSString*)pageTitle {
ditto, here you should be able to just use _webStateImpl directly:

- (NSString*)pageTitle {
  return _webStateImpl->GetTitle();
}

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:98: if (_webStateImpl)
Why check whether _webStateImpl can be null here and not in other methods (like
visibleURL for example). AFAICT from reading the code, _webStateImpl is owned by
self, is a private ivar so no sub-class should be able to change it, and is
never reset outside of the initializer, so this ivar *cannot* be null and the
check is useless.

I think you should remove it, or change it to a check that
_webStateImpl->GetNavigationManager() is not null (which would make more sense).

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:103: if (_webStateImpl)
ditto

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:123: [[self
webState]->GetJSInjectionReceiver()
s/[self webState]/self.webState/ or s/[self webState]/_webStateImpl/

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:131: // Set up the translate
delegate.
nit:, I would first check whether _delegate responds to -translateDelegate
selector, like this

  // Set up the translate delegate.
  if ([_delegate respondsToSelector:@selector(translateDelegate)]) {
    id<CRWVTranslateDelegate> translateDelegate = [_delegate translateDelegate];
    ios_web_view::CRIWVTranslateClient::FromWebState(_webStateImpl.get())
        ->set_translate_delegate(translateDelegate);
  }

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:133:
ios_web_view::CRIWVTranslateClient::FromWebState([self webState]);
ditto, self.webState

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_client.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.h:11: #import
"base/mac/scoped_nsobject.h"
Remove.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_client.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.mm:34: new
translate::TranslateManager(this, prefs::kAcceptLanguages)),
nit: base::MakeUnique<translate::TranslateManager>(this,
prefs::kAcceptLanguages)

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.mm:109: return
std::unique_ptr<translate::TranslatePrefs>(
nit:

  return base::MakeUnique<translate::TranslatePrefs>(
      GetPrefs(), prefs::kAcceptLanguages, nullptr);

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_manager_impl.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.h:10: #include
"base/memory/weak_ptr.h"
Remove?

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.h:24:
targetLanguage:(const std::string&)target;
NS_DESIGNATED_INITIALIZER

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_manager_impl.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.mm:11: @interface
CRIWVTranslateManagerImpl () {
Move to @implementation since your interface extension is empty:

@implementation CRIWVTranslateManagerImpl {
  std::unique_ptr<translate::TranslateUIDelegate> _translateUIDelegate;
}

- (instancetype)initWithTranslateManager:(translate::TranslateManager*)manager
...

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.mm:23:
_translateUIDelegate.reset(new translate::TranslateUIDelegate(
nit:

  _translateUIDelegate = base::MakeUnique<translate::TranslateUIDelegate>(
      manager->GetWeakPtr(), source, target);

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/public/criwv_w...
File ios/web_view/public/criwv_web_view.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/public/criwv_w...
ios/web_view/public/criwv_web_view.h:11: // Primary objective-c interface for
web/.  Just like a UIWebView, but better.
UIWebView -> WKWebView?

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/BUILD.gn
File ios/web_view/shell/BUILD.gn (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/BUILD.gn...
ios/web_view/shell/BUILD.gn:65:
"$root_gen_dir/components/components_resources.pak",
Are you sure you do not need ios/web or ui/base resources?

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/shell_ap...
File ios/web_view/shell/shell_app_delegate.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/shell_ap...
ios/web_view/shell/shell_app_delegate.mm:19: @synthesize window = _window;
_window is never deallocated and thus is leaked. Converting the file to ARC,
using a base::scoped_nsobject<> ivar, using a property releaser or implementing
-dealloc could fix this leak.

[michaeldo, 2017-01-20 18:43:30]

Thank you for all the comments! I've only addressed the minimal requested
comments in order to keep this CL (mostly) clean. Other issues will be addressed
separately.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv.mm
File ios/web_view/internal/criwv.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv.mm:86: _httpProtocolDelegate.reset(new
web::WebHTTPProtocolHandlerDelegate(
On 2017/01/20 10:28:12, sdefresne wrote:
> This is only required for UIWebView IIUC. Given that ios/web_view presents
> itself as a better WKWebView, I assume it is based on it and thus
NSURLProtocol
> and RequestTracker are unnecessary. Can we remove them before committing to an
> API? AFAICT, removing everything below in that method should not change the
app
> behaviour unless it uses UIWebView (which it should not be doing).
> 
> If you remove those lines, then don't forget to remove the corresponding
> cleanup, ivar and DEPS exceptions.

Done.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_client.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_client.mm:21: web_main_parts_.reset(new
CRIWVWebMainParts(delegate_));
On 2017/01/20 10:28:12, sdefresne wrote:
> This is seriously incorrect as this will cause two std::unique_ptr<> to owns
the
> same object.
> 
> Look, this is how web calls CreateWebMainParts:
> 
>   void WebMainLoop::Init() {
>     parts_.reset(web::GetWebClient()->CreateWebMainParts());
>   }
> 
> So the value returned by this factory method will be owned by WebMainLoop. It
> should not be owned by CRIWVWebClient otherwise it will lead to undefined
> behaviour when the last of those two objects is detroyed (I guess this never
> happen currently because there is no clean shutdown of the app).
> 
> BTW, this is why factory method should be defined to return std::unique_ptr<>
to
> make it clear what code owns the object. Please fix CRIWVWebClient to not own
> the object and create a bug to fix CreateWebMainParts() API.

Done. I've filed crbug.com/683207 to fix the return value.

[michaeldo, 2017-01-20 18:43:40]

Description was changed from

==========
Upstream ios/web_view source code.

ios/web_view will provide a view similar to WKWebView while additionally
exposing Chromium features.

BUG=none
==========

to

==========
Upstream ios/web_view source code.

ios/web_view will provide a view similar to WKWebView while additionally
exposing Chromium features.

BUG= 622967
==========

[rohitrao (ping after 24h), 2017-01-24 14:41:51]

RS LGTM for the overall move if sdefresne is happy with the code.

I continue to be concerned that ios_web_view_shell and ios_web_shell will be
easily confused =)

[sdefresne, 2017-01-24 14:54:30]

lgtm (please do a followup to address the other issues raised)

https://codereview.chromium.org/2643773005/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/criwv_web_client.h (right):

https://codereview.chromium.org/2643773005/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_client.h:39: CRIWVWebMainParts* web_main_parts_;
// weak
nit: remove "// weak" as it is (IMO) redundant. All raw pointers are weak in C++
(and this is a pointer to a C++ object), a strong pointer would use
std::unique_ptr<> or shared_ptr<>.

[michaeldo, 2017-01-24 17:34:01]

Nico, ptal at DEPS additions.

[michaeldo, 2017-01-24 22:35:26]

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_browser_state.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_browser_state.mm:26: const char
kPreferencesFilename[] = "Preferences";
On 2017/01/20 10:28:12, sdefresne wrote:
> nit: const char kPreferencesFilename[] = FILE_PATH_LITERAL("Preferences");

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_delegate.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_delegate.mm:12:
CRIWVWebMainDelegate::CRIWVWebMainDelegate(id<CRIWVDelegate> delegate) {
On 2017/01/20 10:28:12, sdefresne wrote:
> nit:
> 
> CRIWVWebMainDelegate::CRIWVWebMainDelegate(id<CRIWVDelegate> delegate)
>     : delegate_(delegate) {}

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_delegate.mm:19: web_client_.reset(new
CRIWVWebClient(delegate_));
On 2017/01/20 10:28:12, sdefresne wrote:
> nit: web_client_ = base::MakeUnique<CRIWVWebClient>(delegate_);

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_parts.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:27: "", NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
On 2017/01/20 10:28:12, sdefresne wrote:
> s/""/std::string()/

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:30: pak_file =
pak_file.Append("web_view_resources.pak");
On 2017/01/20 10:28:12, sdefresne wrote:
> nit: pak_file = pak_file.Append(FILE_PATH_LITERAL("web_view_resources.pak"));
> or:  pak_file = pak_file.AppendASCII("web_view_resources.pak");

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:34: browser_state_.reset(new
CRIWVBrowserState);
On 2017/01/20 10:28:12, sdefresne wrote:
> nit: browser_state_ = base::MakeUnique<CRIWVBrowserState>();

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_view_impl.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.h:4: #ifndef
IOS_WEB_VIEW_INTERNAL_CRIWV_WEB_VIEW_IMPL_H_
On 2017/01/20 10:28:12, sdefresne wrote:
> Insert blank line.

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.h:15: // Designated initializer.
On 2017/01/20 10:28:12, sdefresne wrote:
> Remove comment and instead add NS_DESIGNATED_INITIALIZER macro after the
> initializer.

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_view_impl.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:52: _webStateImpl.reset(new
web::WebStateImpl(_browserState));
On 2017/01/20 10:28:13, sdefresne wrote:
> nit: _webStateImpl = base::MakeUnique<web::WebStateImpl>(_browserState);

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:55: _webStateDelegate.reset(new
web::WebStateDelegateBridge(self));
On 2017/01/20 10:28:13, sdefresne wrote:
> nit: _webStateDelegate = base::MakeUnique<web::WebStateDelegateBridge>(self);

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:69: // TODO(crbug.com/679895): Stop
using the private CRWWebController API.
On 2017/01/20 10:28:13, sdefresne wrote:
> I think you can fix this TODO easily by changing this method to the following
> implementation:
> 
> - (web::WebState*)webState {
>   return )webStateImpl.get();
> }

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:86: return [_webController
webStateImpl]->IsLoading();
On 2017/01/20 10:28:13, sdefresne wrote:
> ditto, here you should be able to just use _webStateImpl directly:
> 
> - (BOOL)isLoading {
>   return _webStateImpl->IsLoading();
> }

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:90: return net::NSURLWithGURL([self
webState]->GetVisibleURL());
On 2017/01/20 10:28:13, sdefresne wrote:
> nit: use "self.webState" instead of "[self webState]"
> 
> BTW, I think you should be consistent and use either "self.webState"
everywhere
> or use "_webStateImpl".

Agreed, done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:93: - (NSString*)pageTitle {
On 2017/01/20 10:28:13, sdefresne wrote:
> ditto, here you should be able to just use _webStateImpl directly:
> 
> - (NSString*)pageTitle {
>   return _webStateImpl->GetTitle();
> }

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:98: if (_webStateImpl)
On 2017/01/20 10:28:13, sdefresne wrote:
> Why check whether _webStateImpl can be null here and not in other methods
(like
> visibleURL for example). AFAICT from reading the code, _webStateImpl is owned
by
> self, is a private ivar so no sub-class should be able to change it, and is
> never reset outside of the initializer, so this ivar *cannot* be null and the
> check is useless.
> 
> I think you should remove it, or change it to a check that
> _webStateImpl->GetNavigationManager() is not null (which would make more
sense).

In dep CL, I updated the check for the navigationManager. The check isn't useful
now, but is consistent with canGoBack/Forward and may be useful so I'll leave it
for now.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:103: if (_webStateImpl)
On 2017/01/20 10:28:13, sdefresne wrote:
> ditto

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:123: [[self
webState]->GetJSInjectionReceiver()
On 2017/01/20 10:28:12, sdefresne wrote:
> s/[self webState]/self.webState/ or s/[self webState]/_webStateImpl/

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:131: // Set up the translate
delegate.
On 2017/01/20 10:28:13, sdefresne wrote:
> nit:, I would first check whether _delegate responds to -translateDelegate
> selector, like this
> 
>   // Set up the translate delegate.
>   if ([_delegate respondsToSelector:@selector(translateDelegate)]) {
>     id<CRWVTranslateDelegate> translateDelegate = [_delegate
translateDelegate];
>     ios_web_view::CRIWVTranslateClient::FromWebState(_webStateImpl.get())
>         ->set_translate_delegate(translateDelegate);
>   }

I think the way it is now may be better since it will ensure the translate
delegate is set to nil in the case where translate is not implemented in the
delegate. By changing it, if the delegate is changed, the old delegate's
translateDelegate may still get calls.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:133:
ios_web_view::CRIWVTranslateClient::FromWebState([self webState]);
On 2017/01/20 10:28:13, sdefresne wrote:
> ditto, self.webState

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_client.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.h:11: #import
"base/mac/scoped_nsobject.h"
On 2017/01/20 10:28:13, sdefresne wrote:
> Remove.

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_client.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.mm:34: new
translate::TranslateManager(this, prefs::kAcceptLanguages)),
On 2017/01/20 10:28:13, sdefresne wrote:
> nit: base::MakeUnique<translate::TranslateManager>(this,
> prefs::kAcceptLanguages)

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_client.mm:109: return
std::unique_ptr<translate::TranslatePrefs>(
On 2017/01/20 10:28:13, sdefresne wrote:
> nit:
> 
>   return base::MakeUnique<translate::TranslatePrefs>(
>       GetPrefs(), prefs::kAcceptLanguages, nullptr);

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_manager_impl.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.h:10: #include
"base/memory/weak_ptr.h"
On 2017/01/20 10:28:13, sdefresne wrote:
> Remove?

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.h:24:
targetLanguage:(const std::string&)target;
On 2017/01/20 10:28:13, sdefresne wrote:
> NS_DESIGNATED_INITIALIZER

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/criwv_translate_manager_impl.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.mm:11: @interface
CRIWVTranslateManagerImpl () {
On 2017/01/20 10:28:13, sdefresne wrote:
> Move to @implementation since your interface extension is empty:
> 
> @implementation CRIWVTranslateManagerImpl {
>   std::unique_ptr<translate::TranslateUIDelegate> _translateUIDelegate;
> }
> 
> - (instancetype)initWithTranslateManager:(translate::TranslateManager*)manager
> ...

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/criwv_translate_manager_impl.mm:23:
_translateUIDelegate.reset(new translate::TranslateUIDelegate(
On 2017/01/20 10:28:13, sdefresne wrote:
> nit:
> 
>   _translateUIDelegate = base::MakeUnique<translate::TranslateUIDelegate>(
>       manager->GetWeakPtr(), source, target);

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/public/criwv_w...
File ios/web_view/public/criwv_web_view.h (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/public/criwv_w...
ios/web_view/public/criwv_web_view.h:11: // Primary objective-c interface for
web/.  Just like a UIWebView, but better.
On 2017/01/20 10:28:13, sdefresne wrote:
> UIWebView -> WKWebView?

Done in dep CL.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/BUILD.gn
File ios/web_view/shell/BUILD.gn (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/BUILD.gn...
ios/web_view/shell/BUILD.gn:65:
"$root_gen_dir/components/components_resources.pak",
On 2017/01/20 10:28:13, sdefresne wrote:
> Are you sure you do not need ios/web or ui/base resources?

Good question, the js resources will certainly be required, I will investigate
further to determine if they are missing.

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/shell_ap...
File ios/web_view/shell/shell_app_delegate.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/shell/shell_ap...
ios/web_view/shell/shell_app_delegate.mm:19: @synthesize window = _window;
On 2017/01/20 10:28:13, sdefresne wrote:
> _window is never deallocated and thus is leaked. Converting the file to ARC,
> using a base::scoped_nsobject<> ivar, using a property releaser or
implementing
> -dealloc could fix this leak.

In dep CL, I added dealloc. Conversion to arc will likely happen, but will be a
separate effort.

https://codereview.chromium.org/2643773005/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/criwv_web_client.h (right):

https://codereview.chromium.org/2643773005/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_client.h:39: CRIWVWebMainParts* web_main_parts_;
// weak
On 2017/01/24 14:54:30, sdefresne wrote:
> nit: remove "// weak" as it is (IMO) redundant. All raw pointers are weak in
C++
> (and this is a pointer to a C++ object), a strong pointer would use
> std::unique_ptr<> or shared_ptr<>.

Done in dep CL.

[sdefresne, 2017-01-25 09:13:27]

still lgtm

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_view_impl.mm (right):

https://codereview.chromium.org/2643773005/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_view_impl.mm:131: // Set up the translate
delegate.
On 2017/01/24 22:35:26, michaeldo wrote:
> On 2017/01/20 10:28:13, sdefresne wrote:
> > nit:, I would first check whether _delegate responds to -translateDelegate
> > selector, like this
> > 
> >   // Set up the translate delegate.
> >   if ([_delegate respondsToSelector:@selector(translateDelegate)]) {
> >     id<CRWVTranslateDelegate> translateDelegate = [_delegate
> translateDelegate];
> >     ios_web_view::CRIWVTranslateClient::FromWebState(_webStateImpl.get())
> >         ->set_translate_delegate(translateDelegate);
> >   }
> 
> I think the way it is now may be better since it will ensure the translate
> delegate is set to nil in the case where translate is not implemented in the
> delegate. By changing it, if the delegate is changed, the old delegate's
> translateDelegate may still get calls.

Acknowdleged. I didn't think of that edge case. Thank you.

[Nico, 2017-01-25 15:39:22]

DEPS lgtm

[michaeldo, 2017-01-25 16:52:02]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-01-25 16:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
juliatuttle@chromium.org
Link to the patchset: https://codereview.chromium.org/2643773005/#ps20001
(title: "Remove request tracker and fix pointer to CRIWVWebMainParts.")

[commit-bot: I haz the power, 2017-01-25 16:52:17]

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-01-25 18:00:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485363122589820,
"parent_rev": "cb0c494fbf164dd200674ef41e7d203933b22c80", "commit_rev":
"05089d2310a1aa7158a569fc6994672842fe6e44"}

[commit-bot: I haz the power, 2017-01-25 18:01:22]

Description was changed from

==========
Upstream ios/web_view source code.

ios/web_view will provide a view similar to WKWebView while additionally
exposing Chromium features.

BUG= 622967
==========

to

==========
Upstream ios/web_view source code.

ios/web_view will provide a view similar to WKWebView while additionally
exposing Chromium features.

BUG= 622967

Review-Url: https://codereview.chromium.org/2643773005
Cr-Commit-Position: refs/heads/master@{#446056}
Committed:
https://chromium.googlesource.com/chromium/src/+/05089d2310a1aa7158a569fc6994...
==========

[commit-bot: I haz the power, 2017-01-25 18:01:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/05089d2310a1aa7158a569fc6994...