Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(521)

Issue 257723002: Prepare browser_tests for data: urls being async (Closed)

Created:
6 years, 8 months ago by eseidel
Modified:
6 years, 8 months ago
Reviewers:
sky, abarth-chromium
CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen (gone - plz use gerrit), dcheng
Visibility:
Public.

Description

Prepare browser_tests for data: urls being async RenderViewTest::LoadHTML assumed that issuing a load of a data url, posting a task to quit the message loop and then pumping the message loop until it quits was sufficient to end up with a fully parsed page. That very soon won't be true once we send data: urls to the threaded parser, as the parser thread posts back to the main thread and may post tasks after the quit message. https://codereview.chromium.org/210253003/ Instead I've added a FrameLoadWaiter class which implements RenderViewObserver to listen for the load event for the frame and only post the quit task once the load event had come in. BUG=308321 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266163

Patch Set 1 #

Total comments: 5

Patch Set 2 : Fix style #

Patch Set 3 : more style fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -3 lines) Patch
M content/public/test/render_view_test.cc View 1 2 1 chunk +23 lines, -3 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
eseidel
6 years, 8 months ago (2014-04-24 23:00:52 UTC) #1
eseidel
6 years, 8 months ago (2014-04-24 23:01:01 UTC) #2
sky
LGTM with the style fixes. https://codereview.chromium.org/257723002/diff/1/content/public/test/render_view_test.cc File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/257723002/diff/1/content/public/test/render_view_test.cc#newcode117 content/public/test/render_view_test.cc:117: public: indent this and ...
6 years, 8 months ago (2014-04-24 23:19:10 UTC) #3
eseidel
The CQ bit was checked by eseidel@chromium.org
6 years, 8 months ago (2014-04-24 23:25:45 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/257723002/40001
6 years, 8 months ago (2014-04-24 23:28:57 UTC) #5
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-25 02:18:15 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
6 years, 8 months ago (2014-04-25 02:18:16 UTC) #7
eseidel
The CQ bit was checked by eseidel@chromium.org
6 years, 8 months ago (2014-04-25 02:26:38 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/257723002/40001
6 years, 8 months ago (2014-04-25 02:28:28 UTC) #9
commit-bot: I haz the power
Change committed as 266163
6 years, 8 months ago (2014-04-25 08:54:48 UTC) #10
eseidel
6 years, 7 months ago (2014-04-28 18:09:23 UTC) #11
Message was sent while issue was closed.
A revert of this CL has been created in
https://codereview.chromium.org/252743006/ by eseidel@chromium.org.

The reason for reverting is: This didn't fully solve the issue, no need to add
the complexity..

Powered by Google App Engine
This is Rietveld 408576698