Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
776553003:
Fix html viewer. (Closed)
|
Created:
6 years ago by qsr Modified:
6 years ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix html viewer.
R=aa@chromium.org
Committed: https://crrev.com/7221cc36c54297f3d9c435e4e83c6a3d78ea1171
Cr-Commit-Position: refs/heads/master@{#307666}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Follow review #Patch Set 3 : Follow review #
Total comments: 3
Messages
Total messages: 25 (2 generated)
Not sure we want to push this. This is fixing html_viewer related to the change in mojo. Unfortunately, I cannot run html_viewer even with this fix right now, because there is some GL issues that are unrelated.
gentle ping?
Doh, thought I did this. Looking...
On 2014/12/03 15:57:59, Aaron Boodman wrote: > Doh, thought I did this. Looking... BTW, we did some exploration with Dave, and were able to run the html_viewer, so I can confirm that this works. So I now think it should be pushed.
https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document_view.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:97: shell_(shell), This raw ptr is another way we can crash if the application goes away before us. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:105: WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); What deletes this class? https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_viewer.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_viewer.cc:100: new HTMLDocumentView(base::Bind(&HTMLViewerApplication::OnViewDestroyed, I know this already existed, but naming nit: this thing isn't really a 'view', as it has nothing to do with an embedding in ViewManager. It's really just an HTMLDocument. We should rename it. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_viewer.cc:101: base::Unretained(this)), Won't the application crash if HTMLViewerApplication is destroyed before one of the HTMLDocumentView's it created is? Perhaps HTMLViewerApplication should either directly own, or maintain raw pointers to, the HTMLDocumentViews it creates. Then when HTMLViewerApplication goes away, it can take all the remaining HTMLDocumentView instances with it.
https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document_view.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:97: shell_(shell), On 2014/12/03 16:42:11, Aaron Boodman wrote: > This raw ptr is another way we can crash if the application goes away before us. This raw ptr is owned by the application, and the application cannot delete itself before this class is deleted. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:105: WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); On 2014/12/03 16:42:11, Aaron Boodman wrote: > What deletes this class? Which class? exported_services_ is a member of this HTMLDocumentView, so it is deleted at the same time as HTMLDocumentView But I miss a delete this in OnViewDestroyed https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_viewer.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_viewer.cc:100: new HTMLDocumentView(base::Bind(&HTMLViewerApplication::OnViewDestroyed, On 2014/12/03 16:42:11, Aaron Boodman wrote: > I know this already existed, but naming nit: this thing isn't really a 'view', > as it has nothing to do with an embedding in ViewManager. It's really just an > HTMLDocument. We should rename it. Done. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_viewer.cc:101: base::Unretained(this)), On 2014/12/03 16:42:11, Aaron Boodman wrote: > Won't the application crash if HTMLViewerApplication is destroyed before one of > the HTMLDocumentView's it created is? > > Perhaps HTMLViewerApplication should either directly own, or maintain raw > pointers to, the HTMLDocumentViews it creates. Then when HTMLViewerApplication > goes away, it can take all the remaining HTMLDocumentView instances with it. HTMLViewerApplication is self owned. It can only delete itself when there is no more HTMLDocumentView
https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document_view.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:97: shell_(shell), On 2014/12/03 16:56:53, qsr wrote: > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > This raw ptr is another way we can crash if the application goes away before > us. > > This raw ptr is owned by the application, and the application cannot delete > itself before this class is deleted. Right. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:105: WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); On 2014/12/03 16:56:53, qsr wrote: > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > What deletes this class? > > Which class? exported_services_ is a member of this HTMLDocumentView, so it is > deleted at the same time as HTMLDocumentView > > But I miss a delete this in OnViewDestroyed I meant HTMLDocumentView. I don't think OnViewDestroyed is the right place to delete this. It is possible to create an HTMLDocumentView, but never have any associated view. In that case OnViewDestroyed won't ever be called. https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_viewer.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_viewer.cc:101: base::Unretained(this)), On 2014/12/03 16:56:53, qsr wrote: > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > Won't the application crash if HTMLViewerApplication is destroyed before one > of > > the HTMLDocumentView's it created is? > > > > Perhaps HTMLViewerApplication should either directly own, or maintain raw > > pointers to, the HTMLDocumentViews it creates. Then when HTMLViewerApplication > > goes away, it can take all the remaining HTMLDocumentView instances with it. > > HTMLViewerApplication is self owned. It can only delete itself when there is no > more HTMLDocumentView Ah, true.
https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document_view.cc (right): https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document_view.cc:105: WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); On 2014/12/03 17:32:37, Aaron Boodman wrote: > On 2014/12/03 16:56:53, qsr wrote: > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > What deletes this class? > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so it is > > deleted at the same time as HTMLDocumentView > > > > But I miss a delete this in OnViewDestroyed > > I meant HTMLDocumentView. > > I don't think OnViewDestroyed is the right place to delete this. It is possible > to create an HTMLDocumentView, but never have any associated view. In that case > OnViewDestroyed won't ever be called. Hum, what would be the right signal? This is not the shell closing the application, you can imagine this with some more connection to this. This is not the other side closing its ServiceProvider, because it can do this after getting a reference to the embedder service
On 2014/12/04 10:11:17, qsr wrote: > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > File mojo/services/html_viewer/html_document_view.cc (right): > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > mojo/services/html_viewer/html_document_view.cc:105: > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > On 2014/12/03 16:56:53, qsr wrote: > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > What deletes this class? > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so it > is > > > deleted at the same time as HTMLDocumentView > > > > > > But I miss a delete this in OnViewDestroyed > > > > I meant HTMLDocumentView. > > > > I don't think OnViewDestroyed is the right place to delete this. It is > possible > > to create an HTMLDocumentView, but never have any associated view. In that > case > > OnViewDestroyed won't ever be called. > > Hum, what would be the right signal? Probably the most reasonable thing for now is to quit when the SP that is passed to Application::AcceptConnection() dies. We should probably make it an official part of Shell::ConnectToApplication() that the SP passed there controls the lifetime of the created application.
On 2014/12/04 18:40:19, Aaron Boodman wrote: > On 2014/12/04 10:11:17, qsr wrote: > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > File mojo/services/html_viewer/html_document_view.cc (right): > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > mojo/services/html_viewer/html_document_view.cc:105: > > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > > On 2014/12/03 16:56:53, qsr wrote: > > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > > What deletes this class? > > > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so > it > > is > > > > deleted at the same time as HTMLDocumentView > > > > > > > > But I miss a delete this in OnViewDestroyed > > > > > > I meant HTMLDocumentView. > > > > > > I don't think OnViewDestroyed is the right place to delete this. It is > > possible > > > to create an HTMLDocumentView, but never have any associated view. In that > > case > > > OnViewDestroyed won't ever be called. > > > > Hum, what would be the right signal? > > Probably the most reasonable thing for now is to quit when the SP that is passed > to Application::AcceptConnection() dies. We should probably make it an official > part of Shell::ConnectToApplication() that the SP passed there controls the > lifetime of the created application. Err, this would of course have to actually be when *all* of the sp's passed to a single application's AC() method die.
On 2014/12/04 23:59:09, Aaron Boodman wrote: > On 2014/12/04 18:40:19, Aaron Boodman wrote: > > On 2014/12/04 10:11:17, qsr wrote: > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > File mojo/services/html_viewer/html_document_view.cc (right): > > > > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > mojo/services/html_viewer/html_document_view.cc:105: > > > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > > > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > > > On 2014/12/03 16:56:53, qsr wrote: > > > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > > > What deletes this class? > > > > > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so > > it > > > is > > > > > deleted at the same time as HTMLDocumentView > > > > > > > > > > But I miss a delete this in OnViewDestroyed > > > > > > > > I meant HTMLDocumentView. > > > > > > > > I don't think OnViewDestroyed is the right place to delete this. It is > > > possible > > > > to create an HTMLDocumentView, but never have any associated view. In that > > > case > > > > OnViewDestroyed won't ever be called. > > > > > > Hum, what would be the right signal? > > > > Probably the most reasonable thing for now is to quit when the SP that is > passed > > to Application::AcceptConnection() dies. We should probably make it an > official > > part of Shell::ConnectToApplication() that the SP passed there controls the > > lifetime of the created application. > > Err, this would of course have to actually be when *all* of the sp's passed to a > single application's AC() method die. This is far from being what we are doing today, for example, when we need the network service from an application, we connect to it, do a single request on the SP, and then drop the SP. If we want to do this, we should document this clearly.
On 2014/12/04 18:40:19, Aaron Boodman wrote: > On 2014/12/04 10:11:17, qsr wrote: > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > File mojo/services/html_viewer/html_document_view.cc (right): > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > mojo/services/html_viewer/html_document_view.cc:105: > > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > > On 2014/12/03 16:56:53, qsr wrote: > > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > > What deletes this class? > > > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so > it > > is > > > > deleted at the same time as HTMLDocumentView > > > > > > > > But I miss a delete this in OnViewDestroyed > > > > > > I meant HTMLDocumentView. > > > > > > I don't think OnViewDestroyed is the right place to delete this. It is > > possible > > > to create an HTMLDocumentView, but never have any associated view. In that > > case > > > OnViewDestroyed won't ever be called. > > > > Hum, what would be the right signal? > > Probably the most reasonable thing for now is to quit when the SP that is passed > to Application::AcceptConnection() dies. We should probably make it an official > part of Shell::ConnectToApplication() that the SP passed there controls the > lifetime of the created application. Also, for the current time this will be hard. There is no way to register a error handler on a ServiceProviderImpl. I'll add a CL to allow this, but we will need to wait that it is roll in chromium. Do you want to commit in the current state (that works with our current embedder, as they will always embed just after connecting), or do you prefer to wait?
On 2014/12/05 13:02:08, qsr wrote: > On 2014/12/04 18:40:19, Aaron Boodman wrote: > > On 2014/12/04 10:11:17, qsr wrote: > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > File mojo/services/html_viewer/html_document_view.cc (right): > > > > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > mojo/services/html_viewer/html_document_view.cc:105: > > > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > > > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > > > On 2014/12/03 16:56:53, qsr wrote: > > > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > > > What deletes this class? > > > > > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, so > > it > > > is > > > > > deleted at the same time as HTMLDocumentView > > > > > > > > > > But I miss a delete this in OnViewDestroyed > > > > > > > > I meant HTMLDocumentView. > > > > > > > > I don't think OnViewDestroyed is the right place to delete this. It is > > > possible > > > > to create an HTMLDocumentView, but never have any associated view. In that > > > case > > > > OnViewDestroyed won't ever be called. > > > > > > Hum, what would be the right signal? > > > > Probably the most reasonable thing for now is to quit when the SP that is > passed > > to Application::AcceptConnection() dies. We should probably make it an > official > > part of Shell::ConnectToApplication() that the SP passed there controls the > > lifetime of the created application. > > Also, for the current time this will be hard. There is no way to register a > error handler on a ServiceProviderImpl. I'll add a CL to allow this, but we will > need to wait that it is roll in chromium. > > Do you want to commit in the current state (that works with our current > embedder, as they will always embed just after connecting), or do you prefer to > wait? There are Google-internal clients right now that never embed. I thought about this some more, and as crazy as it probably sounds, my preference is to just not bother with any shutdown code in Mojo right now. I think somebody needs to take it as a goal to go investigate how shutdown and IPC work in other platforms and propose something for Mojo. This is going to be a pretty big project, and it doesn't seem like every app trying to do something halfway is a good approach. So I recommend just deleting the shutdown code entirely.
On 2014/12/05 21:24:10, Aaron Boodman wrote: > On 2014/12/05 13:02:08, qsr wrote: > > On 2014/12/04 18:40:19, Aaron Boodman wrote: > > > On 2014/12/04 10:11:17, qsr wrote: > > > > > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > > File mojo/services/html_viewer/html_document_view.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/776553003/diff/1/mojo/services/html_viewer/ht... > > > > mojo/services/html_viewer/html_document_view.cc:105: > > > > WeakBindToPipe(&exported_services_, provider.PassMessagePipe()); > > > > On 2014/12/03 17:32:37, Aaron Boodman wrote: > > > > > On 2014/12/03 16:56:53, qsr wrote: > > > > > > On 2014/12/03 16:42:11, Aaron Boodman wrote: > > > > > > > What deletes this class? > > > > > > > > > > > > Which class? exported_services_ is a member of this HTMLDocumentView, > so > > > it > > > > is > > > > > > deleted at the same time as HTMLDocumentView > > > > > > > > > > > > But I miss a delete this in OnViewDestroyed > > > > > > > > > > I meant HTMLDocumentView. > > > > > > > > > > I don't think OnViewDestroyed is the right place to delete this. It is > > > > possible > > > > > to create an HTMLDocumentView, but never have any associated view. In > that > > > > case > > > > > OnViewDestroyed won't ever be called. > > > > > > > > Hum, what would be the right signal? > > > > > > Probably the most reasonable thing for now is to quit when the SP that is > > passed > > > to Application::AcceptConnection() dies. We should probably make it an > > official > > > part of Shell::ConnectToApplication() that the SP passed there controls the > > > lifetime of the created application. > > > > Also, for the current time this will be hard. There is no way to register a > > error handler on a ServiceProviderImpl. I'll add a CL to allow this, but we > will > > need to wait that it is roll in chromium. > > > > Do you want to commit in the current state (that works with our current > > embedder, as they will always embed just after connecting), or do you prefer > to > > wait? > > There are Google-internal clients right now that never embed. > > I thought about this some more, and as crazy as it probably sounds, my > preference is to just not bother with any shutdown code in Mojo right now. > > I think somebody needs to take it as a goal to go investigate how shutdown and > IPC work in other platforms and propose something for Mojo. This is going to be > a pretty big project, and it doesn't seem like every app trying to do something > halfway is a good approach. > > So I recommend just deleting the shutdown code entirely. Ok, PTAL, but just to be sure, what will msan says when we plug it in?
gentle ping?
https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... File mojo/services/html_viewer/html_document.h (right): https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... mojo/services/html_viewer/html_document.h:10: #include "base/callback.h" unused https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... File mojo/services/html_viewer/html_viewer.cc (right): https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... mojo/services/html_viewer/html_viewer.cc:91: new HTMLDocument(provider.Pass(), response.Pass(), shell_.get(), Now, HTMLDocument can live past the lifetime of shell_ and web_media_player_factory_, right?
lgtm to unblock, i'm out for a week.
On 2014/12/10 08:54:29, Aaron Boodman wrote: > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > File mojo/services/html_viewer/html_document.h (right): > > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > mojo/services/html_viewer/html_document.h:10: #include "base/callback.h" > unused > > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > File mojo/services/html_viewer/html_viewer.cc (right): > > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > mojo/services/html_viewer/html_viewer.cc:91: new HTMLDocument(provider.Pass(), > response.Pass(), shell_.get(), > Now, HTMLDocument can live past the lifetime of shell_ and > web_media_player_factory_, right? It cannot outlive shell_, as the HTMLViewer never dies. It can outlive web_media_player_factory_ in the sense that it leaks, but when web_media_player_factory_ dies, it means the application dies, so all the thread dies, and the .so is unloaded, so I don't think it will be able to call anything on it.
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776553003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7221cc36c54297f3d9c435e4e83c6a3d78ea1171 Cr-Commit-Position: refs/heads/master@{#307666}
Message was sent while issue was closed.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... File mojo/services/html_viewer/html_document.cc (right): https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... mojo/services/html_viewer/html_document.cc:124: Load(response_.Pass()); This is breaking my local test. In my chromium checkout, if I run out/GN/mojo_shell <URL> OnEmbed() is not called. If I move Load(response_.Pass()) to HTMLDocument::HTMLDocument() my test is working. I don't fully understand this code. I don't have the html viewer window loaded in my chromium checkout (due to missing services). Is this a possible cause that OnEmbed() is not called?
Message was sent while issue was closed.
On 2014/12/12 01:50:14, xhwang wrote: > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > File mojo/services/html_viewer/html_document.cc (right): > > https://codereview.chromium.org/776553003/diff/40001/mojo/services/html_viewe... > mojo/services/html_viewer/html_document.cc:124: Load(response_.Pass()); > This is breaking my local test. In my chromium checkout, if I run > > out/GN/mojo_shell <URL> > > OnEmbed() is not called. If I move Load(response_.Pass()) to > HTMLDocument::HTMLDocument() my test is working. > > I don't fully understand this code. I don't have the html viewer window loaded > in my chromium checkout (due to missing services). Is this a possible cause that > OnEmbed() is not called? FWIW: this "fix" works for me https://codereview.chromium.org/796963003/, but I am pretty sure I am missing the big picture... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
