[ios] Pass more --jscomp_error switches to the closure Compiler.

[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.

BUG=487804
Committed: https://crrev.com/cda1c632a2cbe11af48a9ef5bb7b7e568934611f
Cr-Commit-Position: refs/heads/master@{#429344}

[Eugene But (OOO till 7-30), 2016-10-26 02:19:59]

Description was changed from

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

BUG=487804
==========

to

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.
 
BUG=487804
==========

[Eugene But (OOO till 7-30), 2016-10-26 02:20:18]

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

[Eugene But (OOO till 7-30), 2016-10-26 02:20:21]

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

[commit-bot: I haz the power, 2016-10-26 02:20:36]

Dry run: 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, 2016-10-26 02:23:27]

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

[commit-bot: I haz the power, 2016-10-26 02:23:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-10-26 17:05:24]

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
File components/autofill/ios/browser/resources/autofill_controller.js (right):

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
components/autofill/ios/browser/resources/autofill_controller.js:59:
__gCrWeb['autofill'] = __gCrWeb.autofill;
What does this line do? Seems unnecessary at first glance (sets the |autofill|
property to itself?)

Same question for similar pattern in other files in this CL.

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/message.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/message.js:38: * @public
No need for @public. I don't even think such an annotation exists, but
regardless, no @private/@protected implies public.

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/post_request.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/post_request.js:79:
window.webkit.messageHandlers['POSTSuccessHandler'].postMessage("");
Can you add externs definitions for those? Then you don't need to bypass the
compiler by using quoted notation.

[Eugene But (OOO till 7-30), 2016-10-26 17:54:45]

Thanks for quick review!

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
File components/autofill/ios/browser/resources/autofill_controller.js (right):

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
components/autofill/ios/browser/resources/autofill_controller.js:59:
__gCrWeb['autofill'] = __gCrWeb.autofill;
On 2016/10/26 17:05:24, dpapad wrote:
> What does this line do? Seems unnecessary at first glance (sets the |autofill|
> property to itself?)
> 
> Same question for similar pattern in other files in this CL.
Stores autofill namespace object in a global __gCrWeb referenced by a string, so
it does not get renamed by closure compiler during the minification. Previous
line is necessary to tell Closure that __gCrWeb.autofill property actually
exists. Added comments.

dbeam@ suggested to follow | window['displayNoteTitle'] = displayNoteTitle;|
approach from this doc:
https://developers.google.com/closure/compiler/docs/api-tutorial3#export
hopefully I understood it correctly :)

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/message.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/message.js:38: * @public
On 2016/10/26 17:05:24, dpapad wrote:
> No need for @public. I don't even think such an annotation exists, but
> regardless, no @private/@protected implies public.

Done.

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/post_request.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/post_request.js:79:
window.webkit.messageHandlers['POSTSuccessHandler'].postMessage("");
On 2016/10/26 17:05:24, dpapad wrote:
> Can you add externs definitions for those? Then you don't need to bypass the
> compiler by using quoted notation.
I would prefer not to. messageHandlers is an object which is extended in native
code and using dot syntax for it does not feel right in the first place. If
someone adds a new handler they should not be forces to change the code in 3
places (one JS file, Native code, externs file).

[Eugene But (OOO till 7-30), 2016-10-26 18:03:18]

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

[commit-bot: I haz the power, 2016-10-26 18:03:52]

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

[dpapad, 2016-10-26 18:18:35]

LGTM

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
File components/autofill/ios/browser/resources/autofill_controller.js (right):

https://codereview.chromium.org/2449333002/diff/1/components/autofill/ios/bro...
components/autofill/ios/browser/resources/autofill_controller.js:59:
__gCrWeb['autofill'] = __gCrWeb.autofill;
> dbeam@ suggested to follow | window['displayNoteTitle'] = displayNoteTitle;|
approach from this doc:
> https://developers.google.com/closure/compiler/docs/api-tutorial3#export
> hopefully I understood it correctly :)

