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

Issue 780413002: Make 'reload' in skydb work again (Closed)

Created:
6 years ago by eseidel
Modified:
6 years ago
Reviewers:
Aaron Boodman, qsr
CC:
mojo-reviews_chromium.org, ojan, esprehn, abarth-chromium
Base URL:
git@github.com:domokit/mojo.git@master
Project:
mojo
Visibility:
Public.

Description

Make 'reload' in skydb work again Looks like it broke with: https://codereview.chromium.org/741453002 I don't really like this fix, (it just leaks the SkyApplication object) but I don't have a better one at the moment. I suspect the problem may actually be in ApplicationManager::LoadWithContentHandler in its attempt to re-use connections, unclear. The bug was that you would launch skydb and then try to 'reload' the url and the reload would be ignored every other attempt. Reload just tells the view to embed the url again. The ViewManager was telling its client OnEmbed, but that message was never arriving every other time. After many printfs I traced this to the DocumentView being destroyed (which calls SkyApplication::OnViewDestroyed) and causing the SkyApplication to disappear right before the ViewManager tried to talk to it. R=aa@chromium.org BUG=

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -3 lines) Patch
M sky/viewer/content_handler_impl.cc View 1 chunk +13 lines, -3 lines 1 comment Download

Messages

Total messages: 7 (1 generated)
eseidel
6 years ago (2014-12-05 22:14:28 UTC) #1
eseidel
Looks like this broke with https://codereview.chromium.org/741453002
6 years ago (2014-12-05 23:08:19 UTC) #3
Aaron Boodman
https://codereview.chromium.org/780413002/diff/1/sky/viewer/content_handler_impl.cc File sky/viewer/content_handler_impl.cc (right): https://codereview.chromium.org/780413002/diff/1/sky/viewer/content_handler_impl.cc#newcode60 sky/viewer/content_handler_impl.cc:60: // TODO(eseidel): We can't destroy ourselves synchronously here This ...
6 years ago (2014-12-05 23:29:50 UTC) #4
eseidel
It appears that the code as-written creates a new SkyApplication each time content is loaded. ...
6 years ago (2014-12-05 23:33:04 UTC) #5
Aaron Boodman
lgtm
6 years ago (2014-12-06 00:02:30 UTC) #6
qsr
6 years ago (2014-12-08 14:39:56 UTC) #7
On 2014/12/06 00:02:30, Aaron Boodman wrote:
> lgtm

 if we remove the cleanup code, let's remove everything:
https://codereview.chromium.org/783973002

Powered by Google App Engine
This is Rietveld 408576698