Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737893002:
Mojoify the RequestThumbnailForContextNode IPC message and reply (Closed)
|
Created:
3 years, 9 months ago by watk Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, ajwong+watch_chromium.org, Avi (use Gerrit), chromium-reviews, creis+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org, tibell, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojoify the RequestThumbnailForContextNode IPC message and reply
Previously thumbnail requests for context nodes were done with two IPC
messages: ChromeViewMsg_RequestThumbnailForContextNode &
ChromeViewHostMsg_RequestThumbnailForContextNode_ACK.
This CL introduces a new per-frame mojo interface, ThumbnailCapturer,
that's exposed by the renderer to the browser.
BUG=699368
TEST=existing browsertest, manually tested on linux, mac, android
Review-Url: https://codereview.chromium.org/2737893002
Cr-Commit-Position: refs/heads/master@{#456632}
Committed: https://chromium.googlesource.com/chromium/src/+/2c472009300a73512a5ace3543e2eec9e2cdd5d9
Patch Set 1 #
Total comments: 3
Patch Set 2 : Bind the callback to weak ptrs #
Total comments: 3
Patch Set 3 : Remove watcher; use ContextMenuWaiter #
Total comments: 6
Patch Set 4 : sammc's comments #Patch Set 5 : rebase & pull out raw proxy for making calls #
Total comments: 4
Patch Set 6 : change swap() call and rebase #Messages
Total messages: 56 (36 generated)
watk@chromium.org changed reviewers: + sammc@chromium.org
I'm sending this out a bit early for feedback on how idiomatic it is. I still have to update a test and manually test this on Android. https://codereview.chromium.org/2737893002/diff/1/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/2737893002/diff/1/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/core_tab_helper.cc:127: thumbnail_capturers_.Add(base::WrapUnique(thumbnail_capturer)); WDYT about this? I don't like that it can leak InterfacePtrs because they're only removed when the response comes back. On the other hand the old code could leak callbacks, so I don't think it's worse. I think the ideal API would be to pass ownership of the InterfacePtr to the callback. And use a weak_ptr to allow the callback to outlive CoreTabHelper. But it's maybe more subtle. The other advantage is that we could remove this function and have the two callsites use the mojo interface directly.
tibell@chromium.org changed reviewers: + tibell@chromium.org
https://codereview.chromium.org/2737893002/diff/1/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/2737893002/diff/1/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/core_tab_helper.cc:127: thumbnail_capturers_.Add(base::WrapUnique(thumbnail_capturer)); On 2017/03/08 03:48:51, watk wrote: > WDYT about this? I don't like that it can leak InterfacePtrs because they're > only removed when the response comes back. On the other hand the old code could > leak callbacks, so I don't think it's worse. > > I think the ideal API would be to pass ownership of the InterfacePtr to the > callback. And use a weak_ptr to allow the callback to outlive CoreTabHelper. > > But it's maybe more subtle. > > The other advantage is that we could remove this function and have the two > callsites use the mojo interface directly. As discussed offline. Lets try the weak ptr approach. https://codereview.chromium.org/2737893002/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_frame_observer.h (right): https://codereview.chromium.org/2737893002/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_frame_observer.h:49: void RequestThumbnailForContextNode( We typically add a // chrome::mojom::ThumbnailCapturer: comment before the overrides.
Ok, updated to use weak pointers. It's pretty clean.. The only weird thing is that the response callbacks have to accept an InterfacePtr, but do nothing with it, just so we can bind it into the Callback. It would be handy to have some kind of wrapper that would let you bind state into the callback without passing it to the target. I also updated the test.
The CQ bit was checked by watk@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.cc:162: base::Passed(&thumbnail_capturer))); This move is probably undefined behavior since you're calling methods on thumbnail_capturer after moving. Take a raw pointer to it first and use that for the call and keep the move as is. https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/tab_c... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/tab_c... chrome/browser/ui/tab_contents/core_tab_helper.cc:119: weak_factory_.GetWeakPtr(), base::Passed(&thumbnail_capturer), Ditto
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by watk@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...
Description was changed from ========== Convert the RequestThumbnailForContextNode IPC message and response to mojo Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 ========== to ========== Convert the RequestThumbnailForContextNode IPC message and response to mojo Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=exiting browsertest, manually tested on linux, mac, android ==========
Thanks, the test was broken on mac so I modified it a little bit and managed to delete a bunch of helper code. https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2737893002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.cc:162: base::Passed(&thumbnail_capturer))); On 2017/03/09 02:21:49, tibell wrote: > This move is probably undefined behavior since you're calling methods on > thumbnail_capturer after moving. Take a raw pointer to it first and use that for > the call and keep the move as is. Discussed offline. Looks like this is guaranteed to work by the standard. The expression naming the function is sequenced before the arguments are evaluated. But it relies on the move constructor not changing the address of the underlying proxy object.
Description was changed from ========== Convert the RequestThumbnailForContextNode IPC message and response to mojo Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=exiting browsertest, manually tested on linux, mac, android ========== to ========== Convert the RequestThumbnailForContextNode IPC message and response to mojo Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=existing browsertest, manually tested on linux, mac, android ==========
lgtm
https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:656: auto callback = [](bool* response_received, base::RunLoop* run_loop, Pass in the quit closure instead of the run loop itself. https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:667: base::Bind(callback, base::Unretained(&response_received), Is Unretained needed here? https://codereview.chromium.org/2737893002/diff/40001/chrome/common/thumbnail... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2737893002/diff/40001/chrome/common/thumbnail... chrome/common/thumbnail_capturer.mojom:17: => (string thumbnail_data, gfx.mojom.Size original_size); Use array<uint8> for data. Mention that it's JPEG-encoded somewhere too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, adding owners: dcheng@ PTAL at: chrome/browser/chrome_content_renderer_manifest_overlay.json chrome/common/render_messages.h chrome/common/thumbnail_capturer.mojom jam@ PTAL at everything as a chrome/ owner. https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:656: auto callback = [](bool* response_received, base::RunLoop* run_loop, On 2017/03/09 04:17:13, Sam McNally wrote: > Pass in the quit closure instead of the run loop itself. Done. https://codereview.chromium.org/2737893002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:667: base::Bind(callback, base::Unretained(&response_received), On 2017/03/09 04:17:13, Sam McNally wrote: > Is Unretained needed here? Nope, TIL Bind must only care about the receiver for member functions being annotated https://codereview.chromium.org/2737893002/diff/40001/chrome/common/thumbnail... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2737893002/diff/40001/chrome/common/thumbnail... chrome/common/thumbnail_capturer.mojom:17: => (string thumbnail_data, gfx.mojom.Size original_size); On 2017/03/09 04:17:13, Sam McNally wrote: > Use array<uint8> for data. Mention that it's JPEG-encoded somewhere too. Done.
watk@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org
Oops, forgot to add reviewers. Please see the previous message.
Description was changed from ========== Convert the RequestThumbnailForContextNode IPC message and response to mojo Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=existing browsertest, manually tested on linux, mac, android ========== to ========== Mojoify the RequestThumbnailForContextNode IPC message and reply Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=existing browsertest, manually tested on linux, mac, android ==========
The CQ bit was checked by watk@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/09 06:38:41, watk wrote: > Thanks, adding owners: > > dcheng@ PTAL at: > chrome/browser/chrome_content_renderer_manifest_overlay.json > chrome/common/render_messages.h > chrome/common/thumbnail_capturer.mojom > > jam@ PTAL at everything as a chrome/ owner. please pick an owner from chrome/owners
watk@chromium.org changed reviewers: + jochen@chromium.org - jam@chromium.org
Ah, thought jam@ was a chrome/ owner. jochen@ PTAL
lgtm
Oops, I was reading a new c++ standard to conclude that the "call member function on interface ptr and move it into the function" pattern is safe. c++11 has no such guarantee, so as written this was undefined behavior. The latest patchset fixes it. Also, friendly ping dcheng@
The CQ bit was checked by watk@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2737893002/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2737893002/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:160: // there's either a connection error or a response. Hmm... how come we need to write this? Similar code doesn't seem to: https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... https://codereview.chromium.org/2737893002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2737893002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_render_frame_observer.cc:222: std::swap(thumbnail_data, data); The standard mechanism for using swap is: using std::swap; swap(thumbnail_data, data); This ensures that you get the efficient swap() overload for the type (if defined). In this case, it doesn't matter, because std::vector is in namespace std, but it's probably best to use the standard pattern. Alternatively, just write: thumbnail_data.swap(data); And avoid this entire question =)
https://codereview.chromium.org/2737893002/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2737893002/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:160: // there's either a connection error or a response. On 2017/03/13 07:49:29, dcheng wrote: > Hmm... how come we need to write this? Similar code doesn't seem to: > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... The one you linked to doesn't have a reply callback. So it's fine to delete it as soon as you make the call because AFAIK the lifetime of the outgoing message isn't tied to the InterfacePtr. In this case we have a reply callback, which will not run after the InterfacePtr is deleted, so we need to keep it alive until the callback has run (or there's a connection error). Moving it into the callback is clean because it saves us from having to maintain any state (a collection of InterfacePtrs with pending replies). https://codereview.chromium.org/2737893002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2737893002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_render_frame_observer.cc:222: std::swap(thumbnail_data, data); On 2017/03/13 07:49:29, dcheng wrote: > The standard mechanism for using swap is: > > using std::swap; > swap(thumbnail_data, data); > > This ensures that you get the efficient swap() overload for the type (if > defined). In this case, it doesn't matter, because std::vector is in namespace > std, but it's probably best to use the standard pattern. > > Alternatively, just write: > > thumbnail_data.swap(data); > > And avoid this entire question =) Ah thanks, I wasn't aware. I just like how the nonmember function looks :P Fixed.
The CQ bit was checked by watk@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by watk@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM The pattern feels a bit odd... if this shows up more, maybe it's worth having a one-shot helper. (We previously had this debate for whether or not we should have a lazy connection helper... currently, we've decided not to, but perhaps it'd be useful to have these abstractions for documentation purposes...)
On 2017/03/14 05:16:44, dcheng wrote: > LGTM > > The pattern feels a bit odd... if this shows up more, maybe it's worth having a > one-shot helper. > > (We previously had this debate for whether or not we should have a lazy > connection helper... currently, we've decided not to, but perhaps it'd be useful > to have these abstractions for documentation purposes...) Thanks. I think a helper makes sense but I'm not sure if anyone else has wanted to do this before. (sammc@ might have said he's seen it.)
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tibell@chromium.org, sammc@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2737893002/#ps120001 (title: "change swap() call and rebase")
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": 120001, "attempt_start_ts": 1489470462992410,
"parent_rev": "5d30f77bb7b8a403b61a09dbe1d8d896f1fd232d", "commit_rev":
"2c472009300a73512a5ace3543e2eec9e2cdd5d9"}
Message was sent while issue was closed.
Description was changed from ========== Mojoify the RequestThumbnailForContextNode IPC message and reply Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=existing browsertest, manually tested on linux, mac, android ========== to ========== Mojoify the RequestThumbnailForContextNode IPC message and reply Previously thumbnail requests for context nodes were done with two IPC messages: ChromeViewMsg_RequestThumbnailForContextNode & ChromeViewHostMsg_RequestThumbnailForContextNode_ACK. This CL introduces a new per-frame mojo interface, ThumbnailCapturer, that's exposed by the renderer to the browser. BUG=699368 TEST=existing browsertest, manually tested on linux, mac, android Review-Url: https://codereview.chromium.org/2737893002 Cr-Commit-Position: refs/heads/master@{#456632} Committed: https://chromium.googlesource.com/chromium/src/+/2c472009300a73512a5ace3543e2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2c472009300a73512a5ace3543e2... |