Ah ok, it makes sense now. I am used to seeing goog.exportSymbol() calls to do
that, but you are not using Closure library here, just Closure compiler, so this
is OK.

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/post_request.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/post_request.js:79:
window.webkit.messageHandlers['POSTSuccessHandler'].postMessage("");
On 2016/10/26 at 17:54:45, Eugene But wrote:
> On 2016/10/26 17:05:24, dpapad wrote:
> > Can you add externs definitions for those? Then you don't need to bypass the
> > compiler by using quoted notation.
> I would prefer not to. messageHandlers is an object which is extended in
native code and using dot syntax for it does not feel right in the first place.
If someone adds a new handler they should not be forces to change the code in 3
places (one JS file, Native code, externs file).

It is a tradeoff between inconvenience and better type coverage VS more
flexibility and less type coverage. If programmer makes a typo let's say,
window.webkit.messageHandlers['POSTSucessHandler'].postMessage(), you would get
a runtime error, instead of a compile time error.

Also note that when Closure compiler does not know the type of an object, it is
extremely lenient. The following would not be an error

window.webkit.messageHandlers['POSTSuccessHandler'].foo();
as long as there is any type known to Closure compiler that has a 'foo' method.

[commit-bot: I haz the power, 2016-10-26 19:27:55]

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

[commit-bot: I haz the power, 2016-10-26 19:27:56]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2016-10-26 20:30:10]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2016-10-26 20:31:09]

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, 2016-10-26 20:41:15]

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

[commit-bot: I haz the power, 2016-10-26 20:41:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eugene But (OOO till 7-30), 2016-10-26 20:58:52]

eugenebut@chromium.org changed reviewers:
+ jdonnelly@chromium.org, sdefresne@chromium.org

[Eugene But (OOO till 7-30), 2016-10-26 20:58:53]

jdonnelly for autofill

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/post_request.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/post_request.js:79:
window.webkit.messageHandlers['POSTSuccessHandler'].postMessage("");
On 2016/10/26 18:18:35, dpapad wrote:
> On 2016/10/26 at 17:54:45, Eugene But wrote:
> > On 2016/10/26 17:05:24, dpapad wrote:
> > > Can you add externs definitions for those? Then you don't need to bypass
the
> > > compiler by using quoted notation.
> > I would prefer not to. messageHandlers is an object which is extended in
> native code and using dot syntax for it does not feel right in the first
place.
> If someone adds a new handler they should not be forces to change the code in
3
> places (one JS file, Native code, externs file).
> 
> It is a tradeoff between inconvenience and better type coverage VS more
> flexibility and less type coverage. If programmer makes a typo let's say,
> window.webkit.messageHandlers['POSTSucessHandler'].postMessage(), you would
get
> a runtime error, instead of a compile time error.
> 
> Also note that when Closure compiler does not know the type of an object, it
is
> extremely lenient. The following would not be an error
> 
> window.webkit.messageHandlers['POSTSuccessHandler'].foo();
> as long as there is any type known to Closure compiler that has a 'foo'
method.

Acknowledged.

[Eugene But (OOO till 7-30), 2016-10-26 20:59:24]

jdonnelly for autofill
sdefresne for translate

[Justin Donnelly, 2016-10-26 21:06:52]

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/message.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/message.js:38: * @public
On 2016/10/26 17:05:24, dpapad wrote:
> No need for @public. I don't even think such an annotation exists, but
> regardless, no @private/@protected implies public.

