Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6880139:
Changing URL match method to support fragments. (Closed)
|
Created:
9 years, 8 months ago by dominich Modified:
9 years, 7 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChanging URL match method to support fragments.
BUG=79898
TEST=PrerenderBrowserTest.PrerenderPageNavigateFragment and PrerenderBrowserTest.PrerenderFragmentNavigatePage
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82928
Patch Set 1 #Patch Set 2 : Removing unnecessary whitespace. #
Total comments: 6
Patch Set 3 : Response to comments. #Patch Set 4 : rebase #
Total comments: 5
Patch Set 5 : New browser tests #
Total comments: 2
Patch Set 6 : Remove set_dest_url(). Add unit tests. #Patch Set 7 : Adding comment to unittest #Patch Set 8 : Better comments and a rebase. #
Total comments: 3
Patch Set 9 : Added other_fragment and fixed bug numbers in comment. #Patch Set 10 : Add more browser tests. #
Total comments: 7
Patch Set 11 : Replace _refresh.html with CreateClientRedirect. Ensure location is correct." #
Messages
Total messages: 27 (0 generated)
A fix for http://crbug.com/79898. By changing how we match URLs in PrerenderContents we can support fragments in prerendering correctly. Please feel free to suggest any other regression tests you think would be useful.
Is the new hash actually used, or just completely ignored, treated as if it were the same has as when prerendering? Is a window.onhashchange event triggered? http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:36: #include <algorithm> This should go up top, above #include "base/process_util.h" http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:48: return url.host() == url_.host() && Should probably have scheme in here to reduce the chance of breakages if/when we allow prerendering of SSL URLs. http://codereview.chromium.org/6880139/diff/2001/chrome/test/data/prerender/p... File chrome/test/data/prerender/prerender_fragment.html (right): http://codereview.chromium.org/6880139/diff/2001/chrome/test/data/prerender/p... chrome/test/data/prerender/prerender_fragment.html:3: <title>Prerender Fragment testing</title> tiny nit: Fragment and testing should both be lowercase or both be capitalized.
http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:36: #include <algorithm> On 2011/04/22 18:24:45, Matt Menke wrote: > This should go up top, above #include "base/process_util.h" Done. http://codereview.chromium.org/6880139/diff/2001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:48: return url.host() == url_.host() && On 2011/04/22 18:24:45, Matt Menke wrote: > Should probably have scheme in here to reduce the chance of breakages if/when we > allow prerendering of SSL URLs. Done. http://codereview.chromium.org/6880139/diff/2001/chrome/test/data/prerender/p... File chrome/test/data/prerender/prerender_fragment.html (right): http://codereview.chromium.org/6880139/diff/2001/chrome/test/data/prerender/p... chrome/test/data/prerender/prerender_fragment.html:3: <title>Prerender Fragment testing</title> On 2011/04/22 18:24:45, Matt Menke wrote: > tiny nit: Fragment and testing should both be lowercase or both be capitalized. Done.
http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:175: void NavigateToURL(const std::string& dest_html_file) { Note: I use set_dest_url in other tests for this. Would that be sufficient? http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:693: // but navigate to the main page. Can you add a test for prerendering with one fragment and navigating to another fragment?
On 2011/04/22 18:24:45, Matt Menke wrote: > Is the new hash actually used, or just completely ignored, treated as if it were > the same has as when prerendering? Is a window.onhashchange event triggered? The onhashchange event shouldn't fire in this case as we are navigating from a different URL. I'm not doing anything to change the navigation URL or the URL that is preloaded, I'm just changing how we decide if the URL we're navigating to matches one that we have preloaded.
http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:175: void NavigateToURL(const std::string& dest_html_file) { On 2011/04/22 19:49:34, cbentzel wrote: > Note: I use set_dest_url in other tests for this. Would that be sufficient? That's subtly different. Here I'm navigating to a new URL but the CHECK that calls FindEntry is checking against the old URL. Is it possible that where you've used set_dest_url() you didn't want to change that CHECK? http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:693: // but navigate to the main page. On 2011/04/22 19:49:34, cbentzel wrote: > Can you add a test for prerendering with one fragment and navigating to another > fragment? Done.
http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:175: void NavigateToURL(const std::string& dest_html_file) { On 2011/04/22 20:48:11, dominic wrote: > On 2011/04/22 19:49:34, cbentzel wrote: > > Note: I use set_dest_url in other tests for this. Would that be sufficient? > > That's subtly different. Here I'm navigating to a new URL but the CHECK that > calls FindEntry is checking against the old URL. > > Is it possible that where you've used set_dest_url() you didn't want to change > that CHECK? OK, I see your point. To capture this, I'd recommend doing a CHECK before the navigation to make sure that FindEntry is non-NULL on dest_url_, do the navigation, and then test again that it FindEntry is NULL. In the fragment mismatch case this will work due to your predicate function. Either way, I want either the NavigateToURL with an overrideable URL, or set_dest_url, not both. If you keep this one, I'd suggest just passing in a const-ref-GURL rather than constructing one. http://codereview.chromium.org/6880139/diff/6005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:702: Comment is incorrect.
Also: you should probably add some tests to prerender_manager_unittest as well. Pretty similar in terms of coverage, but the unit test completes faster and is easier to debug if a regression is added. On Fri, Apr 22, 2011 at 5:01 PM, <cbentzel@chromium.org> wrote: > > > http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > > http://codereview.chromium.org/6880139/diff/5003/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_browsertest.cc:175: void > NavigateToURL(const std::string& dest_html_file) { > On 2011/04/22 20:48:11, dominic wrote: > >> On 2011/04/22 19:49:34, cbentzel wrote: >> > Note: I use set_dest_url in other tests for this. Would that be >> > sufficient? > > That's subtly different. Here I'm navigating to a new URL but the >> > CHECK that > >> calls FindEntry is checking against the old URL. >> > > Is it possible that where you've used set_dest_url() you didn't want >> > to change > >> that CHECK? >> > > OK, I see your point. > > To capture this, I'd recommend doing a CHECK before the navigation to > make sure that FindEntry is non-NULL on dest_url_, do the navigation, > and then test again that it FindEntry is NULL. In the fragment mismatch > case this will work due to your predicate function. > > Either way, I want either the NavigateToURL with an overrideable URL, or > set_dest_url, not both. If you keep this one, I'd suggest just passing > in a const-ref-GURL rather than constructing one. > > > http://codereview.chromium.org/6880139/diff/6005/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_browsertest.cc:702: > Comment is incorrect. > > > http://codereview.chromium.org/6880139/ >
On 2011/04/22 21:01:16, cbentzel wrote: > Either way, I want either the NavigateToURL with an overrideable URL, or > set_dest_url, not both. If you keep this one, I'd suggest just passing in a > const-ref-GURL rather than constructing one. In previous reviews, it's been pointed out that the rest of the public interface uses std::string html files and converts to a GURL through the test server internally. I'd like to keep that consistency. Given how the CHECKs are being done, I think the removal of set_dest_url is more correct and will fix everything up accordingly.
http://codereview.chromium.org/6880139/diff/6005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/6005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:702: // but navigate to the main page. On 2011/04/22 21:01:16, cbentzel wrote: > Comment is incorrect. Done.
I'm not sure why the try bots are failing - the errors are referencing a line that is a line of whitespace. It's as if the sync or patch was bad.
On 2011/04/22 22:36:57, dominic wrote: > I'm not sure why the try bots are failing - the errors are referencing a line > that is a line of whitespace. It's as if the sync or patch was bad. Or as if the file has changed in the meantime: patching file chrome/browser/prerender/prerender_manager_unittest.cc Hunk #1 succeeded at 489 (offset -6 lines).
On 2011/04/22 22:42:41, Matt Menke wrote: > On 2011/04/22 22:36:57, dominic wrote: > > I'm not sure why the try bots are failing - the errors are referencing a line > > that is a line of whitespace. It's as if the sync or patch was bad. > > Or as if the file has changed in the meantime: > > patching file chrome/browser/prerender/prerender_manager_unittest.cc > Hunk #1 succeeded at 489 (offset -6 lines). I've done a fetch/rebase and added a comment. Let's see what happens...
Tests are now passing.
LGTM, though I think we should look into updating the fragment the renderer is using, too. http://codereview.chromium.org/6880139/diff/2004/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/2004/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:554: // Disabled, http://crbug.com/80324. While you're here anyways...Could you please fix these for me? This should be 77870 and the one below should be 80324. Thanks.
http://codereview.chromium.org/6880139/diff/2004/chrome/test/data/prerender/p... File chrome/test/data/prerender/prerender_fragment.html (right): http://codereview.chromium.org/6880139/diff/2004/chrome/test/data/prerender/p... chrome/test/data/prerender/prerender_fragment.html:22: <a name="fragment">Fragment</a> nit: It doesn't really seem to matter for the tests, but should there be an 'other_fragment' here, too?
http://codereview.chromium.org/6880139/diff/2004/chrome/test/data/prerender/p... File chrome/test/data/prerender/prerender_fragment.html (right): http://codereview.chromium.org/6880139/diff/2004/chrome/test/data/prerender/p... chrome/test/data/prerender/prerender_fragment.html:22: <a name="fragment">Fragment</a> On 2011/04/25 17:58:20, Matt Menke wrote: > nit: It doesn't really seem to matter for the tests, but should there be an > 'other_fragment' here, too? I didn't think it necessary as we never navigate to it, but it also doesn't cost anything so I'll add it :)
LGTM It seems like a few more browser_tests should be added: - Updating hash via <meta http-equiv=refresh> - Updating hash via window.location.hash while the page is being prerendered.
On 2011/04/25 18:16:56, cbentzel wrote: > LGTM > > It seems like a few more browser_tests should be added: > - Updating hash via <meta http-equiv=refresh> > - Updating hash via window.location.hash while the page is being prerendered. Speaking of which...we don't have a browser test where window.location is set, either, I believe.
On 2011/04/25 18:16:56, cbentzel wrote: > LGTM > > It seems like a few more browser_tests should be added: > - Updating hash via <meta http-equiv=refresh> > - Updating hash via window.location.hash while the page is being prerendered. This is a little tricky with how we currently test that prerendering happened. pageWasPrerendered will be false after we refresh the page after navigating so the test will fail if the refresh happens too soon. Given we don't currently test even basic prerendering under a refresh, I'd like to get this in and then add tests for refresh and window.location changes across the board in a new CL.
On 2011/04/25 18:54:04, dominic wrote: > On 2011/04/25 18:16:56, cbentzel wrote: > > LGTM > > > > It seems like a few more browser_tests should be added: > > - Updating hash via <meta http-equiv=refresh> > > - Updating hash via window.location.hash while the page is being > prerendered. > > This is a little tricky with how we currently test that prerendering happened. > pageWasPrerendered will be false after we refresh the page after navigating so > the test will fail if the refresh happens too soon. This isn't actually a problem. Just set the 3rd parameter of PrerenderTestURL to 2. We do in fact test under a refresh - at least I assume that's what the client redirects actually are. Suppose they could actually be Javascript.
Yes, all the PrerenderClientRedirect tests use <meta http-equiv="refresh"> On Mon, Apr 25, 2011 at 2:56 PM, <mmenke@chromium.org> wrote: > On 2011/04/25 18:54:04, dominic wrote: > >> On 2011/04/25 18:16:56, cbentzel wrote: >> > LGTM >> > >> > It seems like a few more browser_tests should be added: >> > - Updating hash via <meta http-equiv=refresh> >> > - Updating hash via window.location.hash while the page is being >> prerendered. >> > > This is a little tricky with how we currently test that prerendering >> happened. >> pageWasPrerendered will be false after we refresh the page after >> navigating so >> the test will fail if the refresh happens too soon. >> > > This isn't actually a problem. Just set the 3rd parameter of > PrerenderTestURL > to 2. We do in fact test under a refresh - at least I assume that's what > the > client redirects actually are. Suppose they could actually be Javascript. > > > http://codereview.chromium.org/6880139/ >
On 2011/04/25 18:58:43, cbentzel wrote: > Yes, all the PrerenderClientRedirect tests use <meta http-equiv="refresh"> > Oh great, I completely misunderstood. I'll add that then :)
Browser tests added for client refresh and client location.hash setting.
http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:714: PrerenderPageChangeFragmentRefresh) { Nit: I'd name something like PrerenderClientRedirectToFragment or PrerenderFragmentClientRedirect to match other tests. http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:715: PrerenderTestURL("files/prerender/prerender_fragment_refresh.html", You may be able to just use CreateClientRedirect("files/prerender/prereder_fragment.html#other_fragment") and remove the prerender_fragment_refresh.html page. It's possible that this won't work due to the page rewriter not handling fragments correctly. http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:728: NavigateToURL("files/prerender/prerender_fragment_location_hash.html"); How are you guaranteed that location is being set correctly? Maybe do inside the Prerendered javascript call? Does it complete synchronously? http://codereview.chromium.org/6880139/diff/8005/chrome/test/data/prerender/p... File chrome/test/data/prerender/prerender_fragment_location_hash.html (right): http://codereview.chromium.org/6880139/diff/8005/chrome/test/data/prerender/p... chrome/test/data/prerender/prerender_fragment_location_hash.html:19: console.log("Current hash is '" + window.location.hash + "'"); Get rid of console.log
http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:714: PrerenderPageChangeFragmentRefresh) { On 2011/04/25 19:49:48, cbentzel wrote: > Nit: I'd name something like PrerenderClientRedirectToFragment or > PrerenderFragmentClientRedirect to match other tests. Done. http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:715: PrerenderTestURL("files/prerender/prerender_fragment_refresh.html", On 2011/04/25 19:49:48, cbentzel wrote: > You may be able to just use > CreateClientRedirect("files/prerender/prereder_fragment.html#other_fragment") > and remove the prerender_fragment_refresh.html page. > > It's possible that this won't work due to the page rewriter not handling > fragments correctly. Done. http://codereview.chromium.org/6880139/diff/8005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:728: NavigateToURL("files/prerender/prerender_fragment_location_hash.html"); On 2011/04/25 19:49:48, cbentzel wrote: > How are you guaranteed that location is being set correctly? > > Maybe do inside the Prerendered javascript call? Does it complete synchronously? Done.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
