Attempt 2 to enable async data: URL loading.

Attempt 2 to enable async data: URL loading.

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

[dcheng, 2014-04-29 21:49:41]

I took this over from eseidel@ and fixed all the locations in
content_browsertests that I found with this dependency. There are probably a
couple other places that need to be fixed in Chrome proper. I tested this with
https://codereview.chromium.org/253823003/ patched in and the setting enabled.

I'm a bit fuzzy on why the PostTask() helps the browser plugin tests pass, so
I've CCed lazyboy@ as well who can hopefully comment on that...

[dcheng, 2014-04-29 21:50:15]

+jam as well for content/renderer changes.

[lazyboy, 2014-04-29 22:38:39]

On 2014/04/29 21:49:41, dcheng wrote:
> I took this over from eseidel@ and fixed all the locations in
> content_browsertests that I found with this dependency. There are probably a
> couple other places that need to be fixed in Chrome proper. I tested this with
> https://codereview.chromium.org/253823003/ patched in and the setting enabled.
> 
> I'm a bit fuzzy on why the PostTask() helps the browser plugin tests pass, so
> I've CCed lazyboy@ as well who can hopefully comment on that...
I'm assuming these are content_browsertests:BrowserPluginTest.*
I think what's happening here is we expect an IPC
(BrowserPluginHostMsg_Attach) right after sticking browser plugin
into the DOM.
You need a ProcessPendingMessages() call before that.
See http://crrev.com/259363003.

[dcheng, 2014-04-29 23:00:32]

I've updated the patch per your suggestion. Thanks for the tips!

[sky, 2014-04-30 13:30:59]

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
File content/public/test/frame_load_waiter.h (right):

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
content/public/test/frame_load_waiter.h:11: class FrameLoadWaiter : public
content::RenderViewObserver {
Add description.

https://codereview.chromium.org/251903006/diff/60001/content/renderer/render_...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/251903006/diff/60001/content/renderer/render_...
content/renderer/render_view_browsertest.cc:1824:
FrameLoadWaiter(view()).Wait();
Why does this code need FrameLoadWaiter? Should FrameLoadWaiter be more central,
say in RenderViewTest::GoToOffset?

[jam, 2014-04-30 16:08:19]

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
File content/public/test/frame_load_waiter.h (right):

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
content/public/test/frame_load_waiter.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
note: the guidelines for what goes in content/public/test and content/test are
the same as other Content APIs: i.e. stuff should go in content/public/test if
it's used by a higher module (chrome, components...), otherwise it should go
into content/test

in this particular case, this class is only used by one test, so you should
inline it in that cc file until we need it in other tests

[dcheng, 2014-04-30 17:56:06]

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
File content/public/test/frame_load_waiter.h (right):

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
content/public/test/frame_load_waiter.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/04/30 16:08:19, jam wrote:
> note: the guidelines for what goes in content/public/test and content/test are
> the same as other Content APIs: i.e. stuff should go in content/public/test if
> it's used by a higher module (chrome, components...), otherwise it should go
> into content/test
> 
> in this particular case, this class is only used by one test, so you should
> inline it in that cc file until we need it in other tests

Moved to content/test (RenderViewTest needs this as well)

https://codereview.chromium.org/251903006/diff/60001/content/public/test/fram...
content/public/test/frame_load_waiter.h:11: class FrameLoadWaiter : public
content::RenderViewObserver {
On 2014/04/30 13:30:59, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/251903006/diff/60001/content/renderer/render_...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/251903006/diff/60001/content/renderer/render_...
content/renderer/render_view_browsertest.cc:1824:
FrameLoadWaiter(view()).Wait();
On 2014/04/30 13:30:59, sky wrote:
> Why does this code need FrameLoadWaiter? Should FrameLoadWaiter be more
central,
> say in RenderViewTest::GoToOffset?

Done. Thanks for noticing this.

[jam, 2014-04-30 22:18:35]

lgtm

I missed that it was used in two test files, sorry.

Now that I look closer, can you make this work on RenderFrame instead of
RenderView? RenderFrameObserver also has DidFinishLoad. It would save us the
effort of converting it later.

[dcheng, 2014-05-01 23:58:47]

Good suggestion--I've gone ahead and converted it to use RenderFrameObserver.

[dcheng, 2014-05-01 23:58:52]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:03:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/251903006/100001

[commit-bot: I haz the power, 2014-05-02 00:10:26]

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

[commit-bot: I haz the power, 2014-05-02 00:10:27]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium

[eseidel, 2014-05-02 00:25:04]

So when I attempted to fix some of these browser_tests, I had to worry about
dependent frame loads.  data: urls inside data: urls.  Are you sure that
FrameLoadWaiter only watching the main frame is sufficient?

[dcheng, 2014-05-02 00:36:02]

On 2014/05/02 00:25:04, eseidel wrote:
> So when I attempted to fix some of these browser_tests, I had to worry about
> dependent frame loads.  data: urls inside data: urls.  Are you sure that
> FrameLoadWaiter only watching the main frame is sufficient?

This is sufficient for all test cases I'm currently covering in
content_browsertests. I didn't find any failures that seem to be directly
related that in browser_tests proper, but I'm going to be sending some tryjobs
to verify those separately. If I do find instances where it needs to be
adjusted, I'll update the helper as needed.

[dcheng, 2014-05-02 00:36:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:37:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/251903006/100001

[commit-bot: I haz the power, 2014-05-02 01:00:10]

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

[commit-bot: I haz the power, 2014-05-02 01:00:11]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[dcheng, 2014-05-02 07:39:24]

I ended up reverting this to use the original strategy (PostTask to quit the
RunLoop) since it turns out some other tests (translation) were depending on
spinning the run loop once to make sure various IPCs happened.

[dcheng, 2014-05-02 07:39:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-05-02 07:39:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/251903006/120001

[commit-bot: I haz the power, 2014-05-02 07:59:57]

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

[commit-bot: I haz the power, 2014-05-02 07:59:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[dcheng, 2014-05-02 17:25:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-05-02 17:25:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/251903006/120001

[commit-bot: I haz the power, 2014-05-02 18:48:17]

Change committed as 267886