If you prefer not to use it for stylistic reasons, that's fine. But @public
exists
(https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the...)
and seems to be in wide use
(https://cs.corp.google.com/search/?q=@public+file:%5C.js). One person's noise
is another person's explicitness and clarity. :)

[Eugene But (OOO till 7-30), 2016-10-26 21:16:24]

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/message.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/message.js:38: * @public
On 2016/10/26 21:06:52, Justin Donnelly wrote:
> On 2016/10/26 17:05:24, dpapad wrote:
> > No need for @public. I don't even think such an annotation exists, but
> > regardless, no @private/@protected implies public.
> 
> If you prefer not to use it for stylistic reasons, that's fine. But @public
> exists
>
(https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the...)
> and seems to be in wide use
> (https://cs.corp.google.com/search/?q=@public+file:%5C.js). One person's noise
> is another person's explicitness and clarity. :)

Thanks! I would prefer to use @public if it's a real thing. Demetrios, what do
you think?

[Eugene But (OOO till 7-30), 2016-10-27 23:23:21]

PTAL

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/message.js (right):

https://codereview.chromium.org/2449333002/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/message.js:38: * @public
On 2016/10/26 21:16:23, Eugene But wrote:
> On 2016/10/26 21:06:52, Justin Donnelly wrote:
> > On 2016/10/26 17:05:24, dpapad wrote:
> > > No need for @public. I don't even think such an annotation exists, but
> > > regardless, no @private/@protected implies public.
> > 
> > If you prefer not to use it for stylistic reasons, that's fine. But @public
> > exists
> >
>
(https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the...)
> > and seems to be in wide use
> > (https://cs.corp.google.com/search/?q=@public+file:%5C.js). One person's
noise
> > is another person's explicitness and clarity. :)
> 
> Thanks! I would prefer to use @public if it's a real thing. Demetrios, what do
> you think?
Added @public back.

[Eugene But (OOO till 7-30), 2016-11-01 21:59:09]

Justin?

[Justin Donnelly, 2016-11-01 22:15:28]

lgtm

Sorry, I thought you were waiting on dpapad. Looks good!

[Eugene But (OOO till 7-30), 2016-11-01 22:19:00]

On 2016/11/01 22:15:28, Justin Donnelly wrote:
> lgtm
> 
> Sorry, I thought you were waiting on dpapad. Looks good!
I just added @public back :) Demetrios, please let me know if you think it's
incorrect.

[dpapad, 2016-11-01 22:34:48]

On 2016/11/01 at 22:19:00, eugenebut wrote:
> On 2016/11/01 22:15:28, Justin Donnelly wrote:
> > lgtm
> > 
> > Sorry, I thought you were waiting on dpapad. Looks good!
> I just added @public back :) Demetrios, please let me know if you think it's
incorrect.

LGTM
@public is fine. Personally I've never seen it being used, but it is listed in
the docs at
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the...,
with the comment

"This is the implicit default and rarely used"

[Justin Donnelly, 2016-11-02 13:28:33]

On 2016/11/01 22:34:48, dpapad wrote:
> "This is the implicit default and rarely used"

I noticed that as well. But I honestly don't think that's accurate. It's not
terribly common in Chromium where there are only 36 files using it
(https://cs.chromium.org/search/?q=@public+file:%5C.js) but there are quite a
lot in internal code (http://cs/search/?q=@public+file:%5C.js).

[Eugene But (OOO till 7-30), 2016-11-02 15:40:09]

Sylvain?

[sdefresne, 2016-11-02 17:08:30]

lgtm

[Eugene But (OOO till 7-30), 2016-11-02 17:12:59]

Thanks everyone!

[Eugene But (OOO till 7-30), 2016-11-02 17:13:05]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2016-11-02 17:13:46]

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, 2016-11-02 18:14:32]

Description was changed from

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.
 
BUG=487804
==========

to

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.
 
BUG=487804
==========

[commit-bot: I haz the power, 2016-11-02 18:14:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-02 18:23:43]

Description was changed from

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.
 
BUG=487804
==========

to

==========
[ios] Pass more --jscomp_error switches to the closure Compiler.

1.) Allow compiler to see __gCrWeb, __gCrWeb.autofill,
__gCrWeb.languageDetection, __gCrWeb.suggestions, __gCrWeb.translate,
__gCrWeb.autofill, __gCrWeb.common objects by creating them via dot
syntax.

2.) Made __gCrWeb.message.invokeOnHost public as it's used in other
files.

3.) Access windowId, via bracket syntax as it may not be defined.

4.) Access POSTSuccessHandler and POSTErrorHandler as they are
installed by WKWebView from the native code.

BUG=487804

Committed: https://crrev.com/cda1c632a2cbe11af48a9ef5bb7b7e568934611f
Cr-Commit-Position: refs/heads/master@{#429344}
==========

[commit-bot: I haz the power, 2016-11-02 18:23:44]

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

[sdefresne, 2016-11-03 16:10:13]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2472593005/ by sdefresne@chromium.org.

The reason for reverting is: Reverting as this break the iOS autoroller and the
CL was implemented by a member of the Chrome on iOS team..