Port identity_internals to mojo

mojo: Port chrome://identity-internals to mojo Web UI.

BUG=391095
NOPRESUBMIT=true
R=fgorski@chromium.org, jam@chromium.org, jschuh@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286247

[erg, 2014-07-02 22:01:08]

msw: fyi
sky: first round review + request for advice:

The one remaining thing to do in this patch is fix the failing javascript based
browsertests...and I'm drawing a blank trying to figure out why they're busted.
How did you go around fixing the omnibox browser tests when you ported the
omnibox to MojoWebUI?

[darin (slow to review), 2014-07-04 14:27:06]

https://codereview.chromium.org/365513002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/identity_internals/identity_internals.mojom
(right):

https://codereview.chromium.org/365513002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/identity_internals/identity_internals.mojom:21:
GetTokensDone(IdentityTokenMojo[] tokens);  // was returnTokens
perhaps these should be modeled as callbacks?

GetTokens() => (IdentityTokenMojo[] tokens);
RevokeToken(string extension_id, string access_token) => (string access_token);

[sky, 2014-07-09 19:42:58]

On 2014/07/02 22:01:08, erg wrote:
> msw: fyi
> sky: first round review + request for advice:
> 
> The one remaining thing to do in this patch is fix the failing javascript
based
> browsertests...and I'm drawing a blank trying to figure out why they're
busted.
> How did you go around fixing the omnibox browser tests when you ported the
> omnibox to MojoWebUI?

It was extremely easy to get the omnibox browser tests to work with the mojo
changes because there were none;)
I don't know enough about the JS test harness that is in use here to understand
the failure. My guess is the dependencies aren't quite right, or some how the
mojo JS files aren't found correctly. I poked Darin and he said he would look at
little more deeply.

[erg, 2014-07-11 19:29:54]

https://codereview.chromium.org/365513002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/identity_internals.js (right):

https://codereview.chromium.org/365513002/diff/60001/chrome/browser/resources...
chrome/browser/resources/identity_internals.js:203:
Object.create(browser.InternalsPageStub.prototype);
So I've moved this to callbacks and promises. However, I'm not sure how to pull
out the empty InternalsPage interface, as it appears necessary for the
Connection() machinery.

[erg, 2014-07-16 22:11:07]

So the test failures look like a timing problem.

If I put a console.log() before the define, and right inside the function()
passed to define, the outer console.log() triggers and the inner one doesn't.

If I instrument gin's ModuleRegistry::Load() so that it prints out on execution
(which is where define() is defined), the ModuleRegistry::Load() LOG prints
_after_ the test assertion failure, suggesting that the problem is that the test
harness isn't waiting for all the gin stuff to load.

However, I don't get how I'd do this wait. It looks like things like we're
already using a TestNavigationObserver in WebUIBrowserTest::BrowsePreload() to
block until DidStopLoading().

Is there a different event that we should be waiting on instead, or will this
require all new plumbing?

[sky, 2014-07-16 23:06:44]

No doubt this will come up with other tests. I doubt there is a way now to
know when main in the page has been run. I think we'll need to plumb
through a message specifically for this so that tests know when they can
continue on.

  -Scott


On Wed, Jul 16, 2014 at 3:11 PM, <erg@google.com> wrote:

> So the test failures look like a timing problem.
>
> If I put a console.log() before the define, and right inside the function()
> passed to define, the outer console.log() triggers and the inner one
> doesn't.
>
> If I instrument gin's ModuleRegistry::Load() so that it prints out on
> execution
> (which is where define() is defined), the ModuleRegistry::Load() LOG prints
> _after_ the test assertion failure, suggesting that the problem is that
> the test
> harness isn't waiting for all the gin stuff to load.
>
> However, I don't get how I'd do this wait. It looks like things like we're
> already using a TestNavigationObserver in WebUIBrowserTest::BrowsePreload()
> to
> block until DidStopLoading().
>
> Is there a different event that we should be waiting on instead, or will
> this
> require all new plumbing?
>
> https://codereview.chromium.org/365513002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2014-07-21 21:56:03]

