Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1264923002:
Introduce cross_site_iframe_factory.html, and use it in a test (Closed)
|
Created:
5 years, 4 months ago by ncarter (slow) Modified:
5 years, 4 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce cross_site_iframe_factory.html, and use it to test
the site isolation code in MemoryMetricsDetails under chrome/.
[ See crbug.com/515672 for a screenshot ]
/cross_site_iframe_factory.html?a(b(c, d)) gives you a simple three-level,
four-site iframe test. The trick it uses, is that the query parameter
is parsed and passed recursively to children, so the first level is:
<iframe src="b.com:1234/cross_site_iframe_factory.html?b(c(),d())">
Which then contains the two leaf iframes:
<iframe src="c.com:1234/cross_site_iframe_factory.html?c()">
<iframe src="d.com:1234/cross_site_iframe_factory.html?d()">
To to cut down on noise. ".com" is automatically affixed to the hostname
if a TLD isn't present.
As a convenience, the page supports local preview if loaded via file:// URI.
file:// URIs are technically cross-origin, although --site-per-process doesn't
isolate them today.
Each site has a random (but consistent) background color as a visual aid.
The page contains some crude layout logic to make sure the outer levels
are big enough to fit the inner levels.
The parsing logic is in tree_parser_util.js, which has unittests.
MetricsMemoryDetailsBrowserTest.TestSiteIsolation is a new test
that uses the above facility to creates a bunch of complex iframe
cases, and tests that our process-counting UMA stats are as expected.
BUG=515672
Committed: https://crrev.com/02292e994c9ec62deecec3bec2132500ca68a5de
Cr-Commit-Position: refs/heads/master@{#342447}
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Self-review fixes. #
Total comments: 22
Patch Set 4 : dcheng's fixes #Patch Set 5 : Remove unittest.html, since I don't know how to hook it up. #
Total comments: 4
Patch Set 6 : Move test. #Patch Set 7 : Remove _lex #Patch Set 8 : Attempt to fix Linux test by +1 for zygote. #
Total comments: 2
Patch Set 9 : const #
Messages
Total messages: 36 (10 generated)
nick@chromium.org changed reviewers: + dcheng@chromium.org
Daniel, could you take a look at this CL? Thanks!
https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:72: ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady()); Out of curiosity, why CHECK() above and ASSERT_TRUE() here? https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:96: "/cross_site_iframe_factory.html?a(b(a(b,c,d,e,f,g,h)),c,d,e,i(f))"); This is pretty neat, but the query string is a bit difficult for humans to read, especially if it's deeply nested. Out of curiosity, what do you think of a C++ builder approach? For example: TestFrameBuilder() .addFrame("b", TestFrameBuilder().addFrame("c").addFrame("d")) .addFrame("e").BuildURL() might get you a page like: ,------------------------------. | a.com | |,-------------------.,-------.| || b.com || e.com || ||,-------. ,-------.|`-------'| ||| c.com | | d.com || | ||`-------'-'-------'| | |`-------------------' | `------------------------------' BuildURL() can dump out base64 JSON or something silly like that, so the parser logic in JS can be made a lot shorter. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... File content/test/data/cross_site_iframe_factory.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:1: <html><!-- Super minor nit: I feel like placing the <!-- might be more consistent with the style of the rest of this page. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:38: border-radius: 7px; Fancy!
https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:96: "/cross_site_iframe_factory.html?a(b(a(b,c,d,e,f,g,h)),c,d,e,i(f))"); On 2015/07/30 23:35:04, dcheng wrote: > This is pretty neat, but the query string is a bit difficult for humans to read, > especially if it's deeply nested. > > Out of curiosity, what do you think of a C++ builder approach? For example: > > TestFrameBuilder() > .addFrame("b", > TestFrameBuilder().addFrame("c").addFrame("d")) > .addFrame("e").BuildURL() I don't think the builder is necessary, at least not right away. Let me argue both sides: Argument against: ================= For our existing test pages, you have to look at the HTML to know what the frame tree structure actually is -- so I'd argue that having even a terse representation of the structure in the URL is moving the ball forward forward. With syntax highlighting, things actually look a little better than they do in rietveld: the arguments inside the string literal don't blend together with the surrounding c++ code so much. And when you're writing a test, it's pretty deeply satisfying to be able to add a a new cross-site iframe with just two characters. Without any additional infrastructure, you have the option to add indentation to the string (since whitespace is ignored), like this: "a.com(b.com(a.com(b.com," " c.com," " d.com," " e.com," " f.com," " g.com," " h.com)), " c.com," " d.com," " e.com, " i.com(f.com))" However, my eyeballs prefer the compact form. If I want a visualization of the structure, I can just load the page. Argument for: ============= Having said the above, I do see some upside to adding a builder. If we expand the grammar at all so that it supports any kind of operator or additional arguments (e.g. you want "iframe sandbox" or using title1.html as your leaf node, or options for https, or "extension scheme"), I think a builder will start to be be useful at that point. Another advantage to having a C++ builder would be to provide a place where we could ensure that the host resolver rule and the other test boilerplate has been configured properly. I worry that such setup conventions (especially the host resolver (and eventually, cert verifier) mucking) is an impediment to people actually using these facilities from whichever test fixture. > > might get you a page like: > > ,------------------------------. > | a.com | > |,-------------------.,-------.| > || b.com || e.com || > ||,-------. ,-------.|`-------'| > ||| c.com | | d.com || | > ||`-------'-'-------'| | > |`-------------------' | > `------------------------------' > > BuildURL() can dump out base64 JSON or something silly like that, so the parser > logic in JS can be made a lot shorter. The downside to a base64/json scheme is that it totally breaks crafting the test page under file:// preview, which is actually a really nice feature of this. If we isolated file origins (or had a mode to do that), this would be really useful for local debugging. The other reason I prefer what I've implemented, is that it's implemented already :)
A more minor comments. Overall, it looks reasonable. https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:96: "/cross_site_iframe_factory.html?a(b(a(b,c,d,e,f,g,h)),c,d,e,i(f))"); On 2015/07/31 at 17:16:01, ncarter wrote: > On 2015/07/30 23:35:04, dcheng wrote: > > This is pretty neat, but the query string is a bit difficult for humans to read, > > especially if it's deeply nested. > > > > Out of curiosity, what do you think of a C++ builder approach? For example: > > > > TestFrameBuilder() > > .addFrame("b", > > TestFrameBuilder().addFrame("c").addFrame("d")) > > .addFrame("e").BuildURL() > > I don't think the builder is necessary, at least not right away. Let me argue both sides: > > Argument against: > ================= > > For our existing test pages, you have to look at the HTML to know what the frame tree structure actually is -- so I'd argue that having even a terse representation of the structure in the URL is moving the ball forward forward. With syntax highlighting, things actually look a little better than they do in rietveld: the arguments inside the string literal don't blend together with the surrounding c++ code so much. And when you're writing a test, it's pretty deeply satisfying to be able to add a a new cross-site iframe with just two characters. > > Without any additional infrastructure, you have the option to add indentation to the string (since whitespace is ignored), like this: > "a.com(b.com(a.com(b.com," > " c.com," > " d.com," > " e.com," > " f.com," > " g.com," > " h.com)), > " c.com," > " d.com," > " e.com, > " i.com(f.com))" > > However, my eyeballs prefer the compact form. If I want a visualization of the structure, I can just load the page. > > Argument for: > ============= > > Having said the above, I do see some upside to adding a builder. If we expand the grammar at all so that it supports any kind of operator or additional arguments (e.g. you want "iframe sandbox" or using title1.html as your leaf node, or options for https, or "extension scheme"), I think a builder will start to be be useful at that point. > > Another advantage to having a C++ builder would be to provide a place where we could ensure that the host resolver rule and the other test boilerplate has been configured properly. I worry that such setup conventions (especially the host resolver (and eventually, cert verifier) mucking) is an impediment to people actually using these facilities from whichever test fixture. > > > > > might get you a page like: > > > > ,------------------------------. > > | a.com | > > |,-------------------.,-------.| > > || b.com || e.com || > > ||,-------. ,-------.|`-------'| > > ||| c.com | | d.com || | > > ||`-------'-'-------'| | > > |`-------------------' | > > `------------------------------' > > > > BuildURL() can dump out base64 JSON or something silly like that, so the parser > > logic in JS can be made a lot shorter. > > The downside to a base64/json scheme is that it totally breaks crafting the test page under file:// preview, which is actually a really nice feature of this. If we isolated file origins (or had a mode to do that), this would be really useful for local debugging. > > The other reason I prefer what I've implemented, is that it's implemented already :) I don't see myself using the ability to preview it on a file:// URL much, as I'd expect it to Just Work =). That being said, I'm not a huge fan of the C++ builder stuff either: though I think it's much better in terms of readability, it's too verbose. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... File content/test/data/cross_site_iframe_factory.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:115: var minX = 110; // Needs to be tall enough for for what? =) https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:156: url += '?' + escape(subtreeString); // query escape => encodeURIComponent https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:162: iframe.scrolling = 'no'; Is this necessary given the layout() call? It might be nice to omit it: that way, if it does overflow, at least you can scroll around still? https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... File content/test/data/tree_parser_util.js (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util.js:104: if (!token.match(/[a-zA-Z0-9_.]+/)) Technically, _ isn't allowed in domain names (though - is, as long as it's not the first or last character). https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... File content/test/data/tree_parser_util_unittest.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util_unittest.html:1: <html><body> Is this run automatically somewhere, or is this essentially a manual test? https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util_unittest.html:70: </body> </html>
https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:72: ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady()); On 2015/07/30 23:35:04, dcheng wrote: > Out of curiosity, why CHECK() above and ASSERT_TRUE() here? Done. https://codereview.chromium.org/1264923002/diff/30001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:96: "/cross_site_iframe_factory.html?a(b(a(b,c,d,e,f,g,h)),c,d,e,i(f))"); On 2015/08/04 00:14:41, dcheng wrote: > On 2015/07/31 at 17:16:01, ncarter wrote: > > On 2015/07/30 23:35:04, dcheng wrote: > > > This is pretty neat, but the query string is a bit difficult for humans to > read, > > > especially if it's deeply nested. > > > > > > Out of curiosity, what do you think of a C++ builder approach? For example: > > > > > > TestFrameBuilder() > > > .addFrame("b", > > > TestFrameBuilder().addFrame("c").addFrame("d")) > > > .addFrame("e").BuildURL() > > > > I don't think the builder is necessary, at least not right away. Let me argue > both sides: > > > > Argument against: > > ================= > > > > For our existing test pages, you have to look at the HTML to know what the > frame tree structure actually is -- so I'd argue that having even a terse > representation of the structure in the URL is moving the ball forward forward. > With syntax highlighting, things actually look a little better than they do in > rietveld: the arguments inside the string literal don't blend together with the > surrounding c++ code so much. And when you're writing a test, it's pretty deeply > satisfying to be able to add a a new cross-site iframe with just two characters. > > > > Without any additional infrastructure, you have the option to add indentation > to the string (since whitespace is ignored), like this: > > "a.com(b.com(a.com(b.com," > > " c.com," > > " d.com," > > " e.com," > > " f.com," > > " g.com," > > " h.com)), > > " c.com," > > " d.com," > > " e.com, > > " i.com(f.com))" > > > > However, my eyeballs prefer the compact form. If I want a visualization of the > structure, I can just load the page. > > > > Argument for: > > ============= > > > > Having said the above, I do see some upside to adding a builder. If we expand > the grammar at all so that it supports any kind of operator or additional > arguments (e.g. you want "iframe sandbox" or using title1.html as your leaf > node, or options for https, or "extension scheme"), I think a builder will start > to be be useful at that point. > > > > Another advantage to having a C++ builder would be to provide a place where we > could ensure that the host resolver rule and the other test boilerplate has been > configured properly. I worry that such setup conventions (especially the host > resolver (and eventually, cert verifier) mucking) is an impediment to people > actually using these facilities from whichever test fixture. > > > > > > > > might get you a page like: > > > > > > ,------------------------------. > > > | a.com | > > > |,-------------------.,-------.| > > > || b.com || e.com || > > > ||,-------. ,-------.|`-------'| > > > ||| c.com | | d.com || | > > > ||`-------'-'-------'| | > > > |`-------------------' | > > > `------------------------------' > > > > > > BuildURL() can dump out base64 JSON or something silly like that, so the > parser > > > logic in JS can be made a lot shorter. > > > > The downside to a base64/json scheme is that it totally breaks crafting the > test page under file:// preview, which is actually a really nice feature of > this. If we isolated file origins (or had a mode to do that), this would be > really useful for local debugging. > > > > The other reason I prefer what I've implemented, is that it's implemented > already :) > > I don't see myself using the ability to preview it on a file:// URL much, as I'd > expect it to Just Work =). That being said, I'm not a huge fan of the C++ > builder stuff either: though I think it's much better in terms of readability, > it's too verbose. If we wind up adding any kind of per-node options to this thing, the builder will make sense. So I'm open to adding it in that case, but I'm pretty attached to keeping the URLs human readable. The readable URLs help not just with local preview, but also with running under a debugger and printf debugging. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... File content/test/data/cross_site_iframe_factory.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:1: <html><!-- On 2015/07/30 23:35:04, dcheng wrote: > Super minor nit: I feel like placing the <!-- might be more consistent with the > style of the rest of this page. Done -- though check my work, since I wasn't 100% sure what you wanted. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:38: border-radius: 7px; On 2015/07/30 23:35:04, dcheng wrote: > Fancy! Acknowledged. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:115: var minX = 110; // Needs to be tall enough for On 2015/08/04 00:14:41, dcheng wrote: > for what? =) Done. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:156: url += '?' + escape(subtreeString); // query On 2015/08/04 00:14:41, dcheng wrote: > escape => encodeURIComponent Done. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/cross... content/test/data/cross_site_iframe_factory.html:162: iframe.scrolling = 'no'; On 2015/08/04 00:14:41, dcheng wrote: > Is this necessary given the layout() call? It might be nice to omit it: that > way, if it does overflow, at least you can scroll around still? Good call. I added this early on, before the layout got good enough to avoid clipping. You can definitely get into trouble since I don't account for the length of the text. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... File content/test/data/tree_parser_util.js (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util.js:104: if (!token.match(/[a-zA-Z0-9_.]+/)) On 2015/08/04 00:14:41, dcheng wrote: > Technically, _ isn't allowed in domain names (though - is, as long as it's not > the first or last character). Good catch. Fixed. https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... File content/test/data/tree_parser_util_unittest.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util_unittest.html:1: <html><body> On 2015/08/04 00:14:41, dcheng wrote: > Is this run automatically somewhere, or is this essentially a manual test? It's run manually. I couldn't quite bring myself to add a browsertest to run it on the trybots continuously, in part because there's nothing like this whatsoever in content/test/data, and in part because I figured there was probably some kind of jsunit framework somewhere that this should plug into. Any ideas?
https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... File content/test/data/tree_parser_util_unittest.html (right): https://codereview.chromium.org/1264923002/diff/30001/content/test/data/tree_... content/test/data/tree_parser_util_unittest.html:1: <html><body> On 2015/08/04 23:51:21, ncarter wrote: > On 2015/08/04 00:14:41, dcheng wrote: > > Is this run automatically somewhere, or is this essentially a manual test? > > It's run manually. I couldn't quite bring myself to add a browsertest to run it > on the trybots continuously, in part because there's nothing like this > whatsoever in content/test/data, and in part because I figured there was > probably some kind of jsunit framework somewhere that this should plug into. > > Any ideas? I went ahead and just removed this file.
nick@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in chrome/browser/metrics
lgtm https://codereview.chromium.org/1264923002/diff/70001/content/test/data/tree_... File content/test/data/tree_parser_util.js (right): https://codereview.chromium.org/1264923002/diff/70001/content/test/data/tree_... content/test/data/tree_parser_util.js:150: _lex: lex, // Exposed for testing. I guess this doesn't need to be exposed anymore.
https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:104: details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"), These histograms are logged by site_details.cc. Perhaps this should be in site_details_browsertest.cc instead?
https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:104: details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"), On 2015/08/05 18:46:47, Alexei Svitkine wrote: > These histograms are logged by site_details.cc. Perhaps this should be in > site_details_browsertest.cc instead? This is as much a test of what MemoryMetricsDetails passes to SiteDetails as it is a test of what SiteDetails does with those arguments. The way I see the code, SiteDetails is really just a helper class for MetricsMemoryDetails.
https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... chrome/browser/metrics/metrics_memory_details_browsertest.cc:104: details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"), On 2015/08/05 22:52:03, ncarter wrote: > On 2015/08/05 18:46:47, Alexei Svitkine wrote: > > These histograms are logged by site_details.cc. Perhaps this should be in > > site_details_browsertest.cc instead? > > This is as much a test of what MemoryMetricsDetails passes to SiteDetails as it > is a test of what SiteDetails does with those arguments. The way I see the code, > SiteDetails is really just a helper class for MetricsMemoryDetails. So one issue is that we'll likely want to componentize this class. You're adding extra dependencies on content/ - which doesn't exist on iOS. So this will make componentizing harder. I'd rather this code not get all of these dependencies. The only part of MetricsMemoryDetails this is testing is the fact that it calls SiteDetails and I guess the parameters of that function. This might be better tested with dependency injection - i.e. passing a callback to the SiteDetails function to the MetricsMemoryDetails constructor - and testing with a test callback here to make sure the right params are passed. Whereas the tests of what SiteDetails actually logs should really be next to the code that logs them.
On 2015/08/06 14:45:03, Alexei Svitkine wrote: > https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... > File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right): > > https://codereview.chromium.org/1264923002/diff/70001/chrome/browser/metrics/... > chrome/browser/metrics/metrics_memory_details_browsertest.cc:104: > details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"), > On 2015/08/05 22:52:03, ncarter wrote: > > On 2015/08/05 18:46:47, Alexei Svitkine wrote: > > > These histograms are logged by site_details.cc. Perhaps this should be in > > > site_details_browsertest.cc instead? > > > > This is as much a test of what MemoryMetricsDetails passes to SiteDetails as > it > > is a test of what SiteDetails does with those arguments. The way I see the > code, > > SiteDetails is really just a helper class for MetricsMemoryDetails. > > So one issue is that we'll likely want to componentize this class. You're adding > extra dependencies on content/ - which doesn't exist on iOS. So this will make > componentizing harder. > > I'd rather this code not get all of these dependencies. The only part of > MetricsMemoryDetails this is testing is the fact that it calls SiteDetails and I > guess the parameters of that function. This might be better tested with > dependency injection - i.e. passing a callback to the SiteDetails function to > the MetricsMemoryDetails constructor - and testing with a test callback here to > make sure the right params are passed. Whereas the tests of what SiteDetails > actually logs should really be next to the code that logs them. I've moved the file to chrome/browser/site_details_browsertest.cc but the test still exercises MetricsMemoryDetails, because of how the code is currently entangled.
lgtm
nick@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in chrome/browser/site_details_browsertest.cc
Can you find an owner that better understand this code?
nick@chromium.org changed reviewers: - sky@chromium.org
nick@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Please review changes in chrome/browser/site_details_browsertest.cc
On 2015/08/06 20:24:25, ncarter wrote: > mailto:jam@chromium.org: Please review changes in > chrome/browser/site_details_browsertest.cc since the test is in chrome, the html file should be in chrome/test/data
On 2015/08/06 21:35:47, jam wrote: > On 2015/08/06 20:24:25, ncarter wrote: > > mailto:jam@chromium.org: Please review changes in > > chrome/browser/site_details_browsertest.cc > > since the test is in chrome, the html file should be in chrome/test/data It's a general purpose page, and we will use it from content tests too (like, as soon as this lands). It's too complex to put in both places.
On 2015/08/06 21:50:54, ncarter wrote: > On 2015/08/06 21:35:47, jam wrote: > > On 2015/08/06 20:24:25, ncarter wrote: > > > mailto:jam@chromium.org: Please review changes in > > > chrome/browser/site_details_browsertest.cc > > > > since the test is in chrome, the html file should be in chrome/test/data > > It's a general purpose page, and we will use it from content tests too (like, as > soon as this lands). It's too complex to put in both places. i see, ok rubberstamp lgtm
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1264923002/#ps110001 (title: "Remove _lex")
The CQ bit was unchecked by nick@chromium.org
dcheng: could you take another pass at patchset 7 vs 8? I had to change it to account for the zygote process.
lgtm https://codereview.chromium.org/1264923002/diff/130001/chrome/browser/site_de... File chrome/browser/site_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/130001/chrome/browser/site_de... chrome/browser/site_details_browsertest.cc:77: int other_process_count() { nit: const
https://codereview.chromium.org/1264923002/diff/130001/chrome/browser/site_de... File chrome/browser/site_details_browsertest.cc (right): https://codereview.chromium.org/1264923002/diff/130001/chrome/browser/site_de... chrome/browser/site_details_browsertest.cc:77: int other_process_count() { On 2015/08/07 20:03:50, dcheng wrote: > nit: const Done.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jam@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1264923002/#ps150001 (title: "const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264923002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264923002/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/02292e994c9ec62deecec3bec2132500ca68a5de Cr-Commit-Position: refs/heads/master@{#342447}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:150001) has been created in https://codereview.chromium.org/1282623002/ by nasko@chromium.org. The reason for reverting is: Fails on Windows Vista and Mac 10.6 http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20(1)/builds/... http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/5408 ../../chrome/browser/site_details_browsertest.cc:121: Failure Value of: details->uma()->GetAllSamples( "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate") Expected: has 1 element that is equal to Bucket 11: 1 Actual: { Bucket 10: 1 }, whose element #0 doesn't match. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
