WebUI for WKWebView

WebUI for WKWebView

Implements WebUI for WKWebView by using URLFetcher to manually request
and retrieve WebUI resources and subresources, and load the page via
loadHTMLString. Sets favicons for history page using JavaScript.

BUG=450362
Committed: https://crrev.com/3d8ced8a28bccc7d2f0a62e5a3df0173a1f4b713
Cr-Commit-Position: refs/heads/master@{#329720}

[Jackie Quinn, 2015-05-12 17:16:04]

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

[Eugene But (OOO till 7-30), 2015-05-12 17:47:10]

https://codereview.chromium.org/1137143004/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:179: virtual void
ExecuteJavaScriptAsync(const base::string16& javascript);
Optional NIT:

Since you touching this line please: s/javascript/java_script or
s/javascript/script

No need to change implementation file though.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:40: // Handles JavaScript message from the
WebUI page.
Please expand the comments, describing what this method does.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:102: if ([weakSelf webState]) {
Compiler may show a warning for this code, because more than one method is
called on weakSelf. This is because "[weakSelf webState]" can potentially
deallocate self, though it will never happen in practice.

Weak-Strong dance is definitely an overkill here, so please extract "[weakSelf
webState]" call into a separate local variable.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:115: -
(void)webUIPageBuilder:(CRWWebUIPageBuilder*)webUIPage
NIT: s/webUIPage/webUIPageBuilder

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
File ios/web/webui/resources/web_ui.js (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:11: window['chrome'] = {};
Is "if (!window['chrome']) check necessary here (to make sure that existing
chrome object is not overridden)?

if so, please use the following approach:
window['chrome'] = window['chrome'] || {};

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:21: var m = this;
m is not very informative variable name.

s/m/self or s/m/that

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:37: * Send message requesting favicon at the
URL from imageSet for el. Sets
s/Send/Sends

[Jackie Quinn, 2015-05-12 18:14:55]

https://codereview.chromium.org/1137143004/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:179: virtual void
ExecuteJavaScriptAsync(const base::string16& javascript);
On 2015/05/12 17:47:09, eugenebut wrote:
> Optional NIT:
> 
> Since you touching this line please: s/javascript/java_script or
> s/javascript/script
> 
> No need to change implementation file though.

Done.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:40: // Handles JavaScript message from the
WebUI page.
On 2015/05/12 17:47:09, eugenebut wrote:
> Please expand the comments, describing what this method does.

I was asked by Stuart to make the method name and comment generic, which is why
I removed reference to requestFavicon.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:102: if ([weakSelf webState]) {
On 2015/05/12 17:47:09, eugenebut wrote:
> Compiler may show a warning for this code, because more than one method is
> called on weakSelf. This is because "[weakSelf webState]" can potentially
> deallocate self, though it will never happen in practice.
> 
> Weak-Strong dance is definitely an overkill here, so please extract "[weakSelf
> webState]" call into a separate local variable.

Done.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/crw_web_ui_ma...
ios/web/webui/crw_web_ui_manager.mm:115: -
(void)webUIPageBuilder:(CRWWebUIPageBuilder*)webUIPage
On 2015/05/12 17:47:09, eugenebut wrote:
> NIT: s/webUIPage/webUIPageBuilder

Done.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
File ios/web/webui/resources/web_ui.js (right):

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:11: window['chrome'] = {};
On 2015/05/12 17:47:09, eugenebut wrote:
> Is "if (!window['chrome']) check necessary here (to make sure that existing
> chrome object is not overridden)?
> 
> if so, please use the following approach:
> window['chrome'] = window['chrome'] || {};

I think this may be the only file that defines window.chrome, in which case we
don't have to worry about it (this was how it was in the original file). I don't
think it hurts to use window['chrome'] = window['chrome'] || {} though, so I'll
make that change.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:21: var m = this;
On 2015/05/12 17:47:10, eugenebut wrote:
> m is not very informative variable name.
> 
> s/m/self or s/m/that

Done.

https://codereview.chromium.org/1137143004/diff/1/ios/web/webui/resources/web...
ios/web/webui/resources/web_ui.js:37: * Send message requesting favicon at the
URL from imageSet for el. Sets
On 2015/05/12 17:47:10, eugenebut wrote:
> s/Send/Sends

Done.

[Eugene But (OOO till 7-30), 2015-05-12 18:26:36]

lgtm with a change

https://codereview.chromium.org/1137143004/diff/40001/ios/web/webui/crw_web_u...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/1137143004/diff/40001/ios/web/webui/crw_web_u...
ios/web/webui/crw_web_ui_manager.mm:102: scoped_ptr<web::WebState>
webState([weakSelf webState]);
web::WebState* webState = [weakSelf webState];

Wrapping webState into scoped_ptr will make webState a dangling pointer.

[Jackie Quinn, 2015-05-13 21:05:29]

https://codereview.chromium.org/1137143004/diff/40001/ios/web/webui/crw_web_u...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/1137143004/diff/40001/ios/web/webui/crw_web_u...
ios/web/webui/crw_web_ui_manager.mm:102: scoped_ptr<web::WebState>
webState([weakSelf webState]);
On 2015/05/12 18:26:36, eugenebut wrote:
> web::WebState* webState = [weakSelf webState];
> 
> Wrapping webState into scoped_ptr will make webState a dangling pointer.

Done!

[Jackie Quinn, 2015-05-13 21:06:08]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2015-05-13 21:06:09]

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/1137143004/#ps60001
(title: "Fix unittest issues")

[commit-bot: I haz the power, 2015-05-13 21:06:54]

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

[commit-bot: I haz the power, 2015-05-13 21:59:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-13 22:01:10]

Patchset 4 (id:??) landed as
https://crrev.com/3d8ced8a28bccc7d2f0a62e5a3df0173a1f4b713
Cr-Commit-Position: refs/heads/master@{#329720}