Do not create CRWWebUIManager for non-WebUI URLs.

Do not create CRWWebUIManager for non-WebUI URLs.

This will prevent web controller from loading WebUI for chrome:// URLs
that neither have WebUI nor NativeContent.

CL should fix the issue with loading WebUI for chrome://crash and
chrome://newtab (New Architecture only).

There are no new tests because this case is covered by various existing
tests, so simple |DCHECK(HTML.length);| in |loadHTML:forURL:| is
sufficient.

BUG=None

Review-Url: https://codereview.chromium.org/2888773003
Cr-Commit-Position: refs/heads/master@{#472547}
Committed: https://chromium.googlesource.com/chromium/src/+/b3bc86ce3f500cb9db8e4eb01af089cf0654bf2b

[Eugene But (OOO till 7-30), 2017-05-16 23:45:18]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 23:46:32]

Dry run: CQ is trying da patch.

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

[PL, 2017-05-17 00:04:26]

https://codereview.chromium.org/2888773003/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2888773003/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3165:
_webStateImpl->CreateWebUI(URL);
For chrome://crash we would still run CreateWebUI even though we weren't
creating a web UI, is that a problem?

[commit-bot: I haz the power, 2017-05-17 01:10:13]

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

[commit-bot: I haz the power, 2017-05-17 01:10:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Eugene But (OOO till 7-30), 2017-05-17 14:49:26]

https://codereview.chromium.org/2888773003/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2888773003/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3165:
_webStateImpl->CreateWebUI(URL);
On 2017/05/17 00:04:26, PL wrote:
> For chrome://crash we would still run CreateWebUI even though we weren't
> creating a web UI, is that a problem?
That should not be a problem. Added explanation comment to the code:
"|CreateWebUI| will do nothing if |URL| is not a WebUI URL.".

[Eugene But (OOO till 7-30), 2017-05-17 14:49:36]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 14:50:00]

Dry run: CQ is trying da patch.

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

[Eugene But (OOO till 7-30), 2017-05-17 14:51:49]

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

[Eugene But (OOO till 7-30), 2017-05-17 14:51:50]

michaeldo@ for WebUI owner review

peterlaurens@ FYI, change should fix chrome://crash issue
sczs@ FYI, change should fix DCHECK with chrome://newtab for New Architecture

[commit-bot: I haz the power, 2017-05-17 15:03:04]

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

[commit-bot: I haz the power, 2017-05-17 15:03:05]

Dry run: This issue passed the CQ dry run.

[michaeldo, 2017-05-17 15:17:31]

lgtm

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3165: // |CreateWebUI| will do
nothing if |URL| is not a WebUI URL.
Can you add to this something like "but must be called before checking
HasWebUI()" below"? I was going to suggest that CreateWebUI be moved into the if
statement, but I believe it must be called before HasWebUI will return the
correct value and it isn't intuitive.

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4125: DCHECK(HTML.length);
Should we add a note to the comment for this method that |HTML| must not be
empty?

[Eugene But (OOO till 7-30), 2017-05-17 18:36:41]

Thanks!

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3165: // |CreateWebUI| will do
nothing if |URL| is not a WebUI URL.
On 2017/05/17 15:17:31, michaeldo wrote:
> Can you add to this something like "but must be called before checking
> HasWebUI()" below"? I was going to suggest that CreateWebUI be moved into the
if
> statement, but I believe it must be called before HasWebUI will return the
> correct value and it isn't intuitive.
Updated comment.

https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4125: DCHECK(HTML.length);
On 2017/05/17 15:17:31, michaeldo wrote:
> Should we add a note to the comment for this method that |HTML| must not be
> empty?

Done.

[michaeldo, 2017-05-17 18:43:46]

On 2017/05/17 18:36:41, Eugene But wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
> File ios/web/web_state/ui/crw_web_controller.mm (right):
> 
>
https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
> ios/web/web_state/ui/crw_web_controller.mm:3165: // |CreateWebUI| will do
> nothing if |URL| is not a WebUI URL.
> On 2017/05/17 15:17:31, michaeldo wrote:
> > Can you add to this something like "but must be called before checking
> > HasWebUI()" below"? I was going to suggest that CreateWebUI be moved into
the
> if
> > statement, but I believe it must be called before HasWebUI will return the
> > correct value and it isn't intuitive.
> Updated comment.
> 
>
https://codereview.chromium.org/2888773003/diff/40001/ios/web/web_state/ui/cr...
> ios/web/web_state/ui/crw_web_controller.mm:4125: DCHECK(HTML.length);
> On 2017/05/17 15:17:31, michaeldo wrote:
> > Should we add a note to the comment for this method that |HTML| must not be
> > empty?
> 
> Done.

Thank you!

[Eugene But (OOO till 7-30), 2017-05-17 20:04:40]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-05-17 20:04:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeldo@chromium.org
Link to the patchset: https://codereview.chromium.org/2888773003/#ps60001
(title: "Updated comments")

[commit-bot: I haz the power, 2017-05-17 20:05:56]

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-05-17 20:23:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495051480380660,
"parent_rev": "a54497f5b8d234513196f991d3748afc6755a2e7", "commit_rev":
"b3bc86ce3f500cb9db8e4eb01af089cf0654bf2b"}

[commit-bot: I haz the power, 2017-05-17 20:23:30]

Description was changed from

==========
Do not create CRWWebUIManager for non-WebUI URLs.

This will prevent web controller from loading WebUI for chrome:// URLs
that neither have WebUI nor NativeContent.

CL should fix the issue with loading WebUI for chrome://crash and
chrome://newtab (New Architecture only).

There are no new tests because this case is covered by various existing
tests, so simple |DCHECK(HTML.length);| in |loadHTML:forURL:| is
sufficient.

BUG=None
==========

to

==========
Do not create CRWWebUIManager for non-WebUI URLs.

This will prevent web controller from loading WebUI for chrome:// URLs
that neither have WebUI nor NativeContent.

CL should fix the issue with loading WebUI for chrome://crash and
chrome://newtab (New Architecture only).

There are no new tests because this case is covered by various existing
tests, so simple |DCHECK(HTML.length);| in |loadHTML:forURL:| is
sufficient.

BUG=None

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

[commit-bot: I haz the power, 2017-05-17 20:23:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b3bc86ce3f500cb9db8e4eb01af0...