+jschuh: I have added one IPC message, which has no parameters, and only signals
that code has been run on webui pages. Requesting security stamp.

Everyone else: I've added waiting for mojo 'main' to the javascript generating
browser test compiler. All the browser tests pass now. PTAL.

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js
(left):

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js:249:
identity_internals.tokenRevokeDone = this.continueTest(
This test was removed because I don't believe we can hot patch this in a mojo
world.

[jschuh, 2014-07-21 23:55:15]

On 2014/07/21 21:56:03, Elliot Glaysher wrote:
> +jschuh: I have added one IPC message, which has no parameters, and only
signals
> that code has been run on webui pages. Requesting security stamp.

ipc security lgtm (notes: ^^^)

[Elliot Glaysher, 2014-07-22 21:52:40]

ping sky

[sky, 2014-07-22 22:28:03]

Remind me which parts of this you want me to review.

[Elliot Glaysher, 2014-07-22 23:40:02]

fgorski: Hi, we're currently porting various webui pages to mojo from the legacy
framework. As the original implementer of the identity-internals page, I was
wondering if I could get a review here?

[fgorski, 2014-07-23 19:02:10]

overall code looks ok in identity_internals, but I have a huge concerns about
not being able to test user interactions.

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js
(left):

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js:249:
identity_internals.tokenRevokeDone = this.continueTest(
On 2014/07/21 21:56:03, Elliot Glaysher wrote:
> This test was removed because I don't believe we can hot patch this in a mojo
> world.

Is there a long term solution to a problem. I was a noogler when writing this
piece of code, so it is hard to believe (and remember) that I didn't see this
approach used someplace else. That means this you may have to delete a few more
tests at least. I don't think that's reasonable and there should be an approach
worked out for this problem.

Would you consider disabling the test instead of deleting it? Or is testing
user's actions on WebUI never meant to work?

https://codereview.chromium.org/365513002/diff/100001/chrome/chrome_resources...
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/365513002/diff/100001/chrome/chrome_resources...
chrome/chrome_resources.gyp:10: 'identity_internals_mojom_file':
'<(SHARED_INTERMEDIATE_DIR)/chrome/browser/ui/webui/identity_internals/identity_internals.mojom.js',
nit: in case the order of these does not matter, a convention (supported by a
comment if necessary) would be useful. Either append at the end or keep ordered
alphabetically works for me.

https://codereview.chromium.org/365513002/diff/100001/chrome/chrome_resources...
chrome/chrome_resources.gyp:203: '-E',
'identity_internals_mojom_file=<(identity_internals_mojom_file)',
nit: ditto

https://codereview.chromium.org/365513002/diff/100001/chrome/chrome_web_ui_mo...
File chrome/chrome_web_ui_mojo_bindings.gyp (right):

https://codereview.chromium.org/365513002/diff/100001/chrome/chrome_web_ui_mo...
chrome/chrome_web_ui_mojo_bindings.gyp:15:
'browser/ui/webui/identity_internals/identity_internals.mojom',
keep sorted

[Elliot Glaysher, 2014-07-23 20:20:05]

ptal

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js
(left):

https://codereview.chromium.org/365513002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/identity_internals/identity_internals_ui_browsertest.js:249:
identity_internals.tokenRevokeDone = this.continueTest(
On 2014/07/23 19:02:09, fgorski wrote:
> On 2014/07/21 21:56:03, Elliot Glaysher wrote:
> > This test was removed because I don't believe we can hot patch this in a
mojo
> > world.
> 
> Is there a long term solution to a problem. I was a noogler when writing this
> piece of code, so it is hard to believe (and remember) that I didn't see this
> approach used someplace else. That means this you may have to delete a few
more
> tests at least. I don't think that's reasonable and there should be an
approach
> worked out for this problem.
> 
> Would you consider disabling the test instead of deleting it? Or is testing
> user's actions on WebUI never meant to work?
> 

There isn't a generic way to do this (and long term, one will have to exist for
testing). I've readded this test and modified it to use a bit of a hack.

[fgorski, 2014-07-23 20:47:05]

lgtm

I'd still like to see the long term solution at some point, but this seems to be
doing the job for now and I don't think it exposes us more than before (where
injection of code was possible on identity_internals and tests took advantage of
that).

[Elliot Glaysher, 2014-07-23 22:20:07]

sky: owners stamp / general review

[sky, 2014-07-23 23:35:11]

https://codereview.chromium.org/365513002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/identity_internals/identity_internals.mojom
(right):

https://codereview.chromium.org/365513002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/identity_internals/identity_internals.mojom:5: struct
IdentityTokenMojo {
It would be nice if this had some comments.

https://codereview.chromium.org/365513002/diff/140001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.cc (right):

https://codereview.chromium.org/365513002/diff/140001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.cc:79: void
WebUIMojoContextState::RunMain(base::WeakPtr<gin::Runner> runner,
You shouldn't need to pass in runner_ since it's already a member.

[Elliot Glaysher, 2014-07-24 00:13:59]

done

[sky, 2014-07-24 16:50:28]

LGTM

[Elliot Glaysher, 2014-07-24 17:12:51]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-07-24 17:13:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/365513002/160001

[commit-bot: I haz the power, 2014-07-24 18:30:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-24 18:35:19]

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

[commit-bot: I haz the power, 2014-07-24 18:35:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Elliot Glaysher, 2014-07-25 19:34:37]

+jam for content/ owners stamp

[jam, 2014-07-25 21:29:59]

we're working on removing RenderView as part of site isolation, so use
RenderFrame instead. i've left comments on what to do

https://codereview.chromium.org/365513002/diff/160001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/365513002/diff/160001/content/common/view_mes...
content/common/view_messages.h:1458:
IPC_MESSAGE_ROUTED0(ViewHostMsg_WebUIMojoMainRan)
this should go to frame_messages.h

https://codereview.chromium.org/365513002/diff/160001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/365513002/diff/160001/content/public/browser/...
content/public/browser/web_contents_observer.h:330: virtual void
DidRunWebUIMojoMain() {}
this would have a RenderFrame argument if it's called for all frames, or no
argument if it's only called for main frame (and if so, please document that)

https://codereview.chromium.org/365513002/diff/160001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/365513002/diff/160001/content/renderer/render...
content/renderer/render_view_impl.h:324: void WebUIMojoMainRan();
no changes here or in the cc file

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
File content/renderer/web_ui_mojo.h (right):

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
content/renderer/web_ui_mojo.h:31: explicit WebUIMojo(RenderViewImpl*
render_view);
ditto, no changes in this file or the cc

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.cc (right):

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.cc:87:
render_view_impl_->WebUIMojoMainRan();
here, you can use frame_ to get to a RenderFrame, and just call
render_frame->Send(new FooMsg(render_frame->GetRoutingID(), ...

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.h (right):

https://codereview.chromium.org/365513002/diff/160001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.h:42:
WebUIMojoContextState(RenderViewImpl* render_view,
leave as is

[Elliot Glaysher, 2014-07-25 23:35:16]

jam: I believe I've done everything here. If I've missed anything, please give a
holler.

[jam, 2014-07-28 06:41:59]

content lgtm

https://codereview.chromium.org/365513002/diff/200001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.cc (right):

https://codereview.chromium.org/365513002/diff/200001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.cc:87: if (render_frame) {
nit: no check is needed here, since there's always a RF per WF

[Elliot Glaysher, 2014-07-28 19:37:26]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-07-28 20:35:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/365513002/240001

[commit-bot: I haz the power, 2014-07-29 00:04:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-07-29 00:07:43]

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

[commit-bot: I haz the power, 2014-07-29 00:07:44]

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

[Elliot Glaysher, 2014-07-29 00:09:31]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-07-29 00:10:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/365513002/240001

[commit-bot: I haz the power, 2014-07-29 00:14:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-07-29 00:17:18]

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

[commit-bot: I haz the power, 2014-07-29 00:17:19]

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

[Elliot Glaysher, 2014-07-29 17:42:13]

Committed patchset #14 manually as r286247 (presubmit successful).