Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787703002:
Add mojo service for CopylessPaste in Blink (Closed)
|
Created:
3 years, 8 months ago by wychen Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, kinuko+watch, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd mojo service for CopylessPaste in Blink
A CopylessPaste mojo service in Blink is added. The parsed data would
be forwarded to the app indexing service.
BUG=693650
Review-Url: https://codereview.chromium.org/2787703002
Cr-Commit-Position: refs/heads/master@{#462826}
Committed: https://chromium.googlesource.com/chromium/src/+/0be0df4f013607e35411a085511f7accfef26bbb
Patch Set 1 #
Total comments: 2
Patch Set 2 : singlular #
Total comments: 2
Patch Set 3 : rebase across https://codereview.chromium.org/2783543004 #
Total comments: 5
Patch Set 4 : makeUnique #
Total comments: 11
Patch Set 5 : rebase, separate data type to another CL #Patch Set 6 : rebase #Patch Set 7 : return null if no results #Patch Set 8 : only main frame #
Total comments: 3
Patch Set 9 : comment #Patch Set 10 : add todo #Patch Set 11 : rebase #Patch Set 12 : update to new mojo namespace #
Messages
Total messages: 47 (15 generated)
wychen@chromium.org changed reviewers: + dcheng@chromium.org, dproctor@google.com, esprehn@chromium.org
PTAL. This is just the mojo interface. The parsing logic (https://codereview.chromium.org/2777623002/) would be revised according to the data structure here.
https://codereview.chromium.org/2787703002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/document_metadata/copyless_paste.mojom (right): https://codereview.chromium.org/2787703002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/document_metadata/copyless_paste.mojom:14: struct Properties { nit: Property?
https://codereview.chromium.org/2787703002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/document_metadata/copyless_paste.mojom (right): https://codereview.chromium.org/2787703002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/document_metadata/copyless_paste.mojom:14: struct Properties { On 2017/03/30 17:21:31, dproctor wrote: > nit: Property? Done.
dglazkov@chromium.org changed reviewers: + dglazkov@chromium.org
https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h (right): https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:26: WeakPersistent<LocalFrame> m_frame; Could you help me understand why WeakPersistent is used here?
https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h (right): https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:26: WeakPersistent<LocalFrame> m_frame; On 2017/03/30 17:52:50, dglazkov wrote: > Could you help me understand why WeakPersistent is used here? I followed other similar examples like: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/instal... https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/app_ba... To be honest I don't understand this pattern very well.
On 2017/03/30 at 18:00:30, wychen wrote: > https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h (right): > > https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:26: WeakPersistent<LocalFrame> m_frame; > On 2017/03/30 17:52:50, dglazkov wrote: > > Could you help me understand why WeakPersistent is used here? > > I followed other similar examples like: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/instal... > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/app_ba... > To be honest I don't understand this pattern very well. I looked into this, and I think I understand. It's a non-Oilpan class holding a reference to Oilpan object, and the lifetime of CopylessPasteServer is guaranteed to be at least span the Frame. Does that make sense to you? Could you double-check my thinking?
On 2017/03/30 19:20:39, dglazkov wrote: > On 2017/03/30 at 18:00:30, wychen wrote: > > > https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h > (right): > > > > > https://codereview.chromium.org/2787703002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:26: > WeakPersistent<LocalFrame> m_frame; > > On 2017/03/30 17:52:50, dglazkov wrote: > > > Could you help me understand why WeakPersistent is used here? > > > > I followed other similar examples like: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/instal... > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/app_ba... > > To be honest I don't understand this pattern very well. > > I looked into this, and I think I understand. It's a non-Oilpan class holding a > reference to Oilpan object, and the lifetime of CopylessPasteServer is > guaranteed to be at least span the Frame. Does that make sense to you? Could you > double-check my thinking? Thanks for explaining! This makes sense.
lgtm
Description was changed from ========== Add mojo service for CopylessPaste A CopylessPaste mojo service in Blink is added. The parsed data would be forwarded to the app indexing service. BUG=693650 ========== to ========== Add mojo service for CopylessPaste in Blink A CopylessPaste mojo service in Blink is added. The parsed data would be forwarded to the app indexing service. BUG=693650 ==========
https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: mojo::MakeStrongBinding(WTF::wrapUnique(new CopylessPasteServer(*frame)), Nit: WTF::makeUnique? https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:24: void CopylessPasteServer::GetEntities(const GetEntitiesCallback& callback) { What's the expected lifetime of this service? Is it supposed to persist between navigations? (Most Mojo things in Blink subclass ContextLifecycleObserver and close the binding on contextDestroyed() -- should we be doing that here?)
https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:24: void CopylessPasteServer::GetEntities(const GetEntitiesCallback& callback) { On 2017/03/30 21:21:00, dcheng wrote: > What's the expected lifetime of this service? Is it supposed to persist between > navigations? > > (Most Mojo things in Blink subclass ContextLifecycleObserver and close the > binding on contextDestroyed() -- should we be doing that here?) This service is stateless. When getting a request (happens at onLoad), it would scan the DOM and return some metadata. I think it should work either way.
On 2017/03/30 21:33:28, wychen wrote: > https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp > (right): > > https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:24: > void CopylessPasteServer::GetEntities(const GetEntitiesCallback& callback) { > On 2017/03/30 21:21:00, dcheng wrote: > > What's the expected lifetime of this service? Is it supposed to persist > between > > navigations? > > > > (Most Mojo things in Blink subclass ContextLifecycleObserver and close the > > binding on contextDestroyed() -- should we be doing that here?) > > This service is stateless. When getting a request (happens at onLoad), it would > scan the DOM and return some metadata. I think it should work either way. What about potential IPC races? Is it possible for us to query this as a navigation is committing and return the wrong state?
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:15: void CopylessPasteServer::bindMojoRequest( Would you help me understand why you adopted the bindMojoRequest pattern? I'd like to understand how CopylessPasteServer is going to be used in the browser side. https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:25: if (!m_frame || !m_frame->document()) { Instead of doing this, I'd prefer making CopylessPasteServer a ContextClient. Then you can check if the document has been detached or not by checking getExecutionContext().
On 2017/03/30 22:14:50, dcheng wrote: > On 2017/03/30 21:33:28, wychen wrote: > > > https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... > > File > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp > > (right): > > > > > https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:24: > > void CopylessPasteServer::GetEntities(const GetEntitiesCallback& callback) { > > On 2017/03/30 21:21:00, dcheng wrote: > > > What's the expected lifetime of this service? Is it supposed to persist > > between > > > navigations? > > > > > > (Most Mojo things in Blink subclass ContextLifecycleObserver and close the > > > binding on contextDestroyed() -- should we be doing that here?) > > > > This service is stateless. When getting a request (happens at onLoad), it > would > > scan the DOM and return some metadata. I think it should work either way. > > What about potential IPC races? Is it possible for us to query this as a > navigation is committing and return the wrong state? The returned metadata is tagged with the URL, so it won't be mixed with other entries in the navigation history. There's no guarantee that the DOM would be static after loading, but practically Tab.onPageLoadFinished() should be good enough to get a consistent result. The parsing logic is also fast enough so that racing condition is unlikely to happen. Is this the IPC races you are talking about? I feel I still don't fully understand your concerns. Could this IPC race be avoided by "pushing" from Blink like https://codereview.chromium.org/2709893002/?
https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: mojo::MakeStrongBinding(WTF::wrapUnique(new CopylessPasteServer(*frame)), On 2017/03/30 21:21:00, dcheng wrote: > Nit: WTF::makeUnique? Done. https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:15: void CopylessPasteServer::bindMojoRequest( On 2017/03/31 04:20:09, haraken wrote: > > Would you help me understand why you adopted the bindMojoRequest pattern? I'd > like to understand how CopylessPasteServer is going to be used in the browser > side. For all the visited pages, we want to collect some metadata on the Java side. The Java side would call the Mojo server in Blink at Tab.onPageLoadFinished(), and forward the gathered metadata to a system service. I guess the bindMojoRequest pattern is most suitable for Mojo services that require its life time persisting across navigations, right? https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:25: if (!m_frame || !m_frame->document()) { On 2017/03/31 04:20:09, haraken wrote: > > Instead of doing this, I'd prefer making CopylessPasteServer a ContextClient. > Then you can check if the document has been detached or not by checking > getExecutionContext(). > I'm still trying to figure out whether it makes more sense to put the Mojo server in Blink or in the Java side. Do you think the data flow in the following CL makes more sense? https://codereview.chromium.org/2709893002/
https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/BUILD.gn:705: "platform/modules/document_metadata/copyless_paste.mojom", I think you want to put this instead in android_mojo_bindings below, so that it is visible in clank?
https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/BUILD.gn:705: "platform/modules/document_metadata/copyless_paste.mojom", On 2017/03/31 23:18:14, dproctor wrote: > I think you want to put this instead in android_mojo_bindings below, so that it > is visible in clank? It's still visible in clank. See https://cs.chromium.org/chromium/src/chrome/android/BUILD.gn?type=cs&q=public... And it worked in https://codereview.chromium.org/2700193002/. I'll move it below since it looks better.
https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/BUILD.gn:705: "platform/modules/document_metadata/copyless_paste.mojom", On 2017/04/01 05:16:00, wychen wrote: > On 2017/03/31 23:18:14, dproctor wrote: > > I think you want to put this instead in android_mojo_bindings below, so that > it > > is visible in clank? > > It's still visible in clank. > See > https://cs.chromium.org/chromium/src/chrome/android/BUILD.gn?type=cs&q=public... > And it worked in https://codereview.chromium.org/2700193002/. > > I'll move it below since it looks better. Ah, sorry for the confusion, but the internal clank only imports //third_party/WebKit/public:android_mojo_bindings_java.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:15: void CopylessPasteServer::bindMojoRequest( On 2017/03/31 07:51:54, wychen wrote: > On 2017/03/31 04:20:09, haraken wrote: > > > > Would you help me understand why you adopted the bindMojoRequest pattern? I'd > > like to understand how CopylessPasteServer is going to be used in the browser > > side. > > For all the visited pages, we want to collect some metadata on the Java side. > The Java side would call the Mojo server in Blink at Tab.onPageLoadFinished(), > and forward the gathered metadata to a system service. > > I guess the bindMojoRequest pattern is most suitable for Mojo services that > require its life time persisting across navigations, right? Makes sense. https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:25: if (!m_frame || !m_frame->document()) { On 2017/03/31 07:51:54, wychen wrote: > On 2017/03/31 04:20:09, haraken wrote: > > > > Instead of doing this, I'd prefer making CopylessPasteServer a ContextClient. > > Then you can check if the document has been detached or not by checking > > getExecutionContext(). > > > > I'm still trying to figure out whether it makes more sense to put the Mojo > server in Blink or in the Java side. > Do you think the data flow in the following CL makes more sense? > https://codereview.chromium.org/2709893002/ I'd prefer this CL. Here I'm just asking making CopylessPasteServer inherit from ContextClient, so that it doesn't need to hold m_frame.
https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:24: void CopylessPasteServer::GetEntities(const GetEntitiesCallback& callback) { On 2017/03/30 21:33:28, wychen wrote: > On 2017/03/30 21:21:00, dcheng wrote: > > What's the expected lifetime of this service? Is it supposed to persist > between > > navigations? > > > > (Most Mojo things in Blink subclass ContextLifecycleObserver and close the > > binding on contextDestroyed() -- should we be doing that here?) > > This service is stateless. When getting a request (happens at onLoad), it would > scan the DOM and return some metadata. I think it should work either way. Most of the Mojo things in Blink are Mojo clients, not Mojo servers. Mojo clients for JavaScript should terminate when the context is gone. In this case, it's a Mojo server, so I think it is not necessary for it to self-destruct when the context is gone. https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:25: if (!m_frame || !m_frame->document()) { On 2017/04/04 06:27:57, haraken wrote: > On 2017/03/31 07:51:54, wychen wrote: > > On 2017/03/31 04:20:09, haraken wrote: > > > > > > Instead of doing this, I'd prefer making CopylessPasteServer a > ContextClient. > > > Then you can check if the document has been detached or not by checking > > > getExecutionContext(). > > > > > > > I'm still trying to figure out whether it makes more sense to put the Mojo > > server in Blink or in the Java side. > > Do you think the data flow in the following CL makes more sense? > > https://codereview.chromium.org/2709893002/ > > I'd prefer this CL. > > Here I'm just asking making CopylessPasteServer inherit from ContextClient, so > that it doesn't need to hold m_frame. I gave it a try using ContextClient, but it seems garbage collection and mojo isn't particularly compatible, since the binding process requires unique_ptr. https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2787703002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/BUILD.gn:705: "platform/modules/document_metadata/copyless_paste.mojom", On 2017/04/03 17:43:10, dproctor wrote: > On 2017/04/01 05:16:00, wychen wrote: > > On 2017/03/31 23:18:14, dproctor wrote: > > > I think you want to put this instead in android_mojo_bindings below, so that > > it > > > is visible in clank? > > > > It's still visible in clank. > > See > > > https://cs.chromium.org/chromium/src/chrome/android/BUILD.gn?type=cs&q=public... > > And it worked in https://codereview.chromium.org/2700193002/. > > > > I'll move it below since it looks better. > > Ah, sorry for the confusion, but the internal clank only imports > //third_party/WebKit/public:android_mojo_bindings_java. Done.
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
How does this look now? ContextClient doesn't seem compatible in our use case.
On 2017/04/05 23:27:59, wychen wrote: > How does this look now? ContextClient doesn't seem compatible in our use case. Yeah, makes sense. It's fair to use WeakPersistent<LocalFrame> at the moment. LGTM. https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h (right): https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:16: class MODULES_EXPORT CopylessPasteServer final Add a class-level comment.
https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp (right): https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: mojo::MakeStrongBinding(WTF::makeUnique<CopylessPasteServer>(*frame), To be honest, I'm not convinced that we should be using MakeStrongBinding in Blink. It seems like we should have a Blink equivalent for automatically creating frame-lifetime-associated services. Then we don't need code like line 25-28. Is this something you would be willing to investigate in a followup?
On 2017/04/06 05:51:16, dcheng wrote: > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp > (right): > > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: > mojo::MakeStrongBinding(WTF::makeUnique<CopylessPasteServer>(*frame), > To be honest, I'm not convinced that we should be using MakeStrongBinding in > Blink. It seems like we should have a Blink equivalent for automatically > creating frame-lifetime-associated services. Then we don't need code like line > 25-28. > > Is this something you would be willing to investigate in a followup? If we can do that, that should be nicer. I want to remove the bindMojoRequest pattern in AppBannerController and InstallationServiceImpl.
On 2017/04/06 06:18:27, haraken wrote: > On 2017/04/06 05:51:16, dcheng wrote: > > > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > > File > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp > > (right): > > > > > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: > > mojo::MakeStrongBinding(WTF::makeUnique<CopylessPasteServer>(*frame), > > To be honest, I'm not convinced that we should be using MakeStrongBinding in > > Blink. It seems like we should have a Blink equivalent for automatically > > creating frame-lifetime-associated services. Then we don't need code like line > > 25-28. > > > > Is this something you would be willing to investigate in a followup? > > If we can do that, that should be nicer. I want to remove the bindMojoRequest > pattern in AppBannerController and InstallationServiceImpl. I can follow up on how to do this, but I'll need more guidance from you. The only references I can find in Blink are AppBannerController and InstallationServiceImpl, and that's why I followed their pattern.
https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h (right): https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.h:16: class MODULES_EXPORT CopylessPasteServer final On 2017/04/06 02:26:32, haraken wrote: > > Add a class-level comment. I think the coding style in Blink was more like preferring "self-explanatory" code than comments. Neither one of InstallationServiceImpl and AppBannerController have class-level comments. Looks like the new coding style does require class-level comment. Thanks for pointing that out. Done.
On 2017/04/06 at 06:18:27, haraken wrote: > On 2017/04/06 05:51:16, dcheng wrote: > > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp > > (right): > > > > https://codereview.chromium.org/2787703002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/document_metadata/CopylessPasteServer.cpp:20: > > mojo::MakeStrongBinding(WTF::makeUnique<CopylessPasteServer>(*frame), > > To be honest, I'm not convinced that we should be using MakeStrongBinding in > > Blink. It seems like we should have a Blink equivalent for automatically > > creating frame-lifetime-associated services. Then we don't need code like line > > 25-28. > > > > Is this something you would be willing to investigate in a followup? > > If we can do that, that should be nicer. I want to remove the bindMojoRequest pattern in AppBannerController and InstallationServiceImpl. In the long term, would something like Web Agents be the answer to these types of concerns?
I've added a todo to remove bindMojoRequest pattern. dcheng@, before we have a way to introduce frame-lifetime services, does this CL look good?
LGTM
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dglazkov@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2787703002/#ps200001 (title: "add todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #11 (id:220001) has been deleted
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dglazkov@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2787703002/#ps260001 (title: "update to new mojo namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1491552589161000,
"parent_rev": "986188a907c91d38938e24604f7b4397fdaaff75", "commit_rev":
"0be0df4f013607e35411a085511f7accfef26bbb"}
Message was sent while issue was closed.
Description was changed from ========== Add mojo service for CopylessPaste in Blink A CopylessPaste mojo service in Blink is added. The parsed data would be forwarded to the app indexing service. BUG=693650 ========== to ========== Add mojo service for CopylessPaste in Blink A CopylessPaste mojo service in Blink is added. The parsed data would be forwarded to the app indexing service. BUG=693650 Review-Url: https://codereview.chromium.org/2787703002 Cr-Commit-Position: refs/heads/master@{#462826} Committed: https://chromium.googlesource.com/chromium/src/+/0be0df4f013607e35411a085511f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as https://chromium.googlesource.com/chromium/src/+/0be0df4f013607e35411a085511f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
