Upstream web_controller_provider and dependencies

Upstream web_controller_provider.{h,mm} and block_types.h

WebControllerProvider provides URL-loading and JavaScript injection with
optional dialog suppression.

BUG=459678, 429756
Committed: https://crrev.com/9a5cfc9389557f2e27dcd307f25690cb1c407d7d
Cr-Commit-Position: refs/heads/master@{#317041}

[sdefresne, 2015-02-18 18:54:53]

sdefresne@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[sdefresne, 2015-02-18 18:54:54]

Please take a look.

[sdefresne, 2015-02-19 09:32:56]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org
- stuartmorgan@chromium.org

[sdefresne, 2015-02-19 09:33:27]

-stuartmorgan, +droger: using a local reviewer for quicker review cycle

[droger, 2015-02-19 10:51:04]

CC stuartmorgan: Can you look at the comment about web_view_completion_handler
below?

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
File ios/public/provider/web/web_controller_provider.h (right):

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:9: #include
"ios/web/public/web_state/web_state_observer.h"
Does this pass DEPS checks? If not, where is the DEPS rule that allows this?

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:10: #include
"ios/web/web_state/ui/web_view_js_utils.h"
This include seems needed because of web::JavaScriptCompletion.
Could we move web::JavaScriptCompletion in public/?

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:35:
WebControllerProvider(web::BrowserState* browser_state);
explicit

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:45: // Retyrns the WebState
associated with this web controller.
s/Retyrns/Returns/

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
File ios/web/web_state/ui/web_view_js_utils.mm (right):

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:56: id web_view_completion_handler =
nil;
Why id?

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:68: [web_view_completion_handler
autorelease];
This looks like a bug.
I think that the block captures web_view_completion_handler by value (since the
variable is not __block), and thus it is nil (as defined on line 56 above).

I am not sure what the intent was, but maybe the variable should be declared as
__block.

stuartmorgan: can you comment on this?

[sdefresne, 2015-02-19 13:39:08]

PTAL

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
File ios/public/provider/web/web_controller_provider.h (right):

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:9: #include
"ios/web/public/web_state/web_state_observer.h"
On 2015/02/19 10:51:04, droger wrote:
> Does this pass DEPS checks? If not, where is the DEPS rule that allows this?

I'll cleanup DEPS in another CL. It looks like everything that is not forbidden
is allowed which looks wrong.

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:10: #include
"ios/web/web_state/ui/web_view_js_utils.h"
On 2015/02/19 at 10:51:03, droger wrote:
> This include seems needed because of web::JavaScriptCompletion.
> Could we move web::JavaScriptCompletion in public/?

Done.

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:35:
WebControllerProvider(web::BrowserState* browser_state);
On 2015/02/19 at 10:51:03, droger wrote:
> explicit

Done.

https://codereview.chromium.org/914993006/diff/40001/ios/public/provider/web/...
ios/public/provider/web/web_controller_provider.h:45: // Retyrns the WebState
associated with this web controller.
On 2015/02/19 at 10:51:03, droger wrote:
> s/Retyrns/Returns/

Done.

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
File ios/web/web_state/ui/web_view_js_utils.mm (right):

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:56: id web_view_completion_handler =
nil;
On 2015/02/19 at 10:51:04, droger wrote:
> Why id?

Good catch.

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:68: [web_view_completion_handler
autorelease];
On 2015/02/19 at 10:51:04, droger wrote:
> This looks like a bug.
> I think that the block captures web_view_completion_handler by value (since
the variable is not __block), and thus it is nil (as defined on line 56 above).
> 
> I am not sure what the intent was, but maybe the variable should be declared
as __block.
> 
> stuartmorgan: can you comment on this?

I no longer require this file, so the question applies to the downstream version
of the file.

[droger, 2015-02-19 13:49:35]

lgtm

[sdefresne, 2015-02-19 13:52:00]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-02-19 13:52:01]

lgtm

[commit-bot: I haz the power, 2015-02-19 13:52:18]

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

[commit-bot: I haz the power, 2015-02-19 14:10:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-19 14:10:57]

Patchset 5 (id:??) landed as
https://crrev.com/9a5cfc9389557f2e27dcd307f25690cb1c407d7d
Cr-Commit-Position: refs/heads/master@{#317041}

[stuartmorgan, 2015-02-19 14:23:54]

CCing Eugene for the block autorelease comment.

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
File ios/web/web_state/ui/web_view_js_utils.mm (right):

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:68: [web_view_completion_handler
autorelease];
On 2015/02/19 10:51:04, droger wrote:
> This looks like a bug.
> I think that the block captures web_view_completion_handler by value (since
the
> variable is not __block), and thus it is nil (as defined on line 56 above).
> 
> I am not sure what the intent was, but maybe the variable should be declared
as
> __block.
> 
> stuartmorgan: can you comment on this?

Yep, this does look worrying. I'll investigate.

[Eugene But (OOO till 7-30), 2015-02-23 03:20:11]

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

[Eugene But (OOO till 7-30), 2015-02-23 03:20:12]

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
File ios/web/web_state/ui/web_view_js_utils.mm (right):

https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
ios/web/web_state/ui/web_view_js_utils.mm:68: [web_view_completion_handler
autorelease];
On 2015/02/19 14:23:54, stuartmorgan wrote:
> On 2015/02/19 10:51:04, droger wrote:
> > This looks like a bug.
> > I think that the block captures web_view_completion_handler by value (since
> the
> > variable is not __block), and thus it is nil (as defined on line 56 above).
> > 
> > I am not sure what the intent was, but maybe the variable should be declared
> as
> > __block.
> > 
> > stuartmorgan: can you comment on this?
> 
> Yep, this does look worrying. I'll investigate.
What exactly looks wrong to you? Functionality or memory management? AFAIK
__block keyword is required only if variable is changed inside the block.

[sdefresne, 2015-02-23 08:57:41]

On 2015/02/23 at 03:20:12, eugenebut wrote:
>
https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
> File ios/web/web_state/ui/web_view_js_utils.mm (right):
> 
>
https://codereview.chromium.org/914993006/diff/40001/ios/web/web_state/ui/web...
> ios/web/web_state/ui/web_view_js_utils.mm:68: [web_view_completion_handler
autorelease];
> On 2015/02/19 14:23:54, stuartmorgan wrote:
> > On 2015/02/19 10:51:04, droger wrote:
> > > This looks like a bug.
> > > I think that the block captures web_view_completion_handler by value
(since
> > the
> > > variable is not __block), and thus it is nil (as defined on line 56
above).
> > > 
> > > I am not sure what the intent was, but maybe the variable should be
declared
> > as
> > > __block.
> > > 
> > > stuartmorgan: can you comment on this?
> > 
> > Yep, this does look worrying. I'll investigate.
> What exactly looks wrong to you? Functionality or memory management? AFAIK
__block keyword is required only if variable is changed inside the block.

Without __block, the variable is captured by copy in the block. Since the block
is defined before the variable is assigned, the block capture a value of nil.
The current code looks equivalent to:

id web_completion_handler = nil;
id another_block_variable = ^(NSString*, NSError*) {
  [web_completion_handler release];
};
web_completion_handler = [another_block_variable copy];