|
|
Created:
6 years, 9 months ago by hush (inactive) Modified:
6 years, 8 months ago Reviewers:
sgurun-gerrit only, boliu, benm (inactive), darin (slow to review), Nate Chapin, Charlie Reis CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSet the original url correctly if the frame is loaded via loadData
BUG=348234
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264247
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments about const and null checks #
Total comments: 10
Patch Set 3 : Replaced the unit test with a browser test. Added LoadDataWithBaseURL on shell #
Total comments: 1
Patch Set 4 : add // static comment #
Total comments: 14
Patch Set 5 : address comments and rebase #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : Rebase correctly #
Messages
Total messages: 34 (0 generated)
ptal
redirecting review to creis@
[+darin,rdsmith for unreachable URL knowledge] I'm a little skeptical about the unreachable URL check, since that's used in more than one way. I'm not sure if it's wrong, though-- Darin or Randy might know. I haven't looked at the test yet, but there's a lot of style issues throughout. You might want to refer to http://www.chromium.org/developers/coding-style and http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:138: void GetRedirectChain(const WebDataSource* ds, std::vector<GURL>* result) { Please don't change this signature. (Why make the pointer const?) https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:139: if (!result || !ds) { style nit: No braces for a one-line body. In what circumstances do we expect these to be null? I'm not sure this is needed. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2283: const GURL RenderFrameImpl::GetOriginalRequestUrl(const WebDataSource* ds) { No const in either part. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2284: if (!ds) { This check seems unnecessary. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2293: // history_url_for_data_url as unreachableURL in blink. The I'm not sure this comment is accurate. That's not the only time hasUnreachableURL is true, is it? We use it for error pages as well. @darin or @rdsmith might be able to comment more, since I think they know a bit more about the error pages than I do. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2300: if (redirects.size() > 0) { No braces for a one-line body. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2303: else { No need for an else block if your if block returns.
Hi! Thanks for the comments! Please see my responses below https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:138: void GetRedirectChain(const WebDataSource* ds, std::vector<GURL>* result) { On 2014/03/03 18:21:11, Charlie Reis wrote: > Please don't change this signature. (Why make the pointer const?) http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of... I want to make WebDataSource* const because GetRedirectChain is only supposed to read from WebDataSource and not change its states. Making it const ensures that we don't accidentally change WebDataSource in later code. And the readers of the code can easily reason about what the function does by looking at the signature, without worrying the state of WebDataSource being changed. However I'm new to the code base and if this is inconsistent with current chromium code styles I will remove the "const". https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:139: if (!result || !ds) { On 2014/03/03 18:21:11, Charlie Reis wrote: > style nit: No braces for a one-line body. > > In what circumstances do we expect these to be null? I'm not sure this is > needed. I was just doing this defensively in case that code in the future uses the function incorrectly to introduce a null pointer exception. There won't be any null pointers for the code right now. I will remove the null checks if you are okay with it. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2283: const GURL RenderFrameImpl::GetOriginalRequestUrl(const WebDataSource* ds) { On 2014/03/03 18:21:11, Charlie Reis wrote: > No const in either part. Please see the previous reply about "const" https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2284: if (!ds) { On 2014/03/03 18:21:11, Charlie Reis wrote: > This check seems unnecessary. Please the previous reply about null check. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2293: // history_url_for_data_url as unreachableURL in blink. The On 2014/03/03 18:21:11, Charlie Reis wrote: > I'm not sure this comment is accurate. That's not the only time > hasUnreachableURL is true, is it? We use it for error pages as well. > > @darin or @rdsmith might be able to comment more, since I think they know a bit > more about the error pages than I do. You are right, error page uses unreachableURL as well. (I'm currently not aware of a 3rd case where unreachableURL is used.) If I try to visit "http://non.existent.url" in a tab, the error page is displayed by calling loadData with the parameters: baseUrl = chrome error page, historyUrl = "http://non.existent.url" That means the original url for the tab is the chrome error page. My change will make the original url for the tab to "http://non.existent.url". But I think this is reasonable because there is in fact no redirect from chrome error page to "http://non.existent.url" https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2300: if (redirects.size() > 0) { On 2014/03/03 18:21:11, Charlie Reis wrote: > No braces for a one-line body. Okay https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2303: else { On 2014/03/03 18:21:11, Charlie Reis wrote: > No need for an else block if your if block returns. okay
@rdsmith: Are you familiar enough with unreachable URL to take a look at this change? https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:138: void GetRedirectChain(const WebDataSource* ds, std::vector<GURL>* result) { On 2014/03/03 23:12:52, hush wrote: > On 2014/03/03 18:21:11, Charlie Reis wrote: > > Please don't change this signature. (Why make the pointer const?) > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of... > I want to make WebDataSource* const because GetRedirectChain is only supposed to > read from WebDataSource and not change its states. Making it const ensures that > we don't accidentally change WebDataSource in later code. And the readers of the > code can easily reason about what the function does by looking at the signature, > without worrying the state of WebDataSource being changed. > > However I'm new to the code base and if this is inconsistent with current > chromium code styles I will remove the "const". Yes, we generally don't put const on pointer parameters. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:139: if (!result || !ds) { On 2014/03/03 23:12:52, hush wrote: > On 2014/03/03 18:21:11, Charlie Reis wrote: > > style nit: No braces for a one-line body. > > > > In what circumstances do we expect these to be null? I'm not sure this is > > needed. > > I was just doing this defensively in case that code in the future uses the > function incorrectly to introduce a null pointer exception. There won't be any > null pointers for the code right now. > I will remove the null checks if you are okay with it. Yes, please remove them. Better to crash early if this is being used in unexpected ways. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2293: // history_url_for_data_url as unreachableURL in blink. The On 2014/03/03 23:12:52, hush wrote: > On 2014/03/03 18:21:11, Charlie Reis wrote: > > I'm not sure this comment is accurate. That's not the only time > > hasUnreachableURL is true, is it? We use it for error pages as well. > > > > @darin or @rdsmith might be able to comment more, since I think they know a > bit > > more about the error pages than I do. > > You are right, error page uses unreachableURL as well. (I'm currently not aware > of a 3rd case where unreachableURL is used.) > If I try to visit "http://non.existent.url" in a tab, the error page is > displayed by calling loadData with the parameters: baseUrl = chrome error page, > historyUrl = "http://non.existent.url" > That means the original url for the tab is the chrome error page. My change will > make the original url for the tab to "http://non.existent.url". > But I think this is reasonable because there is in fact no redirect from chrome > error page to "http://non.existent.url" That's a behavior change that I'm not sure how to evaluate. I defer to @darin or @rdsmith there.
Thanks for the comments. I have uploaded a new patch set. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:138: void GetRedirectChain(const WebDataSource* ds, std::vector<GURL>* result) { On 2014/03/04 20:32:03, Charlie Reis wrote: > On 2014/03/03 23:12:52, hush wrote: > > On 2014/03/03 18:21:11, Charlie Reis wrote: > > > Please don't change this signature. (Why make the pointer const?) > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of... > > I want to make WebDataSource* const because GetRedirectChain is only supposed > to > > read from WebDataSource and not change its states. Making it const ensures > that > > we don't accidentally change WebDataSource in later code. And the readers of > the > > code can easily reason about what the function does by looking at the > signature, > > without worrying the state of WebDataSource being changed. > > > > However I'm new to the code base and if this is inconsistent with current > > chromium code styles I will remove the "const". > > Yes, we generally don't put const on pointer parameters. Done. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:139: if (!result || !ds) { On 2014/03/04 20:32:03, Charlie Reis wrote: > On 2014/03/03 23:12:52, hush wrote: > > On 2014/03/03 18:21:11, Charlie Reis wrote: > > > style nit: No braces for a one-line body. > > > > > > In what circumstances do we expect these to be null? I'm not sure this is > > > needed. > > > > I was just doing this defensively in case that code in the future uses the > > function incorrectly to introduce a null pointer exception. There won't be any > > null pointers for the code right now. > > I will remove the null checks if you are okay with it. > > Yes, please remove them. Better to crash early if this is being used in > unexpected ways. Done.
ping? Hi darin@ and rdsmith@ what are your opinions for the behavior change? if you load the frame with loadData (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) is it okay to regard the original url for the frame load to be the unreachable url instead of the base url?
On 2014/03/05 21:48:28, hush wrote: > ping? > Hi darin@ and rdsmith@ > what are your opinions for the behavior change? > if you load the frame with loadData > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > is it okay to regard the original url for the frame load to be the unreachable > url instead of the base url? Sorry, it looks like all the folks I know to ask about unreachable URL are OOO this week (darin@, rdsmith@, japhet@). Hopefully someone can look next week.
https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2280: GURL RenderFrameImpl::GetOriginalRequestUrl(WebDataSource* ds) { nit: We aren't terribly consistent, but a quick grep reveals that we have more method names including "URL" than "Url". I'd probably go with "URL". https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2281: std::vector<GURL> redirects; nit: I'd move this below the hasUnreachableURL branch. That way you don't bother getting the redirect chain unless it is needed. https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2284: // The frame is loaded via WebFrame::loadData, where we pass the nit: This comment needs some work. It appears to be referring to some variable names "history_url_for_data_url" that don't have much meaning in the context of this function. I think it is enough to indicate that the unreachableURL parameter of WebFrame::loadData() should be treated as the original URL if set. https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2292: if (redirects.size() > 0) nit: I'd use the .empty() method here instead: if (!redirects.empty())
https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl_unittest.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl_unittest.cc:28: class WebDataSourceMock : public WebDataSource { Note: This is not obvious, but if you implement WebDataSource in the Chromium repository, then it makes it a bit harder to evolve the interface. Interfaces intended to be implemented only by Blink have pure virtual methods, and interfaces only intended to be implemented by Chromium have non-pure virtual methods. This helps save some steps when evolving interfaces. I'm also not sure that this unit test really provides that much value. A slightly larger test that invokes loadData() and observes the resulting original URL might be better.
Hi! Thanks for the comments! I will upload a new patch set with a browser test. https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2280: GURL RenderFrameImpl::GetOriginalRequestUrl(WebDataSource* ds) { On 2014/03/11 20:04:32, darin wrote: > nit: We aren't terribly consistent, but a quick grep reveals that we have more > method names including "URL" than "Url". I'd probably go with "URL". Done https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2281: std::vector<GURL> redirects; On 2014/03/11 20:04:32, darin wrote: > nit: I'd move this below the hasUnreachableURL branch. That way you don't bother > getting the redirect chain unless it is needed. Done https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2284: // The frame is loaded via WebFrame::loadData, where we pass the On 2014/03/11 20:04:32, darin wrote: > nit: This comment needs some work. It appears to be referring to some variable > names "history_url_for_data_url" that don't have much meaning in the context of > this function. > > I think it is enough to indicate that the unreachableURL parameter of > WebFrame::loadData() should be treated as the original URL if set. Okay. I just reworded the comments by only referring to baseURL and unreachableURL. These two are the input params to WebFrame::loadData. https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:2292: if (redirects.size() > 0) On 2014/03/11 20:04:32, darin wrote: > nit: I'd use the .empty() method here instead: if (!redirects.empty()) Done https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl_unittest.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_... content/renderer/render_frame_impl_unittest.cc:28: class WebDataSourceMock : public WebDataSource { Thanks for note! In order to call loadData() and observe the original URL, I think I will make a browser test. I will loadData from the web contents, or content shell level, then I will observe the top navigation entry and ensure the entry's original url is the one we passed in as "history url" On 2014/03/11 20:09:52, darin wrote: > Note: This is not obvious, but if you implement WebDataSource in the Chromium > repository, then it makes it a bit harder to evolve the interface. > > Interfaces intended to be implemented only by Blink have pure virtual methods, > and interfaces only intended to be implemented by Chromium have non-pure virtual > methods. This helps save some steps when evolving interfaces. > > I'm also not sure that this unit test really provides that much value. A > slightly larger test that invokes loadData() and observes the resulting original > URL might be better.
https://codereview.chromium.org/176883012/diff/40001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/40001/content/renderer/render_... content/renderer/render_frame_impl.cc:2279: nit: add // static here (code convention, see how static methods are defined above)
I don't think I have any special expertise in this code; I'm at the beginning of trying to understand it myself. Given that Darin's already engaged, I'm going to take myself off the reviewer list.
ping?
Thanks, this is looking good. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Please drop the "(c)" from the header. We used to include that, but the policy for new files is to leave it out. Thanks! https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:13: namespace content { nit: add a new line here between the namespace and the class. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:22: NavigationController& controller = shell()->web_contents()->GetController(); nit: We generally avoid non-const references in Chromium code. I'm surprised to see GetController() return a non-const reference. That said, you can use a const reference here because GetVisibleEntry() is a const method. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:31: } // namespace content nit: add a new line above here. https://codereview.chromium.org/176883012/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2281: GURL RenderFrameImpl::GetOriginalRequestURL(WebDataSource* ds) { Since this method is static, and not called by any other .cc files, it should just be made file static and moved to the anonymous namespace at the top of this file. https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... content/shell/browser/shell.cc:182: void Shell::LoadDataWithBaseURL(const GURL& base_url, const std::string& data, nit: since this method is named LoadDataWithBaseURL, it might be helpful to list the base_url parameter after the data to better match the order in which those items are mentioned in the method name. Also, since history_url is sort of similar to the url parameter to LoadURLForFrame, you might consider listing it first and just call it |url|. void Shell::LoadDataWithBaseURL(const GURL& url, const std::string& data, const GURL& base_url); https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... content/shell/browser/shell.cc:192: } nit: add a new line below here.
Hi! I just uploaded a new patch and the tests are passing in the bots https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/03/27 08:10:28, darin wrote: > nit: Please drop the "(c)" from the header. We used to include that, but the > policy for new files is to leave it out. Thanks! Done. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:13: namespace content { On 2014/03/27 08:10:28, darin wrote: > nit: add a new line here between the namespace and the class. Done. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:22: NavigationController& controller = shell()->web_contents()->GetController(); On 2014/03/27 08:10:28, darin wrote: > nit: We generally avoid non-const references in Chromium code. I'm surprised to > see GetController() return a non-const reference. That said, you can use a const > reference here because GetVisibleEntry() is a const method. Done. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_browsertest.cc:31: } // namespace content On 2014/03/27 08:10:28, darin wrote: > nit: add a new line above here. Done. https://codereview.chromium.org/176883012/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2281: GURL RenderFrameImpl::GetOriginalRequestURL(WebDataSource* ds) { On 2014/03/27 08:10:28, darin wrote: > Since this method is static, and not called by any other .cc files, it should > just be made file static and moved to the anonymous namespace at the top of this > file. Done. https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... content/shell/browser/shell.cc:182: void Shell::LoadDataWithBaseURL(const GURL& base_url, const std::string& data, On 2014/03/27 08:10:28, darin wrote: > nit: since this method is named LoadDataWithBaseURL, it might be helpful to list > the base_url parameter after the data to better match the order in which those > items are mentioned in the method name. > > Also, since history_url is sort of similar to the url parameter to > LoadURLForFrame, you might consider listing it first and just call it |url|. > > void Shell::LoadDataWithBaseURL(const GURL& url, const std::string& data, const > GURL& base_url); Done. https://codereview.chromium.org/176883012/diff/60001/content/shell/browser/sh... content/shell/browser/shell.cc:192: } On 2014/03/27 08:10:28, darin wrote: > nit: add a new line below here. Done.
ping :)
LGTM https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:175: static GURL GetOriginalRequestURL(WebDataSource* ds) { nit: It looks like this function is defined inside an anonymous namespace. Therefore the "static" keyword is redundant. I see that other functions are also prefixed with static. That's a shame :-/
https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:175: static GURL GetOriginalRequestURL(WebDataSource* ds) { I see "static" used in an anonymous namespace in other places too. I googled around and I guess the point of it was that in C++03 without "static" keyword, the functions in an anonymous namespace would have external linkage. But this is changed in C++11, where anonymous namespace has internal linkage by default. I will keep "static" to be consistent with other parts of the code. On 2014/04/15 22:18:49, darin wrote: > nit: It looks like this function is defined inside an anonymous namespace. > Therefore the "static" keyword is redundant. I see that other functions are also > prefixed with static. That's a shame :-/
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/test/content_browser_test_utils.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; can't find file to patch at input line 6 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: content/test/content_browser_test_utils.cc |diff --git a/content/test/content_browser_test_utils.cc b/content/test/content_browser_test_utils.cc |index 083f4045ee39a6b94b96f6385b9679381cdb3b25..b7bd1f8d4b2793bb260a3768a6d38ffd1cc6b908 100644 |--- a/content/test/content_browser_test_utils.cc |+++ b/content/test/content_browser_test_utils.cc -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored Patch: content/test/content_browser_test_utils.cc Index: content/test/content_browser_test_utils.cc diff --git a/content/test/content_browser_test_utils.cc b/content/test/content_browser_test_utils.cc index 083f4045ee39a6b94b96f6385b9679381cdb3b25..b7bd1f8d4b2793bb260a3768a6d38ffd1cc6b908 100644 --- a/content/test/content_browser_test_utils.cc +++ b/content/test/content_browser_test_utils.cc @@ -43,6 +43,15 @@ void NavigateToURLBlockUntilNavigationsComplete(Shell* window, same_tab_observer.Wait(); } +void LoadDataWithBaseURL(Shell* window, const GURL& url, + const std::string data, const GURL& base_url) { + WaitForLoadStop(window->web_contents()); + TestNavigationObserver same_tab_observer(window->web_contents(), 1); + + window->LoadDataWithBaseURL(url, data, base_url); + same_tab_observer.Wait(); +} + void NavigateToURL(Shell* window, const GURL& url) { NavigateToURLBlockUntilNavigationsComplete(window, url, 1); }
On 2014/04/15 22:58:49, I haz the power (commit-bot) wrote: > Failed to apply patch for content/test/content_browser_test_utils.cc: > While running patch -p1 --forward --force --no-backup-if-mismatch; > can't find file to patch at input line 6 > Perhaps you used the wrong -p or --strip option? > The text leading up to this was: > -------------------------- > |Index: content/test/content_browser_test_utils.cc > |diff --git a/content/test/content_browser_test_utils.cc > b/content/test/content_browser_test_utils.cc > |index > 083f4045ee39a6b94b96f6385b9679381cdb3b25..b7bd1f8d4b2793bb260a3768a6d38ffd1cc6b908 > 100644 > |--- a/content/test/content_browser_test_utils.cc > |+++ b/content/test/content_browser_test_utils.cc > -------------------------- > No file to patch. Skipping patch. > 1 out of 1 hunk ignored > > Patch: content/test/content_browser_test_utils.cc > Index: content/test/content_browser_test_utils.cc > diff --git a/content/test/content_browser_test_utils.cc > b/content/test/content_browser_test_utils.cc > index > 083f4045ee39a6b94b96f6385b9679381cdb3b25..b7bd1f8d4b2793bb260a3768a6d38ffd1cc6b908 > 100644 > --- a/content/test/content_browser_test_utils.cc > +++ b/content/test/content_browser_test_utils.cc > @@ -43,6 +43,15 @@ void NavigateToURLBlockUntilNavigationsComplete(Shell* > window, > same_tab_observer.Wait(); > } > > +void LoadDataWithBaseURL(Shell* window, const GURL& url, > + const std::string data, const GURL& base_url) { > + WaitForLoadStop(window->web_contents()); > + TestNavigationObserver same_tab_observer(window->web_contents(), 1); > + > + window->LoadDataWithBaseURL(url, data, base_url); > + same_tab_observer.Wait(); > +} > + > void NavigateToURL(Shell* window, const GURL& url) { > NavigateToURLBlockUntilNavigationsComplete(window, url, 1); > } I will rebase the patch.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/100001
The CQ bit was unchecked by hush@chromium.org
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/120001
Message was sent while issue was closed.
Change committed as 264247 |