Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
518693002:
Fix a crash when saving a <canvas> or <img> image which is large. (Closed)
|
Created:
6 years, 3 months ago by zino Modified:
6 years, 3 months ago Reviewers:
Ken Russell (switch to Gerrit), Avi (use Gerrit), jamesr, Justin Novosad, jln (very slow on Chromium), palmer CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix a crash when saving a <canvas> or <img> image which is large.
The problem is caused by size-limits of GURL. (the limit < 2MiB)
We can use string of data url instead of GURL to avoid the size-limits.
For this end, added a new method to Blink side:
https://crrev.com/518043003
BUG=69227, 401888
Committed: https://crrev.com/632a1a7256d3aada4c2deefb103cac2ca4fc56d9
Cr-Commit-Position: refs/heads/master@{#295405}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Total comments: 7
Patch Set 3 : #Patch Set 4 : #
Total comments: 16
Patch Set 5 : #
Total comments: 6
Patch Set 6 : kbr's comments #Patch Set 7 : CONTENT_EXPORT #
Total comments: 1
Messages
Total messages: 44 (14 generated)
Please take a look. I tried to make a auto test but I don't know how I can do it.
Not an OWNER, but I think this is fine. You should add test though. https://codereview.chromium.org/518693002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/518693002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:1792: request.url().string().utf8(), So you are just passing the URL as a string to work around the url size limit? It would be more efficient to pass the binary version of the encoded image, rather than the data url/base64 version. If that would be a lot more complicated, then I think this is Okay. This isn't really a performance-critical area.
kbr@chromium.org changed reviewers: + jamesr@chromium.org, jln@chromium.org, palmer@chromium.org
palmer/jln: security review please. jamesr: content/renderer/ review please. I think this approach is the best one all things considered, but needs another check. Also, yes, please add a test for this. If it's only feasible to do so as a Blink layout test, then please upload a separate CL containing the new test which will land after this does. https://codereview.chromium.org/518693002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.cc:908: DownloadUrl(render_view_id, GURL(encoded_image_data), This code needs to check the incoming encoded_image_data's size and return immediately if it's larger than some large value, like 50 MB or similar. Since this check is done in exactly one place I think it's fine to define the constant locally in this method. https://codereview.chromium.org/518693002/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/518693002/diff/1/content/common/view_messages... content/common/view_messages.h:1261: // Save image for <canvas>. Please document that |encoded_image_data| is the contents of a data: URL, and that it's represented as a string only to work around size limitations for GURLs in IPC messages. https://codereview.chromium.org/518693002/diff/1/content/common/view_messages... content/common/view_messages.h:1264: std::string /* encoded_image_data */, Some guard against DOS'ing the browser process is needed here. Using a GURL in the IPC message imposes the limit content::GetMaxURLChars(). See common_param_traits.cc. I think doing the PNG and base64 encoding in the renderer process, as you're currently doing, is a good idea. This usage just needs to pass security review. Most of the time, using string instead of GURL violates the practices in http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc .
https://codereview.chromium.org/518693002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.cc:908: DownloadUrl(render_view_id, GURL(encoded_image_data), And do something to make sure the |suggested_name| is OK, transforming it if necessary. https://codereview.chromium.org/518693002/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/518693002/diff/1/content/common/view_messages... content/common/view_messages.h:1264: std::string /* encoded_image_data */, Yeah, It's probably better to send the browser process a PNG as a vector<uint8_t> or some kind of IPC-sendable image type, if we have one. And then check a size limit (10 MiB or whatever seems reasonable) before sending, and have the browser side enforce that limit. https://codereview.chromium.org/518693002/diff/1/content/common/view_messages... content/common/view_messages.h:1266: base::string16 /* suggested_name */) Make sure the browser side (a) takes the basename of |suggested_name| (to avoid being tricked into path traversal, e.g. ../../../../...); and (b) somehow transforms it for sanity and uniqueness. https://codereview.chromium.org/518693002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/518693002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:1792: request.url().string().utf8(), I agree, just a byte vector is fine.
https://codereview.chromium.org/518693002/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/518693002/diff/1/content/common/view_messages... content/common/view_messages.h:1264: std::string /* encoded_image_data */, On 2014/08/30 00:37:29, Chromium Palmer wrote: > Yeah, It's probably better to send the browser process a PNG as a > vector<uint8_t> or some kind of IPC-sendable image type, if we have one. And > then check a size limit (10 MiB or whatever seems reasonable) before sending, > and have the browser side enforce that limit. Chris, in this case I think it's better to send the image over as the base64-encoded PNG (the data: URL, essentially), because that way the renderer process runs both the PNG encoder and base64 encoding. Do you agree? Sending an SkBitmap is an option (it's done elsewhere) but again that moves the PNG encoding to the browser process. It would be possible to send the encoded PNG from the renderer, but that would require more surgery in the renderer process to avoid an encode -> decode -> encode step. Do you think this is worth it? Right now the code on both sides wants to deal with the contents of a data: URL.
> Chris, in this case I think it's better to send the image over as the > base64-encoded PNG (the data: URL, essentially), because that way the renderer > process runs both the PNG encoder and base64 encoding. Do you agree? > > Sending an SkBitmap is an option (it's done elsewhere) but again that moves the > PNG encoding to the browser process. > > It would be possible to send the encoded PNG from the renderer, but that would > require more surgery in the renderer process to avoid an encode -> decode -> > encode step. Do you think this is worth it? Right now the code on both sides > wants to deal with the contents of a data: URL. Ah, I see. Yeah, send a (size-limited) string then. Maybe add a sentence of documentation.
Dear all, I probably addressed all your comments and I also fixed a bug when saving a image of <img> whose src attribute has a large data url. Could you please review again? Thank you.
https://codereview.chromium.org/518693002/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/20001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:909: const int MAX_LENGTH_OF_DATA_URL = 1024 * 1024 * 10; This should be a size_t. Note that you are comparing it with a size_t (std::string::length returns size_t). You might even get a compiler warning about comparing signed with unsigned; declaring |MAX_LENGTH_OF_DATA_URL| as size_t will fix that. I still think it would be a good idea to check this limit on the renderer side, too — not as a security mechanism, but as an optimization for not even trying to send messages that the browser would reject. If you do that, you'll need to put this declaration in a place that both pieces of code can #include, to keep the policy consistent. https://codereview.chromium.org/518693002/diff/20001/content/common/view_mess... File content/common/view_messages.h (left): https://codereview.chromium.org/518693002/diff/20001/content/common/view_mess... content/common/view_messages.h:1208: bool /* use prompt for save location */) Why take this out? https://codereview.chromium.org/518693002/diff/20001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/518693002/diff/20001/content/renderer/render_... content/renderer/render_view_impl.cc:1727: const int MAX_LENGTH_OF_DATA_URL = 1024 * 1024 * 10; Yeah, define this in 1 canonical place. The 2 declarations could drift over time and become out of sync.
No further comments for now; the issues seem to be covered. https://codereview.chromium.org/518693002/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1089: return params_.src_url.is_empty() || Can you comment that "is_empty" means canvas? (It does, doesn't it?)
Patchset #3 (id:40001) has been deleted
Thank you for review. Could you take a look again? https://codereview.chromium.org/518693002/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1089: return params_.src_url.is_empty() || On 2014/09/02 22:33:12, Avi wrote: > Can you comment that "is_empty" means canvas? (It does, doesn't it?) I added comments. Thank you. https://codereview.chromium.org/518693002/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/20001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:909: const int MAX_LENGTH_OF_DATA_URL = 1024 * 1024 * 10; On 2014/09/02 17:58:04, Chromium Palmer wrote: > This should be a size_t. Note that you are comparing it with a size_t > (std::string::length returns size_t). You might even get a compiler warning > about comparing signed with unsigned; declaring |MAX_LENGTH_OF_DATA_URL| as > size_t will fix that. A stupid mistake. :-P Done. > > I still think it would be a good idea to check this limit on the renderer side, > too — not as a security mechanism, but as an optimization for not even trying to > send messages that the browser would reject. I agree with you. So, I've just removed the check logic on the browser side. (You don't think we need double checks, do you?) https://codereview.chromium.org/518693002/diff/20001/content/common/view_mess... File content/common/view_messages.h (left): https://codereview.chromium.org/518693002/diff/20001/content/common/view_mess... content/common/view_messages.h:1208: bool /* use prompt for save location */) On 2014/09/02 17:58:04, Chromium Palmer wrote: > Why take this out? The parameter was added in https://crrev.com/263453007/. So we don't need it anymore because I already added new IPC message (ViewHostMsg_SaveImageFromDataURL).
> > I still think it would be a good idea to check this limit on the renderer > side, > > too — not as a security mechanism, but as an optimization for not even trying > to > > send messages that the browser would reject. > > I agree with you. > So, I've just removed the check logic on the browser side. > (You don't think we need double checks, do you?) I'm sorry, I should have been more clear. We absolutely need the check on the browser side — the browser is the process that must defend itself against potentially malicious renderers, and as such it can only trust itself to perform the check correctly. It still makes sense to do the check in renderers, as a performance optimization (don't send messages that the browser will drop; fail early and provide a reasonable error code to the caller). If you have the check in only 1 place, it should be in the browser, but I think it still makes sense to do it on both sides. For general ideas about IPC security, please see: http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
On 2014/09/03 03:00:30, Chromium Palmer wrote: > I'm sorry, I should have been more clear. > > We absolutely need the check on the browser side — the browser is the process > that must defend itself against potentially malicious renderers, and as such it > can only trust itself to perform the check correctly. > > It still makes sense to do the check in renderers, as a performance optimization > (don't send messages that the browser will drop; fail early and provide a > reasonable error code to the caller). > > If you have the check in only 1 place, it should be in the browser, but I think > it still makes sense to do it on both sides. > > For general ideas about IPC security, please see: > > http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc Thank you for detailed explanation :) I've just uploaded a new patch set. Please take a look.
https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1089: // If |params_.src_url| is empty, it might be a large data url. The word "might" here makes me uncomfortable. Two cases: - If src_url is empty, does that mean that it is _always_ a large data URL? If so, fix the comment to avoid "might". - If src_url being empty indeed means that it might be a large data URL but maybe not, is it worth checking? If it is, check. If not, explain in your comment why we shouldn't.
IPC security LGTM. Thanks! But Avi makes a good point; you should probably handle his comment before committing. https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:908: // Note: |data_url| has a size limitation. (< 10MiB) Minor nit: I think the code is so clear (line 910) that this comment is unnecessary.
https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1089: // If |params_.src_url| is empty, it might be a large data url. On 2014/09/03 15:21:05, Avi wrote: > The word "might" here makes me uncomfortable. Two cases: > > - If src_url is empty, does that mean that it is _always_ a large data URL? If > so, fix the comment to avoid "might". > - If src_url being empty indeed means that it might be a large data URL but > maybe not, is it worth checking? If it is, check. If not, explain in your > comment why we shouldn't. Agree with Avi's comments; if the changes I suggest in your other CL https://codereview.chromium.org/518043003/ are made, will that clarify this code path? https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:910: if (data_url.length() < kMaxLengthOfDataURLString) Please see my comments in your other CL https://codereview.chromium.org/518043003/ . After thinking about this more carefully, I think that this should construct the GURL for the incoming data_url and make sure the scheme is actually data:, returning immediately otherwise. Then the old code path should be invoked for images, not this one. I suspect there's a reason that images' download was initiated through ResourceRequest and I don't think that should be changed. https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.h:193: const bool use_prompt = false); Default arguments are not allowed per the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments . Please just pass use_prompt explicitly. https://codereview.chromium.org/518693002/diff/80001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/518693002/diff/80001/content/common/view_mess... content/common/view_messages.h:1203: IPC_MESSAGE_CONTROL4(ViewHostMsg_DownloadUrl, Per other comments, I think this IPC should be left alone. https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:1869: } else if (policy == blink::WebNavigationPolicyDownloadTo) { Per my comments on your other CL, I think this code path should remain. https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... content/renderer/render_view_browsertest.cc:295: TEST_F(RenderViewImplTest, SaveImageFromDataURL) { Nice work on the test. https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:1727: // in order to send a larger data url to save a image for <canvas> or <img>. Assuming the code change I suggest is made, this comment needs to be updated.
Patchset #5 (id:100001) has been deleted
Thank you for review :-) My thinking is.. When popup up a context menu, renderer sends a image url to browser all the time in order to decide whether the menu is enabled or not. (and other many reasons) By the way, it can make a delay because canvas has a large data url. (and heavy encoding operation). So, I added saveImageAt() API and used it for canvas. Then renderer sends the url only if users really click the |save image as| menu. <img> also had the same problem and so I marked TODO two month ago. Currently, <img> doesn't use saveImageAt() API and just use WebContents::saveFrame() to save its image. It is okay in most cases but if the <img> has a large data url, then it makes a trouble. (e.g. we can't currently save this image : https://kangax.github.com/jstests/large_data_uri_image/) So, I think it is better to use the API in that case. <canvas> : use saveImageAt() <img> with a large data url : use saveImageAt() <img> with http, https, and so on : use WebContents::saveFrame() I also updated blink side patch. (added test) https://codereview.chromium.org/518043003/ Could you please review again? https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1089: // If |params_.src_url| is empty, it might be a large data url. Done. It is better to use has_image_contents instead of checking url validation. https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:908: // Note: |data_url| has a size limitation. (< 10MiB) On 2014/09/03 17:31:46, Chromium Palmer wrote: > Minor nit: I think the code is so clear (line 910) that this comment is > unnecessary. Done. https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:910: if (data_url.length() < kMaxLengthOfDataURLString) On 2014/09/04 00:23:26, Ken Russell wrote: > Please see my comments in your other CL > https://codereview.chromium.org/518043003/ . After thinking about this more > carefully, I think that this should construct the GURL for the incoming data_url > and make sure the scheme is actually data:, returning immediately otherwise. It will be return in WebViewImpl::saveImageAt(). (if the url is not data url) In that case, this function is not invoked. > Then the old code path should be invoked for images, not this one. I suspect > there's a reason that images' download was initiated through ResourceRequest and > I don't think that should be changed. This new path is only used by canvas or img to have large data url. https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/518693002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_message_filter.h:193: const bool use_prompt = false); On 2014/09/04 00:23:26, Ken Russell wrote: > Default arguments are not allowed per the style guide. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments > . Please just pass use_prompt explicitly. Done. https://codereview.chromium.org/518693002/diff/80001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/518693002/diff/80001/content/common/view_mess... content/common/view_messages.h:1203: IPC_MESSAGE_CONTROL4(ViewHostMsg_DownloadUrl, On 2014/09/04 00:23:26, Ken Russell wrote: > Per other comments, I think this IPC should be left alone. I don't think we need it. It was needed only for "save canvas" feature but introducing new API, we don't need it anymore. https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:1869: } else if (policy == blink::WebNavigationPolicyDownloadTo) { On 2014/09/04 00:23:26, Ken Russell wrote: > Per my comments on your other CL, I think this code path should remain. ditto. https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/518693002/diff/80001/content/renderer/render_... content/renderer/render_view_browsertest.cc:295: TEST_F(RenderViewImplTest, SaveImageFromDataURL) { On 2014/09/04 00:23:26, Ken Russell wrote: > Nice work on the test. Thanks :)
Thanks for your updates. One issue in the browser side code, then this will look good to me. Also one question. https://codereview.chromium.org/518693002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1348: is_large_data_url)) { nit: fix indentation. https://codereview.chromium.org/518693002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_message_filter.cc:915: DownloadUrl(render_view_id, GURL(data_url), Please construct the GURL in a separate statement and verify that the scheme is "data", returning immediately if not (or if something else went wrong in GURL construction). The browser does not trust the renderer. All assumptions about incoming inputs must be verified. https://codereview.chromium.org/518693002/diff/120001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/518693002/diff/120001/content/renderer/render... content/renderer/render_view_browsertest.cc:340: EXPECT_FALSE(msg4); After more thought, is it possible for you to add a test which sends a non-data: URL to RenderMessageFilter::OnSaveImageFromDataURL and verifies that it's ignored? In general, is it possible for you to test the browser-side code too? That's the code that's more important to test since it actually enforces the security requirements.
By the way, thank you for the clear explanation of how this code works and for improving the comments.
Patchset #6 (id:140001) has been deleted
Thank you for review. I addressed comments. Could you take a look again? https://codereview.chromium.org/518693002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/518693002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1348: is_large_data_url)) { On 2014/09/11 00:38:53, Ken Russell wrote: > nit: fix indentation. Done. https://codereview.chromium.org/518693002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/518693002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_message_filter.cc:915: DownloadUrl(render_view_id, GURL(data_url), On 2014/09/11 00:38:53, Ken Russell wrote: > Please construct the GURL in a separate statement and verify that the scheme is > "data", returning immediately if not (or if something else went wrong in GURL > construction). The browser does not trust the renderer. All assumptions about > incoming inputs must be verified. Done. https://codereview.chromium.org/518693002/diff/120001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/518693002/diff/120001/content/renderer/render... content/renderer/render_view_browsertest.cc:340: EXPECT_FALSE(msg4); On 2014/09/11 00:38:53, Ken Russell wrote: > After more thought, is it possible for you to add a test which sends a non-data: > URL to RenderMessageFilter::OnSaveImageFromDataURL and verifies that it's > ignored? In general, is it possible for you to test the browser-side code too? > That's the code that's more important to test since it actually enforces the > security requirements. Done. (Added unittests for OnSaveImageFromDataURL())
Patchset #7 (id:170001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:190001) has been deleted
Ping @kbr. Please take a look.
Very nice. Thank you for the excellent test. LGTM
Ping @Avi, Could you review this again? This CL needs your approval for chrome/* and content/*.
LGTM; thanks!
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/518693002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:250001) has been deleted
Patchset #7 (id:230001) has been deleted
Patchset #8 (id:290001) has been deleted
Patchset #7 (id:270001) has been deleted
Ping @Avi, The RenderMessageFilter class should be exposed for unit test. Would you agree with this? (So, I added CONTENT_EXPORT to the class) Thank you. https://codereview.chromium.org/518693002/diff/310001/content/browser/rendere... File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/518693002/diff/310001/content/browser/rendere... content/browser/renderer_host/render_message_filter.h:90: class CONTENT_EXPORT RenderMessageFilter : public BrowserMessageFilter { I think we should expose this class for using in unit test. Similarly, the existing message filter classes are already exposed. (such as FileAPIMessageFilter, MessagePortMessageFilter, VideoCaptureMessageFilter and so on)
lgtm That's fine.
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/518693002/310001
Message was sent while issue was closed.
Committed patchset #7 (id:310001) as 21eb6b3b04cbae7b185cf2256d20bf5f38e17913
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/632a1a7256d3aada4c2deefb103cac2ca4fc56d9 Cr-Commit-Position: refs/heads/master@{#295405} |
