[Offline pages] Extract internals page interaction with browser to a proxy

[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4627dafbf67605dba4c26bcaa39c9af8ed3949b8
Cr-Commit-Position: refs/heads/master@{#400501}

[chili, 2016-06-15 01:00:30]

Description was changed from

==========
Use browser proxy to handle communication with chrome.

BUG=
==========

to

==========
Use browser proxy to handle communication with chrome.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[chili, 2016-06-15 18:25:29]

Description was changed from

==========
Use browser proxy to handle communication with chrome.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Use browser proxy to handle communication with chrome.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[chili, 2016-06-16 21:13:25]

chili@chromium.org changed reviewers:
+ dewittj@chromium.org, dpapad@chromium.org

[chili, 2016-06-16 21:13:26]

While the one of the main motivations behind this change is to make it easier to
write browser tests, it appears that android does not support browser tests (and
the page is only viewable on android).

Let me know if the fact that this page is android-only makes it *not* a good
idea to use the proxy style :)

[dpapad, 2016-06-16 21:31:10]

Using the browserProxy pattern is beneficial regardless of not being able to
test this page with a WebUI test currently. It gathers the interaction between
C++ and JS in a single location in a well defined manner. This also enables
further changes to this page, specifically Mojo-ification (where the
browserProxy can be auto-generated from a .mojom file). For an example you can
see [1] and [2]. 

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/plugins/plugins....
[2]
https://cs.chromium.org/chromium/src/chrome/browser/resources/plugins.js?q=pl...

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:31:
cr.define('offlineProxy', function() {
Let's use the same "offlineInternals" namespace being used in
offline_internals.js.

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:38: *
@return {!Promise} A promise firing when the list is fetched.
!Promise<!Array<OfflinePage>>

Can we add more specific types on all returned Promises?

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:40:
onGetStoredPages: function() {},
I don't think the 'on' prefix is appropriate here. It is usually used when a
function executes as a result of an event, for example onClick, onTap, etc. Here
you are directly requesting the stored pages, so getStoredPages() seems more
appropriate.

Same for all other methods in this interface.

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:72:
return cr.sendWithPromise('getStoredPages');
Following up on the previous point about naming, the convention we've been
following is to make the proxy method match the name of the string that is sent
to the C++ side, so 

getStoredPages: function() {
  return cr.sendWithPromise('getStoredPages');
};

[dewittj, 2016-06-16 21:40:41]

somewhat aside, is there no infrastructure for jsunit tests in webui?

[dpapad, 2016-06-16 21:52:17]

On 2016/06/16 at 21:40:41, dewittj wrote:
> somewhat aside, is there no infrastructure for jsunit tests in webui?

Are you referring to a test similar to
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/p...
These tests are only useful when testing non UI related logic (they are run
directly on v8's shell (d8), so there is no window, no DOM etc.

[chili, 2016-06-16 22:31:29]

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:31:
cr.define('offlineProxy', function() {
On 2016/06/16 21:31:10, dpapad wrote:
> Let's use the same "offlineInternals" namespace being used in
> offline_internals.js.

Done.

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:38: *
@return {!Promise} A promise firing when the list is fetched.
On 2016/06/16 21:31:10, dpapad wrote:
> !Promise<!Array<OfflinePage>>
> 
> Can we add more specific types on all returned Promises?

Done.

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:40:
onGetStoredPages: function() {},
On 2016/06/16 21:31:10, dpapad wrote:
> I don't think the 'on' prefix is appropriate here. It is usually used when a
> function executes as a result of an event, for example onClick, onTap, etc.
Here
> you are directly requesting the stored pages, so getStoredPages() seems more
> appropriate.
> 
> Same for all other methods in this interface.

Done.

https://codereview.chromium.org/2069933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:72:
return cr.sendWithPromise('getStoredPages');
On 2016/06/16 21:31:10, dpapad wrote:
> Following up on the previous point about naming, the convention we've been
> following is to make the proxy method match the name of the string that is
sent
> to the C++ side, so 
> 
> getStoredPages: function() {
>   return cr.sendWithPromise('getStoredPages');
> };

Done.

[Dan Beam, 2016-06-16 22:54:33]

pretty sure we can run many types of tests on android (browser test, unit tests
"gtestjs", etc.)

I think any history page-related *browsertest.js or C++ browser tests should run
on android as well

[dewittj, 2016-06-16 23:38:33]

this code lgtm,

If tests for this code can run on Android, I'd like to see at least one set up
in this CL.  Otherwise we can wait for
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...

[Dan Beam, 2016-06-16 23:55:32]

On 2016/06/16 23:38:33, dewittj wrote:
> this code lgtm,
> 
> If tests for this code can run on Android, I'd like to see at least one set up
> in this CL.  Otherwise we can wait for
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...

ah, didn't know this ^.  did android testers run browser_tests when they used
GYP?  could've sworn they did?

I also swear gtestjs did at least run on android (I had to turn them off from
running on android here[1]).

[1] https://codereview.chromium.org/1491773002

[dpapad, 2016-06-16 23:58:27]

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:38: *
@return {!Promise<Array<OfflinePage>>} A promise firing when the
Is the returned array ever null? If not, !Array<OfflinePages>, or document in
the comments when does this return null. I would assume that an empty array is
returned instead of null.

Same below.

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:67: *
@implements {offlineProxy.OfflineInternalsBrowserProxy}
Is this code being compiled? offlineProxy namespace does not exist anymore, it
was renamed to offlineInternals.

[chili, 2016-06-17 00:07:50]

On 2016/06/16 23:55:32, Dan Beam wrote:
> On 2016/06/16 23:38:33, dewittj wrote:
> > this code lgtm,
> > 
> > If tests for this code can run on Android, I'd like to see at least one set
up
> > in this CL.  Otherwise we can wait for
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...
> 
> ah, didn't know this ^.  did android testers run browser_tests when they used
> GYP?  could've sworn they did?
> 
> I also swear gtestjs did at least run on android (I had to turn them off from
> running on android here[1]).
> 
> [1] https://codereview.chromium.org/1491773002

According to dpapad@'s previous comment -> [gtestjs] are only useful when
testing non UI related logic (they are run
directly on v8's shell (d8), so there is no window, no DOM etc.

Since the javascript in question interacts directly with window and DOM, I would
assume gtestjs wouldn't work

[chili, 2016-06-17 00:10:40]

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:38: *
@return {!Promise<Array<OfflinePage>>} A promise firing when the
On 2016/06/16 23:58:27, dpapad wrote:
> Is the returned array ever null? If not, !Array<OfflinePages>, or document in
> the comments when does this return null. I would assume that an empty array is
> returned instead of null.
> 
> Same below.

Done.

https://codereview.chromium.org/2069933002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:67: *
@implements {offlineProxy.OfflineInternalsBrowserProxy}
On 2016/06/16 23:58:27, dpapad wrote:
> Is this code being compiled? offlineProxy namespace does not exist anymore, it
> was renamed to offlineInternals.

Done.

[Dan Beam, 2016-06-17 00:37:27]

On 2016/06/17 00:07:50, chili wrote:
> On 2016/06/16 23:55:32, Dan Beam wrote:
> > On 2016/06/16 23:38:33, dewittj wrote:
> > > this code lgtm,
> > > 
> > > If tests for this code can run on Android, I'd like to see at least one
set
> up
> > > in this CL.  Otherwise we can wait for
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...
> > 
> > ah, didn't know this ^.  did android testers run browser_tests when they
used
> > GYP?  could've sworn they did?
> > 
> > I also swear gtestjs did at least run on android (I had to turn them off
from
> > running on android here[1]).
> > 
> > [1] https://codereview.chromium.org/1491773002
> 
> According to dpapad@'s previous comment -> [gtestjs] are only useful when
> testing non UI related logic (they are run
> directly on v8's shell (d8), so there is no window, no DOM etc.
> 
> Since the javascript in question interacts directly with window and DOM, I
would
> assume gtestjs wouldn't work

yes, you could only test logic-only stuff (and would need to pull it out of code
that assume DOM, i.e. Polymer)

[dpapad, 2016-06-17 00:41:59]

LGTM, with nit.

Can you update the CL description to be a bit more specific? How about prefixing
it with
Offline pages: Use browser ....

[chili, 2016-06-17 00:57:49]

Description was changed from

==========
Use browser proxy to handle communication with chrome.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[chili, 2016-06-17 01:00:02]

On 2016/06/17 00:41:59, dpapad wrote:
> LGTM, with nit.
> 
> Can you update the CL description to be a bit more specific? How about
prefixing
> it with
> Offline pages: Use browser ....

Done.

Thanks for the quick review! =)

[chili, 2016-06-17 01:11:32]

On 2016/06/17 00:37:27, Dan Beam wrote:
> On 2016/06/17 00:07:50, chili wrote:
> > On 2016/06/16 23:55:32, Dan Beam wrote:
> > > On 2016/06/16 23:38:33, dewittj wrote:
> > > > this code lgtm,
> > > > 
> > > > If tests for this code can run on Android, I'd like to see at least one
> set
> > up
> > > > in this CL.  Otherwise we can wait for
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...
> > > 
> > > ah, didn't know this ^.  did android testers run browser_tests when they
> used
> > > GYP?  could've sworn they did?
> > > 
> > > I also swear gtestjs did at least run on android (I had to turn them off
> from
> > > running on android here[1]).
> > > 
> > > [1] https://codereview.chromium.org/1491773002
> > 
> > According to dpapad@'s previous comment -> [gtestjs] are only useful when
> > testing non UI related logic (they are run
> > directly on v8's shell (d8), so there is no window, no DOM etc.
> > 
> > Since the javascript in question interacts directly with window and DOM, I
> would
> > assume gtestjs wouldn't work
> 
> yes, you could only test logic-only stuff (and would need to pull it out of
code
> that assume DOM, i.e. Polymer)

Let me know if you think there is some action item here.

For the time being, I think we are ok with having no tests because:
- Browser tests support for android has yet to be supported
- gtestsjs doesn't work because there's really no logic in the js that doesn't
touch window/dom

[Dan Beam, 2016-06-17 17:33:35]

On 2016/06/17 01:11:32, chili wrote:
> On 2016/06/17 00:37:27, Dan Beam wrote:
> > On 2016/06/17 00:07:50, chili wrote:
> > > On 2016/06/16 23:55:32, Dan Beam wrote:
> > > > On 2016/06/16 23:38:33, dewittj wrote:
> > > > > this code lgtm,
> > > > > 
> > > > > If tests for this code can run on Android, I'd like to see at least
one
> > set
> > > up
> > > > > in this CL.  Otherwise we can wait for
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/browse...
> > > > 
> > > > ah, didn't know this ^.  did android testers run browser_tests when they
> > used
> > > > GYP?  could've sworn they did?
> > > > 
> > > > I also swear gtestjs did at least run on android (I had to turn them off
> > from
> > > > running on android here[1]).
> > > > 
> > > > [1] https://codereview.chromium.org/1491773002
> > > 
> > > According to dpapad@'s previous comment -> [gtestjs] are only useful when
> > > testing non UI related logic (they are run
> > > directly on v8's shell (d8), so there is no window, no DOM etc.
> > > 
> > > Since the javascript in question interacts directly with window and DOM, I
> > would
> > > assume gtestjs wouldn't work
> > 
> > yes, you could only test logic-only stuff (and would need to pull it out of
> code
> > that assume DOM, i.e. Polymer)
> 
> Let me know if you think there is some action item here.
> 
> For the time being, I think we are ok with having no tests because:
> - Browser tests support for android has yet to be supported
> - gtestsjs doesn't work because there's really no logic in the js that doesn't
> touch window/dom

i'm fine with it for now

fwiw: there are other browser tests (i.e. content_browsertests,
components_browsertests) running on Android, maybe just that particular target
is super hairy and/or never ran there?

[dewittj, 2016-06-17 17:37:24]

still lgtm.

yeah, I agree that this is shippable as-is, once phase 1 of the browser_tests
design doc is done (minimal browser_tests target being run on Android) we can
add a test to the Android-ok sources of the browser_tests target and get some
coverage.

[chili, 2016-06-17 18:54:44]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2016-06-17 18:55:23]

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

[commit-bot: I haz the power, 2016-06-17 19:14:19]

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

[commit-bot: I haz the power, 2016-06-17 19:14:20]

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

[chili, 2016-06-17 20:14:35]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-17 20:14:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2069933002/#ps80001
(title: "more code review fixes")

[commit-bot: I haz the power, 2016-06-17 20:14:56]

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

[commit-bot: I haz the power, 2016-06-17 20:59:47]

Description was changed from

==========
[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-17 20:59:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-17 21:01:40]

Description was changed from

==========
[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Offline pages] Extract internals page interaction with browser to a proxy

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4627dafbf67605dba4c26bcaa39c9af8ed3949b8
Cr-Commit-Position: refs/heads/master@{#400501}
==========

[commit-bot: I haz the power, 2016-06-17 21:01:42]

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